Position is not updated when fees are collected

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/position.rs#L43-L93
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/lib.rs#L688-L727

Vulnerability details

Impact

When fees for LP positions are being collected they are not being updated and owners should either increase or decrease them in order to get their latest fee accrued.

Proof of Concept

LP position fees (token_owed_0 and token_owed_1) are being updated only when lib::update_position_internal is being called:

lib.rs

pub fn update_position_internal(
    &mut self,
    pool: Address,
    id: U256,
    delta: i128,
    permit2: Option<(Permit2Args, Permit2Args)>,
) -> Result<(I256, I256), Revert> {
    assert_eq_or!(
        msg::sender(),
        self.position_owners.get(id),
        Error::PositionOwnerOnly
    );

    let (token_0, token_1) = self.pools.setter(pool).update_position(id, delta)?;

    #[cfg(feature = "testing-dbg")]
    dbg!(("update position taking", current_test!(), token_0, token_1));

    if delta < 0 {
        erc20::transfer_to_sender(pool, token_0.abs_neg()?)?;
        erc20::transfer_to_sender(FUSDC_ADDR, token_1.abs_neg()?)?;
    } else {
        let (permit_0, permit_1) = match permit2 {
            Some((permit_0, permit_1)) => (Some(permit_0), Some(permit_1)),
            None => (None, None),
        };

        erc20::take(pool, token_0.abs_pos()?, permit_0)?;
        erc20::take(FUSDC_ADDR, token_1.abs_pos()?, permit_1)?;
    }

    #[cfg(feature = "log-events")]
    evm::log(events::UpdatePositionLiquidity {
        id,
        token0: token_0,
        token1: token_1,
    });

    Ok((token_0, token_1))
}

And only update_position_internal function calls position::update. That means owners can’t just collect their fees but they always have modify their position, either by adding or removing liquidity in order to have their position updated:

position.rs

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(())
    }

Owners can’t just call lib::collect_single_to_6_D_76575_F and lib::collect_7_F21947_C because they only process the transfers and doesn’t update the position:

lib.rs

pub fn collect_single_to_6_D_76575_F(
    &mut self,
    pool: Address,
    id: U256,
    recipient: Address,
) -> Result<(u128, u128), Revert> {
    assert_eq_or!(
        msg::sender(),
        self.position_owners.get(id),
        Error::PositionOwnerOnly
    );

    let res = self.pools.setter(pool).collect(id)?;
    let (token_0, token_1) = res;

    #[cfg(feature = "log-events")]
    evm::log(events::CollectFees {
        id,
        pool,
        to: msg::sender(),
        amount0: token_0,
        amount1: token_1,
    });

    erc20::transfer_to_addr(pool, recipient, U256::from(token_0))?;
    erc20::transfer_to_addr(FUSDC_ADDR, recipient, U256::from(token_1))?;

    Ok(res)
}

pub fn collect_7_F21947_C(
    &mut self,
    pools: Vec<Address>,
    ids: Vec<U256>,
) -> Result<Vec<(u128, u128)>, Revert> {
    assert_eq!(ids.len(), pools.len());

    pools
        .iter()
        .zip(ids.iter())
        .map(|(&pool, &id)| self.collect_single_to_6_D_76575_F(pool, id, msg::sender()))
        .collect::<Result<Vec<(u128, u128)>, Revert>>()
}

Basically, collect_7_F21947_C will become ineffective due to the fact that the owner should update all the IDs manually. The bigger impact will be when a position is entirely closed, unsuspecting users will lose all the fees because they won’t be accrued when collect is executed along with a decrease and then burn position, which leaves any liquidity or fees left in the position inaccessible.

For reference, let’s check how collect is implemented in UniswapV3:

NFPM.sol

function collect(CollectParams calldata params)
      external
      payable
      override
      isAuthorizedForToken(params.tokenId)
      returns (uint256 amount0, uint256 amount1)
  {
      // trigger an update of the position fees owed and fee growth snapshots if it has any liquidity
      if (position.liquidity > 0) {
          pool.burn(position.tickLower, position.tickUpper, 0);
          (, uint256 feeGrowthInside0LastX128, uint256 feeGrowthInside1LastX128, , ) =
              pool.positions(PositionKey.compute(address(this), position.tickLower, position.tickUpper));

          tokensOwed0 += uint128(
              FullMath.mulDiv(
                  feeGrowthInside0LastX128 - position.feeGrowthInside0LastX128,
                  position.liquidity,
                  FixedPoint128.Q128
              )
          );
          tokensOwed1 += uint128(
              FullMath.mulDiv(
                  feeGrowthInside1LastX128 - position.feeGrowthInside1LastX128,
                  position.liquidity,
                  FixedPoint128.Q128
              )
          );

          position.feeGrowthInside0LastX128 = feeGrowthInside0LastX128;
          position.feeGrowthInside1LastX128 = feeGrowthInside1LastX128;
      }

      // compute the arguments to give to the pool#collect method
      (uint128 amount0Collect, uint128 amount1Collect) =
          (
              params.amount0Max > tokensOwed0 ? tokensOwed0 : params.amount0Max,
              params.amount1Max > tokensOwed1 ? tokensOwed1 : params.amount1Max
          );

      // the actual amounts collected are returned
      (amount0, amount1) = pool.collect(
          recipient,
          position.tickLower,
          position.tickUpper,
          amount0Collect,
          amount1Collect
      );

      // sometimes there will be a few less wei than expected due to rounding down in core, but we just subtract the full amount expected
      // instead of the actual amount so we can burn the token
      (position.tokensOwed0, position.tokensOwed1) = (tokensOwed0 - amount0Collect, tokensOwed1 - amount1Collect);

      emit Collect(params.tokenId, recipient, amount0Collect, amount1Collect);
  }

Only the relevant:

1. Burn is called with tickLower, tickUpper, and amount = 0, which is the core functionality that is missing in Superposition.
2. tokensOwed0 and tokensOwed1, which are the fees for that position, is increased representing the latest fee for that position, this is also missing here, it’s only done in update_position_internal
3. The same collect as here is called last, which transfers all the fees up-to-date.

Tools Used

Manual Review

Recommended Mitigation Steps

Call update_position_internal with 0 in order to perform taken the latest fee per position and avoid losing them.

Assessed type

Uniswap

[blckhv]

Hello @alex-ppg,

I would like to ask you to revisit this issue because here we explain that LPs have no way to receive their up-to-date fees but always have to first either increase or decrease their positions, which will also call position.update and after that collect their fees. This will be a problem because there is no way to know whether you have pending fees and in case you completely close your position without first updating it you will lose those fees.
We also explain that collect_7_F21947_C which updates positions in a batch will become ineffective since we first have to manually update each of the positions and then call that function.
For reference, we have shown how Uniswap's position manager first calls pool.burn and processes the latest fees.

Thank you for your efforts.

[alex-ppg]

Hey @blckhv, thank you for your PJQA contribution. I would like to clarify that validation repository findings are not directly evaluated by the judge of a contest if they do not pass the validation phase.

This particular finding was properly assessed as a non-HM vulnerability as the behavior outlined does not introduce any vulnerability to the system. This recommendation can be classified as a QA-level observation, however, no direct breach of contract functionality has been outlined.