Missing check if liquidity is not increasing when updating positions

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L94-L255 https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/position.rs#L41-L72

Vulnerability details

Impact

Missing check allows users to create positions where they never deposit and never add liquidity.

Proof of Concept

When an user adds liquidity, it will call adjust_position() which will calculate the delta and then call update_position() where all the calculations are done and the position is updated.

• pool.rs#L94-L255 - update_position()

  pub fn update_position(&mut self, id: U256, delta: i128) -> Result<(I256, I256), Revert> {
      // the pool must be enabled
      assert_or!(self.enabled.get(), Error::PoolDisabled);
  
      let position = self.positions.positions.get(id);
      let lower = position.lower.get().sys();
      let upper = position.upper.get().sys();
  
      // update the ticks
      let cur_tick = self.cur_tick.get().sys();
      let fee_growth_global_0 = self.fee_growth_global_0.get();
      let fee_growth_global_1 = self.fee_growth_global_1.get();
      let max_liquidity_per_tick = self.max_liquidity_per_tick.get().sys();
  
      #[cfg(feature = "testing-dbg")]
      dbg!((
          "inside update_position before delta check",
          delta,
          cur_tick,
          fee_growth_global_0,
          fee_growth_global_1,
      ));
  
      let mut flipped_lower = false;
      let mut flipped_upper = false;
  
      if delta != 0 {
          flipped_lower = self.ticks.update(
              lower,
              cur_tick,
              delta,
              &fee_growth_global_0,
              &fee_growth_global_1,
              false,
              max_liquidity_per_tick,
          )?;
  
          flipped_upper = self.ticks.update(
              upper,
              cur_tick,
              delta,
              &fee_growth_global_0,
              &fee_growth_global_1,
              true,
              max_liquidity_per_tick,
          )?;
  
          #[cfg(feature = "testing-dbg")]
          dbg!(("before if flip lower", flipped_lower, flipped_upper));
  
          // clear unneeded storage
          if flipped_lower {
              self.tick_bitmap.flip(lower, self.tick_spacing.get().sys());
          }
          if flipped_upper {
              self.tick_bitmap.flip(upper, self.tick_spacing.get().sys());
          }
      }
  
      // update the position
      let (fee_growth_inside_0, fee_growth_inside_1) = self.ticks.get_fee_growth_inside(
          lower,
          upper,
          self.cur_tick.get().sys(),
          &self.fee_growth_global_0.get(),
          &self.fee_growth_global_1.get(),
      )?;
  
      self.positions
          .update(id, delta, fee_growth_inside_0, fee_growth_inside_1)?;
  
      if delta < 0 {
          if flipped_lower {
              self.ticks.clear(lower);
          }
          if flipped_upper {
              self.ticks.clear(upper);
          }
      }
  
      // calculate liquidity change and the amount of each token we need
      if delta != 0 {
          let (amount_0, amount_1) = if self.cur_tick.get().sys() < lower {
              #[cfg(feature = "testing-dbg")]
              dbg!((
                  "update_position, cur_tick < lower path",
                  lower,
                  upper,
                  tick_math::get_sqrt_ratio_at_tick(lower)?,
                  tick_math::get_sqrt_ratio_at_tick(upper)?,
                  self.sqrt_price.get(),
                  delta
              ));
  
              // we're below the range, we need to move right, we'll need more token0
              (
                  sqrt_price_math::get_amount_0_delta(
                      tick_math::get_sqrt_ratio_at_tick(lower)?,
                      tick_math::get_sqrt_ratio_at_tick(upper)?,
                      delta,
                  )?,
                  I256::zero(),
              )
          } else if self.cur_tick.get().sys() < upper {
              // we're inside the range, the liquidity is active and we need both tokens
              let new_liquidity = liquidity_math::add_delta(self.liquidity.get().sys(), delta)?;
  
              #[cfg(feature = "testing-dbg")]
              dbg!((
                  "update_position, cur_tick < upper path",
                  lower,
                  upper,
                  tick_math::get_sqrt_ratio_at_tick(lower)?.to_string(),
                  tick_math::get_sqrt_ratio_at_tick(upper)?.to_string(),
                  self.sqrt_price.get().to_string(),
                  delta,
                  self.liquidity.get().sys(),
                  new_liquidity
              ));
  
              self.liquidity.set(U128::lib(&new_liquidity));
  
              (
                  sqrt_price_math::get_amount_0_delta(
                      self.sqrt_price.get(),
                      tick_math::get_sqrt_ratio_at_tick(upper)?,
                      delta,
                  )?,
                  sqrt_price_math::get_amount_1_delta(
                      tick_math::get_sqrt_ratio_at_tick(lower)?,
                      self.sqrt_price.get(),
                      delta,
                  )?,
              )
          } else {
              #[cfg(feature = "testing-dbg")]
              dbg!((
                  "update_position, else",
                  lower,
                  upper,
                  tick_math::get_sqrt_ratio_at_tick(lower)?,
                  tick_math::get_sqrt_ratio_at_tick(upper)?,
                  self.sqrt_price.get(),
                  delta,
              ));
  
              // we're above the range, we need to move left, we'll need token1
              (
                  I256::zero(),
                  sqrt_price_math::get_amount_1_delta(
                      tick_math::get_sqrt_ratio_at_tick(lower)?,
                      tick_math::get_sqrt_ratio_at_tick(upper)?,
                      delta,
                  )?,
              )
          };
  
          Ok((amount_0, amount_1))
      } else {
          Ok((I256::zero(), I256::zero()))
      }
  }

But it is missing a check, if the delta is 0 and always increase the liquidity with delta.

position.rs#L41-L72

/// Updates a position, refreshing the amount of fees the position has earned and updating its
/// liquidity.
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));
    }

This check is present in the Uniswap code and prevents the initiation of a position increase when the liquidity of the position does not change (ie adding 0 as delta).

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

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

    ... MORE CODE

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

Add a check preventing keeping position.liquidity = 0 (the code suggestion is not complete, of course).

/// Updates a position, refreshing the amount of fees the position has earned and updating its
/// liquidity.
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,
    )?;

+       if delta == 0 && info.liquidity.get().sys() == 0 {
+           revert
+       }

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

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

Assessed type

Math