code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The issue lies in the getPrice function inside the ChainlinkOracle contract. The function is intended to retrieve the price of the underlying ERC20 token of a given MToken and it adjusts the price based on the token's decimals before returning the final value. The final returned price is scaled by the quantity decimalDelta which is calculated as the difference between 18 and the number of decimals of the underlying ERC20 token 18 - uint256(token.decimals()). The issue arises when the getPrice function handles underlying ERC20 tokens with more than 18 decimals (which can happen for some tokens), in this case the operation uint256(18).sub(uint256(token.decimals())) will revert (because of underflow check in the SafeMath library) , this means that the protocol will never be able to get the correct price of such tokens and thus all the lending/borrowing markets functionalities will not work.

Proof of Concept

The issue occurs in the getPrice function below :

function getPrice(MToken mToken) internal view returns (uint256 price) {
    EIP20Interface token = EIP20Interface(
        MErc20(address(mToken)).underlying()
    );

    if (prices[address(token)] != 0) {
        price = prices[address(token)];
    } else {
        price = getChainlinkPrice(getFeed(token.symbol()));
    }
    // @audit this will revert for tokens with more than 18 decimals
    uint256 decimalDelta = uint256(18).sub(uint256(token.decimals()));
    // Ensure that we don't multiply the result by 0
    if (decimalDelta > 0) {
        return price.mul(10 ** decimalDelta);
    } else {
        return price;
    }
}

As it can be seen after receiving the price value from getChainlinkPrice, the function calculates the value of decimalDelta which corresponds to the following subtraction : 18 - uint256(token.decimals()), assuming that we always have 18 - uint256(token.decimals()) > 0.

For the calculation the function uses the sub method from the SafeMath library which reverts in case of underflows, this means that if the underlying ERC20 token has more than 18 decimals, we will have token.decimals() > 18 which leads the getPrice function to always revert due to the underflow protection present in the sub function.

This issue will make it impossible to have any lending/borrow market for all the tokens with more than 18 decimals, as it's impossible for the protocol function to retrieve their prices from the oracle.

Tools Used

Manual review

Recommended Mitigation Steps

To solve this issue there are 2 options :

Make sure that all the underlying ERC20 tokens listed in the protocol have less (or equal) than 18 decimals (While this approach guarantees the protocol's compatibility, it might limit its use if any profitable ERC20 token with more than 18 decimals becomes available).
Modify the getPrice function to handle the case where token.decimals() > 18 and allowing the protocol to adapt and avoid encountering a revert situation.