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

calculateUsableTick calculation is not done properly . #267

Closed
c4-bot-5 opened this issue Jun 9, 2024 · 2 comments
Closed

calculateUsableTick calculation is not done properly . #267

c4-bot-5 opened this issue Jun 9, 2024 · 2 comments
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 insufficient quality report This report is not of sufficient quality 🤖_52_group AI based duplicate group recommendation

Comments

@c4-bot-5
Copy link
Contributor

c4-bot-5 commented Jun 9, 2024

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/e96f378007e3dc56b184079f0c0e4fe48a72efaa/src/libraries/Reallocation.sol#L109

Vulnerability details

When calculating the calculateUsableTick is should be considered the fractional part and if fractional part is less than 0.5, it rounds to the lower integer; when it’s greater than 0.5 it rounds to the greater integer; and when it’s 0.5, it rounds to the greater integer as well. But this calculateUsableTick calculation its not consider this & round down to the lower integer always.

Proof of Concept

calculateUsableTick

    function calculateUsableTick(int24 _tick, int24 tickSpacing) internal pure returns (int24 result) {
        require(tickSpacing > 0);

        result = _tick;

        if (result < TickMath.MIN_TICK) {
            result = TickMath.MIN_TICK;
        } else if (result > TickMath.MAX_TICK) {
            result = TickMath.MAX_TICK;
        }

        result = (result / tickSpacing) * tickSpacing;
    }

From Uniswap V3 docs

Where Math.round is rounding to the nearest integer: when the fractional part is less than 0.5, it rounds to the lower integer; when it’s greater than 0.5 it rounds to the greater integer; and when it’s 0.5, it rounds to the greater integer as well.

Impact

Incorrect calculation of nearest usable tick

Tools Used

Manual Review

Recommended Mitigation Steps

Use these 2 functions for calculateUsableTick. (As Uniswap V3 docs)

function nearestUsableTick(int24 tick_, uint24 tickSpacing)
    internal
    pure
    returns (int24 result)
{
    result =
        int24(divRound(int128(tick_), int128(int24(tickSpacing)))) *
        int24(tickSpacing);

    if (result < TickMath.MIN_TICK) {
        result += int24(tickSpacing);
    } else if (result > TickMath.MAX_TICK) {
        result -= int24(tickSpacing);
    }
}
function divRound(int128 x, int128 y)
    internal
    pure
    returns (int128 result)
{
    int128 quot = ABDKMath64x64.div(x, y);
    result = quot >> 64;

    // Check if remainder is greater than 0.5
    if (quot % 2**64 >= 0x8000000000000000) {
        result += 1;
    }
}

Assessed type

Uniswap

@c4-bot-5 c4-bot-5 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 Jun 9, 2024
c4-bot-4 added a commit that referenced this issue Jun 9, 2024
@c4-bot-12 c4-bot-12 added the 🤖_52_group AI based duplicate group recommendation label Jun 14, 2024
@howlbot-integration howlbot-integration bot added the insufficient quality report This report is not of sufficient quality label Jun 17, 2024
@Yasashari
Copy link

Hello @alex-ppg , Please check the provided uniswap v3 docs in the report. Please let me know any feed back here ?
Thank You.

@alex-ppg
Copy link

alex-ppg commented Jul 4, 2024

Hey @Yasashari, thanks for contributing to the PJQA process! This represents a validation repository finding and as such was not evaluated by me directly.

This submission appears to be a duplicate of a QA-level submission as described here: code-423n4/2024-05-predy-findings#295

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 insufficient quality report This report is not of sufficient quality 🤖_52_group AI based duplicate group recommendation
Projects
None yet
Development

No branches or pull requests

4 participants