Missing sqrt_price validation during pool creation/update may render all swaps with MAX price_limit inexecutable

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-10-superposition/blob/7ad51104a8514d46e5c3d756264564426f2927fe/pkg/seawater/src/pool.rs#L48-L58
https://github.com/code-423n4/2024-10-superposition/blob/7ad51104a8514d46e5c3d756264564426f2927fe/pkg/seawater/src/pool.rs#L238-L257

Vulnerability details

Proof of Concept

In the current implementation of the swap() function, there is a potential vulnerability that arises from the handling of the price_limit parameter, particularly when price_limit is set to U256::MAX. This vulnerability can lead to unexpected reverts due to double validation checks that can invalidate the adjusted price limit after it is set.

Current implementation of the swap():

    // Validate price limit based on zero_for_one flag
    match zero_for_one {
        true => {
            if price_limit == U256::MAX {
>>              price_limit = tick_math::MIN_SQRT_RATIO + U256::one(); // Adjusted if MAX
            }
>.          if price_limit >= self.sqrt_price.get() || price_limit <= tick_math::MIN_SQRT_RATIO { // Validated again right after
                Err(Error::PriceLimitTooLow)?;  // Potential Revert
            }
        }
        false => {
            if price_limit == U256::MAX {
>>              price_limit = tick_math::MAX_SQRT_RATIO - U256::one(); // Adjusted if MAX
            }
>>          if price_limit <= self.sqrt_price.get() || price_limit >= tick_math::MAX_SQRT_RATIO { // Validated again right after
                Err(Error::PriceLimitTooHigh)?;  // Potential Revert
            }
        }
    };

Issue Scenario:

1. Initialization Phase:
   A pool is initialized with a price value equal to tick_math::MIN_SQRT_RATIO + U256::one() or tick_math::MAX_SQRT_RATIO - U256::one(). But since there is no validation done on the price parameter, sqrt_price is set to the price provided:

    pub fn init(
        &mut self,
>>      price: U256,
         ---SNIP---
    ) -> Result<(), Revert> {
         ---SNIP---

>>      self.sqrt_price.set(price); // set here with no validation
        ---SNIP---
    }

2. Swap Execution

• A user then attempts to perform a swap with price_limit == U256::MAX and zero_for_one == true.
• The price_limit is adjusted to tick_math::MIN_SQRT_RATIO + U256::one(), but immediately after, it gets validated again.
• Since sqrt_price was set to a value equal to tick_math::MIN_SQRT_RATIO + U256::one(), this will trigger the condition price_limit >= self.sqrt_price.get() to be true, leading to the Err(Error::PriceLimitTooLow)? revert.

The same revert goes for the case when price_limit == U256::MAX, zero_for_one == false and sqrt_price is equal to tick_math::MAX_SQRT_RATIO - U256::one()

Impact

Users attempting to perform a valid swap might face unexpected reverts. This is because the inbuilt mechanism for price adjustment will be flawed therefore rendering all swaps with MAX price_limit inexecutable.

Recommended Mitigation Steps

Ensure that sqrt_price is not set to tick_math::MIN_SQRT_RATIO + U256::one() or tick_math::MAX_SQRT_RATIO - U256::one():

    pub fn init(
        &mut self,
        price: U256,
         ---SNIP---
    ) -> Result<(), Revert> {
         ---SNIP---

        // Ensure price is not equal to critical boundary values
+       assert_or!(price != tick_math::MIN_SQRT_RATIO + U256::one(), Error::InvalidPrice);
+       assert_or!(price != tick_math::MAX_SQRT_RATIO - U256::one(), Error::InvalidPrice);
        self.sqrt_price.set(price);
        ---SNIP---
    }

Modify set_sqrt_price() with the above checks as well:

    pub fn set_sqrt_price(&mut self, new_price: U256) {
+       assert_or!(new_price != tick_math::MIN_SQRT_RATIO + U256::one(), Error::InvalidPrice);
+       assert_or!(new_price != tick_math::MAX_SQRT_RATIO - U256::one(), Error::InvalidPrice);
        self.sqrt_price.set(new_price);
    }

Assessed type

DoS

[alex-ppg]

The Warden has outlined a specific edge case whereby a pair configuration at two specific square root initialization values combined with a price limit of "maximum" would fail to execute swaps properly.

Swaps will still be executable in such a case by utilizing a different price limit culminating in a low likelihood, low impact submission that would be of QA severity at best.

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[c4-judge]

alex-ppg marked the issue as grade-b

[af-afk]

Fix: fluidity-money/long.so@9472140