ChainLink price data could be stale

[code423n4]

Handle

cmichel

Vulnerability details

Vulnerability Details

There is no check in ExchangeRate.buildExchangeRate if the return values indicate stale data. This could lead to stale prices according to the Chainlink documentation:

• under current notifications: "if answeredInRound < roundId could indicate stale data."
• under historical price data: "A timestamp with zero value means the round is not complete and should not be used."

Impact

Stale prices that do not reflect the current market price anymore could be used which would influence the exchange rate of the assets to ETH. This could lead to issues with free collateral and lead to wrong liquidations/borrows.

Recommendation

Add the recommended checks:

(
    uint80 roundID,
    int256 price,
    ,
    uint256 timeStamp,
    uint80 answeredInRound
) = chainlink.latestRoundData();
require(
    timeStamp != 0,
    “ChainlinkOracle::getLatestAnswer: round is not complete”
);
require(
    answeredInRound >= roundID,
    “ChainlinkOracle::getLatestAnswer: stale data”
);
require(price != 0, "Chainlink Malfunction”);

[jeffywu]

Duplicate of #92

[jeffywu]

After some discussion with @T-Woodward we've downgraded how we feel about the severity of this issue (and its duplicates). I think this is a 0 Non-Critical. While true that Chainlink prices can be stale, the alternative solution is to revert when that is the case. That would mean in times of high price volatility and high gas prices we would be unable to liquidate for periods of time while waiting for the Chainlink price to update.

This would be an adverse result for the protocol because liquidations must happen in a timely manner in order for the protocol to stay solvent. If we were to wait for the Chainlink price to update, the prices may continue to fall and cause accounts to fall into insolvency. We believe it is safer to accept a stale price than to prevent liquidations.