Multiple cases being missed in the ```fetchPrice()``` function of PriceFedd.sol

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/PriceFeed.sol#L98

Vulnerability details

Overview

There are some missing edge cases in PriceFeed.sol that would result in returning of stalled or deviated prices.

Proof of Concept

Consider the PriceFeed.sol is deployed and the lastGoodPrice stores the Chainlink response in LOC-85: https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/PriceFeed.sol#L85

_storeChainlinkPrice(chainlinkResponse.answer);

function _storeChainlinkPrice(uint256 _answer) internal returns (uint256) {
        _storePrice(_answer);

        return _answer;
    }

There are multiple cases that are missed in the fetchPrice() function of PriceFedd.sol. Let us understand them one by one.

Case-1: Let us suppose, after deployment, 2 price fetches happen. In the 1st fetch, the Chainlink gets broken and the fallback works. Therefore, the: status becomes usingFallbackChainlinkUntrusted lastGoodPrice becomes the fallback's response, LOC-126: https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/PriceFeed.sol#L126

// --- CASE 1: System fetched last price from Chainlink  ---
        if (status == Status.chainlinkWorking) {
            // If Chainlink is broken, try Fallback
            if (_chainlinkIsBroken(chainlinkResponse, prevChainlinkResponse)) {
                .
                .
                .
                .
                _changeStatus(Status.usingFallbackChainlinkUntrusted);
                return _storeFallbackPrice(fallbackResponse);

Now, let us suppose a scenario in which, during the 2nd fetch (which happens sometime after the 1st fetch), the fallback oracle gets broken and the Chainlink oracle starts working. The status, after the 2nd fetch, should have been "usingChainlinkFallbackUntrusted". However, after the 2nd fetch: status becomes bothOraclesUntrusted lastGoodPrice retains the previous stalled fallback's response LOC-202 https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/PriceFeed.sol#L202

// --- CASE 2: The system fetched last price from Fallback ---
        if (status == Status.usingFallbackChainlinkUntrusted) {
          .
          .
          .
          .
          if (_fallbackIsBroken(fallbackResponse)) {
                _changeStatus(Status.bothOraclesUntrusted);
                return lastGoodPrice;
            }

The fetchPrice() function returns stalled previous fallback response where it could have returned the latest Chainlink oracle response (given the Chainlink oracle has started working in the 2nd fetch).

Case-2: Again suppose 2 price fetches happen after deployment. Let us assume in the 1st fetch, both oracles get broken. So, the: status becomes bothOraclesUntrusted lastGoodPrice retains the Chainlink price at the time of deployment LOC-112 https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/PriceFeed.sol#L112

// --- CASE 1: System fetched last price from Chainlink  ---
        if (status == Status.chainlinkWorking) {
            // If Chainlink is broken, try Fallback
            if (_chainlinkIsBroken(chainlinkResponse, prevChainlinkResponse)) {
                // If Fallback is broken then both oracles are untrusted, so return the last good price
                if (_fallbackIsBroken(fallbackResponse)) {
                    _changeStatus(Status.bothOraclesUntrusted);
                    return lastGoodPrice;
                }

Now, suppose during the 2nd fetch (which happens sometime after the 1st fetch), fallback oracle starts working (Chainlink still doesn't work). The natural action would be to return the latest price fetched from the fallback oracle. However, during the 2nd fetch the: status becomes bothOraclesUntrusted lastGoodPrice retains the Chainlink price at the time of deployment LOC-251 https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/PriceFeed.sol#L251

// --- CASE 3: Both oracles were untrusted at the last price fetch ---
        if (status == Status.bothOraclesUntrusted) {
          .
          .
          return lastGoodPrice;

The fetchPrice() function returns stalled chainlink price (of the time of deployment) when it should have returned the latest fallback's price. This case assumes that the fallback is initialised and is not a zero address.

Case-3: Again suppose 2 fetches happen after deployment. Now, in the 1st fetch, chainlink becomes frozen, but the fallback works. So, the: status becomes usingFallbackChainlinkFrozen lastGoodPrice becomes the fallback's response LOC-146 https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/PriceFeed.sol#L146

// If Chainlink is frozen, try Fallback
            if (_chainlinkIsFrozen(chainlinkResponse)) {
              .
              .
              return _storeFallbackPrice(fallbackResponse);

Now, suppose in the 2nd fetch (which happens after some time the 1st fetch) the chainlink becomes live but gets deviated, that is, it has a price change of > 50% between two consecutive rounds. Suppose the fallback is broken now. Therefore, in this scenario, the fetchPrice() function should return lastGoodPrice and should have a status of "bothOraclesUntrusted" (since both oracles cannot be trusted). However, the: status becomes usingChainlinkFallbackUntrusted lastGoodPrice becomes the chainlink's deviated response LOC-290 https://github.com/code-423n4/2023-10-badger/blob/main/packages/contracts/contracts/PriceFeed.sol#L290

// if Chainlink is live and Fallback is broken, remember Fallback broke, and return Chainlink price
            if (_fallbackIsBroken(fallbackResponse)) {
                _changeStatus(Status.usingChainlinkFallbackUntrusted);
                return _storeChainlinkPrice(chainlinkResponse.answer);
            }

This is super severe if happens. The fetchPrice() function returns a deviated price where it should have returned the cached lastGoodPrice.

NOTE: Cases 1 and 2 are complementary to case-3. This means that, cases 1 and 2 illustrate that falling back on one oracle (assuming it is live) while the other is still broken is not correct (perhaps because there is no way to check if the price provided by the live oracle is deviated or not). However, this very assumption is broken by case-3 where the live oracle's (in our case chainlink) price is returned while the other oracle (fallback in our case) is broken.

Tools Used

Manual review

Impact

The very straightforward impact is that PriceFeed.sol reports wrong prices in some cases. Now, the exact impact depends on which contract uses PriceFeed.sol and how. For eg, one of the many impacts can be the following: Suppose BorrowerOperations.sol wants to calculate ICR of a CDP. It fetches price from PriceFeed.sol. It returns a deviated chainlink price (Case-3 above). The calculated ICR is > MCR. However, an issue would arise when, with the correct un-deviated price, calculated ICR would have been < MCR. This would result in the opening of a super severe CDP. The impact can be HIGH or MEDIUM.

Likelihood

The likelihood depends on the working of both oracles. The mentioned cases should not be very frequent. So, the likelihood seems MEDIUM.

Recommended Mitigation Steps

Either cases-1 and 2 are correct or case-3 is correct. Whichever case is correct, it should be implemented and the other case should be dealt with carefully.

If cases-1 and 2 are not correct then returning of stalled prices should be handled. If case-3 is not correct then deviation of chainlink price should be checked in Case-4 of fetchPrice() function by _chainlinkPriceChangeAboveMax(chainlinkResponse, prevChainlinkResponse).

Assessed type

Oracle

[bytes032]

OOS

[c4-pre-sort]

bytes032 marked the issue as insufficient quality report

[c4-pre-sort]

bytes032 marked the issue as duplicate of #232

[c4-judge]

jhsagd76 marked the issue as unsatisfactory: Invalid