Position's fee growth can revert resulting in funds permanently locked

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

To determine the fee growth for a position, a function similar to the one used by Uniswap V3 is used. The new logic differs from the original logic, as it does not allow underflows.

Due to this, certain operations that depend on fee growth calculations may not execute properly and could revert (removing and adding liquidity), resulting in locked funds.

Context

Similar to Position's owed fees should allow underflow but it reverts instead, resulting in locked funds but with a different function/root cause, both issues must be fixed separately.

Proof of Concept

In tick.get_fee_growth_inside, Superposition's fee logic does not allow underflows:

    //@audit I've removed the testing-dbg flags to make it easier to read
    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 {
            (
                lower.fee_growth_outside_0.get(),
                lower.fee_growth_outside_1.get(),
            )
        } else {
            (
                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 {
            (
                upper.fee_growth_outside_0.get(),
                upper.fee_growth_outside_1.get(),
            )
        } else {
            (
                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)?,
            )
        };

        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)?,
        ))
    }

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

while the original Uniswap version allows underflows:

https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/libraries/Tick.sol#L60-L95

The issue is that negative fees are expected due to how the formula works. It is explained in detail in this Uniswap's issue: https://github.com/Uniswap/v3-core/issues/573

This function is used every time a position is updated, so it will be impossible to remove funds from it when the underflow happens, resulting in locked funds.

Tools Used

Manual Review

Recommended Mitigation Steps

Use wrapping_sub instead, or a simple - operation, as it natively allows underflow in Rust release mode:

    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 {
            (
                lower.fee_growth_outside_0.get(),
                lower.fee_growth_outside_1.get(),
            )
        } else {
            (
                fee_growth_global_0
-                   .checked_sub(lower.fee_growth_outside_0.get())
-                   .ok_or(Error::FeeGrowthSubTick)?,
+                   - lower.fee_growth_outside_0.get(),
                fee_growth_global_1
-                   .checked_sub(lower.fee_growth_outside_1.get())
-                   .ok_or(Error::FeeGrowthSubTick)?,
+                   - lower.fee_growth_outside_1.get()
            )
        };

        let (fee_growth_above_0, fee_growth_above_1) = if cur_tick < upper_tick {
            (
                upper.fee_growth_outside_0.get(),
                upper.fee_growth_outside_1.get(),
            )
        } else {
            (
                fee_growth_global_0
-                   .checked_sub(upper.fee_growth_outside_0.get())
-                   .ok_or(Error::FeeGrowthSubTick)?,
+                   - upper.fee_growth_outside_0.get()
                fee_growth_global_1
-                   .checked_sub(upper.fee_growth_outside_1.get())
-                   .ok_or(Error::FeeGrowthSubTick)?,
+                   - upper.fee_growth_outside_1.get()
            )
        };

        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_below_0 - fee_growth_above_0
            fee_growth_global_1
-               .checked_sub(fee_growth_below_1)
-               .and_then(|x| x.checked_sub(fee_growth_above_1))
-               .ok_or(Error::FeeGrowthSubTick)?,
+               - fee_growth_below_1 - fee_growth_above_1
        ))
    }

Assessed type

Uniswap

[c4-judge]

alex-ppg marked the issue as satisfactory