ChainLink latestRoundData() has no check for round completeness

[hats-bug-reporter[bot]]

Github username: -- Submission hash (on-chain): 0x2c4be5daeef51c5489f6a3dc8bcd7bf127846e08e1a164b30ec4add731f25943 Severity: medium severity

Description:

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.

Vulnerability Detail

The getOracleAssetPrice call out to an oracle with latestRoundData() to get the price of some token.There is no check for round completeness.

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. The impacts vary and depends on the specific situation like the following:

Incorrect liquidation some users could be liquidated when they should not no liquidation is performed when there should be

wrong price feed causing inappropriate loan being taken, beyond the current collateral factor

Code Snippet

https://github.com/hats-finance/VMEX-0x41547b88e8d46bfdb6327c0c3ab4b5a1cffb11cd/blob/4bc577756e15418b919a49a1bdec0a98fd39bdbd/packages/contracts/contracts/protocol/oracles/VMEXOracle.sol#L146

https://github.com/hats-finance/VMEX-0x41547b88e8d46bfdb6327c0c3ab4b5a1cffb11cd/blob/4bc577756e15418b919a49a1bdec0a98fd39bdbd/packages/contracts/contracts/protocol/oracles/VMEXOracle.sol#L214

[ksyao2002]

Based on chainlink docs, if a round has not completed, the updatedAt timestamp would be 0. This would trigger our heartbeat check and would revert, as expected. Check https://github.com/hats-finance/VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f/blob/master/packages/contracts/contracts/protocol/oracles/VMEXOracle.sol#LL223C39-L223C39. If this is not what you meant, please provide more details.

Ref: https://docs.chain.link/data-feeds/historical-data

[Nabeel-javaid]

Based on chainlink docs, if a round has not completed, the updatedAt timestamp would be 0. This would trigger our heartbeat check and would revert, as expected. Check https://github.com/hats-finance/VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f/blob/master/packages/contracts/contracts/protocol/oracles/VMEXOracle.sol#LL223C39-L223C39. If this is not what you meant, please provide more details.

Ref: https://docs.chain.link/data-feeds/historical-data

according to the recommendation of this issue https://solodit.xyz/issues/5699 a check for round completeness is still required

also according to this issue https://solodit.xyz/issues/18255, following statement is required

require(answeredInRound == roundId): As the documentation specifies, “If answeredInRound is less than roundId, the answer is being carried over. If answeredInRound is equal to roundId, then the answer is fresh.

[ksyao2002]

Those links are broken, could you try linking them again?

[Nabeel-javaid]

Those links are broken, could you try linking them again?

both of the links are working for me, taking sometime to Load, but they are working. Do you want me to send you ss?

[ksyao2002]

Sure, thanks

[Nabeel-javaid]

This is first link that i sent you, Check the recommendation

And this is the second one

Sure, thanks

[ksyao2002]

Thanks for the recommendations. Chainlink themselves only recommend a check on the timestamp; it's not clear if checking the answeredInRound==roundID actually gives any additional benefit compared to just checking the timestamp. In fact, based on these posts, it appears that answeredInRound is actually deprecated, and checking if it's equal to roundID is futile: https://stackoverflow.com/questions/70011870/understanding-rounds-in-chainlink.

Regardless, I have reached out to chainlink for their suggestion on this. Will update you when I get a response.

[Nabeel-javaid]

hey @ksyao2002 In the dodo-v3 you can see that they have used require(answeredInRound >= roundID

They are currently being audited on sherlock so surely they are using all kind of latest information

Just wanted to give you some source on this issue on mentioned it

[ksyao2002]

This is copied from our conversation with Chainlink:

Hey @kevinyao02 , we looked into this, and the roundId and answeredInRound have the same value in the current version of the OCR Aggregator. https://github.com/smartcontractkit/libocr/blob/82b910bef5c1c95cc7a886091ccfc1895bde76f5/contract/OffchainAggregator.sol#L859 — you’ll notice that the same value is returned in both. The audit comment trail linked to an older version of the docs and this check may have been needed in the FluxEmulator version but in the current version, this would always return true

Looks like this isn't going to be an issue in current Chainlink versions.