When performing 'swap' and the swap position does not cover 'swap amount', the base price of 'sqrt_price' is set incorrectly.

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

When performing a swap, a position is searched and if a position with an appropriate price exists, the swap is performed using the token amount of that position.

If the positions in the current pool do not cover the swap amount requested by the user, the value of sqrt_price will not be set accurately, which may cause confusion when the next user swaps.

Proof of Concept

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L303-L535
When a swap is performed, the 'swap' function in pools.rs is triggered.

pub fn swap(
        &mut self,
        zero_for_one: bool,
        amount: I256,
        mut price_limit: U256,
    ) -> Result<(I256, I256, i32), Revert> {
        assert_or!(self.enabled.get(), Error::PoolDisabled);

        ''' skip '''

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

        while !state.amount_remaining.is_zero() && state.price != price_limit {
            iters += 1;
            debug_assert!(iters < 500);
            ''' skip '''

When performing a swap, the while statement above iterates through the position that matches the tick and searches for it. If a matching position exists, the token of that position is processed and the state is updated.

The while statement will continue until the swap amount requested by the user becomes 0 or the price reaches the price_limit requested by the user.

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

        self.sqrt_price.set(state.price);

After the while loop is finished, the final sqrt_price value is updated to the state.price value.

If the swap amount within the price_limit range requested by the user is not covered by the positions in the pool, the position price will be set to the price_limit value set by the user, not the position price that was actually swapped last, which will be different from the last swapped price, and the user requesting the next swap may be confused about the swap price.

Scenario)

1. Assume that the following positions are currently formed in the pool
   
   pool1 state:
   lower = 1000
   upper = 2000
   amount = 20

   pool2 state:
   lower = 3000
   upper = 4000
   amount = 20

2. Assume that the user sets swap amount to 100 and price_limit to 10000 and requests a swap

3. When the swap is in progress, it searches for a pool that can be swapped and processes amount 20 of pool1 and amount 30 of pool2 to swap a total of 50 tokens.

   At this time, the position of the pool within the price_limit set by the user cannot process the swap amount requested by the user, so the while statement is performed until state.price == price_limit, and finally the sqrt_price value is set to the state.price value. (state.price is equal to price_limit)

4. After the swap routine is finished, the reference swap price will be set to 10000, which is the price_limit set by the user, and not the swap price of pool2 where the last swap was done.

5. Users requesting the next swap may experience confusion when processing the swap because the swap base price starts at the price of the price_limit set by the previous user, rather than the base price of the pool where the last swap was made.

Test Code

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

        match zero_for_one {
            true => {
                if price_limit == U256::MAX {
                    price_limit = tick_math::MIN_SQRT_RATIO + U256::one();
                }
                if price_limit >= self.sqrt_price.get() || price_limit <= tick_math::MIN_SQRT_RATIO
                {
                    Err(Error::PriceLimitTooLow)?;
                }
            }
            false => {
                if price_limit == U256::MAX {
                    price_limit = tick_math::MAX_SQRT_RATIO - U256::one();
                }
                if price_limit <= self.sqrt_price.get() || price_limit >= tick_math::MAX_SQRT_RATIO
                {
                    Err(Error::PriceLimitTooHigh)?;
                }
            }
        };

Test Code POC is the code that sets zero_for_one to true and price_limit to U256::MAX and returns an error return in the above conditional statement when the second swap is performed.

Write Test code

write Test Code in test/lib.rs file

#[test]
fn test_poc1() -> 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.seawater_admin.set(msg::sender());
            let token_addr = address!("97392C28f02AF38ac2aC41AF61297FA2b269C3DE");

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

            contract.enable_pool_579_D_A658(token_addr, true)?;

            let lower_tick = 39122;
            let upper_tick = 50108;
            let liquidity_delta = 20000;

            // create first position
            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();

            // set position liquidity
            contract.update_position_C_7_F_1_F_740(token_addr, position_id, liquidity_delta)?;


            let (amount_out_0, amount_out_1) = contract.swap_904369_B_E(
                token_addr,
                true,
                I256::try_from(100000_i32).unwrap(),
                U256::MAX,
            )?;

            // At this time, the value of ```self.sqrt_price``` is ```tick_math::MIN_SQRT_RATIO```


            // create second position
            // Set lower_tick to the last swapped tick
            let lower_tick = -887272;
            let upper_tick = 887272;
            let liquidity_delta = 20000000;
            contract.mint_position_B_C5_B086_D(token_addr, lower_tick, upper_tick)?;
            let id = U256::try_from(1).unwrap();
            // set enough liquidity
            contract.update_position_C_7_F_1_F_740(token_addr, id, liquidity_delta)?;

            // try second swap
            let (amount_out_0, amount_out_1) = contract.swap_904369_B_E(
                token_addr,
                true,
                I256::try_from(100_i32).unwrap(),
                U256::MAX,
            )?;

            Ok(())
        },
    )
}

