code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/OndoPriceOracleV2.sol#L77 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/lending/OndoPriceOracleV2.sol#L277-L301

Vulnerability details

Impact

maxChainlinkOracleTimeDelay is shared across all assets price feeds from Chainlink. This will cause some assets price feeds to have stale prices if the maxChainlinkOracleTimeDelay is set to a high value, and cause some assets price feeds to revert frequently if maxChainlinkOracleTimeDelay is set to a low value.

Proof of Concept

maxChainlinkOracleTimeDelay determines whether the oracle price returned from Chainlink is stale, synonomous to what other protocol calls a heartbeat.

  uint256 public maxChainlinkOracleTimeDelay = 90000; // 25 hours

The initial maxChainlinkOracleTimeDelay is 90000, 25 hours. This is not an issue as setMaxChainlinkOracleTimeDelay can override this value. The issue arises from the fact that all assets price feeds from Chainlink use the same maxChainlinkOracleTimeDeplay. Setting it to 3 hours might result in stale prices as some price feeds have heartbeats of 20 mins. Setting it to 20 mins will cause other price feeds with slower heartbeats to revert frequently due to the small tolerance that their slower heartbeats cannot meet.

  function getChainlinkOraclePrice(
    address fToken
  ) public view returns (uint256) {
    require(
      fTokenToOracleType[fToken] == OracleType.CHAINLINK,
      "fToken is not configured for Chainlink oracle"
    );
    ChainlinkOracleInfo memory chainlinkInfo = fTokenToChainlinkOracle[fToken];
    (
      uint80 roundId,
      int answer,
      ,
      uint updatedAt,
      uint80 answeredInRound
    ) = chainlinkInfo.oracle.latestRoundData();
    require(
      (answeredInRound >= roundId) &&
        (updatedAt >= block.timestamp - maxChainlinkOracleTimeDelay),
      "Chainlink oracle price is stale"
    );
    require(answer >= 0, "Price cannot be negative");
    // Scale to decimals needed in Comptroller (18 decimal underlying -> 18 decimals; 6 decimal underlying -> 30 decimals)
    // Scales by same conversion factor as in Compound Oracle
    return uint256(answer) * chainlinkInfo.scaleFactor;
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend setting different maxChainlinkOracleTimeDelay for different price feeds.

trust1995 commented 1 year ago

I think it's an excellent recommendation but is a little shy of Med severity.

c4-judge commented 1 year ago

trust1995 changed the severity to QA (Quality Assurance)