Oracle.transform() calculates the wrong secondsPerLiquidityCumulativeX128 in the transform() functions during zero liquidity period, which affects the results for functions in KatanaV3Pool.sol.

[c4-bot-6]

Lines of code

Oracle.sol#L30-L42
KatanaV3Pool.sol#L205
KatanaV3Pool.sol#L358
KatanaV3Pool.sol#L660

Vulnerability details

Proof of Concept

Oracle.transform() calculates the wrong secondsPerLiquidityCumulativeX128 in the transform() functions, which affects the results for functions in KatanaV3Pool.sol.

Although Oracle.sol is not listed in the scope, this finding is valid according to the criterion below: "If you discover a bug in any contract or library outside of the files listed above that impact following contracts, we will consider the issue valid" since this impacts KatanaV3Pool.sol

First of all, Oracle.transfrom still accumulate seconds for secondsPerLiquidityCumulativeX128 even when liquidity = 0. This is so because it uses liquidity = 1 as the denominator to avoid the divide-by-zero error. However, such dealing will accumulate seconds for secondsPerLiquidityCumulativeX128 even when liquidity = 0, which affects the value of secondsPerLiquidityCumulativeX128 wrongly.

function transform(Observation memory last, uint32 blockTimestamp, int24 tick, uint128 liquidity)
    private
    pure
    returns (Observation memory)
  {
    uint32 delta = blockTimestamp - last.blockTimestamp;
    return Observation({
      blockTimestamp: blockTimestamp,
      tickCumulative: last.tickCumulative + int56(tick) * delta,
      secondsPerLiquidityCumulativeX128: last.secondsPerLiquidityCumulativeX128
@        + ((uint160(delta) << 128) / (liquidity > 0 ? liquidity : 1)),
      initialized: true
    });
  }

Scenario:
T=0 to T=1000: Liquidity = 200
T=1000 to T=2000: Liquidity drops to 0
T=2000 to T=3000: Liquidity increases to 500
An observation is taken at T=300.

The current implementation will accumulate secondsPerLiquidityCumulativeX128 by:

T=0 to T=1000: 1000 >> 128 / 200 = 5 >> 128;
T=1000 to T=2000: 1000 >> 128 / 1 = 1000 >> 128
T=2000 to T=3000: 1000 >> 128 / 500 = 2 >> 128
So a total of accumulation of 1007 >> 128.

The correct accumulation should be:
T=0 to T=1000: 1000 >> 128 / 200 = 5 >> 128;
T=1000 to T=2000: no accumulation
T=2000 to T=3000: 1000 >> 128 / 500 = 2 >> 128

So a total of accumulation of 7 >> 128.

There is a big difference between 1007 >> 128 and 7 >> 128.

Since transform() is called by write() and observeSingle(). Write() is called by KavanaV3Pool.swap(). observeSingle() is called by KavanaV3Pool.snapshotCumulativesInside(). Therefore, the error caused by transform() will affect the functions in KavanaV3Pool, which justifies this is a valid finding.

Recommended Mitigation Steps

The correct implementation is to have no accumulation when liquidity = 0.

function transform(Observation memory last, uint32 blockTimestamp, int24 tick, uint128 liquidity)
    private
    pure
    returns (Observation memory)
  {
    uint32 delta = blockTimestamp - last.blockTimestamp;

    uint160 = secondsPerLiquidityCumulativeX128 = last.secondsPerLiquidityCumulativeX128;
    if(liquidity > 0) secondsPerLiquidityCumulativeX128 = (uint160(delta) << 128) / liquidity;

    return Observation({
      blockTimestamp: blockTimestamp,
      tickCumulative: last.tickCumulative + int56(tick) * delta,
      secondsPerLiquidityCumulativeX128: secondsPerLiquidityCumulativeX128,
      initialized: true
    });
  }

Assessed type

Math

[nevillehuang]

This might be OOS, since uniswap has the same exact design as seen here.

All public known issues, including public audit reports of Uniswap V3 that affect Katana V3