Precision Loss in Oracle's Tick Cumulative Calculation Due to Integer Division Order

[howlbot-integration[bot]]

Lines of code

https://github.com/ronin-chain/katana-v3-contracts/blob/03c80179e04f40d96f06c451ea494bb18f2a58fc/src/core/libraries/Oracle.sol#L231

Vulnerability details

Issue Details

The Oracle's tick cumulative calculation suffers from significant precision loss due to performing division before multiplication in integer arithmetic operations. This implementation can lead to substantial undervaluation of tick cumulative values, potentially affecting pricing and fee calculations.

Code part

https://github.com/ronin-chain/katana-v3-contracts/blob/03c80179e04f40d96f06c451ea494bb18f2a58fc/src/core/libraries/Oracle.sol#L231

  function observeSingle(
    Observation[65535] storage self,
    uint32 time,
    uint32 secondsAgo,
    int24 tick,
    uint16 index,
    uint128 liquidity,
    uint16 cardinality
  ) internal view returns (int56 tickCumulative, uint160 secondsPerLiquidityCumulativeX128) {
    if (secondsAgo == 0) {
      Observation memory last = self[index];
      if (last.blockTimestamp != time) last = transform(last, time, tick, liquidity);
      return (last.tickCumulative, last.secondsPerLiquidityCumulativeX128);
    }

    uint32 target = time - secondsAgo;

    (Observation memory beforeOrAt, Observation memory atOrAfter) =
      getSurroundingObservations(self, time, target, tick, index, liquidity, cardinality);

    if (target == beforeOrAt.blockTimestamp) {
      // we're at the left boundary
      return (beforeOrAt.tickCumulative, beforeOrAt.secondsPerLiquidityCumulativeX128);
    } else if (target == atOrAfter.blockTimestamp) {
      // we're at the right boundary
      return (atOrAfter.tickCumulative, atOrAfter.secondsPerLiquidityCumulativeX128);
    } else {
      // we're in the middle
      uint32 observationTimeDelta = atOrAfter.blockTimestamp - beforeOrAt.blockTimestamp;
      uint32 targetDelta = target - beforeOrAt.blockTimestamp;
      return (
        beforeOrAt.tickCumulative
          + ((atOrAfter.tickCumulative - beforeOrAt.tickCumulative) / observationTimeDelta) * targetDelta,
        beforeOrAt.secondsPerLiquidityCumulativeX128
          + uint160(
            (
              uint256(atOrAfter.secondsPerLiquidityCumulativeX128 - beforeOrAt.secondsPerLiquidityCumulativeX128)
                * targetDelta
            ) / observationTimeDelta
          )
      );
    }
  }

Technical Analysis

Current Implementation

tickCumulative = beforeOrAt.tickCumulative + 
    ((atOrAfter.tickCumulative - beforeOrAt.tickCumulative) / observationTimeDelta) * targetDelta

The current implementation divides first, which can result in the quotient being rounded down to zero, especially when the difference between tick cumulatives is smaller than the observation time delta.

Proof Of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

contract OracleTest {
    // Function to test precision loss
    function test_precisionLoss() public {

        uint beforeOrAtTickCumulative = 150000;
        uint atOrAfterTickCumulative = 400000;
        uint observationTimeDelta = 300000;
        uint targetDelta = 250000;

        uint tickCumulative_actual = beforeOrAtTickCumulative + 
            (((atOrAfterTickCumulative - beforeOrAtTickCumulative) / observationTimeDelta) * (targetDelta));
        uint tickCumulative_precise = beforeOrAtTickCumulative + 
            (((atOrAfterTickCumulative - beforeOrAtTickCumulative) * targetDelta) / (observationTimeDelta));

        emit TestResult(tickCumulative_actual, tickCumulative_precise);
    }

    event TestResult(uint tickCumulativeActual, uint tickCumulativePrecise);
}

Results:

[
    {
        "from": 
        "topic": 
        "event": "TestResult",
        "args": {
            "0": "150000",
            "1": "358333",
            "tickCumulativeActual": "150000",
            "tickCumulativePrecise": "358333"
        }
    }
]

Recommended Solution

// In the observeSingle function, modify the return statement to:
return (
    beforeOrAt.tickCumulative + 
        ((atOrAfter.tickCumulative - beforeOrAt.tickCumulative) * targetDelta) / observationTimeDelta,
    beforeOrAt.secondsPerLiquidityCumulativeX128 + 
        uint160((uint256(atOrAfter.secondsPerLiquidityCumulativeX128 - 
        beforeOrAt.secondsPerLiquidityCumulativeX128) * targetDelta) / observationTimeDelta)
);

This modification ensures multiplication is performed before division, preserving precision in the calculation. The fix maintains the same mathematical logic while preventing potential undervaluation of tick cumulative calculations.

---

Even if this contract is out of scope, as you mentioned:
"If you discover a bug in any contract or library outside of the files listed above that impacts the following contracts, we will consider the issue valid:

• KatanaGovernance
• AggregateRouter
• KatanaV3Factory
• NonfungiblePositionManager
• V3Migrator
• KatanaV3PoolBeacon
• KatanaV3Pool"

So, this issue does apply.

Audit method

Manual Review

Assessed type

Math

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Out of scope