Incorrect slippage protection order in `withdrawLiquidityAndClaim()`

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L170 https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Liquidity.sol#L157 https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Liquidity.sol#L121 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolUtils.sol#L32

Vulnerability details

Impact

In the withdrawLiquidityAndClaim() function, an issue arises when the flipped == true in the removeLiquidity() function. The ordering of minReclaimedA and minReclaimedB becomes incorrect, potentially resulting in unexpected reversions or substantial losses for users due to incorrect slippage protection.

Proof of Concept

The problem originates in the pools.removeLiquidity() function, which utilizes PoolUtils._poolIDAndFlipped() to determine the proper order of tokens.

When withdrawLiquidityAndClaim() is invoked, calling _withdrawLiquidityAndClaim(), the issue becomes evident:

    function _withdrawLiquidityAndClaim( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToWithdraw, uint256 minReclaimedA, uint256 minReclaimedB ) internal returns (uint256 reclaimedA, uint256 reclaimedB)
        {
        bytes32 poolID = PoolUtils._poolID( tokenA, tokenB );
        require( userShareForPool(msg.sender, poolID) >= liquidityToWithdraw, "Cannot withdraw more than existing user share" );

        (reclaimedA, reclaimedB) = pools.removeLiquidity( tokenA, tokenB, liquidityToWithdraw, minReclaimedA, minReclaimedB, totalShares[poolID] );

        tokenA.safeTransfer( msg.sender, reclaimedA );
        tokenB.safeTransfer( msg.sender, reclaimedB );

        _decreaseUserShare( msg.sender, poolID, liquidityToWithdraw, true );

        emit LiquidityWithdrawn(msg.sender, address(tokenA), address(tokenB), reclaimedA, reclaimedB, liquidityToWithdraw);
        }

In removeLiquidity(), when flipped == true, reclaimedA and reclaimedB are swapped into the correct order. reclaimedA and reclaimedB are then used to check against minReclaimedA and minReclaimedB-which should have been swapped too. The function is effectively checking incorrect slippage protection against the wrong tokens.

    function removeLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToRemove, uint256 minReclaimedA, uint256 minReclaimedB, uint256 totalLiquidity ) external nonReentrant returns (uint256 reclaimedA, uint256 reclaimedB)
        {
        require( msg.sender == address(collateralAndLiquidity), "Pools.removeLiquidity is only callable from the CollateralAndLiquidity contract" );
        require( liquidityToRemove > 0, "The amount of liquidityToRemove cannot be zero" );

          ->(bytes32 poolID, bool flipped) = PoolUtils._poolIDAndFlipped(tokenA, tokenB);

        // Determine how much liquidity is being withdrawn and round down in favor of the protocol
        PoolReserves storage reserves = _poolReserves[poolID];
        reclaimedA = ( reserves.reserve0 * liquidityToRemove ) / totalLiquidity;
        reclaimedB = ( reserves.reserve1 * liquidityToRemove ) / totalLiquidity;

        reserves.reserve0 -= uint128(reclaimedA);
        reserves.reserve1 -= uint128(reclaimedB);

        // Make sure that removing liquidity doesn't drive either of the reserves below DUST.
        // This is to ensure that ratios remain relatively constant even after a maximum withdrawal.
        require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

        // Switch reclaimed amounts back to the order that was specified in the call arguments so they make sense to the caller
          ->if (flipped)
            (reclaimedA,reclaimedB) = (reclaimedB,reclaimedA);

          ->require( (reclaimedA >= minReclaimedA) && (reclaimedB >= minReclaimedB), "Insufficient underlying tokens returned" );

        // Send the reclaimed tokens to the user
        tokenA.safeTransfer( msg.sender, reclaimedA );
        tokenB.safeTransfer( msg.sender, reclaimedB );

        emit LiquidityRemoved(tokenA, tokenB, reclaimedA, reclaimedB, liquidityToRemove);
        }

