PRICE's getCurrentPrice() can return zero price

[code423n4]

Lines of code

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

Vulnerability details

Currently no price validity check is performed in getCurrentPrice(). This way zero _ohmEthPriceFeed.latestRoundData produced prices will yield zero getCurrentPrice() which will be passed over to the logic. Also, negative OHM price or zero / negative reserve _reserveEthPriceFeed.latestRoundData produced price will yield low level error from uint256 currentPrice = (ohmEthPrice * _scaleFactor) / reserveEthPrice logic.

Passing zero price to the protocol logic can lead to understated moving average values that will break up the downstream logic as it heavily relies on the correct market prices being aggregated to the moving average.

Proof of Concept

Now Oracle read price isn't controlled to be positive:

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

    /// @notice Get the current price of OHM in the Reserve asset from the price feeds
    function getCurrentPrice() public view returns (uint256) {
        if (!initialized) revert Price_NotInitialized();

        // Get prices from feeds
        uint256 ohmEthPrice;
        uint256 reserveEthPrice;
        {
            (, int256 ohmEthPriceInt, , uint256 updatedAt, ) = _ohmEthPriceFeed.latestRoundData();
            // Use a multiple of observation frequency to determine what is too old to use.
            // Price feeds will not provide an updated answer if the data doesn't change much.
            // This would be similar to if the feed just stopped updating; therefore, we need a cutoff.
            if (updatedAt < block.timestamp - 3 * uint256(observationFrequency))
                revert Price_BadFeed(address(_ohmEthPriceFeed));
            ohmEthPrice = uint256(ohmEthPriceInt);

            int256 reserveEthPriceInt;
            (, reserveEthPriceInt, , updatedAt, ) = _reserveEthPriceFeed.latestRoundData();
            if (updatedAt < block.timestamp - uint256(observationFrequency))
                revert Price_BadFeed(address(_reserveEthPriceFeed));
            reserveEthPrice = uint256(reserveEthPriceInt);
        }

        // Convert to OHM/RESERVE price
        uint256 currentPrice = (ohmEthPrice * _scaleFactor) / reserveEthPrice;

        return currentPrice;
    }

getCurrentPrice() produced prices are aggregated into moving average:

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

    function updateMovingAverage() external permissioned {
        // Revert if not initialized
        if (!initialized) revert Price_NotInitialized();

        // Cache numbe of observations to save gas.
        uint32 numObs = numObservations;

        // Get earliest observation in window
        uint256 earliestPrice = observations[nextObsIndex];

        uint256 currentPrice = getCurrentPrice();

updateMovingAverage() is regularly called via heartbeats:

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Heart.sol#L91-L97

    /// @inheritdoc IHeart
    function beat() external nonReentrant {
        if (!active) revert Heart_BeatStopped();
        if (block.timestamp < lastBeat + frequency()) revert Heart_OutOfCycle();

        // Update the moving average on the Price module
        PRICE.updateMovingAverage();

And then it's used downstream in the business logic:

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Operator.sol#L642-L649

    /// @notice Update the prices on the RANGE module
    function _updateRangePrices() internal {
        /// Get latest moving average from the price module
        uint256 movingAverage = PRICE.getMovingAverage();

        /// Update the prices on the range module
        RANGE.updatePrices(movingAverage);
    }

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Operator.sol#L652-L654

    function _addObservation() internal {
        /// Get latest moving average from the price module
        uint256 movingAverage = PRICE.getMovingAverage();

Recommended Mitigation Steps

Consider adding a non-zero Oracle price check, i.e. requiring that ohmEthPriceInt > 0 and reserveEthPriceInt > 0 as otherwise the current Oracle reading is incorrect, being the result of a malfunction on Oracle side, and the moving average shouldn't be updated.

[Oighty]

Duplicate. See comment on #441.