Fees can be locked for some time due to how collect is working

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/lib.rs#L611-L639 https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/position.rs#L107-L121

Vulnerability details

Impact

Position fee collection functionality always tries to take all the fees and will lead to prolonged periods with no collecting available and race conditions between users when trying to collect them.

Proof of Concept

The fee collection mechanism in Seawater always tries to send all the accumulated fees to the LP position owner and does not allow him to pass the desired amount he wants to receive to be able to sweep how many tokens there are available as fees:

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

pool.rs

pub fn collect(&mut self, id: U256) -> Result<(u128, u128), Revert> {
        assert_or!(self.enabled.get(), Error::PoolDisabled);
        Ok(self.positions.collect_fees(id))
    }

position.rs

 pub fn collect_fees(&mut self, id: U256) -> (u128, u128) {
        let mut position = self.positions.setter(id);

        let amount_0 = position.token_owed_0.get().sys();
        let amount_1 = position.token_owed_1.get().sys();

        if amount_0 > 0 {
            position.token_owed_0.set(U128::ZERO);
        }
        if amount_1 > 0 {
            position.token_owed_1.set(U128::ZERO);
        }

        (amount_0, amount_1)
    }

Because of that, there can be scenarios when certain users are unable to take their fees since there won’t be enough to be distributed amongst all the LPs.

Consider the following example: 3 LPs all have 5 tokens, accumulated as fees (token_owed_0 and token_owed_1) pending, but the available funds are only 14 tokens, so only 2 of the LPs will be able to harvest positions while the third user will be left to wait for another 1 tokens to be accumulated, although there are 4 tokens available.

This functionality differs from the UniswapV3, where users can specify how many tokens they want to receive knowing the balance of the pool:

NFPM.sol

// 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
      );

Furthermore, this mechanism is implemented in Superposition but only for the protocol fees, where the seawater admin can decide how many tokens he wants to receive:

pub fn collect_protocol(
        &mut self,
        amount_0: u128,
        amount_1: u128,
    ) -> Result<(u128, u128), Revert> {
        assert_or!(self.enabled.get(), Error::PoolDisabled);

        let owed_0 = self.protocol_fee_0.get().sys();
        let owed_1 = self.protocol_fee_1.get().sys();

        let amount_0 = u128::min(amount_0, owed_0);
        let amount_1 = u128::min(amount_1, owed_1);

        if amount_0 > 0 {
            self.protocol_fee_0.set(U128::lib(&(owed_0 - amount_0)));
        }
        if amount_1 > 0 {
            self.protocol_fee_1.set(U128::lib(&(owed_1 - amount_1)));
        }

        Ok((amount_0, amount_1))
    }

Especially in the pools with low liquidity and low trading activity, this will be a serious issue that will prevent attracting liquidity when LPs know that there can be scenarios when they won’t be able to claim fees.

Tools Used

Manual Review

Recommended Mitigation Steps

Add arguments that will allow the LPs to specify how many tokens they will collect, also change the implementation of the collect_fees to not zero the fees but subtract only the needed tokens that are being taken out.

Assessed type

Invalid Validation