code423n4 commented 1 year ago

Lines of code

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/OracleLib.sol#L14-L31

Vulnerability details

Impact

Chainlink aggregators have a built-in circuit breaker to prevent the price of an asset from deviating outside a predefined price range. This circuit breaker may cause the oracle to persistently return the minPrice instead of the actual asset price in the event of a significant price drop, as witnessed during the LUNA crash.

Proof of Concept

The following library method is heavily used to extract linked aggregators and request round data from them. If an asset's price falls below the minPrice, the protocol continues to value the token at the minPrice rather than its real value. For instance, if TokenA's minPrice is 1 USD and its price falls to $0.10, the aggregator continues to report 1 USD, rendering the related function calls to entail a value that is ten times the actual value.

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/OracleLib.sol#L14-L31

    function price(AggregatorV3Interface chainlinkFeed, uint48 timeout)
        internal
        view
        returns (uint192)
    {
        (uint80 roundId, int256 p, , uint256 updateTime, uint80 answeredInRound) = chainlinkFeed
            .latestRoundData();

        if (updateTime == 0 || answeredInRound < roundId) {
            revert StalePrice();
        }
        // Downcast is safe: uint256(-) reverts on underflow; block.timestamp assumed < 2^48
        uint48 secondsSince = uint48(block.timestamp - updateTime);
        if (secondsSince > timeout) revert StalePrice();

        // {UoA/tok}
        return shiftl_toFix(uint256(p), -int8(chainlinkFeed.decimals()));
    }

defaultThreshold set at the constructor typically does not have a higher limit threshold; and, if there is one, it will be up to FIX_ONE. Depending on the value assigned to defaultThreshold, this minAnswer discrepancy could possibly have the pegPrice still fall between pegBottom and pegTop, and hence dodging the marking of a collateral IFFY status. Consequently, a larger than expected amount of RToken could be issued, fostering an imperceptibly unhealthy basket of collaterals.

It's important to note that while Chainlink oracles form part of the OracleAggregator system and the use of a combination of oracles could potentially prevent such a situation, there's still a risk. Secondary oracles, such as Band, could potentially be exploited by a malicious user who can DDOS relayers to prevent price updates. Once the price becomes stale, the Chainlink oracle's price would be the sole reference, posing a significant risk.

Tools Used

Manual

Recommended Mitigation Steps

As recommended by Chainlink, OracleLib.price should cross-check the returned answer against the minPrice/maxPrice and revert if the answer is outside of these bounds:

        (uint80 roundId, int256 p, , uint256 updateTime, uint80 answeredInRound) = chainlinkFeed
            .latestRoundData();
+        if (p >= maxPrice || p <= minPrice) revert();

This ensures that a false price will not be returned if the underlying asset's value hits the minPrice.

Assessed type

Oracle

c4-judge commented 1 year ago

thereksfour marked the issue as primary issue

tbrent commented 1 year ago

We have not seen this mitigation elsewhere and will be reaching out to chainlink to try to understand more about the issue.

c4-sponsor commented 1 year ago

pmckelvy1 (sponsor) disputed

pmckelvy1 commented 1 year ago

from chainlink: Hello @pmckelvy1 , there's no need to add this step as it would be redundant. If the answer falls outside the min/max values, the price feed won't update. All our feeds are set with a minimum value of 0 and a maximum value that's the highest possible number that can be generated. Please let me know if you've got any more questions. all min/max values are set at 0/UINT_MAX, so this step would be unnecessary

thereksfour commented 1 year ago

According to the sponsor's reply, consider it invalid.