The Standard

Summary

The issue in the LiquidationPool contract arises when a user's confirmed stake is completely withdrawn using the decreasePosition() function, which results in the user being removed from the holders array through the deletePosition() function. However, if the user still has pending stakes in the pendingStakes array that have not yet been consolidated, this premature removal can lead to several problems.

Vulnerability Details

The deletePosition() function is called within decreasePosition() when a user's confirmed stake in both TST and EUROs tokens becomes zero. The deletePosition() function removes the user's address from the holders array and deletes their Position from the positions mapping.

However, if the user still has pending stakes in the pendingStakes array that have not yet been consolidated into their confirmed position, deleting the user from the holders array could lead to a situation where the user has a stake in the system that is not properly accounted for once the pending stakes are consolidated.

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

POC

This could happen in the following scenario:

1. A user has a confirmed position with some amount of TST and EUROs.

2. The user initiates a new stake, which is added to the pendingStakes array but not immediately to their confirmed position.

3. Before the pending stake is consolidated (which happens after 1 day), the user decides to withdraw their entire confirmed stake using decreasePosition().

4. The decreasePosition() function calls deletePosition() because the confirmed stake is now zero, removing the user from the holders array.

5. When consolidatePendingStakes() is eventually called (e.g., the next day or during another interaction with the contract), the pending stake is added to the user's position, but since the user is no longer in the holders array, their stake may not be properly represented in the system.

Impact

If a holder is deleted from the holders list even if they have a stake in the pendingStakes list can be significant and affect multiple functionalities of the LiquidationPool contract.

1. Incorrect Fee Distribution: The distributeFees() function relies on the holders array to distribute EUROs fees proportionally based on the TST stake each holder has. If a holder is incorrectly removed, they will not receive their rightful share of the fees once their pending stakes are consolidated.

2. Asset Distribution Issues: The distributeAssets() function also uses the holders array to distribute assets when liquidating positions. If a holder is missing from this array, they will not receive their portion of the assets corresponding to their pending stakes.

3. Stake Accounting Errors: The getStakeTotal() and getTstTotal() functions iterate over the holders array to calculate the total staked amounts. If a holder with pending stakes is not included, these totals will be incorrect, leading to inaccurate calculations for reward distributions, fee allocations, and more.

4. Reward Claiming Problems: When claiming rewards through the claimRewards() function, if a holder's address is not in the holders array, they might not be able to claim rewards for pending stakes that have been consolidated after they were removed.

5. https://www.codehawks.com/contests/clql6lvyu0001mnje1xpqcuvl > Known Issues > LiquidationPool

   Let's validate the accuracy of two statements mentioned in the documentation under known issues for certain edge cases.

   • "position function depends on getTstTotal not being 0. However, if current position has TST, then TSTtotal will never be 0."

     Under the assumption that every user is affected by the bug, the getTstTotal() function could potentially return 0 if all users have withdrawn their confirmed TST stakes and only have pending TST stakes that have not yet been consolidated. This would mean that the position() function could incorrectly calculate a user's share of EUROs as zero, since it would be based on a TST total that does not account for pending stakes.

   • "distributeAssets function requires stakeTotal to be greater than 0, but this will always be the case if any _positionStake > 0."

     Similarly, if every user has been removed from the holders array due to the bug, the stakeTotal could also be incorrectly calculated as 0, since it is derived from the confirmed stakes of users in the holders array. If stakeTotal is 0, the distributeAssets() function would not be able to distribute assets correctly, as it relies on a positive stakeTotal to determine each user's share of the assets.

   You can see that, both the statements are failing.

Tools Used

Manual Review

Recommendations

The contract should be updated to ensure that a user is only removed from the holders array if they have no confirmed position and no pending stakes. This could be done by modifying the deletePosition() function to check for pending stakes before removing a user from the holders array. Additionally, the logic that handles the consolidation of pending stakes should ensure that the holders array is correctly updated to reflect any users who have pending stakes that are being consolidated.

Solution