MultiSig Timelock
First Flight #55
Beginner FriendlyWallet
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Revoked Signers' Confirmations Remain Valid After Role Revocation

chavanrutik133

Root + Impact

Description

When a signer is revoked via revokeSigningRole, their confirmations on pending transactions remain counted, but they cannot revoke them because revokeConfirmation requires onlyRole(SIGNING_ROLE). Execution only checks the confirmation count, not whether confirmers are still active signers.

// Root cause in the codebase with @> marks to highlight the relevant section
function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
​
// CHECKS
// 1. Check if enough confirmations
if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
}
// @audit Missing validation: Does not verify confirmations are from active signers
// @audit Revoked signers' confirmations remain valid
// 2. Check if timelock period has passed
uint256 requiredDelay = _getTimelockDelay(txn.value);
uint256 executionTime = txn.proposedAt + requiredDelay;
if (block.timestamp < executionTime) {
revert MultiSigTimelock__TimelockHasNotExpired(executionTime);
}
// ... rest of function
}

Comparison with revocation:

  • revokeSigningRole (line 209) properly removes signers from s_signers array and s_isSigner mapping

  • However, it does not clear their confirmations from pending transactions in s_signatures mapping

  • Revoked signers cannot revoke their own confirmations because revokeConfirmation requires onlyRole(SIGNING_ROLE) (line 282)

Risk

Likelihood:

  • Occurs during normal operations

  • Timelocks create windows for revocation

  • No attacker needed

  • Common in production multisig wallets

Impact:

  • Security Model Violation: Transactions can execute with confirmations from revoked/inactive signers, breaking the fundamental security assumption that only active signers should influence execution

  • Permanent Confirmations: Revoked signers cannot revoke their confirmations because they lose SIGNING_ROLE, leaving their confirmations permanently counted

  • Operational Risk: If a signer is revoked due to compromise or misconduct, their prior confirmations remain valid, potentially allowing malicious transactions to execute

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 PoC_RevokedSignerConfirmations is Test {
MultiSigTimelock public multiSig;
address owner = address(this);
address signerA = makeAddr("signerA");
address signerB = makeAddr("signerB");
address signerC = makeAddr("signerC");
address recipient = makeAddr("recipient");
​
function setUp() public {
multiSig = new MultiSigTimelock();
multiSig.grantSigningRole(signerA);
multiSig.grantSigningRole(signerB);
multiSig.grantSigningRole(signerC);
}
​
function test_RevokedSignerConfirmationRemainsValid() public {
// Step 1: Propose transaction
uint256 txnId = multiSig.proposeTransaction(recipient, 1 ether, "");
// Step 2: SignerA confirms
vm.prank(signerA);
multiSig.confirmTransaction(txnId);
// Step 3: SignerB confirms
vm.prank(signerB);
multiSig.confirmTransaction(txnId);
// Step 4: Revoke signerA BEFORE third confirmation
multiSig.revokeSigningRole(signerA);
// Step 5: Verify signerA can no longer revoke (they lost SIGNING_ROLE)
vm.prank(signerA);
vm.expectRevert(); // Reverts: AccessControl error
multiSig.revokeConfirmation(txnId);
// Step 6: SignerC confirms (now we have 3 confirmations)
vm.prank(signerC);
multiSig.confirmTransaction(txnId);
// Step 7: Owner confirms (4th confirmation)
multiSig.confirmTransaction(txnId);
// Step 8: Execute transaction - SUCCEEDS even though signerA was revoked!
vm.deal(address(multiSig), 1 ether);
vm.warp(block.timestamp + 1 days); // Wait for timelock
multiSig.executeTransaction(txnId);
// Transaction executed successfully with confirmation from revoked signer!
assertTrue(multiSig.getTransaction(txnId).executed);
}
​
function test_ExecuteDoesNotCheckActiveSigners() public {
// This demonstrates that execution doesn't validate confirmers are active
uint256 txnId = multiSig.proposeTransaction(recipient, 1 ether, "");
// All signers confirm
vm.prank(signerA);
multiSig.confirmTransaction(txnId);
vm.prank(signerB);
multiSig.confirmTransaction(txnId);
vm.prank(signerC);
multiSig.confirmTransaction(txnId);
// Revoke all signers except owner
multiSig.revokeSigningRole(signerA);
multiSig.revokeSigningRole(signerB);
multiSig.revokeSigningRole(signerC);
// Verify they're revoked
assertFalse(multiSig.hasRole(multiSig.getSigningRole(), signerA));
// But transaction can still execute with their confirmations!
vm.deal(address(multiSig), 1 ether);
vm.warp(block.timestamp + 1 days);
multiSig.executeTransaction(txnId); // This succeeds!
assertTrue(multiSig.getTransaction(txnId).executed);
}
}

Steps to Reproduce:

  1. Deploy MultiSigTimelock contract and grant SIGNING_ROLE to multiple signers (e.g., signerA, signerB, signerC)

  1. Propose a transaction with a timelock (e.g., 1 ETH → 1 day delay)

  1. Have signerA and signerB confirm the transaction

  1. Revoke signerA's role via revokeSigningRole(signerA) (e.g., due to compromised key)

  1. Verify signerA cannot revoke their confirmation (reverts due to missing SIGNING_ROLE)

  1. Have signerC confirm the transaction (now has 3 confirmations, including revoked signerA)

  1. Wait for timelock period to expire

  1. Execute the transaction - it succeeds despite signerA being revoked

Recommended Mitigation

Add validation in _executeTransaction to ensure all confirmations come from active signers:

function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
​
// CHECKS
// 1. Check if enough confirmations
if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
}
+
+ // 2. Verify all confirmations are from active signers
+ uint256 activeConfirmations = 0;
+ for (uint256 i = 0; i < s_signerCount; i++) {
+ if (s_signatures[txnId][s_signers[i]] && s_isSigner[s_signers[i]]) {
+ activeConfirmations++;
+ }
+ }
+ if (activeConfirmations < REQUIRED_CONFIRMATIONS) {
+ revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, activeConfirmations);
+ }
- // 2. Check if timelock period has passed
+ // 3. Check if timelock period has passed
uint256 requiredDelay = _getTimelockDelay(txn.value);
uint256 executionTime = txn.proposedAt + requiredDelay;
if (block.timestamp < executionTime) {
revert MultiSigTimelock__TimelockHasNotExpired(executionTime);
}
if (txn.value > address(this).balance) {
revert MultiSigTimelock__InsufficientBalance(address(this).balance);
}
​
// EFFECTS
- // 3. Mark as executed BEFORE the external call (prevent reentrancy)
+ // 4. Mark as executed BEFORE the external call (prevent reentrancy)
txn.executed = true;
​
// INTERACTIONS
- // 4. Execute the transaction
+ // 5. Execute the transaction
(bool success,) = payable(txn.to).call{value: txn.value}(txn.data);
if (!success) {
revert MultiSigTimelock__ExecutionFailed();
}
​
- // 5. Emit eventt
+ // 6. Emit event
emit TransactionExecuted(txnId, txn.to, txn.value);
}
Updates

Lead Judging Commences

kelechikizito Lead Judge 4 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Stale Confirmation Vulnerability/Ghost Voting Issue

Support

FAQs

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

Give us feedback!