Unhandled chainlink revert would lock price oracle access in `price()`

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0x27095c8f3023725b44de9df598d7603ab0c7ff9d0da5d3dd8cc767f458f40979 Severity: medium

Description: Description

The price() function from chainlink library is extensively used in contracts.

    function price(
        Config memory self,
        uint256 stalenessThreshold,
        OrigamiMath.Rounding roundingMode
    ) internal view returns (uint256) {
        (uint80 roundId, int256 feedValue, , uint256 lastUpdatedAt, uint80 answeredInRound) = self.oracle.latestRoundData();

     . . . some code

    }

price() makes use of Chainlink's latestRoundData() to get the latest price. However, there is no fallback logic to be executed when the access to the Chainlink data feed is denied by Chainlink's multisigs. 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.

Referring chainlink documentation on how chainlink services are updated. Please note chainlink multisig holds the power of Chainlink’s multisigs can immediately block access to price feeds at will.

Onchain updates take place at the smart contract level, where a multi-signature safe (multisig) is used to modify onchain parameters relating to a Chainlink service. This can include replacing faulty nodes on a specific oracle network, introducing new features such as Offchain Reporting, or resolving a smart contract logic error. The multisig-coordinated upgradability of Chainlink services involves time-tested processes that balance collusion-resistance with the flexibility required to implement improvements and adjust parameters. Reference link

This chainlink price feeds are passed in constructor with no setter functions and these feeds used in contracts will acts as immutable.

Impact

Call to latestRoundData could potentially revert and make it impossible to query any prices. This could lead to permanent denial of service.

Recommendation to fix

Surround the call to latestRoundData() with try/catch instead of calling it directly. In a scenario where the call reverts, the catch block can be used to call a fallback oracle or handle the error in any other suitable way.

For example understanding:

function getPrice(address priceFeedAddress) external view returns (int256) {
        try AggregatorV3Interface(priceFeedAddress).latestRoundData() returns (
            uint80,         // roundID
            int256 price,   // price
            uint256,        // startedAt
            uint256,        // timestamp
            uint80          // answeredInRound
        ) {
            return price;
        } catch Error(string memory) {            
            // handle failure here:
            // revert, call propietary fallback oracle, fetch from another 3rd-party oracle, etc.
        }
    }

References

1)Openzeppelin reference: 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.

2)Reference news article: https://cryptonews.net/news/defi/20502745/

hats-finance / Origami-0x998f1b716a5022be026ca6b919c0ddf45ca31abd

Unhandled chainlink revert would lock price oracle access in `price()` #20