Lack of stale data checks when retrieving price data from Chainlink

Summary

There are no checks in place, inside of the PriceOracle contract that verify that the price returned by the Chainlink price feed is fresh and accurate.

Vulnerability Detail

As stated in Chainlink's documentation:

If answeredInRound is less than roundId, the answer is being carried over. If answeredInRound is equal to roundId, then the answer is fresh.

Since there is no such check in palce after calling the latestRoundData method on Chainlink's feed registry, there is no guarantee that the protocol won't end up using stale price data.

Additionally, there is also no check that validates the updatedAt timestamp value.

Impact

The protocol can potentially end up using stale prices, which can be detrimental in places where more volatile assets are used.

Code Snippet

https://github.com/sherlock-audit/2023-05-ironbank/blob/9ebf1702b2163b55479624794ab7999392367d2a/ib-v2/src/protocol/oracle/PriceOracle.sol#L107

https://github.com/sherlock-audit/2023-05-ironbank/blob/9ebf1702b2163b55479624794ab7999392367d2a/ib-v2/src/protocol/oracle/PriceOracle.sol#L67

Tool used

Manual Review

Recommendation

Consider adding the following checks:

-- (, int256 price, , , ) = registry.latestRoundData(base, quote);
++ (uint80 roundId, int256 price, , uint256 updatedAt, uint80 answeredInRound) = registry.latestRoundData(aggrs[i].base, aggrs[i].quote);
++ require(answeredInRound >= roundId, "Stale price");
++ require(updatedAt >= block.timestamp - maxDelayTimes[keccak256(abi.encodePacked(base, quote))], "Last price update was too long ago.");

Where maxDelayTimes is a mapping that contains the max delay times for each asset pair aggregator that you intend to use (the value for each one should usually be set to the heartbeat of the aggregator, unless the needs of your protocol require else wise).

Duplicate of #9

sherlock-audit / 2023-05-ironbank-judging

Arabadzhiev - Lack of stale data checks when retrieving price data from Chainlink #394