search

sherlock-audit / 2023-10-aloe-judging

9 stars 6 forks source link

feelereth - The metric calculation in consult() is vulnerable to manipulation further back than 2*UNISWAP_AVG_WINDOW. #114

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

feelereth

high

The metric calculation in consult() is vulnerable to manipulation further back than 2*UNISWAP_AVG_WINDOW.

Summary

The metric calculation in consult() assumes manipulation only happened in the last 2*UNISWAP_AVG_WINDOW seconds. An attacker could manipulate further back to evade detection

Vulnerability Detail

The metric calculation in consult() is vulnerable to manipulation further back than 2*UNISWAP_AVG_WINDOW. Here is how the vulnerability works: The key part of the metric calculation is:

   // Compute arithmetic mean tick over the interval [-2w, 0)
   int256 meanTick0To2W = (tickCumulatives[2] - tickCumulatives[0]) / int32(UNISWAP_AVG_WINDOW * 2);

   // Compute arithmetic mean tick over the interval [-2w, -w]  
   int256 meanTickWTo2W = (tickCumulatives[1] - tickCumulatives[0]) / int32(UNISWAP_AVG_WINDOW);

   // The metric compares the difference in the above means
   metric = uint56(SoladyMath.dist(meanTick0To2W, meanTickWTo2W));

The key assumption is that only manipulation in the last UNISWAP_AVG_WINDOW is reflected in meanTick0To2W. An attacker could manipulate further back, in the range [-2*UNISWAP_AVG_WINDOW, -UNISWAP_AVG_WINDOW]. This would affect meanTickWTo2W but NOT meanTick0To2W. So the difference and metric would be small, evading detection. The impact is allowing manipulation without detection. An attacker could systematically manipulate in alternating windows to suppress the metric.

Impact

The effect is that price manipulation attacks could go undetected if the manipulation happened further back than 2*UNISWAP_AVG_WINDOW seconds. This could allow attackers to manipulate prices without being caught. The severity depends on the value of UNISWAP_AVG_WINDOW, but I would categorize this as a high severity issue since it

Code Snippet

https://github.com/sherlock-audit/2023-10-aloe/blob/main/aloe-ii/core/src/libraries/Oracle.sol#L98-L100 https://github.com/sherlock-audit/2023-10-aloe/blob/main/aloe-ii/core/src/libraries/Oracle.sol#L135

Tool used

Manual Review

Recommendation

The metric should compare current values versus a longer historical baseline, rather than just recent windows. A suggestive example:

   // Calculate metric versus 30-day moving average rather than just 2*UNISWAP_AVG_WINDOW
   int256 historicalMeanTick = ...

   metric = uint56(SoladyMath.dist(meanTick0To2W, historicalMeanTick));

This would detect large deviations versus a longer history, making manipulation harder. Other options like expanding the windows, detecting threshold breaches, or adding reference checks could also help strengthen monitoring.

sherlock-admin2 commented 1 year ago

2 comment(s) were left on this issue during the judging contest.

panprog commented:

invalid, because any manipulation in the "older" period will also be detected while it's more recent and will trigger trading pause, besides manipulations in older time windows won't affect the average price (because it's calculated over AVG_UNISWAP_WINDOW)

MohammedRizwan commented:

valid

haydenshively commented 1 year ago

Invalid because any manipulation/data outside of the 2 * UNISWAP_AVG_WINDOW period is irrelevant and does not impact the calculations in any way. Not a duplicate of #40, which is valid and deals with a separate issue.