c4-bot-10 commented 1 month ago

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/maths/sqrt_price_math.rs#L252

Vulnerability details

Impact

The function get_amounts_for_delta() at the sqrt_price_math.rs contract is used to compute the quantity of token0 and token1 to add to the position given a delta of liquidity. These quantities depend on the delta of liquidity, the current tick and the ticks of the range boundaries. Actually, get_amounts_for_delta() uses the sqrt prices instead of the ticks, but they are equivalent since each tick represents a sqrt price.

There exists 3 cases: The current tick is outside the range from the left, this means only token0 should be added. The current tick is within the range, this means both token0 and token1 should be added. The current tick is outside the range from the right, this means only token1 should be added.

When the current price is equal to the left boundary of the range, the uniswap pool will request both token0 and token1, but this implementation will only request from the user token0 so the pool will lose some token1 if it has enough to cover it.

Reference finding: https://github.com/sherlock-audit/2023-06-arrakis-judging/issues/8

Proof of concept

If we look at the function get_amounts_for_delta() :

pub fn get_amounts_for_delta(
    sqrt_ratio_x_96: U256,
    mut sqrt_ratio_a_x_96: U256,
    mut sqrt_ratio_b_x_96: U256,
    liquidity: i128,
) -> Result<(I256, I256), Error> {
    if sqrt_ratio_a_x_96 > sqrt_ratio_b_x_96 {
        (sqrt_ratio_a_x_96, sqrt_ratio_b_x_96) = (sqrt_ratio_b_x_96, sqrt_ratio_a_x_96)
    };
@>   Ok(if sqrt_ratio_x_96 <= sqrt_ratio_a_x_96 {
        (
            get_amount_0_delta(sqrt_ratio_a_x_96, sqrt_ratio_b_x_96, liquidity)?,
            I256::ZERO,
        )

    } else if sqrt_ratio_x_96 < sqrt_ratio_b_x_96 {
        (
            get_amount_0_delta(sqrt_ratio_x_96, sqrt_ratio_b_x_96, liquidity)?,
            get_amount_1_delta(sqrt_ratio_a_x_96, sqrt_ratio_x_96, liquidity)?,
        )
    } else {
        (
            I256::ZERO,
            get_amount_1_delta(sqrt_ratio_a_x_96, sqrt_ratio_b_x_96, liquidity)?,
        )
    })
}

The implementation says that if the current price is equal to the price of the lower tick, it means that it is outside of the range and hence only token0 should be added to the position.

But for the UniswapV3 implementation, the current price must be lower in order to consider it outside:

if (_slot0.tick < params.tickLower) {
   // current tick is below the passed range; liquidity can only become in range by crossing from left to
   // right, when we'll need _more_ token0 (it's becoming more valuable) so user must provide it
   amount0 = SqrtPriceMath.getAmount0Delta(
          TickMath.getSqrtRatioAtTick(params.tickLower),
          TickMath.getSqrtRatioAtTick(params.tickUpper),
          params.liquidityDelta
    );
}

Tools Used

Manual Review

Recommendation

We recommend to change to the following:

pub fn get_amounts_for_delta(
    sqrt_ratio_x_96: U256,
    mut sqrt_ratio_a_x_96: U256,
    mut sqrt_ratio_b_x_96: U256,
    liquidity: i128,
) -> Result<(I256, I256), Error> {
    if sqrt_ratio_a_x_96 > sqrt_ratio_b_x_96 {
        (sqrt_ratio_a_x_96, sqrt_ratio_b_x_96) = (sqrt_ratio_b_x_96, sqrt_ratio_a_x_96)
    };
+    Ok(if sqrt_ratio_x_96 < sqrt_ratio_a_x_96 {
-   Ok(if sqrt_ratio_x_96 <= sqrt_ratio_a_x_96 {
        (
            get_amount_0_delta(sqrt_ratio_a_x_96, sqrt_ratio_b_x_96, liquidity)?,
            I256::ZERO,
        )
    })
    ...
    ...

}

Assessed type

Uniswap

af-afk commented 4 weeks ago

https://github.com/fluidity-money/long.so/commit/7132f9643052b5fcd416c4fc1058a547cdf894f7