Wrong Implementation of LiquidationPool::empty excludes holder with pending stakes when decreasing a position, resulting in exclusion from asset distribution

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L92-L94

Description

If a holder has withdrawn from a previous consolidated position but still has pending stakes eligible for consolidation, they will not be included in the asset distribution. This is due to their prior exclusion from the LiquidationPool.holder array through LiquidationPool::deletePosition, given that the position will be considered as empty when position.TST == 0 && position.EUROs == 0.

Impact

Pending stakes that are consolidated during asset distribution will be excluded from rewards if there is no previous consolidated position due to a prior complete withdrawal, leading to wrong asset distribution when a liquidation occurs

POC

1. Bob has a position of (TST = 100, EUROS = 100).
2. Alice has a position of (TST = 100, EUROS = 100).
3. Alice increases her position by (TST = 100, EUROS = 100). Consequently, a pending stake in her favor is added to the LiquidationPool.pendingStake array.
4. Immediately after step 3, Alice does a complete withdrawal of her consolidated position. Given that 24 hours have not passed since step 3, the pending stake created during the previous step is not consolidated. Her current position is considered empty; therefore, she is excluded from holders.
5. After 24 hours, a vault is liquidated. Now, Alice's pending stake is consolidated, but since she is not included in holders, she is not considered for asset distribution. All assets go to Bob.

Coded POC

Severity justification

Lost of unclaimed yield

Recommended mitigation

When decreasing a position, verify if there is a pending stake from the same holder. If there is, do not exclude the holder from holders array. This means:

    function empty(Position memory _position) private pure returns (bool) {
        if (_position.TST == 0 && _position.EUROs == 0){
            address positionHolder = _position.holder;
            uint256 pendingStakesLen = pendingStakes.length();
            for(uint i; i < pendingStakesLen;){
                if(pendingStakes[i].holder == positionHolder){
                    return false;
                }
                unchecked{ ++i};
            }
            return true;
        }else{
            return false;
        }
    }