DittoETH

Summary

No check for round completeness could lead to stale prices and wrong price return value, or outdated price. The functions rely on accurate price feed might not work as expected, sometimes can lead to fund loss. It is also important to provide additional checks that the data is fresh, no check for the updatedAt parameter returned from latestRoundData().

Vulnerability Details

getOraclePrice() baseOracleCircuitBreaker() methods call out to an oracle with latestRoundData() to get the price of some asset. Although the returned timestamp and roundID, is checked, there is no check for round completeness and no additional check for updatedAt parameter.

According to Chainlink's documentation, this function does not error if no answer has been reached but returns 0 or outdated round data. The external Chainlink oracle, which provides index price information to the system, introduces risk inherent to any dependency on third-party data sources. For example, the oracle could fall behind or otherwise fail to be maintained, resulting in outdated data being fed to the index price calculations. Oracle reliance has historically resulted in crippled on-chain systems, and complications that lead to these outcomes can arise from things as simple as network congestion.

Impact

If there is a problem with chainlink starting a new round and finding consensus on the new value for the oracle (e.g. chainlink nodes abandon the oracle, chain congestion, vulnerability/attacks on the chainlink system) consumers of this contract may continue using outdated stale data (if oracles are unable to submit no new round is started).

This could lead to stale prices and wrong price return value, or outdated price.

As a result, the functions rely on accurate price feed might not work as expected, sometimes can lead to fund loss

Tools Used

Manual Review

Recommendations

Add the following checks to getOraclePrice() baseOracleCircuitBreaker() functions to ensure the data is fresh and accurate:

Validate data feed for round completeness:
If answeredInRound is less than roundId, the answer is being carried over.
A timestamp with zero value means the round is not complete and should not be used.
read the updatedAt parameter from the calls to latestRoundData() and verify that it isn't older than basePrice

require(answeredInRound >= roundID, "Stale price");

if (updatedAt < block.timestamp - basePrice) revert PriceOutdated();