c4-bot-8 commented 9 months ago

Lines of code

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

Vulnerability details

Impact

An attacker can steal funds which first depositor added with sandwich-attack. Generally, when a user adds liquidity to the pool, tokens are deposited as current rate.
But in case of first deposit, full amount of tokens provided by user is deposited and pool's rate becomes the rate in which the user deposited.
By using this, an attacker can steal funds from first depositor with sandwich-attack.
An attacker who has been looking pending transactions deposits little funds as large deviated rate from Uniswap with front-running before the user's deposit. Then, user's deposition transaction is run and pool's liquidity becomes much larger but pool's rate keeps large deviated from real.
Then, the attacker adds transaction to swap and he can acquire much benefits from user's funds.

Proof of Concept

Liquidity.sol#_depositLiquidityAndIncreaseShare function which is called when a user adds liquidity to the pool is as follows.

    function _depositLiquidityAndIncreaseShare( IERC20 tokenA, IERC20 tokenB, uint256 maxAmountA, uint256 maxAmountB, uint256 minLiquidityReceived, bool useZapping ) internal returns (uint256 addedAmountA, uint256 addedAmountB, uint256 addedLiquidity)
        {
        ...

        // Deposit the specified liquidity into the Pools contract
        // The added liquidity will be owned by this contract. (external call to Pools contract)
        bytes32 poolID = PoolUtils._poolID( tokenA, tokenB );
102@>   (addedAmountA, addedAmountB, addedLiquidity) = pools.addLiquidity( tokenA, tokenB, maxAmountA, maxAmountB, minLiquidityReceived, totalShares[poolID]);

        // Increase the user's liquidity share by the amount of addedLiquidity.
        // Cooldown is specified to prevent reward hunting (ie - quickly depositing and withdrawing large amounts of liquidity to snipe rewards as they arrive)
        // _increaseUserShare confirms the pool as whitelisted as well.
        _increaseUserShare( msg.sender, poolID, addedLiquidity, true );

        ...
        }

Pools.sol#addLiquidity function which is called on L102 is as follows.

    function addLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 maxAmountA, uint256 maxAmountB, uint256 minLiquidityReceived, uint256 totalLiquidity ) external nonReentrant returns (uint256 addedAmountA, uint256 addedAmountB, uint256 addedLiquidity)
        {
        ...
        if ( flipped )
153         (addedAmountB, addedAmountA, addedLiquidity) = _addLiquidity( poolID, maxAmountB, maxAmountA, totalLiquidity );
        else
155         (addedAmountA, addedAmountB, addedLiquidity) = _addLiquidity( poolID, maxAmountA, maxAmountB, totalLiquidity );

        ...
        }

The _addLiquidity() function which is called on L153, 155 is as follows.

    function _addLiquidity( bytes32 poolID, uint256 maxAmount0, uint256 maxAmount1, uint256 totalLiquidity ) internal returns(uint256 addedAmount0, uint256 addedAmount1, uint256 addedLiquidity)
        {
        PoolReserves storage reserves = _poolReserves[poolID];
        uint256 reserve0 = reserves.reserve0;
        uint256 reserve1 = reserves.reserve1;

        // If either reserve is zero then consider the pool to be empty and that the added liquidity will become the initial token ratio
        if ( ( reserve0 == 0 ) || ( reserve1 == 0 ) )
            {
            // Update the reserves
            reserves.reserve0 += uint128(maxAmount0);
            reserves.reserve1 += uint128(maxAmount1);

            // 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) );
            }

        // 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;
            addedAmount1 = proportionalB;
            }

        // Update the reserves
        reserves.reserve0 += uint128(addedAmount0);
        reserves.reserve1 += uint128(addedAmount1);

        // Determine the amount of liquidity that will be given to the user to reflect their share of the total collateralAndLiquidity.
        // Use whichever added amount was larger to maintain better numeric resolution.
        // Rounded down in favor of the protocol.
        if ( addedAmount0 > addedAmount1)
            addedLiquidity = (totalLiquidity * addedAmount0) / reserve0;
        else
            addedLiquidity = (totalLiquidity * addedAmount1) / reserve1;
        }

As we can see above, full amount of tokens is deposited in first deposit and then tokens are deposited as rate of current reserve.
By using this, an attacker can steal funds with sandwiching first deposit.

For example,
We say that a user deposits first to [WBTC/WETH] pool and the current rate in Uniswap is 1:1000. And we assume that the user deposits [100e18/100_000e18].
An attacker deposits [1e18/1000] with front-running before the user. Then, current rate of the pool becomes [1e15:1] so the user deposits only [100e18/100_000] and remained eth is refunded.
Then, the attacker adds transaction to swap 100_000 wei in this pool and steals most amount of btc.

Tools Used

Manual Review

Recommended Mitigation Steps

We have to modify logic for adding liquidity so that a user sets minimum and maximum rate in which funds are deposited, and in case of out-of-range it has to be reverted.

Assessed type

MEV

c4-judge commented 9 months ago

Picodes marked the issue as primary issue

c4-judge commented 9 months ago

Picodes marked issue #221 as primary and marked this issue as a duplicate of 221