MultiSig Timelock
First Flight #55
Beginner FriendlyWallet
100 EXP
View all submissions
Previous Next
Submission Details
Impact: high
Likelihood: medium

Signer Can Voluntarily Renounce Role Leading to Contract State Inconsistency

Author Revealed upon completion

Signer Can Voluntarily Renounce Role Leading to Contract State Inconsistency

Description

The MultiSigTimelock contract inherits OpenZeppelin's AccessControl but does not override the renounceRole function. Any account holding the SIGNING_ROLE can call renounceRole on its own to abandon the signer identity, which causes an inconsistency between the contract's internal signer array and the AccessControl role system state. The contract's custom grantSigningRole function checks the s_isSigner mapping, preventing the administrator from re-granting permissions to accounts that have renounced their roles.

// Original function in OpenZeppelin AccessControl
function renounceRole(bytes32 role, address callerConfirmation) public virtual {
if (callerConfirmation != _msgSender()) {
revert AccessControlBadConfirmation();
}
​
_revokeRole(role, callerConfirmation); // @> Removes the role from AccessControl
}
​
// State management in MultiSigTimelock
function revokeSigningRole(address _account) external onlyOwner {
// ... Complex array removal logic
s_isSigner[_account] = false; // @> Updates internal mapping
_revokeRole(SIGNING_ROLE, _account); // @> Calls parent class role revocation
}
​
function grantSigningRole(address _account) external onlyOwner {
if (s_isSigner[_account]) { // @> Checks internal mapping
revert MultiSigTimelock__AccountIsAlreadyASigner();
}
// ... Addition logic
s_isSigner[_account] = true;
_grantRole(SIGNING_ROLE, _account);
}

Risk

Likelihood:

  • Signers accidentally or maliciously call renounceRole(SIGNING_ROLE, self)

  • Automated scripts incorrectly invoke the role renunciation function

  • Attackers proactively renounce the role after compromising a signer's account

Impact:

  • The signer is removed from the AccessControl system, but the contract's internal s_isSigner mapping remains true

  • Administrators cannot re-grant permissions because grantSigningRole will revert due to s_isSigner[_account] == true

  • The contract enters an inconsistent state: the signer exists in the array but has no actual signing permissions

  • May cause miscalculations of the required number of signatures, rendering the multi-signature wallet inoperable

Proof of Concept

  • Add the test_revokeSigningRole_ByAccessControl function to test/unit/MultiSigTimelockTest.t.sol as follows:

function test_revokeSigningRole_ByAccessControl() public grantSigningRoles {
// SIGNER_TWO accidentally revokes permissions on its own
vm.startPrank(SIGNER_TWO);
multiSigTimelock.renounceRole(multiSigTimelock.getSigningRole(), SIGNER_TWO);
vm.stopPrank();
// OWNER attempts to re-grant permissions but fails
vm.startPrank(OWNER);
vm.expectRevert(MultiSigTimelock.MultiSigTimelock__AccountIsAlreadyASigner.selector);
multiSigTimelock.grantSigningRole(SIGNER_TWO);
vm.stopPrank();
}

  • Run in the console: forge test --mt test_revokeSigningRole_ByAccessControl -vv

Ran 1 test for test/unit/MultiSigTimelockTest.t.sol:MultiSigTimeLockTest
[PASS] test_revokeSigningRole_ByAccessControl() (gas: 309334)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.78ms (193.40µs CPU time)

Recommended Mitigation

// Solution 1: Override the renounceRole function (Recommended)
function renounceRole(bytes32 role, address callerConfirmation) public virtual override {
// Prohibit renunciation of the SIGNING_ROLE
if (role == SIGNING_ROLE) {
revert MultiSigTimelock__CannotRenounceSigningRole();
}
super.renounceRole(role, callerConfirmation);
}

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!