Potential Exploitation due to Lack of Price Range Checks in Oracle Implementation

[code423n4]

Lines of code

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

Vulnerability details

Impact

Chainlink aggregators have a built in circuit breaker if the price of an asset goes outside of a predetermined price band. The result is that if an asset experiences a huge drop in value (i.e. LUNA crash) the price of the oracle will continue to return the minPrice instead of the actual price of the asset. This would allow user to continue exploring the protocol with the asset but at the wrong price. This is exactly what happened to Venus on BSC when LUNA imploded.

The wrong price may be returned in the event of a market crash. An adversary will then be able to use this to get the wrong price and take advantage of the protocol.

Proof of Concept

Note there is only a check for price to be non-negative and if the answeredInRound is equal to roundId (which is a small issue itself) but no check if the price is within an acceptable range.

    function getPriceAndDecimals(
        address oracleAddress
    ) public view returns (int256, uint8) {
        (
            uint80 roundId,
            int256 price,
            ,
            ,
            uint80 answeredInRound
        ) = AggregatorV3Interface(oracleAddress).latestRoundData();
        bool valid = price > 0 && answeredInRound == roundId;
        require(valid, "CLCOracle: Oracle data is invalid");
        uint8 oracleDecimals = AggregatorV3Interface(oracleAddress).decimals();

        return (price, oracleDecimals); /// price always gt 0 at this point
    }

Tools Used

Manual review

Recommended Mitigation Steps

Implement a proper check for each asset [or at least very important ones]. It must revert in the case of bad price.

Assessed type

Oracle

[0xSorryNotSorry]

The implementation does not set a min/max value by design. Also Chainlink does not return min/max price as per the AggregatorV3 docs HERE contrary to the reported below;

ChainlinkAggregators have minPrice and maxPrice circuit breakers built into them.

Further proof required as per the context.

[c4-pre-sort]

0xSorryNotSorry marked the issue as low quality report

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as duplicate of #340

[c4-judge]

alcueca marked the issue as duplicate of #340