Values are not casted into uint160 in multiple occasions in sqrt_price_math library

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

The current implementation of sqrt_price_math library deviates from the one from UniswapV3 as it does not check if the values fit in uint160 (they always should) as it's done in the SqrtPriceMath library of Uniswap.

Proof of Concept

Let's take a look into getNextSqrtPriceFromAmount0RoundingUp(). The formula function returns the price after adding or removing amount. Note that it returns uint256 instead of uint160 that's needed to be returned:

https://github.com/Uniswap/v3-core/blob/main/contracts/libraries/SqrtPriceMath.sol#L28-33

   function getNextSqrtPriceFromAmount0RoundingUp(
        uint160 sqrtPX96,
        uint128 liquidity,
        uint256 amount,
        bool add
    ) internal pure returns (uint160)

The values that are returned should always fit into uint160 as noted by Uniswap:

https://github.com/Uniswap/v3-core/blob/main/contracts/libraries/SqrtPriceMath.sol#L43

 // always fits in 160 bits

However, due to the protocol deviation from the standard implementation of this library, different kind of issues may arise as the value may not always fit in 160 bits:

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

pub fn get_next_sqrt_price_from_amount_0_rounding_up(
    sqrt_price_x_96: U256,
    liquidity: u128,
    amount: U256,
    add: bool,
) -> Result<U256, Error> 

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

return mul_div_rounding_up(numerator_1, sqrt_price_x_96, denominator);

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

 Ok(div_rounding_up(
            numerator_1,
            (numerator_1.wrapping_div(sqrt_price_x_96)).wrapping_add(amount),
        ))

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

   mul_div_rounding_up(numerator_1, sqrt_price_x_96, denominator)

Tools Used

Manual review.

Recommended Mitigation Steps

Always cast the returning value of this function to the uint160 type making sure it always fits into 160 bits.

Assessed type

Other

[c4-judge]

alex-ppg marked the issue as not a duplicate

[alex-ppg]

The submission claims that the return value of several Uniswap-V3 like functions may exceed the uint160 limit but fails to substantiate this claim with examples.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Insufficient proof

[rodiontr]

I believe this issue should be reconsidered for the following reasoning:

1. It's clearly shown that the value may not fit 160 bits in the current implementation of the function

2. One of the main impacts can be the fact that the algorithms in Uniswap's SqrtPriceMath library are carefully designed with uint160 limits in mind. Using larger values in the future by the protocol can produce incorrect results because the mathematical relationships may not hold outside the intended value range

3. It's said on the contest page that one of the main invariants is:

We should follow Uniswap V3's math faithfully.

This basically means that every deviation from the Uniswap spec should be considered as a broken invariant and therefore medium severity.

4. Overall, this would create a bad practice when users will not spend time on reporting such deviations if impact at the moment is seen as vague but can lead to catastrophic consequences for the protocol in the future when introducing new functionality. Moreover, the protocol confirmed that the compatibility with Uniswap has to be 1:1