Unhandled chainlink revert can lock price oracle access

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-05-bakerfi/blob/main/contracts/oracles/EthOracle.sol#L31

Vulnerability details

Description

Chainlink's multisigs can immediately block access to price feeds at will. Therefore, to prevent denial of service scenarios, it is recommended to query Chainlink price feeds using a defensive approach with Solidity’s try/catch structure. In this way, if the call to the price feed fails, the caller contract is still in control and can handle any errors safely and explicitly.

Refer to https://blog.openzeppelin.com/secure-smart-contract-guidelines-the-dangers-of-price-oracles/ for more information regarding potential risks to account for when relying on external price feed providers.

Link to code:

    function getLatestPrice() public view override returns (IOracle.Price memory price) {
@-->    (, int256 answer, uint256 startedAt, uint256 updatedAt,) = _ethPriceFeed.latestRoundData();
        if ( answer<= 0 ) revert InvalidPriceFromOracle();        
        if ( startedAt ==0 || updatedAt == 0 ) revert InvalidPriceUpdatedAt();    

        price.price = uint256(answer);
        price.lastUpdate = updatedAt;
    }

Similar past issues

• https://github.com/code-423n4/2022-07-juicebox-findings/issues/59
• https://github.com/sherlock-audit/2023-02-blueberry-judging/issues/161

Tools Used

Manual review

Recommended Mitigation Steps

Surround the call to latestRoundData() with try/catch instead of calling it directly and provide a graceful alternative/exit.

Assessed type

Other

[c4-judge]

0xleastwood marked the issue as primary issue

[0xleastwood]

I would not consider this medium severity because the likelihood is extremely low. Downgrading to QA.

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[c4-judge]

0xleastwood marked the issue as grade-a

[c4-judge]

0xleastwood marked the issue as selected for report

[0xleastwood]

Agree with all of warden's QA issues, including #5 and #4

[t0x1cC0de]

Hi @0xleastwood, Can I enquire the reason to downgrade this when traditionally such issues are always graded as Medium?. I have provided reference links (one from C4 itself) in my report too.

Here's another one from C4 Inverse Audit to bolster my case. Please refer the judge's comment here. Can you please have a re-look?

Thanks

[0xleastwood]

It's QA because it assumes some degree of likelihood that the Chainlink multisig is compromised or becomes malicious. These sorts of things have traditionally been treated as QA because the likelihood is ultimately close to zero but not zero;)

[t0x1cC0de]

It's QA because it assumes some degree of likelihood that the Chainlink multisig is compromised or becomes malicious. These sorts of things have traditionally been treated as QA because the likelihood is ultimately close to zero but not zero;)

Could you provide some examples from the past so that I can understand better please? I ask because I did provide two examples where it's just the opposite.

[thebrittfactor]

For awarding purposes, C4 staff have marked as 1st place.