code423n4 commented 12 months ago

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Oracles/ChainlinkOracle.sol#L79-L81

Vulnerability details

Impact

Override prices from ChainlinkPriceOracle can be out of date which could cause unwanted liquidations, excess borrowing or arbitrage.

Proof of Concept

In ChainlinkPriceOracle there is a function to override the prices from the chainlink feed:

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Oracles/ChainlinkOracle.sol#L79-L81

File: core/Oracles/ChainlinkOracle.sol

79:        if (prices[address(token)] != 0) {
80:            price = prices[address(token)];
81:        } else {

These prices are set by admin in two similar calls setUnderlyingPrice and setDirectPrice:

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Oracles/ChainlinkOracle.sol#L135-L138

File: core/Oracles/ChainlinkOracle.sol

135:    function setDirectPrice(address asset, uint256 price) external onlyAdmin {
136:        emit PricePosted(asset, prices[asset], price, price);
137:        prices[asset] = price;
138:    }

There is little documentation why the admin would do this but I assume it is in the case where the chainlink feed is unavailable (either not existing or down).

The issue is that for the protocol to work properly, prices needs to be fresh. Otherwise unwanted liquidations or excess borrowing/arbitrage can occur. So for the direct price override to work properly when used it must be updated often and regularly.

If you look at in mip00, which details the protol setup, the admin will be the TemporalGovernor:

https://github.com/code-423n4/2023-07-moonwell/blob/main/test/proposals/mips/mip00.sol#L315-L316

File: test/proposals/mips/mip00.sol

315:        /// set temporal governor as the admin of the chainlink feed
316:        oracle.setAdmin(governor);

TemporalGovernor can only act on cross chain messages. The fastest possible action would be for the guardian to fasttrack the messages but they still need to wait for the cross chain latency. Hence there's a high risk that when the direct price override is used in ChainlinkPriceOracle the prices will lag behind and could be abused.

Tools Used

Manual audit

Recommended Mitigation Steps

Consider reverting instead of using a default price for assets. Or at least have the admin of the ChainklinkPriceOracle be a "local" chain account (i.e. guardian) that can take action faster.

Assessed type

Timing

0xSorryNotSorry commented 11 months ago

The manual intervention to the prices is supposed to be a distress handle. It should not be expected that the prices are fresh as in the normal hearth beat feed.

Could be QA.

c4-pre-sort commented 11 months ago

0xSorryNotSorry marked the issue as low quality report

alcueca commented 11 months ago

Valid QA, and for the sponsor to consider whether this is the best approach to the problem of oracle brakdown.

c4-judge commented 11 months ago

alcueca changed the severity to QA (Quality Assurance)