Moonwell - bin2chen's results

Lines of code

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

Vulnerability details

Impact

maybe brick the temporalGovernor

Proof of Concept

when execute grantGuardiansPause() , it will set guardianPauseAllowed to true , but don't execute _unpause()

grantGuardiansPause code:

    function grantGuardiansPause() external {
        require(
            msg.sender == address(this),
            "TemporalGovernor: Only this contract can update grant guardian pause"
        );

@>      guardianPauseAllowed = true;
        lastPauseTime = 0;

        emit GuardianPauseGranted(block.timestamp);
    }

This way there may exist a path when Guardian is held hostage， can putting the contract into a state where it cannot be updated.

Example: Suppose there is currently a legitimate pending proposal: VAA_1.calldatas = grantGuardiansPause() If Guardian is being held hostage at this point, then a malicious Guardian could perform the following steps，putting the contract into a state where it cannot be updated.

1. Guardian execute togglePause() -> change to: _paused = true guardianPauseAllowed = false

2. Guardian execute fastTrackProposalExecution(VAA_1)，Since VAA_1 is legal, it will be executed successfully -> change to: guardianPauseAllowed = true

3. Guardian execute Ownable().renounceOwnership() -> change to: owner = address(0)

After three steps, the state of the contract will become: _paused = true guardianPauseAllowed = true owner = address(0)

These three states cause all contract methods to fail to execute Contains: permissionlessUnpause() executeProposal() fastTrackProposalExecution() togglePause() ...

Since not disabling renounceOwnership() is also one of the main reasons.

Tools Used

Recommended Mitigation Steps

grantGuardiansPause() enforce _unpause(), and disable renounceOwnership()/transferOwnership()

    function grantGuardiansPause() external {
        require(
            msg.sender == address(this),
            "TemporalGovernor: Only this contract can update grant guardian pause"
        );

        guardianPauseAllowed = true;
        lastPauseTime = 0;

+       if (paused()) {
+          _unpause();
+       }

        emit GuardianPauseGranted(block.timestamp);
    }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Missing payable, making it impossible to execute proposals that need eth.

Proof of Concept

When the proposal is executed, it takes the value with it The code is as follows.

    function _executeProposal(bytes memory VAA, bool overrideDelay) private {
...

        for (uint256 i = 0; i < targets.length; i++) {
            address target = targets[i];
            uint256 value = values[i];
            bytes memory data = calldatas[i];

            // Go make our call, and if it is not successful revert with the error bubbling up
@>          (bool success, bytes memory returnData) = target.call{value: value}(
                data
            );

            /// revert on failure with error message if any
            require(success, string(returnData));

            emit ExecutedTransaction(target, value, data);
        }
    }    

However, the current contract does not have a receive() method, and executeProposal() and other methods does not have a modifier payable.

This prevents the contract from receiving any eth.

proposals that need eth will never be successfully executed.

Tools Used

Recommended Mitigation Steps

executeProposal()/fastTrackProposalExecution() add payable

-   function executeProposal(bytes memory VAA) public whenNotPaused {
+   function executeProposal(bytes memory VAA) public payable whenNotPaused {
        _executeProposal(VAA, false);
    }


-   function fastTrackProposalExecution(bytes memory VAA) external onlyOwner {
+   function fastTrackProposalExecution(bytes memory VAA) external payable onlyOwner {
        _executeProposal(VAA, true); /// override timestamp checks and execute
    }    

Assessed type

Context