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

Confirmations From Revoked Signers Still Count Toward Quorum

Author Revealed upon completion

Description

  • When a signer's role is revoked via revokeSigningRole(), their previous confirmations on pending transactions are not invalidated.

  • This means a revoked signer's confirmation still counts toward the required 3 confirmations, even though they are no longer authorized to participate in the multisig.

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// ... validation checks ...
// Find and remove from array
// ...
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
// @> ROOT CAUSE: No invalidation of existing confirmations!
// @> s_signatures[txnId][_account] remains true for all txnIds they confirmed
// @> s_transactions[txnId].confirmations is not decremented
}

Risk

Likelihood:

  • Occurs every time a signer is revoked after they have confirmed pending transactions

  • Common scenario: removing a compromised or malicious signer

Impact:

  • Revoked signers' votes continue to count, undermining governance integrity

  • A malicious signer can confirm multiple transactions, get revoked, but their confirmations persist

  • Effectively allows "ghost voting" from non-signers

  • Can lead to execution of transactions that current signers don't actually approve

Proof of Concept

function testRevokedSignerConfirmationStillCounts() public {
// Setup 4 signers
multiSigTimelock.grantSigningRole(SIGNER_TWO);
multiSigTimelock.grantSigningRole(SIGNER_THREE);
multiSigTimelock.grantSigningRole(SIGNER_FOUR);
vm.deal(address(multiSigTimelock), 1 ether);
// Propose transaction
vm.prank(OWNER);
uint256 txnId = multiSigTimelock.proposeTransaction(address(0x123), 0.5 ether, "");
// SIGNER_TWO confirms
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txnId);
// SIGNER_TWO is revoked (maybe they were compromised)
multiSigTimelock.revokeSigningRole(SIGNER_TWO);
// Verify SIGNER_TWO no longer has role
assertFalse(multiSigTimelock.hasRole(multiSigTimelock.getSigningRole(), SIGNER_TWO));
// But their confirmation STILL COUNTS!
assertEq(multiSigTimelock.getTransaction(txnId).confirmations, 1);
// Only 2 more current signers needed to reach quorum
vm.prank(OWNER);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_THREE);
multiSigTimelock.confirmTransaction(txnId);
// Transaction can execute with only 2 current signers + 1 revoked signer
vm.prank(OWNER);
multiSigTimelock.executeTransaction(txnId); // SUCCESS with ghost vote!
}

Recommended Mitigation

Option 1: Invalidate all confirmations when revoking a signer (gas intensive):

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// ... existing checks ...
+ // Invalidate all pending transaction confirmations from this signer
+ for (uint256 i = 0; i < s_transactionCount; i++) {
+ if (!s_transactions[i].executed && s_signatures[i][_account]) {
+ s_signatures[i][_account] = false;
+ s_transactions[i].confirmations--;
+ }
+ }
// ... existing removal logic ...
}

Option 2: Check signer status at execution time:

function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
+ // Count only current valid signers' confirmations
+ uint256 validConfirmations = 0;
+ for (uint256 i = 0; i < s_signerCount; i++) {
+ if (s_signatures[txnId][s_signers[i]]) {
+ validConfirmations++;
+ }
+ }
+
- if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
+ if (validConfirmations < REQUIRED_CONFIRMATIONS) {
revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
}
// ...
}

Support

FAQs

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

Give us feedback!