Mis-configured ticks are allowed in positions due to insufficient checks, all subsequent swaps can be incorrect

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/seawater/src/pool.rs#L83-L84

Vulnerability details

Impact

Mis-configured ticks are allowed in positions due to insufficient checks, all subsequent swaps can be incorrect.

Proof of Concept

Positions are created through mint_position() -> create_position(). The problem is in create_position() there are insufficient checks to ensure lower tick < upper tick.

//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);
        //@audit insufficient check - low can still be greater than up
|>      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(())
    }

(https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/seawater/src/pool.rs#L83-L84)

Suppose a user minted a position with a flipped lower and upper tick. e.g. (low: 20, up: 10).

When the user adds liquidity to the position, incorrect liquidity_net will be updated in the tick info. In pool::update_position() -> ticks::update(), when the tick is an upper tick, added liquidity(liquidity_delta) should be subtracted from current tick info's liquidity_net because liquidity is removed when the upper tick is crossed from left to right. However, when lower/upper tick flag is flipped, liquidity_net for the upper tick will be added with liquidity_delta, which is incorrect.

//pkg/seawater/src/tick.rs
    pub fn update(
        &mut self,
        tick: i32,
        cur_amm_tick: i32,
        liquidity_delta: i128,
        fee_growth_global_0: &U256,
        fee_growth_global_1: &U256,
        upper: bool,
        max_liquidity: u128,
    ) -> Result<bool, Error> {
...
        //@audit when a position's boundary tick flag is flipped (low: 20, up: 10), the upper tick(20) will be added liquitidy_delta instead of subtraction. flipped tick => the tick info's liquidity_net will be incorrect.
        let new_liquidity_net = match upper {
            true => info
                .liquidity_net
                .get()
|>              .checked_sub(I128::lib(&liquidity_delta))
                .ok_or(Error::LiquiditySub),
            false => info
                .liquidity_net
                .get()
|>              .checked_add(I128::lib(&liquidity_delta))
                .ok_or(Error::LiquidityAdd),
        }?;

        info.liquidity_net.set(new_liquidity_net);
...

(https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/seawater/src/tick.rs#L106-L116)

When a tick info's liquidity_net is incorrect, any swap that crosses the tick will be using incorrect liquidity, resulting in incorrect swap amount calculation. In pool::swap, when reaching the next initialized tick, the swap liquidity (state.liquidity) will be modified with based on the crossed tick's liquidity_net. Incorrect cross tick's liquidity_net => incorrect state.liquidity => incorrect swap_math::compute_swap_step in the next step swap.

//pkg/seawater/src/pool.rs
    pub fn swap(
        &mut self,
        zero_for_one: bool,
        amount: I256,
        mut price_limit: U256,
    ) -> Result<(I256, I256, i32), Revert> {
...
        while !state.amount_remaining.is_zero() && state.price != price_limit {
...
            // find the next tick based on which direction we're swapping
            let (step_next_tick, step_next_tick_initialised) =
                tick_bitmap::next_initialized_tick_within_one_word(
                    &self.tick_bitmap.bitmap,
                    state.tick,
                    self.tick_spacing.get().sys().into(),
                    zero_for_one,
                )?;
...
            // step_fee_amount is reduced by protocol fee later
            let (next_sqrt_price, step_amount_in, step_amount_out, mut step_fee_amount) =
                swap_math::compute_swap_step(
                    state.price,
                    step_clamped_price,
                    state.liquidity,
                    state.amount_remaining,
                    fee,
                )?;
...
            // shift tick
            if state.price == step_next_price {
                if step_next_tick_initialised {
                    let (fee_0, fee_1) = match zero_for_one {
                        true => (state.fee_growth_global, self.fee_growth_global_1.get()),
                        false => (self.fee_growth_global_0.get(), state.fee_growth_global),
                    };
                    //@audit when tick's info's liquidity_net is incorrect, state.liquidity for the next step swap will be incorrect.
|>                  let liquidity_net = self.ticks.cross(step_next_tick, &fee_0, &fee_1);

                    // flip the liquidity delta if we're moving leftwards
                    let liquidity_net = match zero_for_one {
                        true => liquidity_net.wrapping_neg(),
                        false => liquidity_net,
                    };

|>                  state.liquidity = liquidity_math::add_delta(state.liquidity, liquidity_net)?;
                }

                state.tick = match zero_for_one {
                    true => step_next_tick - 1,
                    false => step_next_tick,
                };
...

(https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/seawater/src/pool.rs#L473)

As seen above, state.liquidity is accumulated incorrectly and used for all subsequent swaps. So if one position has flipped lower/upper tick flags, all swaps can be impacted - incorrect token delta amount calculation when a swap crosses the positions' tick boundary.

See add unit test test_minting_positions_with_flipped_ticks:

//pkg/seawater/tests/lib.rs
#[test]
fn test_minting_positions_with_flipped_ticks() -> Result<(), Vec<u8>> {
    test_utils::with_storage::<_, Pools, _>(
        Some(address!("feb6034fc7df27df18a3a6bad5fb94c0d3dcb6d5").into_array()),
        None, // slots map
        None, // caller erc20 balances
        None, // amm erc20 balances
        |contract| {
            // Create the storage
            contract.ctor(msg::sender(), Address::ZERO, Address::ZERO)?;
            let token_addr = address!("97392C28f02AF38ac2aC41AF61297FA2b269C3DE");

            // First, we set up the pool.
            contract.create_pool_D650_E2_D0(
                token_addr,
                test_utils::encode_sqrt_price(50, 1), // the price
                0,
                1,
                100000000000,
            )?;

            contract.enable_pool_579_D_A658(token_addr, true)?;
            //tick is flipped
            let lower_tick = test_utils::encode_tick(150);
            let upper_tick = test_utils::encode_tick(50);
            let liquidity_delta = 20000;

            // Begin to create the position, following the same path as
            // in `createPosition` in ethers-tests/tests.ts
            contract.mint_position_B_C5_B086_D(token_addr, lower_tick, upper_tick)?;
            let position_id = contract
                .next_position_id
                .clone()
                .checked_sub(U256::one())
                .unwrap();

            contract.update_position_C_7_F_1_F_740(token_addr, position_id, liquidity_delta)?;

            let tick_lower = contract.position_tick_lower_2_F_77_C_C_E_1(token_addr, position_id);
            let tick_upper = contract.position_tick_upper_67_F_D_55_B_A(token_addr, position_id);
            //check the position has flipped ticks
            assert_eq!(tick_lower > tick_upper, true);
            Ok(())
        },
    )
}

Test results:

     Running tests/lib.rs (target/debug/deps/lib-b27e97df8f1d4fcd)

running 1 test
test test_minting_positions_with_flipped_ticks ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 18 filtered out; finished in 0.00s

Tools Used

Manual, vscode

Recommended Mitigation Steps

In create_position(), adding a check to ensure lower < upper.

Assessed type

Other

[c4-judge]

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

[c4-judge]

alex-ppg marked the issue as satisfactory