Wrong Median Cumulative TWAP Tick Price in situation of Series of High Price Volatilities in Pool

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/PanopticPool.sol#L1450
https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/libraries/PanopticMath.sol#L249-L253

Vulnerability details

Impact

1. Based on Protocol comment description at L249-L253 of PanopticMath Library : We instead observe the average price over a series of time intervals, and define the TWAP as the median of those averages.
   The problem is that Wrong Cumulative median TWAP tick Price will be returned in situation of series of high Price Volatility which would cause value loss to protocol and poor price integrity in contract

2. Denial of Service when twapTick price is within the allowed ranged to perform Liquidation due to it usage in validation at L1035 of the Panoptic Pool contract. More details of this second impact would be elaborated during the Scenario Proof of this report

Proof of Concept

  /// @notice Compute the TWAP price from the last 600s = 10mins.
    /// @return twapTick The TWAP price in ticks.
    function getUniV3TWAP() internal view returns (int24 twapTick) {
        twapTick = PanopticMath.twapFilter(s_univ3pool, TWAP_WINDOW);
    }

The function provided above from the PanopticPool pool contract shows how TWAP price is derived by calling the twapFilter(...) function in the PanopticMath library, it computes price average based on different points from the last 600s, then a median Price is selected in other to prevent price manipulation as much as possible.
A look at the implementation of twapFilter(...) function provided below from the PanopticMath library and as noted from the first pointer, it can be seen that secondsAgos array is filled up by dividing twapWindow i.e 600, into sections of time intervals. Based on the implementation secondsAgos array would be equivalent to [30, 60, 90, ..., 570, 600], The period in between these different time slots i.e 30 to 60, 60 to 90, 90 to 120 etc is then used to derive tickCumulatives based on the second pointer by calling uniswap observe function i.e univ3pool.observe(...).

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

The problem is that zero was not included in the secondsAgos array which means deriving the price tick since the last 600s would not actually include the most current price between 0 and 30 but only from 30 seconds to 600, the implication of this is that when there is high volatility in the different prices that would have had great impact on the cumulative tick prices a wrong median price would be selected as the TWAP price price to be used by Protocol.

A quote from Uniswap doc on the correct call and usage of the observe(...) function at https://docs.uniswap.org/contracts/v3/reference/core/UniswapV3Pool#observe
To get a time weighted average tick or liquidity-in-range, you must call this with two values, one representing the beginning of the period and another for the end of the period. E.g., to get the last hour time-weighted average tick, you must call it with secondsAgos = [3600, 0].
This shows the importance of including a zero time slot value as the check of previous twapTick is not complete without starting the most current price.

The comment at description at Uniswap code also noted that "0 may be passed as `secondsAgo' to return the current cumulative values"
https://github.com/Uniswap/v3-core/blob/6562c52e8f75f0c10f9deaf44861847585fc8129/contracts/libraries/Oracle.sol#L248
To understand this report a dummy value would be used just to have an idea of how the issue would look like

Scenario Proof

Based on the description at L249-L253 of PanopticMath and the code implementation leading to L266.
Lets assume the sorted cumulative price ticks are
[ 23, 32, 45, ... , 56 , ... , 74, 78, 125]

• Note: most of the prices are not same due to situation of high price volatilities.

from the sample array it can be noted that the final price median is 56.
However if the TWAP tick price from the 0 to 30 time slot that was not included happen to be 50 which would probably have been the right value to hold the median position, the correct array would have looked like this
[ 23, 32, 45, ... , 50 , ... 69, 74, 78]
this shows that the correct median value of 50 is holding it rightful place but that would not be the case as 0 - 30 time slot was left out.
To link this explanation to the second impact at L1035 of the PanopticPool contract,

 // Enforce maximum delta between TWAP and currentTick to prevent extreme price manipulation
 if (Math.abs(currentTick - twapTick) > MAX_TWAP_DELTA_LIQUIDATION)
                revert Errors.StaleTWAP(); 

the code ensures price different between TWAP and currentTick is within the allowed range, of MAX_TWAP_DELTA_LIQUIDATION i.e 513,
assuming current tick is 4900 and the TWAP tick derived is 5600, the difference is 700 which is above 513, therefore the code would would revert, assuming the correct implementation of 0-30 time slot is present and the TWAP tick derived is 5000 instead of 5600, the price difference would be just 100 which is below 513 without denial of service.

Tools Used

Manual Review, Uniswap Documentation

Recommended Mitigation Steps

If protocol must include secondsAgo 600 to the array, the array length to be checked can be increased to 21 to also include zero time stot by increasing the loop implementation. However Protocol can simply adjust the code as provided below to use time stots from 0 to 570 for secondsAgos array

 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);
+++                secondsAgos[i] = uint32(((i) * 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
            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]);
        }
    }

Assessed type

Oracle

[c4-judge]

Picodes marked the issue as primary issue

[dyedm1]

Not sure what the specific issue/impact is with not including the latest 30 seconds in the TWAP? That price is over the past 10 minutes and it's supposed to lag price changes a little, so 30 seconds doesn't make much of a difference.

[Picodes]

As we're taking the median it indeed doesn't change much, so I'll downgrade to Low for "function incorrect as to spec, issues with comments"

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[Topmark1]

Thanks to Judge for how this issue is handled and also to Sponsor opinion on this issue which has made it seems that this report is not indeed qualified as high severity since it does not have a high likelihood since we are taking the median as noted by Judge and sponsor, however would like to argue that this vulnerability qualifies as medium severity for two reasons :

1. Documentation clearly state that there would be a 10 minutes window to handle timing which should clearly include the latest 0-30 seconds and the fact that it is the more recent timing range it gives it more weight and integrity than any other timing range in deciding price even if it is median.
2. Most Importantly as stated in this report There is an event where this vulnerability can affect liquidation call and execution whether median is the one being used or not.
   To back up this argument based on code4rena supreme-court decision on medium severity at https://docs.code4rena.com/awarding/judging-criteria/supreme-court-decisions-fall-2023#trusts-view
   It states that a report is medium as quoted if:

• A non-core invariant of the protocol can be broken for an extended duration or at scale, or an otherwise high-severity issue is reduced due to hypotheticals or external factors affecting likelihood.
  and since the functionality of Liquidation can be affected as stated and confirmed in this report this qualify as a medium severity at the very least, thanks

[Picodes]

@Topmark1 I respectfully disagree. Fundamentally, nothing is broken here if you correct the documentation accordingly.

Indeed there are some cases where the median between the prices starting at 0 and the price starting at 30 sec is not the same, but the main functionality of the twap oracle is to have a robust, slightly lagging oracle returning the asset price. Then the rest is a matter of documentation and oracle definition.