Min tick has wrong rounding making part of the liquidity range unaccessible

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

Min tick math has a wrong rounding direction, so it results in a min tick higher than the expected range for a pool, thus cutting the possible liquidity range.

Proof of Concept

get_min_tick performs a truncation during the division (towards positive infinite):

    pub fn get_min_tick(spacing: u8) -> i32 {
        let spacing = spacing as i32;
        (MIN_TICK / spacing) * spacing
    }

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

But the logic is wrong, it should be a ceiling to capture the full liquidity range (towards negative infinite):

    export const getMinTick = (tickSpacing: number) => Math.ceil(-887272 / tickSpacing) * tickSpacing

https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/test/shared/utilities.ts#L10

Here's an example with a 200 tick space with the current logic:

-887272 / 200 = −4436,36 = -4436 -> -4436 * 200 = −887200

The correct logic should be:

-887272 / 200 = −4436,36 = -4437 -> -4437 * 200 = −887400

So, 200 ticks are cut from the valid liquidity range, but they should have been accessible.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider performing a ceiling op instead of truncation.

Assessed type

Uniswap

[alex-ppg]

The Warden has identified that an extreme subset of the supported ticks in the AMM system is not available for use due to incorrect rounding behavior in the tick_math::get_min_tick function which rounds towards 0.

I believe that the issue outlined is a low-risk issue given that no assets are at risk, and very limited functionality of the system is inaccessible which in most cases is not useful.

[c4-judge]

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

[c4-judge]

alex-ppg marked the issue as grade-c

[DadeKuma]

Dear Judge,

I respectfully ask you to reconsider classifying this issue for the following reasons:

1) According to the README this is an important core variant that is being violated:

We should follow Uniswap V3's math faithfully.

2) Although the impact is categorized as low, the likelihood of occurrence is high (as this issue will always arise).

3) According to the C4 documentation, a Medium severity issue is defined as follows:

2 — Med: Assets are not at direct risk, but the function of the protocol or its availability could be impacted, or value could leak under a hypothetical attack path with stated assumptions and external requirements.

Furthermore, as you correctly pointed out:

limited functionality of the system is inaccessible,

This limitation does indeed affect the availability of the protocol, as the full-range liquidity is not accessible.

Given these points, I believe the severity should be elevated to Medium.

[0xTenma]

Hi @DadeKuma, @alex-ppg This finding is invalid. The submitter uses the [reference]() to test setup of uniswap v3 contracts.

   export const getMinTick = (tickSpacing: number) => Math.ceil(-887272 / tickSpacing) * tickSpacing

The comparison to the Uniswap v3 test utilities is not appropriate. Test utilities often use different implementations for simplicity or specific test scenarios, and should not be considered as the canonical implementation.

What actually Uniswap uses is: https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/libraries/Tick.sol#L45C5-L46C73

@>      int24 minTick = (TickMath.MIN_TICK / tickSpacing) * tickSpacing;

In summary, finding is invalid due as the superposition implementation is correct and faithful to Uniswap v3 math implementation.

[DadeKuma]

@0xTenma respectfully, but your example is not related in any way to this issue.

Your link is related to the function tickSpacingToMaxLiquidityPerTick which uses a truncated minTick to calculate the max liquidity in a single tick.

This is an entirely different concept, as my issue shows that the min_tick used to create a position is wrong. In Superposition, the get_min_tick function is used in fact to limit the position range: https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/seawater/src/pool.rs#L81

If you check any Uniswap test, like this one: https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/test/UniswapV3Pool.spec.ts#L96

You will see that all tests use a position minted with the ceiled minTick: https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/test/UniswapV3Pool.spec.ts#L196

If you don't think this is correct, please show an example of usage where positions should be minted with the truncated minTick instead.

[0xTenma]

@DadeKuma , Thanks for replying, I understood the issue now. I actually couldn't find the implementation of ceiling this function in contracts rather than the tests. But the issue is actually there.

Thanks again!

[alex-ppg]

Hey @DadeKuma and @0xTenma, thank you for your PJQA contributions!

A single tick space is omitted from the full tick range of a particular AMM, which represents a percentage lower than 1% of the available ranges. As such, the impact is very low and effectively inconsequential for most AMM pairs (feel free to demonstrate an AMM pair that actively utilizes the maximum negative tick range available).

[c4-judge]

alex-ppg marked the issue as grade-b