feat: introduce max change per trade

[xenide]

Motivation

Solution

with the implementation of this extra check (max change per trade), there are 4 possible scenarios:

1. raw price doesn't violate both conditions

• simple: clampedPrice = rawPrice

2. raw price only violates rate of change, doesn't violate max change per trade

• use the rate-of-change capped price
• is it possible that the max-change-per-trade capped price is a smaller change than rate-of-change-capped price?
  • e.g. maxChangePerTrade is 2%, rate of change is 3bp/s.
    • trade comes after 30 seconds, doing a 1% change in price.
    • max-change-per-trade capped price is 102%, rate-of-change capped price is 100.9%
  • e.g. maxChangePerTrade is 2%, rate of change is 100bp/s.
    • trade comes after 3 seconds, doing a 1% change in price
    • max-change-per-trade capped price is 102%, rate-of-change capped price is 103%
  • seems that it's not possible

3. raw price only violates max change per trade, doesn't violate rate of change

• use the max-change-per-trade capped price

4. raw violates both rate of change and max change per trade

• should we take the rate-of-change capped or max-change-per-trade capped?
• should choose the closer one? Arithmetic / geometric mean of the two?
  • Arithmetic mean would be cheaper than geometric mean due to gas cost
• Propose to choose the closer one (smaller change)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.21%. Comparing base (5a606bc) to head (5a606bc).

❗ Current head 5a606bc differs from pull request most recent head 18bd916. Consider uploading reports for the commit 18bd916 to get more accurate results

Additional details and impacted files

@@                Coverage Diff                 @@
##           fix/oracle-struct     #228   +/-   ##
==================================================
  Coverage              88.21%   88.21%           
==================================================
  Files                     12       12           
  Lines                    611      611           
==================================================
  Hits                     539      539           
  Misses                    72       72           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[xenide]

I think the auditors missed this. This underflow vuln has got to be a critical, and is live on our production pairs right now

[OliverNChalk]

What's the vuln, it freezes the pair if it overflows? How realistic is 1e18 seconds elapsed, surely that's like super far away?

[xenide]

it's the product of maxChangeRate and aTimeElapsed that's easy to overflow 1e18.

e.g. 86400 (seconds in a day) * 0.00003e18 (0.3bp / s) = 2.592e18 > 1e18

Yes it freezes the pair as it's an arithmetic underflow. I discovered it during regression testing. Kinda weird that it didn't show up before