code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

fastTrackProposalExecution() is used to execute specific proposal while TemporalGovernor is paused. There is invariant in contract that guardian must be not allowed to pause when contract is paused. However this invariant can be broken unintentionally. And the only way to recover in this situation is to revoke guardian. Then protocol won't be able to execute fastTrackProposals and to pause governance.

Proof of Concept

Let's take a look at the following scenario: 1) Emergency situation occurred, one of these assumptions is broken: https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L16-L19

/// There are a few assumptions that are made in this contract:
/// 1. Wormhole is secure and will not send malicious messages or be deactivated.
/// 2. Moonbeam is secure.
/// 3. Governance on Moonbeam cannot be compromised.

2) First of all guardian pauses contract, and now guardianPauseAllowed = false: https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L270-L290

    function togglePause() external onlyOwner {
        if (paused()) {
            _unpause();
        } else {
            require(
                guardianPauseAllowed,
                "TemporalGovernor: guardian pause not allowed"
            );

            guardianPauseAllowed = false;
            lastPauseTime = block.timestamp.toUint248();
            _pause();
        }

        /// statement for SMT solver
        assert(!guardianPauseAllowed); /// this should be an unreachable state
    }

3) Then guardian executes fastTrackProposal, it contains action grantGuardiansPause() to allow pausing again. Now guardianPauseAllowed = true https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L187-L198

    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);
    }

4) As a result, there was no adequate option left to unpause protocol.

There are 3 ways to call unpause: 1) togglePause() doesn't pass assert statement, because guardianPauseAllowed == true

    function togglePause() external onlyOwner {
        if (paused()) {
            _unpause();
        } else {
            require(
                guardianPauseAllowed,
                "TemporalGovernor: guardian pause not allowed"
            );

            guardianPauseAllowed = false;
            lastPauseTime = block.timestamp.toUint248();
            _pause();
        }

        /// statement for SMT solver
        assert(!guardianPauseAllowed); /// this should be an unreachable state
    }

2) permissionlessUnpause() also doesn't pass assert statement

    function permissionlessUnpause() external whenPaused {
        require(
            lastPauseTime + permissionlessUnpauseTime <= block.timestamp,
            "TemporalGovernor: not past pause window"
        );

        lastPauseTime = 0;
        _unpause();

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

        emit PermissionlessUnpaused(block.timestamp);
    }

3) The only way left is to call revokeGuardian(). But you agree this is not ideal

    function revokeGuardian() external {
        address oldGuardian = owner();
        require(
            msg.sender == oldGuardian || msg.sender == address(this),
            "TemporalGovernor: cannot revoke guardian"
        );

        _transferOwnership(address(0));
        guardianPauseAllowed = false;
        lastPauseTime = 0;

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

        emit GuardianRevoked(oldGuardian);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Ensure that current state is unpaused in grantGuardiansPause():

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

        guardianPauseAllowed = true;
        lastPauseTime = 0;

        emit GuardianPauseGranted(block.timestamp);
    }

Assessed type

Governance

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as duplicate of #232

c4-judge commented 1 year ago

alcueca marked the issue as satisfactory