Should check return data from chainlink aggregators

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-mimo/blob/main/core/contracts/inception/priceFeed/ChainlinkInceptionPriceFeed.sol#L78

Vulnerability details

Impact

The getAssetPrice function in the contract ChainlinkInceptionPriceFeed.sol fetches the asset price from a Chainlink aggregator using the latestRoundData function. However, there are no checks on roundID nor timeStamp, resulting in stale prices. The oracle wrapper calls out to a chainlink oracle receiving the latestRoundData(). It then checks freshness by verifying that the answer is indeed for the last known round. The returned updatedAt timestamp is not checked.

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)

Although there is a timestamp check, Round ID is not checked in the function. That can also cause stale price.

Proof of Concept

1. Navigate to the following contract.

https://github.com/code-423n4/2022-04-mimo/blob/main/core/contracts/inception/priceFeed/ChainlinkInceptionPriceFeed.sol#L78

2. Stale prices could put funds at risk. According to Chainlink's documentation, This function does not error if no answer has been reached but returns 0, causing an incorrect price fed to the PriceOracle. 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 of the AMM. 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.

Medium Severity Issue From The FEI Protocol : https://consensys.net/diligence/audits/2021/09/fei-protocol-v2-phase-1/#chainlinkoraclewrapper-latestrounddata-might-return-stale-results

3. Although there is a timestamp check, Round ID is not checked in the function. That can also cause stale price.

Tools Used

Code Review

Recommended Mitigation Steps

Consider to add checks on the return data with proper revert messages if the price is stale or the round is incomplete, for example:

(uint80 roundID, int256 price, , uint256 timeStamp, uint80 answeredInRound) = ETH_CHAINLINK.latestRoundData();
require(price > 0, "Chainlink price <= 0");
require(answeredInRound >= roundID, "...");
require(timeStamp != 0, "...");

Consider checking the oracle responses updatedAt value after calling out to
chainlinkOracle.latestRoundData() verifying that the result is within an allowed margin of freshness.

[Xuefeng-Zhu]

put @yosriady comment here,

We already have a staleness check by checking the updatedAt return value. Checking that the roundId as the auditor suggested is redundant because they are always equal values.

[m19]

We disagree: in all the PriceFeed contracts staleness is correctly checked using the updatedAt field. Checking for both roundID and answeredInRound is redundant because answeredInRound and roundId are always the same in the latest Chainlink aggregators contracts: https://etherscan.io/address/0x37bc7498f4ff12c19678ee8fe19d713b87f6a9e6#code#F2#L786

  function getRoundData(uint80 _roundId)
    public
    override
    view
    virtual
    returns (
      uint80 roundId,
      int256 answer,
      uint256 startedAt,
      uint256 updatedAt,
      uint80 answeredInRound
    )
  {
    require(_roundId <= 0xFFFFFFFF, V3_NO_DATA_ERROR);
    Transmission memory transmission = s_transmissions[uint32(_roundId)];
    return (
      _roundId,
      transmission.answer,
      transmission.timestamp,
      transmission.timestamp,
      _roundId
    );
  }