position.update fees operations must be in unchecked

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

overflow/underflow when updating the position fees is prevented, which may give wrong values.

Proof of Concept

When the position is updated, owed_fees are updated, but all operations (add and subtract) use functions that prevent overflow/underflow (eg checked_sub, wrapping_add).

position.rs#L43-L66

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

https://docs.rs/alloy-primitives/0.7.6/alloy_primitives/aliases/type.U256.html#method.checked_sub

/// Computes `self - rhs`, returning [`None`] if overflow occurred.
#[inline(always)]
#[must_use]
pub const fn checked_sub(self, rhs: Self) -> Option<Self> {
    match self.overflowing_sub(rhs) {
        (value, false) => Some(value),
        _ => None,
    }
}

But this is not the case, how this is done in the Uniswap’s Position library, there all these operations are in an unchecked block, since overflow and underflow should not prevent these operations and are expected.

https://github.com/Uniswap/v3-core/blob/6562c52e8f75f0c10f9deaf44861847585fc8129/contracts/libraries/Position.sol#L66-L91

function update(
    Info storage self,
    int128 liquidityDelta,
    uint256 feeGrowthInside0X128,
    uint256 feeGrowthInside1X128
) internal {
    Info memory _self = self;

    uint128 liquidityNext;
    if (liquidityDelta == 0) {
        if (_self.liquidity <= 0) revert NP(); // disallow pokes for 0 liquidity positions
        liquidityNext = _self.liquidity;
    } else {
        liquidityNext = liquidityDelta < 0
            ? _self.liquidity - uint128(-liquidityDelta)
            : _self.liquidity + uint128(liquidityDelta);
    }

    // calculate accumulated fees. overflow in the subtraction of fee growth is expected
    uint128 tokensOwed0;
    uint128 tokensOwed1;
    unchecked {
        tokensOwed0 = uint128(
            FullMath.mulDiv(
                feeGrowthInside0X128 - _self.feeGrowthInside0LastX128,
                _self.liquidity,
                FixedPoint128.Q128
            )
        );
        tokensOwed1 = uint128(
            FullMath.mulDiv(
                feeGrowthInside1X128 - _self.feeGrowthInside1LastX128,
                _self.liquidity,
                FixedPoint128.Q128
            )
        );

        // update the position
        if (liquidityDelta != 0) self.liquidity = liquidityNext;
        self.feeGrowthInside0LastX128 = feeGrowthInside0X128;
        self.feeGrowthInside1LastX128 = feeGrowthInside1X128;
        if (tokensOwed0 > 0 || tokensOwed1 > 0) {
            // overflow is acceptable, user must withdraw before they hit type(uint128).max fees
            self.tokensOwed0 += tokensOwed0;
            self.tokensOwed1 += tokensOwed1;
        }
    }
}

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

Recommended Mitigation Steps

Move all these operations in unsafe/unchecked block.

Assessed type

Math

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #143

[c4-judge]

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

[c4-judge]

alex-ppg marked the issue as satisfactory