c4-bot-2 commented 1 month ago

Lines of code

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

Vulnerability details

Impact

Provision of liquidity for pools with high volatility will not be possible as such pools cannot be initiated with a tick spacing basis point greater than 256 because of the tick_spacing datatype constrained to a u8.

Proof of Concept

Less volatile pools usually have fee tiers in the range of 0.05% to 1%. But higher volatile pools go over 1% as a baseline and some pools can have a 3-5% fee tier depending on the volatility of the pool pairs. Since the protocol intends to be able to create pools for any tokens with sufficient liquidity, pools with exotic pairs in the fee range of > 1.3% cannot be instantiated.

pub fn init(
        &mut self,
        price: U256,
        fee: u32,
@>        tick_spacing: u8, // e.g 300 for a pool with 1.5% fee tier
        max_liquidity_per_tick: u128,
    ) -> Result<(), Revert> {
        assert_eq_or!(
            self.sqrt_price.get(),
            U256::ZERO,
            Error::PoolAlreadyInitialised
        );

        self.sqrt_price.set(price);
        self.cur_tick
            .set(I32::lib(&tick_math::get_tick_at_sqrt_ratio(price)?));

        self.fee.set(U32::lib(&fee));
@>        self.tick_spacing.set(U8::lib(&tick_spacing)); // @audit would revert at this point because the value exceeds the size of the datatype
        self.max_liquidity_per_tick
            .set(U128::lib(&max_liquidity_per_tick));

        Ok(())
    }

Tools Used

Manual review

Recommended Mitigation Steps

Uniswap uses a bigger datatype for this such as a 24-bit integer type. A bigger datatype would be sufficient for this such as u16.

Assessed type

Context

af-afk commented 4 weeks ago

https://github.com/fluidity-money/long.so/commit/d186c1ae6e65a375eca24f6864cf18a3e4940bc3

We wound up enforcing a limit on the fee after some discussion that we don't intend to set fees that high.

alex-ppg commented 4 weeks ago

The submission has correctly identified that the data type used for the tick spacing of AMM pools is insufficient for volatile pools, causing them to become unusable due to their prices moving across ticks too swiftly.

I believe a medium-risk severity rating is appropriate given that the current AMM model is curated for volatile rather than stable pairs, rendering this scenario to have a non-negligible likelihood of manifesting in practice.

c4-judge commented 4 weeks ago

alex-ppg marked the issue as selected for report