Connext Amarok contest - cccz's results

Chainlink's latestRoundData might return stale or incorrect results

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L122-L140

Vulnerability details

Impact

On ConnextPriceOracle.sol, we are using latestRoundData, but there is no check if the return value indicates stale data.

  function getPriceFromChainlink(address _tokenAddress) public view returns (uint256) {
    AggregatorV3Interface aggregator = aggregators[_tokenAddress];
    if (address(aggregator) != address(0)) {
      (, int256 answer, , , ) = aggregator.latestRoundData();

      // It's fine for price to be 0. We have two price feeds.
      if (answer == 0) {
        return 0;
      }

This could lead to stale prices according to the Chainlink documentation:

https://docs.chain.link/docs/historical-price-data/#historical-rounds https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round

Proof of Concept

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L122-L140

Tools Used

None

Recommended Mitigation Steps

  function getPriceFromChainlink(address _tokenAddress) public view returns (uint256) {
    AggregatorV3Interface aggregator = aggregators[_tokenAddress];
    if (address(aggregator) != address(0)) {
-      (, int256 answer, , , ) = aggregator.latestRoundData();
+    (uint80 roundID, int256 answer, , uint256 timestamp, uint80 answeredInRound) = oracle.latestRoundData();
+    require(answeredInRound >= roundID, "Stale price");
+    require(timestamp != 0,"Round not complete");
+    require(answer > 0,"Chainlink answer reporting 0");