Front-running first deposit in a pool with an imbalanced deposit allow attacker to drain the pool value

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L140-L165

Vulnerability details

Vulnerability details

Salty is an AMM managed by a DAO. For a pool to be deployed, DAO must vote for the whitelisting of a tokens. Once the proposal is executed, a pool is created and users can start depositing their assets to the pool to receive shares tokens. On the very first liquidity deposit into the pool, any ratio of token is accepted. But for next ones, the pool will automatically calculate how much of both tokens must be deposited to keep the ratio equal. Users just have to define the max amount of token0 and token1 they want to tdeposit, and protocol will comply with these limits.

This opens a grief vector where a honest user will try to deposit into a freshly created pool complying with the actual price/ratio of both tokens on open market, but will get front-run by an attacker depositing funds in a really imbalanced ratio. The ratio being set by the attacker, the liquidity deposit of the honest user will be changed to comply with the ratio, forcing him to deposit a lot of higher priced tokens, and a very low value of lower priced tokens.

Then the attacker can swap into the pool and drain all the highly valued tokens.

Impact

First honest depositor can be front-run and see all his deposit to the pool drained.

Proof of Concept

Add this POC to src/pools/tests/Pools.t.sol I've used big values to help the readability, here's the print-out:

Running 1 test for src/pools/tests/Pools.t.sol:TestPools2
[PASS] test_auditImbalancedPool() (gas: 4320409)
Logs:

  ---- front-running liquidity deposit by attacker ----
  ---- the pool is completely imbalanced with ratio as 1 DAI = 100 WETH ----
  DAI reserves (wei): 101
  WETH reserves (wei): 10100

  ---- Alice try to deposit 300.000 DAI and 100 WETH as its the current price ----
  ---- but as she has been front-run, the pool rebalance her deposit to keep the initial ratio indentical ----
  (from here printed values are divided by 1e18 for better readability)
  Alice maxAmount specified: 300000 DAI, actual addedAmount: 1 DAI
  Alice maxAmount WETH specified: 100 WETH, actual addedAmount: 100 WETH
  DAI reserves: 1 DAI
  WETH reserves: 100 WETH

  ---- Attacker back-run Alice's deposit with a swap ----
  attacker swaping 100 DAI => amountOut = 99 WETH
  total profit in $ for the swap: 296929
  DAI reserves: 101 DAI
  WETH reserves: 0 WETH (dust)

Link to the gist: https://gist.github.com/InfectedIsm/02cc5ec7d4bd584edba8b59e0e4c3f1d

Tools Used

Manual review

Recommended Mitigation Steps

When finalizing token whitelisting, the DAO should make a first deposit with a roughly correct ratio to force the pool in a correct state.

Assessed type

MEV

[c4-judge]

Picodes marked the issue as duplicate of #341

[c4-judge]

Picodes marked the issue as not a duplicate

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) confirmed

[othernet-global]

addLiquidity now uses minAddedAmountA and minAddedAmountB.

Fixed in: https://github.com/othernet-global/salty-io/commit/0bb763cc67e6a30a97d8b157f7e5954692b3dd68

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[0xRobocop]

I think this issue was miss-judged.

The issue is invalid, the PoC is not using the interface properly. There is a parameter of min shares that will prevent the exploit.

Basically, since the user is the first depositor he would expect shares equal to amount0 + amount1:

// Default liquidity will be the addition of both maxAmounts in case one of them is much smaller (has smaller decimals)
return ( maxAmount0, maxAmount1, (maxAmount0 + maxAmount1) );

If you modify the warden's PoC to use minShares as the sum of the amounts to add by the first depositor you will see the attack fails

(uint256 addedAmount0, uint256 addedAmount1, ) 
        = collateralAndLiquidity.depositLiquidityAndIncreaseShare( 
            testParams.token0, 
            testParams.token1, 
            testParams.maxAmountLiquidity0, 
            testParams.maxAmountLiquidity1, 
                        // @audit This was zero in the original PoC
            testParams.maxAmountLiquidity0 + testParams.maxAmountLiquidity1, 
            block.timestamp, 
            false 
        );

[etherSky111]

@0xRobocop , you need to check #509.

[Picodes]

@0xRobocop thanks for flagging. The real issue about the slippage being inefficient is #221. This one and its duplicate are incorrectly setting the slippage parameter to 0 to exploit the first deposit.

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid