Incorrect underflow handling in tick::get_fee_growth_inside() may prevent position modifications

[c4-bot-2]

Lines of code

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

Vulnerability details

Impact

tick::get_fee_growth_inside() can revert in certain situations due to not handling underflows correctly. As a result, some positions may become unmodifiable, causing users' funds to become stuck in the AMM.

Proof of Concept

Uniswap's function Tick.getFeeGrowthInside() is responsible for calculating fee growth before any position-modifying actions. In the Uniswap v3 code (below), we see that all subtraction operations are subject to underflow (Uniswap v3 was deployed using Solidity versions prior to 0.8.0, it lacks built-in protections against overflow and underflow)

function getFeeGrowthInside(
    mapping(int24 => Tick.Info) storage self,
    int24 tickLower,
    int24 tickUpper,
    int24 tickCurrent,
    uint256 feeGrowthGlobal0X128,
    uint256 feeGrowthGlobal1X128
) internal view returns (uint256 feeGrowthInside0X128, uint256 feeGrowthInside1X128) {
    Info storage lower = self[tickLower];
    Info storage upper = self[tickUpper];


    // calculate fee growth below
    uint256 feeGrowthBelow0X128;
    uint256 feeGrowthBelow1X128;
    if (tickCurrent >= tickLower) {
        feeGrowthBelow0X128 = lower.feeGrowthOutside0X128;
        feeGrowthBelow1X128 = lower.feeGrowthOutside1X128;
    } else {
        feeGrowthBelow0X128 = feeGrowthGlobal0X128 - lower.feeGrowthOutside0X128;
        feeGrowthBelow1X128 = feeGrowthGlobal1X128 - lower.feeGrowthOutside1X128;
    }


    // calculate fee growth above
    uint256 feeGrowthAbove0X128;
    uint256 feeGrowthAbove1X128;
    if (tickCurrent < tickUpper) {
        feeGrowthAbove0X128 = upper.feeGrowthOutside0X128;
        feeGrowthAbove1X128 = upper.feeGrowthOutside1X128;
    } else {
        feeGrowthAbove0X128 = feeGrowthGlobal0X128 - upper.feeGrowthOutside0X128;
        feeGrowthAbove1X128 = feeGrowthGlobal1X128 - upper.feeGrowthOutside1X128;
    }


    feeGrowthInside0X128 = feeGrowthGlobal0X128 - feeGrowthBelow0X128 - feeGrowthAbove0X128;
    feeGrowthInside1X128 = feeGrowthGlobal1X128 - feeGrowthBelow1X128 - feeGrowthAbove1X128;
}

In contrast, Seawater’s implementation of this function (tick::get_fee_growth_inside()) uses checked_sub for these operations, which causes the execution to revert if an underflow occurs. This presents a problem because the behavior of getFeeGrowthInside() relies on underflow to calculate fee growth. If underflow is prevented, as in Seawater’s version, valid fee growth calculations may revert. Consequently, some positions may become unmodifiable, potentially locking users' funds in the AMM.

Tools Used

Manual Review.

Recommended Mitigation Steps

To resolve this issue, consider replacing checked_sub with wrapping_sub. Additionally, positions::update() must also be modified to properly handle fee growth values.

(tick.rs)

diff --git a/tick.rs b/tick.mod.rs
index ad1c434..c8009ec 100644
--- a/tick.rs
+++ b/tick.mod.rs
@@ -161,10 +161,10 @@ impl StorageTicks {
 
             (
                 fee_growth_global_0
-                    .checked_sub(lower.fee_growth_outside_0.get())
+                    .wrapping_sub(lower.fee_growth_outside_0.get())
                     .ok_or(Error::FeeGrowthSubTick)?,
                 fee_growth_global_1
-                    .checked_sub(lower.fee_growth_outside_1.get())
+                    .wrapping_sub(lower.fee_growth_outside_1.get())
                     .ok_or(Error::FeeGrowthSubTick)?,
             )
         };
@@ -195,10 +195,10 @@ impl StorageTicks {
 
             (
                 fee_growth_global_0
-                    .checked_sub(upper.fee_growth_outside_0.get())
+                    .wrapping_sub(upper.fee_growth_outside_0.get())
                     .ok_or(Error::FeeGrowthSubTick)?,
                 fee_growth_global_1
-                    .checked_sub(upper.fee_growth_outside_1.get())
+                    .wrapping_sub(upper.fee_growth_outside_1.get())
                     .ok_or(Error::FeeGrowthSubTick)?,
             )
         };
@@ -235,12 +235,12 @@ impl StorageTicks {
 
         Ok((
             fee_growth_global_0
-                .checked_sub(fee_growth_below_0)
-                .and_then(|x| x.checked_sub(fee_growth_above_0))
+                .wrapping_sub(fee_growth_below_0)
+                .and_then(|x| x.wrapping_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))
+                .wrapping_sub(fee_growth_below_1)
+                .and_then(|x| x.wrapping_sub(fee_growth_above_1))
                 .ok_or(Error::FeeGrowthSubTick)?,
         ))
     }--

(position.rs)

diff --git a/position.rs b/position.mod.rs
index 51a2228..352d8e8 100644
--- a/position.rs
+++ b/position.mod.rs
@@ -51,7 +51,7 @@ impl StoragePositions {
 
         let owed_fees_0 = full_math::mul_div(
             fee_growth_inside_0
-                .checked_sub(info.fee_growth_inside_0.get())
+                .wrapping_sub(info.fee_growth_inside_0.get())
                 .ok_or(Error::FeeGrowthSubPos)?,
             U256::from(info.liquidity.get()),
             full_math::Q128,
@@ -59,7 +59,7 @@ impl StoragePositions {
 
         let owed_fees_1 = full_math::mul_div(
             fee_growth_inside_1
-                .checked_sub(info.fee_growth_inside_1.get())
+                .wrapping_sub(info.fee_growth_inside_1.get())
                 .ok_or(Error::FeeGrowthSubPos)?,
             U256::from(info.liquidity.get()),
             full_math::Q128,

Assessed type

Under/Overflow

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg changed the severity to 3 (High Risk)

[alex-ppg]

To note, this submission will be considered a duplicate of #143 as well.