The get_amounts_for_delta() function at sqrt_price_math.rs is implemented incorrectly.

[howlbot-integration]

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: sherlock-audit/2023-06-arrakis-judging#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

[alex-ppg]

The submission has identified a flaw in the get_amounts_for_delta calculation that would result in token 1 funds not being acquired when they should if the square ratios match precisely via the equality case.

I believe a medium-risk severity rating is appropriate and the referenced finding provides great insight into properly assessing the severity of the issue.

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory

[Ys-Prakash]

Hi @alex-ppg

Thank you for judging this contest.

The get_amounts_for_delta() function is indeed correct. The issue says:

Actually, get_amounts_for_delta() uses the sqrt prices instead of the ticks, but they are equivalent since each tick represents a sqrt price

This is not correct. Uniswap v3 team published a blog post to describe the relationship between current_sqrt_price and current_tick.

The relation is as follows:

If the current_tick is ${i_c}$ then the current_sqrt_price, $P$, can be anywhere between $[1.0001^{i_c}, 1.0001^{i_c + 1})$, that is, [price_of_current_tick, price_of_one_tick_above). We would use this relation below.

The Uniswap v3 liquidity equations are:

This report talks about the edge of cases 1 and 2 where ${i_c} < {i_l}$, and, ${i_l} \leq {i_c} < {i_u}$.

Let us examine case-1 range ${i_c} < {i_l}$. This means that the maximum value of ${i_c}$, while being strictly lower than ${i_l}$, is ${i_l-1}$. For ${i_c} = {i_l-1}$, the current_sqrt_price, $P$, could be anywhere in the range $[1.0001^{i_l-1}, 1.0001^{i_l})$, that is, $P$ will be strictly lower than price of ${i_l}$ $(p({i_l}))$.

So, case-1 range ${i_c} < {i_l}$ corresponds to $P < p({i_l})$.

The case-2 range is ${i_l} \leq {i_c} < {i_u}$. This means that minimum value of ${i_c}$, while following this range, is ${i_l}$. For ${i_c} = {i_l}$, the current_sqrt_price, $P$, could be anywhere in the range $[1.0001^{i_l}, 1.0001^{i_l+1})$, that is, $P$ will be greater than or equal to the price of ${i_l}$ $(p({i_l}))$.

So, case-2 range ${i_l} \leq {i_c} < {i_u}$ corresponds to $p({i_l}) \leq P < p({i_u})$.

At the point when $P = p({i_l})$ any of the 2 equations, case-1 and 2, could be used as both formulas give the same result.

This is exactly what happens in get_amounts_for_delta():

Case-1 formula for $P \leq p({i_l})$.
Case-2 formula for $p({i_l}) < P < p({i_u})$.

The only difference that this report prescribes is the use of the case-2 formula when $P = p({i_l})$. This would not make any difference as at $P = p({i_l})$ both formulas give the same result.

Uniswap v3 uses ticks to separate the range for using different liquidity formulas. Superposition protocol uses sqrt prices. If one carefully converts the tick ordering to sqrt price ordering (with the relationship specified in this blog) then one would know that get_amounts_for_delta() function works fine.

At max, this issue deserves a QA level severity.

Thank you.

[DadeKuma]

Dear Judge,

I believe the issue referenced in the Sherlock contest was misjudged. The same issue was later submitted in a more recent contest on Code4rena, where another Judge reviewed and confirmed it to be invalid. I strongly recommend reviewing all the comments on the issue:
code-423n4/2024-03-revert-lend-findings#361 (comment).

To summarize, the issue is invalid because using <= or < produces the same result, as the branches are mathematically equivalent.

[alex-ppg]

Hey @Ys-Prakash and @DadeKuma, greatly appreciate your PJQA feedback! After reviewing all comments, assessing the call-chain of this particular finding, and evaluating the precedence set forth within C4 I am inclined to consider this submission as invalid due to the results of the function being mathematically equivalent regardless of how the branching logic is implemented.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid