Position's owed fees should allow underflow but it reverts instead, resulting in locked funds

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/seawater/src/position.rs#L52-L66

Vulnerability details

Impact

The math used to calculate how many fees are owed to the user does not allow underflows, but this is a necessary due to how the Uniswap logic works as fees can be negative. This impact adding/removing funds to a position, resulting in permanently locked funds due to a revert.

Context

Similar to Position's fee growth can revert resulting in funds permanently locked but with a different function/root cause, both issues must be fixed separately.

Proof of Concept

In position.update underflows are not allowed:

    pub fn update(
        &mut self,
        id: U256,
        delta: i128,
        fee_growth_inside_0: U256,
        fee_growth_inside_1: U256,
    ) -> Result<(), Error> {
        let mut info = self.positions.setter(id);

        let owed_fees_0 = full_math::mul_div(
            fee_growth_inside_0
->              .checked_sub(info.fee_growth_inside_0.get())
                .ok_or(Error::FeeGrowthSubPos)?,
            U256::from(info.liquidity.get()),
            full_math::Q128,
        )?;

        let owed_fees_1 = full_math::mul_div(
            fee_growth_inside_1
->              .checked_sub(info.fee_growth_inside_1.get())
                .ok_or(Error::FeeGrowthSubPos)?,
            U256::from(info.liquidity.get()),
            full_math::Q128,
        )?;

        let liquidity_next = liquidity_math::add_delta(info.liquidity.get().sys(), delta)?;

        if delta != 0 { 
            info.liquidity.set(U128::lib(&liquidity_next));
        }

        info.fee_growth_inside_0.set(fee_growth_inside_0);
        info.fee_growth_inside_1.set(fee_growth_inside_1);
        if !owed_fees_0.is_zero() {
            // overflow is the user's problem, they should withdraw earlier
            let new_fees_0 = info
                .token_owed_0
                .get()
                .wrapping_add(U128::wrapping_from(owed_fees_0));
            info.token_owed_0.set(new_fees_0);
        }
        if !owed_fees_1.is_zero() {
            let new_fees_1 = info
                .token_owed_1
                .get()
                .wrapping_add(U128::wrapping_from(owed_fees_1));
            info.token_owed_1.set(new_fees_1);
        }

        Ok(())
    }

https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/seawater/src/position.rs#L52-L66

While the original Uniswap version allows underflows:

    uint128 tokensOwed0 =
        uint128(
            FullMath.mulDiv(
                feeGrowthInside0X128 - _self.feeGrowthInside0LastX128,
                _self.liquidity,
                FixedPoint128.Q128
            )
        );
    uint128 tokensOwed1 =
        uint128(
            FullMath.mulDiv(
                feeGrowthInside1X128 - _self.feeGrowthInside1LastX128,
                _self.liquidity,
                FixedPoint128.Q128
            )
        );

https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/libraries/Position.sol#L61-L76

The issue is that negative fees are expected due to how the formula works. It is explained in detail in this Uniswap's issue:
Uniswap/v3-core#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 because it will revert, resulting in locked funds.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using - to let Rust underflow without errors in release mode:

        let owed_fees_0 = full_math::mul_div(
            fee_growth_inside_0
-               .checked_sub(info.fee_growth_inside_0.get())
-               .ok_or(Error::FeeGrowthSubPos)?,
+				- info.fee_growth_inside_0.get()
            U256::from(info.liquidity.get()),
            full_math::Q128,
        )?;

        let owed_fees_1 = full_math::mul_div(
            fee_growth_inside_1
-              .checked_sub(info.fee_growth_inside_1.get())
-              .ok_or(Error::FeeGrowthSubPos)?,
+			   - info.fee_growth_inside_1.get()
            U256::from(info.liquidity.get()),
            full_math::Q128,
        )?;

Alternatively, consider using wrapping_sub.

Assessed type

Uniswap

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as primary issue

[alex-ppg]

The submission and its duplicate properly identify that the position.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 #46 are distinct due to being located in different files.

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory