code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/libraries/DataStorage.sol#L12

Vulnerability details

Impact

Due to the volatile nature of the market, and volatility clustering, during 1 day time window, the average price and closely related volatility might not be able to reflect the real market condition. Sometimes the market goes up and down quite a bit, requiring finer granularity of the statistics to have reliable results, otherwise different kinds of markets mixture can only introduce noise. The impacts might be:

the average price(tick) will be inaccurate, and can not timely reflect the market situations.
volatility based on average price will lose accuracy, because average price is the center of volatility.
volatility will introduce another source of inaccuracy due to the fluctuation not uniform distributed over time.
the purpose of adaptively adjust fee based on market conditions, and balance supply and demand would be affected.

Proof of Concept

The time window is hard coded.

// src/core/contracts/libraries/DataStorage.sol
  uint32 public constant WINDOW = 1 days;

Since the crypto market can fluctuate significantly, with abrupt volatility change, the price can deviate quite far within the current 1 day time window. If within the 24 hour period, the first 10 hours the market is quiet, but something drives the market go wired for the next 4 hours, and price go up 10% more, later the calm down again. The average price will be in the middle of 2 extremes. Maybe something like this: 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 120, 110, 105, 115, 110, 110, 110, 110, 110, 110, 110, 110, 110, 110. With the first 10 hours centered at 100, laster 10 hours centered at 110, in the middle severe up and down.

Obviously the statistics derived from mixing these 2 periods will not be reliable.

the average price can not reflect the situation
the volatility uses the average price as the center will also lose effectiveness

Even there is no significant changes in overall price level (the average is stable), the volatility often cluster. Such as during market tumbling period, every participator tend to actively buy or sell, driving the price swing significantly. Or during normal days, traders are not active 24-7, because during day time, working day, more trading will be performed, but much less activities for night time and holidays. The volume and volatility daily and weekly cycle could easily been observed in most exchanges.

Therefore, the volatility clustering and the periodic cycle also ask for more flexibility of the time window. When combining higher activity working hours and lower activity off hours, the statistics features of these 2 periods will be lost for both cases, and the purpose of dynamically adjust fee based on market conditions would be affected.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Allow the time window to be adjusted according to market conditions and other needs.

0xVolosnikov commented 2 years ago

Thank you!

The daily window may indeed not be the most optimal for determining the characteristics of price volatility. We took this into account during development. However, at the moment it is not clear which implementation would be more optimal. We are considering various options for the future.

In my opinion, this is more of a protocol design advice than a specific issue.

0xean commented 2 years ago

Warden continually does a great job of presenting information in these issues, but ultimately this is a subjective question of the optimal window and does come down to protocol design. Going to downgrade to QA and a dupe of #198 the wardens QA report.

code-423n4 / 2022-09-quickswap-findings