twapFilter() would return the wrong prices for negative tick deltas since it doesn't round up for them

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L1450-L1452
https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L1026
(https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L1200
https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L1231-L1238
https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L1250-L1262

Vulnerability details

Proof of Concept

First take a look at https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L1450-L1452

    function getUniV3TWAP() internal view returns (int24 twapTick) {
        twapTick = PanopticMath.twapFilter(s_univ3pool, TWAP_WINDOW);
    }

This function is queried to get he TWAP price, using 10 minutes as the TWAP duration.

Now consider the implementation of PanopticMath.twapFilter() at https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/libraries/PanopticMath.sol#L241-L268

    function twapFilter(IUniswapV3Pool univ3pool, uint32 twapWindow) external view returns (int24) {
        uint32[] memory secondsAgos = new uint32[](20);

        int256[] memory twapMeasurement = new int256[](19);

        unchecked {
            // construct the time stots
            for (uint256 i = 0; i < 20; ++i) {
                secondsAgos[i] = uint32(((i + 1) * twapWindow) / 20);
            }

            // observe the tickCumulative at the 20 pre-defined time slots
            (int56[] memory tickCumulatives, ) = univ3pool.observe(secondsAgos);

            // compute the average tick per 30s window
            //@audit
            for (uint256 i = 0; i < 19; ++i) {
                twapMeasurement[i] = int24(
                    (tickCumulatives[i] - tickCumulatives[i + 1]) / int56(uint56(twapWindow / 20))
                );
            }

            // sort the tick measurements
            int256[] memory sortedTicks = Math.sort(twapMeasurement);

            // Get the median value
            return int24(sortedTicks[10]);
        }
    }

This function is used to compute the twap of a Uniswap V3 pool using data from its oracle, and then it returns the final calculated twap tick as the median price, but multiple data in the array could be flawed because current implementation deviates from Uniswap's.

Considering the native implementation in Uniswap), we can see that the differences between the tickCummulatives is considered the tickCummulativeDelta, and in a case where this delta is less than 0, a check is implemented to see if the delta is directly divisible by the twap duration, if not the tick calculated is rounded down, i.e: if (tickCumulativesDelta < 0 && (tickCumulativesDelta % secondsAgo != 0)) arithmeticMeanTick--;

Now note that unlike the original uniswap implementation, here the delta of the tick cummulatives is being calculated in a different manner, i.e Panoptic implements (tickCumulatives[i] - tickCumulatives[i + 1]) instead of tickCumulatives[1] - (tickCumulatives[0] which is cause here our secondsAgos[] array is ascending i.e secondsAgos[0] < secondsAgos[1] < secondsAgos[5] < secondsAgos[10]< secondsAgos[20] , unlike in the Uniswap OracleLibrary where it's descending, albeit only two element are present in the array secondsAgos[0] = secondsAgo; & secondsAgos[1] = 0; i.e secondsAgos[0] > secondsAgos[1] so everything checks out and the tick deltas are calculated accurately, i.e in our case tickCumulativesDelta = (tickCumulatives[i] - tickCumulatives[i + 1]) is valid.

As a result, in Panoptic's current implementation case, if int24(tickCumulatives[i] - tickCumulatives[i + 1]) is negative and int24(tickCumulatives[i] - tickCumulatives[i + 1]) % int56(uint56(twapWindow / 20) != 0 then the returned tick will be bigger then it should be, which opens possibility for some price manipulations and arbitrage opportunities.

Remember that this function is directly queried via getUniV3TWAP(), which is used in both liquidations and whenever force exercising a single position.

Impact

If int24(tickCumulatives[i] - tickCumulatives[i + 1]) is negative and int24(tickCumulatives[i] - tickCumulatives[i + 1]) % int56(uint56(twapWindow / 20) != 0, then the returned tick will be bigger than it should be which places protocol wanting prices to be right not be able to achieve this goal, note that Panoptic relies on the price attached to this to be valid and uses them directly while liquidating and whenever force exercising a single position, having the wrong price being returned easily causes reverts on valid attempts to liquidate since this check in liquidate() could revert could could fail, or the calculation of the fee for force exercising the single position would be off than it should be and this could even cause protocol to assume a wrong amount to be redistributed which is used while settling the differences between delegated amounts and the refund amounts

Tool used

• Manual Review
• Uniswap V3's OracleLibrary
• And two similar findings on Code4rena from Q1 2024... sources 1 & 2.

Recommended Mitigation Steps

Add this line to PanopticMath.twapFilter() after if (int24(tickCumulatives[i] - tickCumulatives[i + 1]) < 0) && ( int24(tickCumulatives[i] - tickCumulatives[i + 1]) % int56(uint56(twapWindow / 20) != 0) twapMeasurement[i] --;.

Assessed type

Oracle

[c4-judge]

Picodes marked the issue as primary issue

[dyedm1]

Uniswap rounds to keep a consistent convention for external integrators, but I don't see why we have to follow the same convention. Rounding toward positive infinity is no more or less risky than rounding toward negative infinity (and, by extension, neither is rounding toward 0).

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)