Moving average precision is lost

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/modules/PRICE.sol#L134-L139

Vulnerability details

Now the precision is lost in moving average calculations as the difference is calculated separately and added each time, while it typically can be small enough to lose precision in the division involved.

For example, 10000 moves of 990 size, numObservations = 1000. This will yield 0 on each update, while must yield 9900 increase in the moving average.

Proof of Concept

Moving average is calculated with the addition of the difference:

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/modules/PRICE.sol#L134-L139

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

/ numObs can lose precision as currentPrice - earliestPrice is usually small.

It is returned on request as is:

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/modules/PRICE.sol#L189-L193

    /// @notice Get the moving average of OHM in the Reserve asset over the defined window (see movingAverageDuration and observationFrequency).
    function getMovingAverage() external view returns (uint256) {
        if (!initialized) revert Price_NotInitialized();
        return _movingAverage;
    }

Recommended Mitigation Steps

Consider storing the cumulative sum, while returning sum / numObs on request:

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/modules/PRICE.sol#L189-L193

    /// @notice Get the moving average of OHM in the Reserve asset over the defined window (see movingAverageDuration and observationFrequency).
    function getMovingAverage() external view returns (uint256) {
        if (!initialized) revert Price_NotInitialized();
-       return _movingAverage;
+       return _movingAverage / numObservations;
    }

[Oighty]

Keeping track of the observations as a sum and then dividing is a good suggestion. The price values have 18 decimals and the max discrepancy introduced is very small (10**-15) with expected parameter ranges. The potential risk to the protocol seems low though.

[dmitriia]

Please notice that discrepancy here is unbounded, i.e. the logic itself do not have any max discrepancy, the divergence between fact and recorded value can pile up over time without a limit.

If you do imply that markets should behave in some way that minuses be matched with pluses, then I must say that they really shouldn't.

[0xean]

Debating between QA and Med on this one. I am going to award it as medium because there is a potential to leak some value do this imprecision compounding over time.