code423n4 commented 1 year ago

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.