Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

X96 sqrt ratio calculated is wrong whenever (abs_tick & 1) != 0 & (abs_tick & 0x2) != 0 #259

Open
c4-bot-2 opened this issue Sep 13, 2024 · 1 comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working 🤖_primary AI based primary recommendation 🤖_27_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality

Comments

@c4-bot-2
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/seawater/src/maths/tick_math.rs#L20-L59
https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/README.md#L70-L92

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/seawater/src/maths/tick_math.rs#L20-L59

pub fn get_sqrt_ratio_at_tick(tick: i32) -> Result<U256, Error> {
    let mut abs_tick = tick.abs();

    if abs_tick > MAX_TICK {
        return Err(Error::T);
    }

    // calculate (1/1.0001)^(i/2)
    // = (1/1.0001)^(floor(i/2)) * maybe (1/1.0001)^(0.5)

    let mut result = if (abs_tick & 0x1) != 0 {
        uint!(0xfffcb933bd6fad37aa2d162d1a594001_U256) // 1 * (1/1.0001)^0.5
    } else {
        U256::one() << 128 // 1
    };

    abs_tick >>= 1;

    let mut ratio = uint!(0xfff97272373d413259a46990580e213a_U256); // 1/1.0001

    while abs_tick != 0 {
        if (abs_tick & 1) != 0 {//@audit
            result = (result * ratio) >> 128;
        }
        ratio = (ratio * ratio) >> 128;
        abs_tick >>= 1;
    }

    // invert back from 1/1.0001^i to 1.0001^i
    if tick > 0 {
        result = U256::MAX / result;
    }

    Ok((result >> 32)
        + if (result % (U256::one() << 32) as U256).is_zero() {
            U256::zero()
        } else {
            U256::one()
        })
}

This function calculates the X96 encoded square root price for a given tick index.

Per the documentation this should be the same as is done in the Uniswap instance, see https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/libraries/TickMath.sol#L23-L54:

    function getSqrtRatioAtTick(int24 tick) internal pure returns (uint160 sqrtPriceX96) {
        uint256 absTick = tick < 0 ? uint256(-int256(tick)) : uint256(int256(tick));
        require(absTick <= uint256(MAX_TICK), 'T');


        uint256 ratio = absTick & 0x1 != 0 ? 0xfffcb933bd6fad37aa2d162d1a594001 : 0x100000000000000000000000000000000;
        if (absTick & 0x2 != 0) ratio = (ratio * 0xfff97272373d413259a46990580e213a) >> 128;//@audit
        // ..snip
    }

Issue however as hinted by the @audit tag is that this deviates from the linked implementation, cause in Superposition's case abs_tick is being AND'd with 1 instead of 0x2. And then the value is multiplied against the ratio value of 0xfffcb933bd6fad37aa2d162d1a594001.

Impact

The X96 encoded square root price for a given tick index would be wrong in this case, considering protocol assumes the Uniswap calculation to be intended/correct.

This then means that all the instances where this price is used to be flawed/ broken too, considering even in swaps or integrations with the AMM the wrong price would be returned, when the real calculation of (abs_tick & 0x2) != 0 since the check done in scope before the multiplication of the ratio is (abs_tick & 1) != 0

This then means that the calculation is broken in two instances:

  • When (abs_tick & 1) != 0 it deviates heavily from the Uniswap standard as it makes and extra calculation, and
  • WHen (abs_tick & 0x2) != 0 it also deviates from the Uniswap standard as it doesn't make the makes expected calculation, see Uniswap's TickMath.sol#L23-L54 for more info.

Recommended Mitigation Steps

Change (abs_tick & 1) != 0 for (abs_tick & 0x2) != 0 .

Assessed type

Uniswap

@c4-bot-2 c4-bot-2 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Sep 13, 2024
c4-bot-10 added a commit that referenced this issue Sep 13, 2024
@c4-bot-12 c4-bot-12 added 🤖_27_group AI based duplicate group recommendation 🤖_primary AI based primary recommendation labels Sep 13, 2024
howlbot-integration bot added a commit that referenced this issue Sep 16, 2024
@howlbot-integration howlbot-integration bot added the sufficient quality report This report is of sufficient quality label Sep 16, 2024
@af-afk
Copy link

af-afk commented Sep 23, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working 🤖_primary AI based primary recommendation 🤖_27_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

3 participants