This issue can cause 2 scenarios:

1. A withdrawLiquidityAndClaim() call might unexpectedly revert during the slippage protection check due to incorrect token ordering.
2. A successful execution of withdrawLiquidityAndClaim() could lead to significant user losses as slippage protection is misaligned against the wrong tokens.

Tools Used

Manual Review

Recommended Mitigation Steps

    function removeLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToRemove, uint256 minReclaimedA, uint256 minReclaimedB, uint256 totalLiquidity ) external nonReentrant returns (uint256 reclaimedA, uint256 reclaimedB)
        {
        require( msg.sender == address(collateralAndLiquidity), "Pools.removeLiquidity is only callable from the CollateralAndLiquidity contract" );
        require( liquidityToRemove > 0, "The amount of liquidityToRemove cannot be zero" );

        (bytes32 poolID, bool flipped) = PoolUtils._poolIDAndFlipped(tokenA, tokenB);

        // Determine how much liquidity is being withdrawn and round down in favor of the protocol
        PoolReserves storage reserves = _poolReserves[poolID];
        reclaimedA = ( reserves.reserve0 * liquidityToRemove ) / totalLiquidity;
        reclaimedB = ( reserves.reserve1 * liquidityToRemove ) / totalLiquidity;

        reserves.reserve0 -= uint128(reclaimedA);
        reserves.reserve1 -= uint128(reclaimedB);

        // Make sure that removing liquidity doesn't drive either of the reserves below DUST.
        // This is to ensure that ratios remain relatively constant even after a maximum withdrawal.
        require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

        // Switch reclaimed amounts back to the order that was specified in the call arguments so they make sense to the caller
        if (flipped)
            (reclaimedA,reclaimedB) = (reclaimedB,reclaimedA);
+                       (minReclaimedA, minReclaimedB) = (minReclaimedB, minReclaimedA);

        require( (reclaimedA >= minReclaimedA) && (reclaimedB >= minReclaimedB), "Insufficient underlying tokens returned" );

        // Send the reclaimed tokens to the user
        tokenA.safeTransfer( msg.sender, reclaimedA );
        tokenB.safeTransfer( msg.sender, reclaimedB );

        emit LiquidityRemoved(tokenA, tokenB, reclaimedA, reclaimedB, liquidityToRemove);
        }

Assessed type

Context

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid

[Picodes]

It looks correct to me. Amounts are flipped before being compared to user's inputs

[Henrychang26]

@Picodes , thanks for judging

Not all amounts are swapped correctly. Only reclaimedA and reclaimedB are flipped. minReclaimedA and minReclaimedB are never flipped, so it is indeed checking slippage in the wrong order.

See following example: When user calls withdrawLiquidityAndClaim() and invokes removeLiquidity() with following inputs:

tokenA = USDC which corresponds to minReclaimedA == minimum amount to receive in USDC. tokenB = ETH which corresponds to minReclaimedB == minimum amount to receive in ETH.

Assuming flipped == true, PRIOR to the if statement here reclaimedA is amount in USDC. reclaimedB is amount in ETH.

After swapping places here reclaimedA is amount in ETH. reclaimedB is amount in USDC.

However, minReclaimedA and minReclaimedB were never swapped. The function goes on to check slippage against the wrong order here reclaimedA(amount in ETH) is checked against minReclaimedA(amount in USDC) reclaimedB(amount in USDC) is checked against minReclaimedB(amount in ETH)

This can lead to two issues mentioned in the original submission.

Perhaps, we can have the sponsor take a look at this? Thanks again for your time!

[Picodes]

@Henrychang26 thanks. Just a side note before I dig into this: in such cases, it's preferable to provide a coded PoC.

[Picodes]

"Assuming flipped == true, PRIOR to the if statement here reclaimedA is amount in USDC. reclaimedB is amount in ETH."

This isn't correct. If flipped = true, it means reclaimedA is in ETH because the reserves are in inverse in the storage