code-423n4 / 2024-08-superposition-findings

0 stars 0 forks source link

Unchecked tickLower < tickUpper #9

Closed c4-bot-3 closed 1 month ago

c4-bot-3 commented 1 month ago

Lines of code

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

Vulnerability details

Impact

Due to the lack of a check for tickLower < tickUpper when creating a position, users can set a higher-priced tick as tickLower and a lower-priced tick as tickUpper. This can lead to severe accounting errors.

Proof of Concept

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

When creating a position, there is no check for low < up.

  1. If a user selects low = 1000 and up = 1 ticks to provide liquidity, the actual range of the provided liquidity changes from [1, 1000] to [-∞, 1] ∪ [1000, +∞].
    This allows them to access tick price ranges that are outside the intended bounds, violating the primary invariants of the system.

  2. For pools with low liquidity, this can even cause the liquidity in certain ranges to become negative:

    If the pool initially has 100 units of liquidity in the [1, 1000] price range and a malicious user adds 1000 units of liquidity using the method described, then as the price moves from left to right across the 1 tick, the liquidity changes as follows:

    • Moving past the 1 tick: liquidity changes from 0 + 100 - 1000 = -900
    • Moving past the 1000 tick: liquidity changes from -900 - 100 + 1000 = 0

    From this, we can calculate:

    • The activated liquidity in the [-∞, 1] range is 900
    • The activated liquidity in the [1000, +∞] range is 0
    • The activated liquidity in the [1, 1000] range becomes negative: -900

    This results in an incorrect distribution of liquidity, severely distorting the pool’s functioning and stability.

Tools Used

Manual Review

Recommended Mitigation Steps

    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

Invalid Validation

c4-judge commented 1 month ago

alex-ppg marked the issue as satisfactory