`get_fee_growth_inside` in `tick.rs` should allow for `underflow`/`overflow` but doesn't

criticalCode4rena

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/seawater/src/tick.rs#L125-L246

Vulnerability details

Impact

When operations need to calculate the Superposition position's fee growth, it uses a similar function implemented by uniswap v3.

However, according to this known issue : Uniswap/v3-core#573. The contract implicitly relies on underflow/overflow when calculating the fee growth, if underflow is prevented, some operations that depend on fee growth will revert.

Proof of Concept

It can be observed that the current implementation of tick::get_fee_growth_inside does not allow underflow/overflow to happen when calculating fee_growth_inside_0 and fee_growth_inside_1 because the contract used checked math with additional overflow/underflow checks, which will Performs subtraction that returns None instead of wrapping around on underflow: tick.rs

rust
pub fn get_fee_growth_inside(
    &mut self,
    lower_tick: i32,
    upper_tick: i32,
    cur_tick: i32,
    fee_growth_global_0: &U256,
    fee_growth_global_1: &U256,
) -> Result<(U256, U256), Error> {
    // the fee growth inside this tick is the total fee
    // growth, minus the fee growth outside this tick
    let lower = self.ticks.get(lower_tick);
    let upper = self.ticks.get(upper_tick);

    let (fee_growth_below_0, fee_growth_below_1) = if cur_tick >= lower_tick {
        #[cfg(feature = "testing-dbg")]
        dbg!((
            "cur_tick >= lower_tick",
            current_test!(),
            lower.fee_growth_outside_0.get().to_string(),
            lower.fee_growth_outside_1.get().to_string()
        ));

        (
            lower.fee_growth_outside_0.get(),
            lower.fee_growth_outside_1.get(),
        )
    } else {
        #[cfg(feature = "testing-dbg")]
        dbg!((
            "cur_tick < lower_tick",
            current_test!(),
            fee_growth_global_0,
            fee_growth_global_1,
            lower.fee_growth_outside_0.get().to_string(),
            lower.fee_growth_outside_1.get().to_string()
        ));

        (
            fee_growth_global_0
                .checked_sub(lower.fee_growth_outside_0.get())
                .ok_or(Error::FeeGrowthSubTick)?,
            fee_growth_global_1
                .checked_sub(lower.fee_growth_outside_1.get())
                .ok_or(Error::FeeGrowthSubTick)?,
        )
    };

    let (fee_growth_above_0, fee_growth_above_1) = if cur_tick < upper_tick {
        #[cfg(feature = "testing-dbg")]
        dbg!((
            "cur_tick < upper_tick",
            current_test!(),
            upper.fee_growth_outside_0.get().to_string(),
            upper.fee_growth_outside_1.get().to_string()
        ));

        (
            upper.fee_growth_outside_0.get(),
            upper.fee_growth_outside_1.get(),
      )
    } else {
        #[cfg(feature = "testing-dbg")]
        dbg!((
            "cur_tick >= upper_tick",
            current_test!(),
            fee_growth_global_0,
            fee_growth_global_1,
            upper.fee_growth_outside_0.get(),
            upper.fee_growth_outside_1.get()
        ));

        (
            fee_growth_global_0
                .checked_sub(upper.fee_growth_outside_0.get())
                .ok_or(Error::FeeGrowthSubTick)?,
            fee_growth_global_1
                .checked_sub(upper.fee_growth_outside_1.get())
                .ok_or(Error::FeeGrowthSubTick)?,
        )
    };

    #[cfg(feature = "testing-dbg")] // REMOVEME
    {
        if *fee_growth_global_0 < fee_growth_below_0 {
            dbg!((
                "fee_growth_global_0 < fee_growth_below_0",
                current_test!(),
                fee_growth_global_0.to_string(),
                fee_growth_below_0.to_string()
            ));
        }
        let fee_growth_global_0 = fee_growth_global_0.checked_sub(fee_growth_below_0).unwrap();
        if fee_growth_global_0 < fee_growth_above_0 {
            dbg!((
                "fee_growth_global_0 < fee_growth_above_0",
                current_test!(),
                fee_growth_global_0.to_string(),
                fee_growth_above_0.to_string()
            ));
        }
    }

    #[cfg(feature = "testing-dbg")]
    dbg!((
        "final stage checked sub below",
        current_test!(),
        fee_growth_global_0
            .checked_sub(fee_growth_below_0)
            .and_then(|x| x.checked_sub(fee_growth_above_0))
    ));

    Ok((
        fee_growth_global_0
            .checked_sub(fee_growth_below_0)
            .and_then(|x| x.checked_sub(fee_growth_above_0))
            .ok_or(Error::FeeGrowthSubTick)?,
        fee_growth_global_1
            .checked_sub(fee_growth_below_1)
            .and_then(|x| x.checked_sub(fee_growth_above_1))
            .ok_or(Error::FeeGrowthSubTick)?,
    ))
}

Especially the last lines where we have:

fee_growth_global_0 - fee_growth_below_0 - fee_growth_above_0

and

fee_growth_global_1 - fee_growth_below_1 - fee_growth_above_1

In case either one of the calculation underflows we will receive Error::FeeGrowthSubTick instead of being wrapped and continue working.

Uniswap allows such underflows to happen because PositionLibrary works with Solidity version < 0.8. Otherwise, this will impact crucial operations that rely on this call, such as liquidation, and will revert unexpectedly. This behavior is quite often, especially for pools that use lower fees.

NOTE: Furthermore, one of the Main Invariants in the README mentioned that - Superposition should follow the UniswapV3 math faithfully, which is not the case here, violating the invariant. https://github.com/code-423n4/2024-08-superposition?tab=readme-ov-file#main-invariants