`_movingAverage` may drift

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/PRICE.sol#L36 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/PRICE.sol#L122-L147 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/PRICE.sol#L135-L139 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Heart.sol#L92-L109

Vulnerability details

Impact

The moving average is critical for the RBS-system. Its current calculation allows for compounding drift, randomly as well as maliciously, detaching from the true value, which invalidates the entire system, including affecting the way funds are handled.

Proof of Concept

The moving average is not directly calculated but is updated in differential increments. This allows for drift when the calculations are inaccurate. The moving average is calculated in updateMovingAverage() by adding the difference of the value to be added and the value to be removed from the average, scaled to the number of data points:

if (currentPrice > earliestPrice) {
            _movingAverage += (currentPrice - earliestPrice) / numObs;
        } else {
            _movingAverage -= (earliestPrice - currentPrice) / numObs;
        }

The division by numObs, proposed as 360 per the documentation, floors the change in the moving average at each heart beat by up to 359 each time. Thus the moving average increases too little when it goes up (compared to the first observation in the window) and decreases too little when it goes down. The significance of this is higher the cheaper OHM is compared to the reserve asset (such that we lose effective decimals).

For example, if the price is increasing very slowly, the moving average will remain stationary. But more generally, the discrepancy between the added and subtracted errors due to flooring can be artificially manipulated by manipulating the price within only 360 price points. By causing a greater error in one direction the moving average can be caused to slowly drift as desired. This is facilitated by the fact that anyone can call beat() in Heart.sol to trigger the updating of the moving average.

Tools Used

Code inspection

Recommended Mitigation Steps

Instead of updating the moving average itself, the sum total of the observations in the moving window should be updated and used as the stored record. Thus no rounding is done and an exact record is kept (up to only the inaccuracies in the observations themselves). This sum can then be scaled by dividing by the number of observations when needed as the average.

[Oighty]

See comment on #483 .

[0xean]

dupe of #483