Moonwell - markus_ether's results

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

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L288-L289 https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Governance/TemporalGovernor.sol#L256

Vulnerability details

Impact

Upon calling the pause function, the guardian's right to pause the protocol is revoked until the governance or the guardian themselves reinstate the right.

If the right to paused is restored when the contract is still paused, the contract becomes permanently unpausable. The only way to unpause the contract is to remove the guardian from the protocol.

Proof of Concept

As soon as the guardian pauses the contract, the boolean guardianPauseAllowed is reset to false. The governance or the guardian themselves can reinstate the guardian's right to pause by invoking grantGuardiansPause via _executeProposal , flipping guardianPauseAllowed back to true. However, this action stops the the unpausing process, as unpausing requires that guardianPauseAllowed is false.

assert(!guardianPauseAllowed); /// this should never revert, statement for SMT solving

The only viable resolution would be for the guardian to call revokeGuardian, which also unpauses the contract. This action removes the guardian from the protocol by transferring ownership to the null address and can lead to a loss of funds when no guard can stop the protocol during a hack.

A proof of concept that illustrates the issue is shown below:

function testBreakPause() public {
    // start: contract is paused
    assertFalse(governor.paused());
    
    // call togglePause(), now the guardianPauseAllowed = false && lastPauseTime = 0
    governor.togglePause();
    assertTrue(governor.paused());
    assertFalse(governor.guardianPauseAllowed());

    // call fastTrackProposalExecution() to set guardianPauseAllowed = true 
    vm.prank(address(governor)); // done as prank to simplify
    governor.grantGuardiansPause();
    assertTrue(governor.guardianPauseAllowed()); 

    // now the contract is can not be unpaused again
    vm.expectRevert();
    governor.togglePause();

    vm.warp(block.timestamp + governor.permissionlessUnpauseTime());
    vm.expectRevert();
    governor.permissionlessUnpause();

    // unpause contract by revoking guardian
    governor.revokeGuardian();
    assertTrue(governor.paused());
    assertEq(governor.owner(), address(0));
}

Tools Used

Manual review.

Recommended Mitigation Steps

As long as is desired behavior that the guardian should not receive the right to pause the contract (before the contract is unpaused), we should encorce that bevaior in the function grantGuardiansPause by enforcing that it can not be called when the contract is not paused.

-    function grantGuardiansPause() external {
+    function grantGuardiansPause() external whenPaused {
    
    }

As an alternative solution, permissionlessUnpause and togglePause could be adjusted to allow to invoke them, even when guardianPauseAllowed is true.

- assert(!guardianPauseAllowed); 

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L385-L388

Vulnerability details

Impact

The _executeProposal fails to validate that the message received from Wormhole is intended for this contract. A malicous guardian can repurpose a message transmitted via the Wormhole Bridge and execute a illegitimate proposal

Proof of Concept

The Wormhole book suggest various checks when receiving a message. One such check isto ensure that the incoming message is inteded explicitly for the recipient contract. The recipient contract is expected to perform the following verification

require(parseIntendedRecipient(vm.payload) == address(this));

While this check is included in the _queueProposal function, is is absent from _executeProposal. Regular proposals are typically processed through the queuedTransactions workflow, which includes this validation. However, the the overrideDelay option allows a guardian to execute proposals instantly, thereby bypassing this check.

Tools Used

Manual review

Recommended Mitigation Steps

Ensure that the received message was crafted for this contract by including the verification step in the code:

+ address intendedRecipient;
- (, targets, values, calldatas) = abi.decode(
+ (intendedRecipient, targets, values, calldatas) = abi.decode(
     vm.payload,
    (address, address[], uint256[], bytes[])
);

+ // check that VAA is for this contract
+ require(intendedRecipient == address(this), "TemporalGovernor: Incorrect destination");

Assessed type

Invalid Validation