c4-bot-6 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/libraries/PanopticMath.sol#L257-L259

Vulnerability details

Impact

The PanopticMath.twapFilter function is used to compute the twap of a Uniswap V3 pool using data from its oracle. The twap value in this case computed by taking the the average price over a series of time intervals and then taking the median of those averages.

The computation of the average price over a series of time intervals is performed as shown below:

            for (uint256 i = 0; i < 19; ++i) {
                twapMeasurement[i] = int24(
                    (tickCumulatives[i] - tickCumulatives[i + 1]) / int56(uint56(twapWindow / 20))
                );
            }

The tickCumulatives value above represents the cumulative sum of the active tick at the time of the observation. This tick accumulator value increases monotonically and grows by the value of the current tick - per second (As per the Uniswap Docs).

Hence it is clear there is an error in the above depicted code snippet since the delta of the two observations (tickCumulatives) is taken in the reverse order tickCumulatives[i] - tickCumulatives[i + 1] where as this should be taken as tickCumulatives[i + 1] - tickCumulatives[i] since the tickCumulatives values increase monotonically.

Since the order of tickCumulatives used to compute the twap is erroneous this will result in wrong value for twap. (Eg: if correct twap value is positive the twapFilter function will return the negative value of it and vice versa).

The PanopticMath.twapFilter function is called by the PanopticPool.getUniV3TWAP function. The PanopticPool.liquidate and PanopticPool.forceExercise functions call the getUniV3TWAP function in their transaction execution flows.

In the case of PanopticPool.liquidate function execution the following issues may occur due to above explained vulnerability.

The liquidate function performs the following check to ensure the maximum allowed delta between the currentTick and the Uniswap TWAP tick during a liquidation is not exceeded (if it exceeds then the transaction will revert).

            if (Math.abs(currentTick - twapTick) > MAX_TWAP_DELTA_LIQUIDATION) 
                revert Errors.StaleTWAP();

Issue here is since the returned twapTick is of opposit sign value the above condition will result in true and transaction will revert even if it should have resulted in false. Because if the correct twapTick is positive the currentTick - twapTick value will be less. But since the returned twapTick by the PanopticMath.twapFilter is negative this will result in currentTick - (-twapTick) => currentTick + twapTick thus increasing on the total which could be > MAX_TWAP_DELTA_LIQUIDATION thus reverting the transaction. Hence this will revert a valid liquidation thus putting the protocol into broken state and a state of insolvency.

Further to the above the liquidate function uses the twapTick value when calling the CollateralTracker.getAccountMarginDetails function. This function is used to calculate the tokenRequirement of the liquidatee to remain healthy in his position. In the execution flow of the getAccountMarginDetails function the _getRequiredCollateralSingleLegNoPartner and _getRequiredCollateralSingleLegPartner functions are called. These functions compute the required amount collateral needed for a specific leg 'index' of the position. And this required amount calculation is dependent on the twapTick value as shown below:

                if (
                    ((atTick >= tickUpper) && (tokenType == 1)) || // strike OTM when price >= upperTick for tokenType=1
                    ((atTick < tickLower) && (tokenType == 0)) // strike OTM when price < lowerTick for tokenType=0
                ) {
                    // position is out-the-money, collateral requirement = SCR * amountMoved
                    required;
                }

                    uint160 ratio = tokenType == 1 // tokenType
                        ? Math.getSqrtRatioAtTick(
                            Math.max24(2 * (atTick - strike), Constants.MIN_V3POOL_TICK)
                        ) // puts ->  price/strike
                        : Math.getSqrtRatioAtTick(
                            Math.max24(2 * (strike - atTick), Constants.MIN_V3POOL_TICK)
                        )

                    if (
                        ((atTick < tickLower) && (tokenType == 1)) || // strike ITM but out of range price < lowerTick for tokenType=1
                        ((atTick >= tickUpper) && (tokenType == 0)) // strike ITM but out of range when price >= upperTick for tokenType=0
                    ) {
                        uint256 c2 = Constants.FP96 - ratio;

                        // compute the tokens required
                        // position is in-the-money, collateral requirement = amountMoved*(1-ratio) + SCR*amountMoved
                        required += Math.mulDiv96RoundingUp(amountMoved, c2);
                    }

Hence as a result the erroneous twapTick value provided to the CollateralTracker.getAccountMarginDetails function could result in wrong required collateral amount returned. Hence this could lead to positions which are unhealthy not being able to be liquidated (if the returned required collateral amount is erroneously larger) and positions which are healthy being liquidated. Hence this leads the Panoptic protocol into a broken state and could make the protocol insolvent since liquidation process is broken.

Similarly the liquidate function uses the twapTick (errorneous value) when calling the _getSolvencyBalances, PanopticMath.getLiquidationBonus functions, which could put the protocol into broken states.

Similarly the twapTick value is used in the PanopticPool.forceExcercise function execution as well which could further prompt the protocol into broken states and undesirable behavior.

Proof of Concept

                twapMeasurement[i] = int24(
                    (tickCumulatives[i] - tickCumulatives[i + 1]) / int56(uint56(twapWindow / 20))
                );

https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/libraries/PanopticMath.sol#L257-L259

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to correct the logic of the PanopticMath.twapFilter function such that the delta of the two adjacent tickCumulatives values are taken in the correct order. The modified code snippet in the PanopticMath.twapFilter should look as follows after the correction.

            for (uint256 i = 0; i < 19; ++i) {
                twapMeasurement[i] = int24(
                    (tickCumulatives[i + 1] - tickCumulatives[i]) / int56(uint56(twapWindow / 20))
                );
            }

Assessed type

Other

c4-judge commented 4 months ago

Picodes marked the issue as primary issue

dyedm1 commented 4 months ago

TickCumulatives are populated in reverse order of time (we increase the secondsAgos for each index in the input array), so this order is actually correct.