code423n4 commented 12 months ago

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Oracles/ChainlinkCompositeOracle.sol#L183-L191

Vulnerability details

Impact

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.

This is more extreme in lesser-known tokens with fewer Chainlink Price feeds to update the price frequently.

The ChainlinkCompositeOracle.getPriceAndDecimals function calls on the AggregatorV3Interface(oracleAddress).latestRoundData() but does not check the timestamp of the returned price.

The ChainlinkCompositeOracle.getPriceAndDecimals function performs the following two checks to mitigate the stale prices:

price > 0 and answeredInRound == roundId

answeredInRound == roundId - in this case, data may be assumed to be fresh while it might not be because the feed was entirely abandoned by nodes (no one starting a new round). Also, there’s a good chance that many feeds won’t always be super up-to-date (it might be acceptable to allow a threshold). A strict check might lead to transactions failing (race; e.g., round just timed out).

Proof of Concept

        (
            uint80 roundId,
            int256 price,
            ,
            ,
            uint80 answeredInRound
        ) = AggregatorV3Interface(oracleAddress).latestRoundData();
        bool valid = price > 0 && answeredInRound == roundId;
        require(valid, "CLCOracle: Oracle data is invalid");

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Oracles/ChainlinkCompositeOracle.sol#L183-L191

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to allow a deviation of threshold rounds as shown below: roundId + threshold >= answeredInRound

This check alone might still result in stale data to be used if there are no more rounds. Therefore, this should be combined with updatedAt + threshold >= block.timestamp.