Untrusted Chainlink price is used without a basis for comparison

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/PriceFeed.sol#L291-L294

Vulnerability details

Description

The PriceFeed contract provides the eBtc price for the system using its fetchPrice function. The function uses two oracles, Chainlink and a fallback oracle. The oracles can either be in a working, untrusted or frozen state (only Chainlink can be frozen). When either or both of them are untrusted or frozen, they have to be compared with each other to become trusted again and enter the working state. The comparison

This is further emphasised by a comment in case 4 (Status.usingChainlinkFallbackUntrusted).

            // If Chainlink is live and Fallback is frozen, just use last good price (no status change) since we have no basis for comparison
            if (_fallbackIsFrozen(fallbackResponse)) {
                return lastGoodPrice;
            }

From the comment above we can see that although Chainlink is live it can't be used because fallback oracle is frozen.

But from the same case 4 (Status.usingChainlinkFallbackUntrusted). Chainlink's price is returned despite it being untrusted and fallback's price not being available for comparison because the oracle is broken.

            if (_fallbackIsBroken(fallbackResponse)) {
                _changeStatus(Status.usingChainlinkFallbackUntrusted);
                return _storeChainlinkPrice(chainlinkResponse.answer);
            }

Impact

An untrusted Chainlink price is returned as price of eBtc when Chainlink is untrusted and fallback is broken.

Proof of Concept

You can run this POC in the PriceFeed.stateTransitions.t.sol file.

function testReturnUntrustedPrice() public {
    // steps to make price feed have a usingFallbackChainlinkFrozen
    _frozeChainlink(_mockChainLinkStEthETH);
    priceFeedTester.fetchPrice();
    assertEq(uint(priceFeedTester.status()), uint(IPriceFeed.Status.usingFallbackChainlinkFrozen));

    // break fallback response to ensure there's no basis for comparison
    _breakFallbackResponse();

    // update Chainlink prices to ensure they aren't frozen
    _restoreChainlinkPriceAndTimestamp(_mockChainLinkStEthETH, 8888e14); // set custom price to ensure different prices
    _restoreChainlinkPriceAndTimestamp(_mockChainLinkEthBTC, initEthBTCPrice);

    uint lastGPrice = priceFeedTester.lastGoodPrice();
    uint price = priceFeedTester.fetchPrice();

    assertEq(uint(priceFeedTester.status()), uint(IPriceFeed.Status.usingChainlinkFallbackUntrusted));
    // returned price is not lastGoodPrice 
    assertTrue(price != lastGPrice);
    // price feed returns untrusted Chainlinks price despite not having a base for comparison
    assertEq(price, priceFeedTester.getCurrentChainlinkResponse().answer);
}

Tools Used

Manual Analysis

Recommended Mitigation Steps

Return the lastGoodPrice when no basis for comparison to move an untrusted or frozen oracle to a working state is present.

Assessed type

Oracle

[c4-pre-sort]

bytes032 marked the issue as insufficient quality report

[bytes032]

OOS

[c4-pre-sort]

bytes032 marked the issue as duplicate of #232

[c4-judge]

jhsagd76 marked the issue as unsatisfactory: Invalid