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

Revoked signer confirmations still count

Author Revealed upon completion

Root + Impact

Description

  • When you revoke a signer, their "yes" votes on pending transactions should be removed, and those transactions should only execute if enough currently active signers have confirmed them.

  • Revoked signers' "yes" votes remain counted, and transactions can execute using those invalid votes because the execution function doesn't check whether the confirming signers are still authorized.

function revokeSigningRole(address _account) external ... {
// ...
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
//No code to invalidate existing confirmations
//No code to update confirmation counts
}

Risk

Likelihood:

  • This is not a complex attack requiring multiple conditions. It's a direct consequence of the code logic:

    • Signer confirms transaction → Confirmation count increases

    • Signer gets revoked → Confirmation count remains unchanged

    • Transaction executes → Uses stale confirmation count

  • An attacker could:

    • Join as a signer

    • Confirm many transactions (legitimate and potentially malicious)

    • Get revoked for suspicious behavior

    • Still have their confirmations count toward execution

Impact:

  • The fundamental principle of multisig wallets is that only currently authorized signers can approve transactions. This vulnerability allows revoked signers to continue influencing transaction execution.

  • Transactions can execute with confirmations from signers who are no longer authorized, potentially leading to:

    • Execution of transactions that shouldn't meet the threshold

    • Bypass of governance controls

    • Loss of funds if revoked signers had malicious intent

Proof of Concept

function test_RevokedSignerConfirmationStillCounts() public {
uint256 txnId = multisig.proposeTransaction(recipient, 1 ether, hex"");
vm.prank(signerA);
multisig.confirmTransaction(txnId);
vm.prank(signerB);
multisig.confirmTransaction(txnId);
vm.prank(signerC);
multisig.confirmTransaction(txnId);
assertEq(multisig.getTransaction(txnId).confirmations, 3, "Should have 3 confirmations");
multisig.revokeSigningRole(signerC);
assertFalse(multisig.hasRole(multisig.getSigningRole(), signerC), "signerC should be revoked");
assertEq(multisig.getTransaction(txnId).confirmations, 3,
"VULNERABILITY: Confirmations still include revoked signer!");
vm.warp(block.timestamp + 1 days + 1);
uint256 balanceBefore = recipient.balance;
vm.prank(signerA);
multisig.executeTransaction(txnId);
assertEq(recipient.balance, balanceBefore + 1 ether,
"VULNERABILITY: Funds sent despite revoked signer's confirmation!");
}

Recommended Mitigation

- remove this code
+ add this code
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;
}
}
​
+ // Invalidate all confirmations from this signer on pending transactions
+ for (uint256 i = 0; i < s_transactionCount; i++) {
+ // Only process non-executed transactions
+ if (!s_transactions[i].executed && s_signatures[i][_account]) {
+ // Remove the signature
+ s_signatures[i][_account] = false;
+ // Decrement confirmation count
+ s_transactions[i].confirmations--;
+ }
+ }
​
// 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);
}

Support

FAQs

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

Give us feedback!