code-423n4 / 2024-01-salty-findings

11 stars 6 forks source link

Initial pool depositors can be frontrun to give cheap swaps at undesired rates #492

Closed c4-bot-10 closed 8 months ago

c4-bot-10 commented 9 months ago

Lines of code

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

Vulnerability details

Impact

When LP providers provide liquidity into a pool, the actual liquidity provided is calculated based on the current existing liquidity. Only 2 scenarios are possible here:

  1. The pool is empty
  2. The pool already has liquidity provided.

In the case that the pool is empty, an LP expects that his liquidity will be absorbed in full, therefore the pool will henceforth use that ratio from the provided token amounts, thus Pools:Ln97-105:

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

If the pool already has liquidity provided, the protocol needs to calculate how much of both tokens will be deposited, thus Pools:Ln97-105:

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

However, there's a possibility for an attacker to front-run a pool initial deposit that ends up with a favorable ratio for him to borrow/swap at low rates.

Proof of Concept

Attack scenario:

  1. Alice sends an initial pool deposit to initialize an empty pool at a ratio 1ETH:1800USD
  2. Bob notices Alice's transaction in the mempool.
  3. Using MEV, Bob sandwiches Alice's transaction with a frontrun that sets the token ratio at 1ETH:1700USD.
  4. Alice transaction is executed. Since the pool is already liquid, Alice tokens are deposited using Bob's token ratio.
  5. Bob can now swap at below-market rates from the pool.

Tools Used

Manual review

Recommended Mitigation Steps

Tp prevent initial pool deposit frontrun, the initial pool deposit should be included as part of the contract creation code.

Assessed type

MEV

c4-judge commented 9 months ago

Picodes marked the issue as duplicate of #527

c4-judge commented 8 months ago

Picodes marked the issue as partial-25

Picodes commented 8 months ago

This report doesn't discuss the slippage parameter

c4-judge commented 8 months ago

Picodes marked the issue as not a duplicate

c4-judge commented 8 months ago

Picodes marked the issue as duplicate of #937

c4-judge commented 8 months ago

Picodes marked the issue as unsatisfactory: Invalid

c4-judge commented 8 months ago

Picodes marked the issue as unsatisfactory: Invalid

c4-judge commented 8 months ago

Picodes marked the issue as unsatisfactory: Invalid