code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/4aaa7d6767da3bc42e31c18ea2e75736a4ea53d4/src/core/Oracles/ChainlinkOracle.sol#L100-L101 https://github.com/code-423n4/2023-07-moonwell/blob/4aaa7d6767da3bc42e31c18ea2e75736a4ea53d4/src/core/Oracles/ChainlinkCompositeOracle.sol#L183-L189

Vulnerability details

Impact

The ChainlinkCompositeOracle and ChainlinkOracle do not verify if the prices fall within the permissible minimum and maximum values established by Chainlink. Without these checks, prices outside of the extreme ends (i.e., minimum and maximum) will be not accurately reported and when lending for these assets is enabled the protocol can be drained.

Proof of Concept

Chainlink aggregators have a built in circuit breaker when 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.
In its current form, the getChainlinkPrice() / getPriceAndDecimals() function retrieve the latest round data from Chainlink. If the asset's market price plummets below minAnswer or skyrockets above maxAnswer, the returned price will still be minAnswer or maxAnswer, respectively, rather than the actual market price. This could potentially lead to an exploitation scenario where the protocol accounts the asset using incorrect price information.

    function getChainlinkPrice(
        AggregatorV3Interface feed
    ) internal view returns (uint256) {
        (, int256 answer, , uint256 updatedAt, ) = AggregatorV3Interface(feed)
            .latestRoundData();

Illustration:

Present price of TokenA is $10 TokenA has a minimum price set at $1 on chainlink The actual price of TokenA dips to $0.10 The aggregator continues to report $1 as the price. Consequently, users can interact with protocol using TokenA as though it were still valued at $1, which is a tenfold overestimate of its real market value.

Tools Used

Manual review

Recommended Mitigation Steps

Compare the current price and compare it to the minPrice/maxPrice and revert when the price reached the bounds.

    (, int256 answer, , uint256 updatedAt, ) = AggregatorV3Interface(feed) .latestRoundData();
+ IChainlinkAggregator aggregator = IChainlinkAggregator(IChainlinkPriceFeed(source).aggregator());
+ require(price > int256(aggregator.minAnswer()) && price < int256(aggregator.maxAnswer());

Alternatively a fallback oracle can be used.

Assessed type

Oracle

0xSorryNotSorry commented 1 year ago

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.

Further proof required as per the context.

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as low quality report

alcueca commented 1 year ago

The mitigation is wrong, but the finding is valid.

c4-judge commented 1 year ago

alcueca marked the issue as satisfactory