Owner of TemporalGovernor could revoke ownership without unpausing the contract

[code423n4]

Lines of code

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

Vulnerability details

Impact

The guardian of the TemporalGovernor contract is equal to the owner. The guardian can either revoke himself or be revoked through a governance proposal through the revokeGuardian function, which transfers ownership of the contract to address(0) and unpauses the contract if paused.

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

https://github.com/code-423n4/2023-07-moonwell/blob/4aaa7d6767da3bc42e31c18ea2e75736a4ea53d4/src/core/Governance/TemporalGovernor.sol#L205-L221

However, the owner can also revoke ownership through the renounceOwnership function inherited from the OpenZeppelin contract, in which case the contract would not be unpaused first.

function renounceOwnership() public virtual onlyOwner {
    _transferOwnership(address(0));
} // (lib > openzeppelin-contracts > contracts > access > Ownable.sol)

As the "normal" unpause functionality (togglePause) is only accessible to the owner, the contract would remain in a paused state until permissionlessUnpauseTime, the time delay until the contract can be unpaused by anyone (set to 30 days in deploy script) has passed. During that time it would not be possible to queue or execute any proposals.

The permissionlessUnpause function assumes that guardianPauseAllowed must be false whenever it is called. As a result, the function reverts if guardianPauseAllowed is true:

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

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

The function can only be called when the contract is paused and guardianPauseAllowed is set to false whenever the owner pauses the contract. However, this assumption is incorrect, as guardianPauseAllowed can always be reset to true through a proposal, even when the contract is currently paused. Only the owner can execute proposals when the contract is paused.

This assumption in the comments is also incorrect, as lastPauseTime can be reset to 0 through the grantGuardiansPause function

In conclusion,
(1) the contract could enter a temporary paused state (30 days) if the owner calls the renounceOwnership function when the contract is paused
(2) the contract could enter an irrevokable / permanent paused state if the owner pauses the contract, then reapproves himself as pauser through a fast-tracked proposal (grantGuardiansPause) and then calls the renounceOwnership function. To my understanding, the owner can only reapprove himself if such a proposal passes the vote on the existing governance system.

Proof of Concept

Add these tests to TemporalGovernorExec.t.sol

/** POC for (1)
If the owner revokes ownership through the renounceOwnership function,
the contract remains paused
*/
function test_RenounceOwnershipDoesntUnpause() public {
    governor.togglePause();
    assertTrue(governor.paused());
    governor.renounceOwnership();
    assertTrue(governor.paused());
}

/** POC for (1)
If the owner revokes ownership while paused, the contract can be unpaused after
the permissionlessUnpauseTime has passed
*/
function test_PermissionlessUnpauseSuccedsAfterRenouncingOwnership() public {
    governor.togglePause();
    governor.renounceOwnership();
    vm.warp(block.timestamp + governor.permissionlessUnpauseTime());
    vm.prank(address(123));
    governor.permissionlessUnpause();
    assertEq(governor.lastPauseTime(), 0);
    assertFalse(governor.paused());
}

/**
The owner can reapprove himself as pauser while the contract is paused
*/
function test_OwnerCanReapproveHimself() public {
    // pause the contract
    governor.togglePause();
    assertTrue(governor.paused());

    // encode proposal payload
    address[] memory targets = new address[](1);
    targets[0] = address(governor);

    uint256[] memory values = new uint256[](1);
    values[0] = 0;

    bytes[] memory payloads = new bytes[](1);
    payloads[0] = abi.encodeCall(governor.grantGuardiansPause, ());

    bytes memory payload = abi.encode(
        address(governor),
        targets,
        values,
        payloads
    );

    // set up mock to verify payload correctly
      _setupMock();
        mockCore.setStorage(
        true,
        trustedChainid,
        governor.addressToBytes(admin),
        "reeeeeee",
        payload
    );

    // normal proposal fails because contract is paused
    vm.startPrank(address(123));
    vm.expectRevert("Pausable: paused");
    governor.queueProposal(payload);
    vm.stopPrank();

    // but owner can reapprove himself
    governor.fastTrackProposalExecution(payload);
    assertEq(governor.owner(), address(this));

    assertTrue(governor.guardianPauseAllowed());
    assertEq(governor.lastPauseTime(), 0);
}

/** POC for (2)
The goal of this test is to show that the governor contract could enter a permanent paused
state through this sequence of events:
    1. owner pauses contract
    2. owner reapproves himself as pauser
    3. owner renounces ownership of the contract
    (4. permissionless unpause doesn't work anymore, previous owner can't unpause anymore)
*/
function test_ContractPausedPermanently() public {
    governor.togglePause();
    // encode payload and set up mock
    address[] memory targets = new address[](1);
    targets[0] = address(governor);
    uint256[] memory values = new uint256[](1);
    values[0] = 0;
    bytes[] memory payloads = new bytes[](1);
    payloads[0] = abi.encodeCall(governor.grantGuardiansPause, ());
    bytes memory payload = abi.encode(
        address(governor),
        targets,
        values,
        payloads
    );
    _setupMock();
    mockCore.setStorage(
        true,
        trustedChainid,
        governor.addressToBytes(admin),
        "reeeeeee",
        payload
    );
    // owner calls the grantGuardiansPause function through fasttracked proposal
    governor.fastTrackProposalExecution(payload);
    // revoke ownership
    governor.renounceOwnership();
    // previous owner tries to unpause
    vm.expectRevert("Ownable: caller is not the owner");
    governor.togglePause();
    // someone else tries to unpause after permissionlessUnpauseTime has passed
    vm.warp(block.timestamp + governor.permissionlessUnpauseTime());
    vm.prank(address(123));
    vm.expectRevert();
    governor.permissionlessUnpause();
    assertTrue(governor.paused());
}

Recommendations

Consider overriding the renounceOwnership function so that it reverts in all cases to ensure the ownership can only be renounced through the revokeGuardian function.

function renounceOwnership() public override onlyOwner {
    revert();
}

To address the incorrect invariant:

• consider removing the assert statement in L289 or
• add the whenNotPaused modifier to the grantGuardiansPause function

Assessed type

Governance

[0xSorryNotSorry]

OOS --> [N‑52] Consider disabling renounceOwnership()

[c4-pre-sort]

0xSorryNotSorry marked the issue as low quality report

[alcueca]

Not really related to N-52. More related to the fact that the guardian can brick governance by renouncing when paused.

There are strong preconditions for this vulnerability to be happen, so downgraded to QA.

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-a