User's / protocol would be unable to withdraw intended amounts from Solana lockbox

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

User's / protocol unable to withdraw intended amounts from the lockbox

Proof of Concept

When withdrawing it is required to pass all the associated accounts correctly. Hence the getLiquidityAmountsAndPositions function is implemented to obtain the required data for a given withdrawal amount. But the function is wrongly implemented and gives incorrect withdrawal amount for the last position

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/lockbox-solana/solidity/liquidity_lockbox.sol#L338-L379

    function getLiquidityAmountsAndPositions(uint64 amount)
        external view returns (uint64[] positionAmounts, address[] positionAddresses, address[] positionPdaAtas)
    {

        ......

        uint64 liquiditySum = 0;

        for (uint32 i = firstAvailablePositionAccountIndex; i < numPositionAccounts; ++i) {

            .....

            liquiditySum += positionLiquidity;

            .....

            if (liquiditySum >= amount) {
                amountLeft = liquiditySum - amount;
                break;
            }
        }

        for (uint32 i = 0; i < numPositions; ++i) {

            ....

            positionAmounts[i] = mapPositionAccountLiquidity[positionAddresses[i]];

            ....
        }

        // @audit incorrect
        if (numPositions > 0 && amountLeft > 0) {
            positionAmounts[numPositions - 1] = amountLeft;
        }
    }

Instead of subtracting amountLeft from the final position, the positionAmount is set as amountLeft. This can cause the overall sum to be greater or less than the actual amount

Example

1 Position with liquidity 150 User wants to withdraw 100

Function walkthrough: After first loop block, liquiditySum == 150 hence amountLeft = 50 In the second loop block, positionAmounts[0] would be set to 50 This would make the totalWithdrawal amount to 50 while the actual intended withdrawal was 100

Tools Used

Manual review

Recommended Mitigation Steps

Subtract from the final position instead of setting

        if (numPositions > 0 && amountLeft > 0) {
            positionAmounts[numPositions - 1] -= amountLeft;
        }

Assessed type

Math

[c4-pre-sort]

alex-ppg marked the issue as duplicate of #452

[c4-pre-sort]

alex-ppg marked the issue as sufficient quality report

[c4-judge]

dmvt marked the issue as satisfactory