Error of computation break the LpTokens supply, causes users to lose funds and make functions using `_getUtilityFinalLp()` broken.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/EvolvingProteus.sol#L426-L453 https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/EvolvingProteus.sol#L463-L490 https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/LiquidityPool.sol#L297-L306

Vulnerability details

Impact

withdrawGivenOutputAmount() and withdrawGivenInputAmount() functions doesn't revert when balance of tokenX/tokenY = 0 and create an offset between reserve tokens and LP total supply. This lead to unwanted behaviors for the next operations on the protocol.

Proof of Concept

https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/EvolvingProteus.sol#L426-L453

https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/EvolvingProteus.sol#L463-L490

https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/LiquidityPool.sol#L297-L306

The withdrawGivenOutputAmount() function is used to retrieve the reserve tokens (liquidity) in exchange of LP tokens, we give an output amount of reserve tokens we want and the function compute an amount of LP tokens that must be burned in order to decrease the total supply. When we call the withdrawGivenOutputAmount() function with a balance of X tokens = 0, the function doesn't revert, give an amout of LP tokens to "burn" and update the LP total supply is made in the LiquidityPool.sol contract.

This error of computation could lead, for example, to the following scenario where Alice lose her funds:

1) Alice want to retrieve her liquidity (Tokens X) with her LP, she call withdrawGivenOutputAmount() to make the operation. 2) Bob see the transaction of Alice and frontrun it with a swap (in example with swapGivenInput() function) to make the xBalance = 0. 3) Bob's transaction is executed first, followed by Alice's. 4) Because withdrawGivenOutputAmount() doesn't revert with the xBalance = 0, Alice "burn" her LP Tokens and receive 0 Tokens X.

Alice lose her LP Tokens and therefore her liquidity. And because the LP total supply balance is broken, computations using _getUtilityFinalLp() becomes broken. All the users on the pool are impacted and can't recover all the liquidity they have gived to the pool.

Another scenario, less interesting for the attacker but possible. The attacker can make a swap to reduce the balance of Tokens X to 0, use withdrawGivenOutputAmount() with his LP tokens, he lose his LP Tokens but impact all the users operations.

PoC :

You can add this test to EvolvingProteusProperties.t.sol

    function testBadComputationForBalance0() public {
    //ADD FOR TEST
    address attacker = address(0xaa);
    //Balance of xToken
    uint256 xi = 0;
    //Balance of yToken
    uint256 yi = 34530900193750501340;
    //Total Supply of Lp Tokens
    uint256 lpTotalSupply = 38219886017803227729;
    //Deposited amount of xTokens
    uint256 depositedAmount = 4530487000000000000;
    SpecifiedToken depositedToken = SpecifiedToken.X;

    vm.label(attacker, "Attacker");
    vm.startPrank(address(attacker));

    try
        DUT.withdrawGivenOutputAmount(
            xi,
            yi,
            lpTotalSupply,
            depositedAmount,
            depositedToken
        )
    returns (uint256 burnedAmount) {
        vm.expectRevert(bytes(""));
        emit log_named_uint("burnedAmount computed", burnedAmount);
        assertGt(burnedAmount, 2000);
    } catch {}
}

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

In the way you want to resolve this issue, you can make verifications like this:

 function withdrawGivenOutputAmount(
    uint256 xBalance,
    uint256 yBalance,
    uint256 totalSupply,
    uint256 withdrawnAmount,
    SpecifiedToken withdrawnToken
) external view returns (uint256 burnedAmount) {
    ++ require(xBalance > 0 && yBalance > 0, "Error in tokens balances");

    ...

    );

It's an example, you can verify that balances exceed a certain amount, for example the amount of the liquidity given by the creator when creating the pool, but it's more complex to implement.

Also makes necessary verifications on the withdrawGivenInputAmount() function.

Assessed type

Invalid Validation

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as low quality report

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[0xRobocop]

May be QA.

The issue is speculating on how other contracts will integrate with the Proteus code. But, given that the impact may result in loss of funds will mark it as valid for sponsor review.

[c4-sponsor]

ishaansinghal (sponsor) disputed

[ishaansinghal]

The pool cannot reach a state with 0 balance of the reserve token given the checks in the swap functions. In addition, contracts that integrate the Proteus code have slippage protection functionality, similar to any other sandwich attack that may occur on the pool.

[viraj124]

just seeing this and agree with @ishaansinghal we have mentioned in the comments https://github.com/code-423n4/2023-08-shell/blob/main/src/test/EvolvingProteusProperties.t.sol#L19 that unit tests are just meant to test the computation since we call the internal methods directly in a real-world scenario the flow is user -> ocean -> liquidity proxy -> evolving proteus and we check initial reserve balances in the proxy contract https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/LiquidityPool.sol#L426

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid