when minting position we can pass low tick > upper tick, unlike UniswapV3

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L75-L87

Vulnerability details

Impact

Missing check allows the user to create a position with a lowerTick > upperTick.

Proof of Concept

Position is created using mint_position_B_C5_B086_D() - https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/lib.rs#L495, and pool.create_position() - https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L75-L87.

📁 pkg/seawater/src/lib.rs
pub fn mint_position_B_C5_B086_D(
    &mut self,
    pool: Address,
    lower: i32,
    upper: i32,
) -> Result<U256, Revert> {
    let id = self.next_position_id.get();
    self.pools.setter(pool).create_position(id, lower, upper)?;

    self.next_position_id.set(id + U256::one());

    let owner = msg::sender();

    self.grant_position(owner, id);

    #[cfg(feature = "log-events")]
    evm::log(events::MintPosition {
        id,
        owner,
        pool,
        lower,
        upper,
    });

    Ok(id)
}

📁 pkg/seawater/src/pool.rs

pub fn create_position(&mut self, id: U256, low: i32, up: i32) -> Result<(), Revert> {
    assert_or!(self.enabled.get(), Error::PoolDisabled);
    let spacing = self.tick_spacing.get().sys() as i32;
    assert_or!(low % spacing == 0, Error::InvalidTickSpacing);
    assert_or!(up % spacing == 0, Error::InvalidTickSpacing);
    let spacing: u8 = spacing.try_into().map_err(|_| Error::InvalidTickSpacing)?;
    let min_tick = tick_math::get_min_tick(spacing);
    let max_tick = tick_math::get_max_tick(spacing);
    assert_or!(low >= min_tick && low <= max_tick, Error::InvalidTick);
    assert_or!(up >= min_tick && up <= max_tick, Error::InvalidTick);
    self.positions.new(id, low, up);
    Ok(())
}

As you can see, the lower and upper ticks are not checked if lower < upper, they are only checked if they are between min_tick and max_tick. Therefore, if min_tick = 0 and max_tick = 20, passing lower = 10 and upper = 5 will pass.

This should be checked when creating a position, as was done in Uniswap - https://github.com/Uniswap/v3-core/blob/6562c52e8f75f0c10f9deaf44861847585fc8129/contracts/UniswapV3Pool.sol#L316

function _modifyPosition(ModifyPositionParams memory params)
    private
    noDelegateCall
    returns (
        Position.Info storage position,
        int256 amount0,
        int256 amount1
    )
{
    checkTicks(params.tickLower, params.tickUpper); <------------------------

    ...
}

https://github.com/Uniswap/v3-core/blob/6562c52e8f75f0c10f9deaf44861847585fc8129/contracts/UniswapV3Pool.sol#L122-L126

function checkTicks(int24 tickLower, int24 tickUpper) private pure {
    if (tickLower >= tickUpper) revert TLU(); <------------------------
    if (tickLower < TickMath.MIN_TICK) revert TLM();
    if (tickUpper > TickMath.MAX_TICK) revert TUM();
}

This can lead to a lot of different problems, since it's something that might not even happen ever because UniswapV3Pool prevents it with this check, here are some problems we thought of:

• When a position is updated, there are different cases based on the current tick and tick limits (tickLower and tickUpper) and based on these, how much of each token (token0, token1) is added to the pool is calculated. pool.rs#L172-L245

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

  But if this is broken (i.e. lower > higher), token amount calculations will be completely wrong, resulting in incorrect liquidity additions.

• Since the price is based on the current tick and where it is positioned between the lower and upper ticks, this can allow liquidity to be added at a lower or higher price than usual.

• LPs can cheat this to only deposit one token (either token0 or token1) even if they need to deposit both, which if done will unbalance the pool and increase the price of the other token.

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 like in UniswapV3Pool:

  pub fn create_position(&mut self, id: U256, low: i32, up: i32) -> Result<(), Revert> {
      assert_or!(self.enabled.get(), Error::PoolDisabled);
      let spacing = self.tick_spacing.get().sys() as i32;
      assert_or!(low % spacing == 0, Error::InvalidTickSpacing);
      assert_or!(up % spacing == 0, Error::InvalidTickSpacing);
      let spacing: u8 = spacing.try_into().map_err(|_| Error::InvalidTickSpacing)?;
      let min_tick = tick_math::get_min_tick(spacing);
      let max_tick = tick_math::get_max_tick(spacing);
+     assert_or!(low < up, Error::InvalidTick);
      assert_or!(low >= min_tick && low <= max_tick, Error::InvalidTick);
      assert_or!(up >= min_tick && up <= max_tick, Error::InvalidTick);
      self.positions.new(id, low, up);
      Ok(())
  }

Assessed type

Uniswap