code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L332 https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L586 https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L631-L632 https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L728

Vulnerability details

Impact

The Comptroller.sol expects getUnderlyingPrice used in multiple functions to return 0 when stale data or errors occur, the same way how Compounds handle errors with no reverts, but in the current way the oracle is implemented it will revert on errors and it will never return 0 values.

Proof of Concept

As you can see here getUnderlyingPrice gets called different times in the Comptroller https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L332 or https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L631-L632 and as you can see it expects to return 0 values in case of errors https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L333 https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Comptroller.sol#L633-L634 but that will not be the case in the current implementation of getUnderlyingPrice. As you can see getChainlinkPrice used in getUnderlyingPrice it uses require statements to check if the prices are stale, and if those require statement will not pass the whole transaction would revert https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Oracles/ChainlinkOracle.sol#L102-L103 Because of how the implementation is done right now, no 0 value will ever be returned on the getUnderlyingPrice in the Comptroller.sol and function would revert.

Tools Used

Manual review

Recommended Mitigation Steps

Consider implementing the getUnderlyingPrice in a way that would respect the no revert style of Compound comptroller, since you are using a similar version, you can implement try/catch blocks for the prices or if statements, so in the case of stale prices the functions would return 0 values as intended and will not revert. A similar finding can be find here https://github.com/code-423n4/2022-09-canto-findings/issues/93

Assessed type

Other

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as duplicate of #404

c4-judge commented 1 year ago

alcueca changed the severity to QA (Quality Assurance)