code423n4 commented 11 months ago

Lines of code

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

Vulnerability details

Impact

Comptroller expect any error getUnderlyingPrice price will return 0, but it can be revert

Proof of Concept

Based on this

File: Comptroller.sol
332:         if (oracle.getUnderlyingPrice(MToken(mToken)) == 0) {
333:             return uint(Error.PRICE_ERROR);
334:         }

File: Comptroller.sol
586:             vars.oraclePriceMantissa = oracle.getUnderlyingPrice(asset);
587:             if (vars.oraclePriceMantissa == 0) {
588:                 return (Error.PRICE_ERROR, 0, 0);
589:             }

File: Comptroller.sol
631:         uint priceBorrowedMantissa = oracle.getUnderlyingPrice(MToken(mTokenBorrowed));
632:         uint priceCollateralMantissa = oracle.getUnderlyingPrice(MToken(mTokenCollateral));
633:         if (priceBorrowedMantissa == 0 || priceCollateralMantissa == 0) {
634:             return (uint(Error.PRICE_ERROR), 0);
635:         }

File: Comptroller.sol
727:         // If collateral factor != 0, fail if price == 0
728:         if (newCollateralFactorMantissa != 0 && oracle.getUnderlyingPrice(mToken) == 0) {
729:             return fail(Error.PRICE_ERROR, FailureInfo.SET_COLLATERAL_FACTOR_WITHOUT_PRICE);
730:         }

The Comptroller is expecting oracle.getUnderlyingPrice to return 0 for errors (Compound style returns, no revert).

However, the current implementation will revert when errored: getUnderlyingPrice -> getChainlinkPrice

File: ChainlinkOracle.sol
097:     function getChainlinkPrice(
098:         AggregatorV3Interface feed
099:     ) internal view returns (uint256) {
100:         (, int256 answer, , uint256 updatedAt, ) = AggregatorV3Interface(feed)
101:             .latestRoundData();
102:         require(answer > 0, "Chainlink price cannot be lower than 0");
103:         require(updatedAt != 0, "Round is in incompleted state");
104:
105:         // Chainlink USD-denominated feeds store answers at 8 decimals
106:         uint256 decimalDelta = uint256(18).sub(feed.decimals());
107:         // Ensure that we don't multiply the result by 0
108:         if (decimalDelta > 0) {
109:             return uint256(answer).mul(10 ** decimalDelta);
110:         } else {
111:             return uint256(answer);
112:         }
113:     }

this getChainlinkPrice contains two require statement, which open for a revert

Reference: https://github.com/code-423n4/2022-09-canto-findings/issues/93

Tools Used

Manual analysis

Recommended Mitigation Steps

Instead of revert, return 0

Assessed type

Error

c4-pre-sort commented 11 months ago

0xSorryNotSorry marked the issue as primary issue

ElliotFriedman commented 11 months ago

tx fails anyway if price is invalid, either by comptroller returning or oracle reverting. doesn't matter either way how it goes down because once a failure happens, no state changes will be made.

c4-sponsor commented 11 months ago

ElliotFriedman marked the issue as sponsor disputed

alcueca commented 11 months ago

Valid QA, recommend to use consistent errors throughout codebase.

c4-judge commented 11 months ago

alcueca changed the severity to QA (Quality Assurance)