Incorrect Rounding in `calculateUsableTick` where negative tick values are rounded down instead of to the nearest multiple of `tickSpacing`.

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Issue Description

Reallocation:calculateUsableTick is responsible for rounding a given _tick value to the nearest multiple of tickSpacing. However, the current implementation always rounds down, which can lead to incorrect rounding for negative _tick values.

Code:

    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;
    }

Line result = (result / tickSpacing) * tickSpacing; is responsible for rounding down the _tick value to the nearest multiple of tickSpacing.

Example: Let's say the case where _tick is -1 and tickSpacing is 2.

Initially, result is set to -1. Since -1 is not less than TickMath.MIN_TICK or greater than TickMath.MAX_TICK, the conditional statements are skipped. The line result = (result / tickSpacing) * tickSpacing; is executed, which becomes result = (-1 / 2) * 2;. The integer division -1 / 2 results in 0 (rounding down), and then 0 * 2 gives 0.

Therefore, the final value of result is 0.

However the correct nearest multiple of 2 to -1 should be -2, not 0.

Impact

The incorrect rounding behavior can lead to the following issues in the Uniswap V3 pool.

Incorrect pricing: The tick values are used to calculate the price of the trading pair. If the tick values are incorrect, the pricing calculations will also be incorrect, leading to potential losses for liquidity providers and traders. Incorrect liquidity distribution: The tick values are used to determine the liquidity distribution within the pool. If the tick values are incorrect, the liquidity may be distributed incorrectly, leading to potential losses or inefficient use of liquidity. Potential arbitrage opportunities: If the pricing and liquidity distribution are incorrect, it may create arbitrage opportunities for traders, potentially leading to losses for liquidity providers.

Proof of Concept

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

Tools Used

Vs Code

Recommended Mitigation Steps

The calculateUsableTick should be modified to round the _tick value to the nearest multiple of tickSpacing, regardless of whether the _tick value is positive or negative.

    if (quotient < TickMath.MIN_TICK / tickSpacing) {
        result = TickMath.MIN_TICK;
    } else if (quotient > TickMath.MAX_TICK / tickSpacing) {
        result = TickMath.MAX_TICK;
    } else {
        result = quotient * tickSpacing;
    }
}

Assessed type

Math

[c4-judge]

alex-ppg marked the issue as not a duplicate

[alex-ppg]

The Warden demonstrates an interesting problem in the Reallocation contract's usable tick calculation which will round towards 0 instead of rounding towards negative infinity.

The function is utilized to calculate the next lower and upper ticks that liquidity should be reallocated in, and the values fed to the Reallocation::calculateUsableTick function utilize a pair's risk parameter configuration and specifically the rangeSize. This value is simply validated as non-zero by the AddPairLogic, thereby permitting a non-zero rangeSize configuration to result in a re-allocation to a single tick position which is something the Predy team does not wish to be possible.

The maximum impact of the vulnerability is a re-allocation being performed across one less tick than expected/configured and will manifest solely when the lower tick is in the negative range but the upper tick is in the positive range.

I appreciate the Warden's due diligence and believe it is an issue that the Sponsor should fix, however, I do not believe its impact can be considered as H or M. It will solely arise for a rangeSize configuration that is a multiple of tickSpacing / 2 (medium likelihood), and will manifest itself only when the lower tick is negative and the upper tick is positive (low likelihood). The impact will be a suboptimal allocation of funds (medium impact), thereby rendering the overall submission to be a QA (L) risk issue.

[c4-judge]

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

[c4-judge]

alex-ppg marked the issue as grade-c

[syuhei176]

@alex-ppg's explanation is very clear and reasonable.