Open c4-bot-4 opened 4 months ago
Picodes changed the severity to QA (Quality Assurance)
This previously downgraded issue has been upgraded by Picodes
Picodes marked the issue as duplicate of #239
Picodes changed the severity to QA (Quality Assurance)
Lines of code
https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/libraries/PanopticMath.sol#L241-L268
Vulnerability details
Proof of Concept
Take a look at https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/libraries/PanopticMath.sol#L241-L268
The
twapFilter
function in the provided code snippet is designed to compute the Time-Weighted Average Price (TWAP) of a Uniswap V3 pool using data from its oracle. The TWAP is calculated by observing the average price over a series of time intervals and defining the TWAP as the median of those averages. However there are significant discrepancies between the documented behaviour and the actual implementation.The documentation states that the TWAP should be computed over a time window specified by the
twapWindow
parameter, Note that protocol has heavily stated that this would be a 10 minutes window, from subtly hinting it in this instance in the readMe to the current hardcoded configuration ofTWAP_WINDOW
here https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L125-L129Currently
PanopticMath.twapFilter()
is only queried via the PanopticPool's getUniV3TWAP() whose implementation is belowThis obviously means that all instances where this price getter (getUniV3TWAP()) is used has a hardcoded
TWAP_WINDOW
attached to it, i.e in both liquidations and whenever force exercising a single position.Now back to the implementation in
twapFilter()
with the average price observed over a series of time intervals. The intervals are supposed to be evenly spaced within the specified time window, starting from 0 seconds and ending at the duration of the TWAP window. However, the implementation starts the intervals at 30 seconds, which is inconsistent with the documentation and the expected behaviour of a TWAP calculation. The current implementation uses the formulasecondsAgos[i] = uint32(((i + 1) * twapWindow) / 20);
and loops through0 <= i < 20
to calculate the time intervals, running a minimalistic python script POC we can see that this formula, is flawed, run the script below on replit.We get the result
The python POC above shows that This formula results the first interval starting at 30 seconds, which is incorrect, cause this way we get the first
secondsAgos
to be30
and the lastsecondsAgos
to be 600, making the real duration of theTWAP
to be570
seconds, instead of the expected600
seconds.Now, according to the documentation it's only right for the first interval to start at 0 seconds, and the last interval should end at the duration of the TWAP window (600 seconds). Would be key to note that this is the right implementation as is being done in Uniswap, though here we have only two entries for the
secondsAgos[]
array, where secondsAgos[0] is thesecondsAgo
argument passed into theconsult()
function and and secondsAgos[1] = 0, to ensure that the real duration used is (secondsAgo - 0 = senodsAgo)... link to Uniswap's original implementation.The correct formula should instead be
secondsAgos[i] = uint32((i * twapWindow) / 20);
, which ensures that the intervals start at 0 seconds and end at the specified duration, and then thesecondsAgos
also needs to be extended to 21 elements to include thesecondsAgos[0] = 0
this way we start calculating the twap from 0 seconds up until 600 seconds using a 30 seconds interval.Impact
The incorrect implementation of the TWAP calculation in the
twapFilter
function has several implications:Protocol's core pricing logic is broken: Panoptic in multiple instances has stated that the
TWAP_WINDOW
to be used is10 minutes
but code implementation states otherwise.The TWAP values computed by this function will be inaccurate for the 10 minutes duration since the time interval is actually for 9 minutes 30 seconds. This could lead to incorrect pricing information, which could in turn affect trading decisions and potentially lead to financial losses.
Extended broken functionality on other functions, considering this function is queried indirectly via the path
PanopticPool.liquidate() -> getUniV3TWAP() -> PanopticMath.twapFilter()
The discrepancy between the expected and actual behaviour of the TWAP calculation could cause this check inliquidate()
to wrongly revert where as it shouldn't, additionally this could cause the amount of fee paid for force exercising a single position to be off or even cause protocol to assume a wrong amount to be redistributed. Even the value of the liquidation bonus calculated would now be off since the wrongtwapTick
would be used in the calculation.Recommended Mitigation Steps
Apply the suggestions from Proof of Concept
First modify the calculation of the
secondsAgos
array to start at 0 seconds and end at the duration of the TWAP window. This can be achieved by using the correct formulasecondsAgos[i] = uint32((i * twapWindow) / 20);
.Then ensure that the
secondsAgos
array is correctly sized to match the number of intervals being calculated. This would involve adjusting the loop that populates the array to use the correct number of elements, from testing, a 21 elements array should be enough, so the below can be considered a pseudo fixAssessed type
Oracle