get_next_sqrt_price_from_amount_1_rounding_down() uses overflowing_sub not making sure the value always fits into uint160

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

In the current functionality, get_next_sqrt_price_from_amount_1_rounding_down() uses overflowing_sub functionality that does not revert in the case of overflow making the function not always fit into uint160.

Proof of Concept

Take a look into get_next_sqrt_price_from_amount_1_rounding_down() functionality:

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

   if next_sqrt_price > MAX_U160 {
            Err(Error::SafeCastToU160Overflow)
        } else {
            Ok(next_sqrt_price)
        }

But at the end of the function overflowing_sub is used that will not revert if there is overflow:

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

Ok(sqrt_price_x_96.overflowing_sub(quotient).0)

The value sqrt_price_x_96 has to always fit into uint160.

Tools Used

Manual review.

Recommended Mitigation Steps

Change the last line on Ok(sqrt_price_x_96 - quotient)

Assessed type

Other

[c4-judge]

alex-ppg marked the issue as not a duplicate

[alex-ppg]

The submission details that an overflowing_sub might be insecure to utilize in the context referenced, however, the code will validate that the sqrt_price_x_96 value is greater than the quotient right before executing the subtraction rendering it safe.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid

[rodiontr]

I think the issue should be reconsidered for the following reasoning:

1. The issue uses overflowing sub and does not ensure that the value fits into 160 bits
2. There is an example of a Uniswap Math Rust library where this is correctly implemented:

            // always fits 160 bits
            return uint160(sqrtPX96 - quotient);

So usage of such overflowing_sub functionality is unsafe as it does not ensure that the value fits into 160 bits.

3. It's said on the contest page that one of the main invariants of the protocol is compatibility with UniV3 library:

We should follow Uniswap V3's math faithfully.

So any deviation from it basically means that the invariant is broken