add println! code line and remove debugassert statement on swap function in src/pool.rs

    pub fn swap(
        &mut self,
        zero_for_one: bool,
        amount: I256,
        mut price_limit: U256,
    ) -> Result<(I256, I256, i32), Revert> {
        assert_or!(self.enabled.get(), Error::PoolDisabled);
        println!("self.sqrt_price: {}", self.sqrt_price.get()); // added

        while !state.amount_remaining.is_zero() && state.price != price_limit {
            iters += 1;
            // debug_assert!(iters < 500);

Run Test

cargo test test_poc1 --package seawater --features testing -- --nocapture

Result

running 1 test
self.sqrt_price: 792281625142643375935439503360
self.sqrt_price: 4295128740
Error: [80, 114, 105, 99, 101, 32, 108, 105, 109, 105, 116, 32, 116, 111, 111, 32, 108, 111, 119]
test test_poc1 ... FAILED

failures:

failures:
    test_poc1

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 16 filtered out; finished in 0.14s

error: test failed, to rerun pass `-p seawater --test lib`

When requesting a second swap, you can see that an error return occurs because sqrt_price is set to the price_limit set in the previous swap even though the position existed.

Tools Used

Visual Studio Code

Recommended Mitigation Steps

Modified to allow setting the value to the price of the last swapped position even if the position amount is insufficient.

@@ -375,9 +375,10 @@ impl StoragePool {
         // continue swapping while there's tokens left to swap
         // and we haven't reached the price limit
         let mut iters = 0;
+        let mut last_valid_price = state.price;
         while !state.amount_remaining.is_zero() && state.price != price_limit {
             iters += 1;
-            debug_assert!(iters < 500);
+            // debug_assert!(iters < 500);

             let step_initial_price = state.price;

@@ -479,6 +480,7 @@ impl StoragePool {
                     };

                     state.liquidity = liquidity_math::add_delta(state.liquidity, liquidity_net)?;
+                    last_valid_price = state.price;
                 }

                 state.tick = match zero_for_one {
@@ -493,7 +495,8 @@ impl StoragePool {

         // write state
         // update price and tick
-        self.sqrt_price.set(state.price);
+        self.sqrt_price.set(if state.price == price_limit { last_valid_price } else { state.price });
+
         if state.tick != self.cur_tick.get().sys() {
             self.cur_tick.set(I32::unchecked_from(state.tick));
         }

Mitigation Test Result

Test Code is same above Test Code

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

running 1 test
self.sqrt_price: 792281625142643375935439503360
self.sqrt_price: 560222498985353939371108591955
test test_poc1 ... ok

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

     Running tests/pools.rs (target/debug/deps/pools-a3343d45185ff606)

Unlike before, it is set to the tick where the last swap occurred.

Assessed type

Loop

[alex-ppg]

The Warden has correctly identified that the configuration of the square root price of the pool will be incorrect in case the price limit configured by the user has been achieved as the state's price is updated on each search iteration rather than being updated on the last valid swapped price.

I believe a medium-risk severity rating is appropriate given that a user can simply swap the price back down to the actual price of the AMM and no active harm may occur to the liquidity within the pool or its assets.

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as selected for report