Withdraw amount returned by `getLiquidityAmountsAndPositions` may be incorrect

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2023-12-autonolas/blob/main/lockbox-solana/solidity/liquidity_lockbox.sol#L377

Vulnerability details

Impact

The getLiquidityAmountsAndPositions() function in the liquidity_lockbox contract is used to calculate the liquidity amounts and positions to be withdrawn for a given total withdrawal amount. It iterates through each deposited position following a FIFO order as shown below:

        uint64 liquiditySum = 0;
        uint32 numPositions = 0;
        uint64 amountLeft = 0;

        // Get the number of allocated positions
        for (uint32 i = firstAvailablePositionAccountIndex; i < numPositionAccounts; ++i) {
            address positionAddress = positionAccounts[i];
            uint64 positionLiquidity = mapPositionAccountLiquidity[positionAddress];

            // Increase a total calculated liquidity and a number of positions to return
            liquiditySum += positionLiquidity;
            numPositions++;

            // Check if the accumulated liquidity is enough to cover the requested amount
            if (liquiditySum >= amount) {
                amountLeft = liquiditySum - amount;
                break;
            }
        }

However, there is an error in the calculation of the last position amount when it entails a partial withdrawal. The amountLeft variable represents the leftover liquidity in the last position after the user's withdrawals, but it is assigned to the returned amounts as if it represented the amount the user should withdraw:

        // Adjust the last position, if it was not fully allocated
        if (numPositions > 0 && amountLeft > 0) {
            positionAmounts[numPositions - 1] = amountLeft;
        }

This discrepancy could lead to users withdrawing an incorrect amount if they use getLiquidityAmountsAndPositions(), as intended, to obtain positions and amounts to use when calling withdraw(). This can result in significant unintended transfer of funds for the user, especially in cases where the last position in positionAmounts is of significant size. Given the fact that this issue can occur under normal usage of the contract, this issue is assessed as medium severity.

Proof of Concept

Consider the following step-by-step scenario:

1. Alice has a large amount of bridged tokens.
2. Alice decides to withdraw a small portion of her liquidity. She has written a script that calls getLiquidityAmountsAndPositions() to calculate the positions and amounts for the withdrawal and iterates through the returned arrays in a series of calls to withdraw().
3. The firstAvailablePositionAccountIndex points to a very large position, which causes getLiquidityAmountsAndPositions() to return an amount much larger than Alice intended.
4. Alice unknowingly passes the returned amount to withdraw and as a result ends up withdrawing far more tokens than she intended, potentially depleting her liquidity in the contract.

Tools Used

Manual review

Recommended Mitigation Steps

To fix this issue, the last position amount should be reduced by the remaining amount when the accumulated liquidity is not fully allocated. This can be achieved by changing the assignment operation to a subtraction operation:

@@ -374,7 +374,7 @@ contract liquidity_lockbox {

         // Adjust the last position, if it was not fully allocated
         if (numPositions > 0 && amountLeft > 0) {
-            positionAmounts[numPositions - 1] = amountLeft;
+            positionAmounts[numPositions - 1] -= amountLeft;
         }
     }

This change ensures that the last position amount is correctly calculated, preventing users from withdrawing an incorrect amount.

Assessed type

Other

[c4-pre-sort]

alex-ppg marked the issue as primary issue

[c4-pre-sort]

alex-ppg marked the issue as sufficient quality report

[c4-sponsor]

kupermind (sponsor) confirmed

[c4-sponsor]

kupermind marked the issue as disagree with severity

[c4-judge]

dmvt marked the issue as selected for report

[kupermind]

@c4-judge The bug is related to the view function, and this can't be considered as Medium risk. It's more like informative, and thus we are disputing this.

Not relevant anymore as the code has been completely re-written: https://github.com/valory-xyz/lockbox-solana