howlbot-integration[bot] commented 1 month ago

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

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

Tools Used

Manual Review & H-04 from Particle Leverage AMM - https://code4rena.com/reports/2023-12-particle#h-04-underflow-could-happened-when-calculating-uniswap-v3-positions-fee-growth-and-can-cause-operations-to-revert

Recommended Mitigation Steps

Use unsafe/unchecked math when calculating the fee growths.

Assessed type

Under/Overflow

thebrittfactor commented 1 month ago

Refer to sponsor comment here on the associated validation repo issue.

alex-ppg commented 1 month ago

The submission and its duplicates properly identify that the tick.rs file fails to permit underflows to occur when calculating fees which might result in inaccessible funds. I have personally observed this vulnerability in production, and I am inclined to agree with a high-risk severity rating as positions resulting in the underflow would effectively become permanently inaccessible.

To note, this submission and submission #143 are distinct due to being located in different files.

c4-judge commented 1 month ago

alex-ppg marked the issue as selected for report