the TemporalGovernor.sol can be unpaused when it should still in the pause mood

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L241-L259 https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L274-L290 https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/Governance/TemporalGovernor.sol#L202-L221

Vulnerability details

Impact

the TemporalGovernor implemented in a way that if the guardians pause the system then the system will be paused to period of time and the system will remove the guradians approve till the system reApprove them again and going to the unpause mood, the system really focus on to unpause the contract only if the specifc time is passed in the permissionlessUnpause function. but any guardian can unpause the system even before the required time is passed. more details in POC

Proof of Concept

the guardians can pause the system by calling the togglePause:

    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
    }

and if the system is paused then the guardians should only be allowed to unpause the system only if period of time has passed in the permissionlessUnpause:

function permissionlessUnpause() external whenPaused {
        /// lastPauseTime cannot be equal to 0 at this point because
        /// block.timstamp on a real chain will always be gt 0 and
        /// toggle pause will set lastPauseTime to block.timestamp
        /// which means if the contract is paused on a live network,
        /// its lastPauseTime cannot be 0
        //@audit can be called if required time is passed
        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);
    }

but this won't be true because the guardians(the 19 guardians) can unpause the systme by calling one of these two functionstogglePause and revokeGuardian which allow to unpause the system without need of passing specific time passed.

the togglePause will unpause the system if the system was paused already without need to wait for specific time to passed:

 function togglePause() external onlyOwner {
    //@audit can be called when the contract is paused and set it to the unpause mood
        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
    }

same thing for the revokeGuardian:

function revokeGuardian() external {
        //@audit bad guardian can unpause the systme even if the time not passed !

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

the protocol trust in the guardians but this not recommended and the protocol should think about this case and how to prevent it in case the guardians get hacked or act malicousliy. even it mention in the docs that If Moonbeam governance is compromised, the contract will be paused until governance control is restored. in this case the system should be care about prevent any mistakliy unpause the system:

https://github.com/code-423n4/2023-07-moonwell/blob/main/docs/TEMPORALGOVERNOR.md

the 19 guardians by wormhole: https://wormholecrypto.medium.com/wormhole-guardians-everstake-83afb735cddd

Tools Used

manual review

Recommended Mitigation Steps

recommend to add the same check in the permissionlessUnpause function to avoid unpause the system in bad time and bad case even by mistake or by an malicious guardians.

Assessed type

Governance

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #232

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as partial-50

[c4-judge]

alcueca changed the severity to 2 (Med Risk)