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

Self-revocation of signing role by owner can lead to loss of control

Author Revealed upon completion

Self-revocation of signing role by owner can lead to loss of control

Description

  • The owner can revoke their own signing role via MultiSigTimelock::revokeSigningRole. While this doesn't remove ownership, it prevents them from confirming/revoking transactions or executing proposals.

  • Though it is possible to regain signing role via MultiSigTimelock::grantSigningRole, as only the owner can do this and if the owner loses access to their key, they cannot recover signing capabilities.

// Root cause in the codebase with @> marks to highlight the relevant section
@> function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// CHECKS
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
// Prevent revoking the first signer (would break the multisig), moreover, the first signer is the owner of the contract(wallet)
if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
​
// Find the index of the account in the array
uint256 indexToRemove = type(uint256).max; // Use max as "not found" indicator
for (uint256 i = 0; i < s_signerCount; i++) {
if (s_signers[i] == _account) {
indexToRemove = i;
break;
}
}
​
// Gas-efficient array removal: move last element to removed position
if (indexToRemove < s_signerCount - 1) {
// Move the last signer to the position of the removed signer
s_signers[indexToRemove] = s_signers[s_signerCount - 1];
}
​
// Clear the last position and decrement count
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
​
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
}

Risk

Likelihood:

  • Low. While ownership remains, the owner loses signing capabilities. If this happens accidentally, they cannot participate in the multi-sig process without granting themselves the role again (which they can do as owner). That's why the likelyhood is low.

Impact:

  • High. With ownership gone to a malicious user, the owner loses complete control over the wallet.

Proof of Concept

  • Owner grants signing roles to multiple signers.

  • Owner proposes a transaction.

  • Owner revokes their own signing role.

  • Owner can no longer confirm or execute the proposed transaction.

  • Can only regain signing role by calling grantSigningRole on themselves. If the owner's key is lost, they cannot recover signing capabilities.

Paste the following test case into test/MultiSigTimelock.t.sol to reproduce:

function test_if_Owner_can_revoke_his_signing_role() external {
vm.startPrank(OWNER);
multiSigTimelock.grantSigningRole(SIGNER_TWO);
multiSigTimelock.grantSigningRole(SIGNER_THREE);
multiSigTimelock.grantSigningRole(SIGNER_FOUR);
multiSigTimelock.grantSigningRole(SIGNER_FIVE);
vm.stopPrank();
​
vm.deal(address(multiSigTimelock), OWNER_BALANCE_ONE);
vm.deal(OWNER, OWNER_BALANCE_ONE);
vm.prank(OWNER);
multiSigTimelock.proposeTransaction(SPENDER_ONE, OWNER_BALANCE_ONE, hex"");
​
vm.startPrank(OWNER);
multiSigTimelock.revokeSigningRole(OWNER);
vm.stopPrank();
}

Recommended Mitigation

Add explicit check to prevent owner from revoking their own signing role from MultiSigTimelock::revokeSigningRole

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// CHECKS
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
// Prevent revoking the first signer (would break the multisig), moreover, the first signer is the owner of the contract(wallet)
if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
+ if (_account == owner()) {
+ revert MultiSigTimelock__CannotRevokeOwner();
+ }
​
[...]
_revokeRole(SIGNING_ROLE, _account);
}

Support

FAQs

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

Give us feedback!