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

Revoked signer confirmations persist, lowering quorum and enabling unstoppable execution

Author Revealed upon completion

Description

  • Intended/normal behavior: confirmations should only count from current, active signers so revoking a signer removes their influence over pending transactions.

  • Specific deviation and why it matters: confirmations are stored as a counter and never invalidated when a signer is revoked, while execution only checks the counter. This allows old approvals from revoked signers to keep counting.

  • Attack story: a compromised signer pre-signs a malicious transaction, gets revoked, and later only two current signers confirm; the stale confirmation still satisfies the 3-confirmation threshold, enabling execution even though only 2 active signers approved. This also makes confirmed transactions unstoppable after signer revocations.

/// File: src/MultiSigTimelock.sol:L209-L377
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
...
@> s_isSigner[_account] = false;
@> _revokeRole(SIGNING_ROLE, _account);
// No invalidation of existing confirmations for this signer
}
​
function _confirmTransaction(uint256 txnId) internal {
...
@> s_transactions[txnId].confirmations++;
}
​
function _executeTransaction(uint256 txnId) internal {
...
@> if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
}
}

Risk

Likelihood

  • Realistic if a signer is compromised or removed while pending transactions exist (confirmed in tests).

  • Requires only 2 current signers once a stale confirmation exists.

Impact

  • Transactions can execute with fewer active signers than intended.

  • Revoking a signer does not stop their previously approved transactions, allowing funds to be moved despite removal.

Proof of Concept

function test_GhostVote() public {
// Deployer, Alice, Bob are signers
uint256 txId = multiSig.proposeTransaction(address(0x123), 0.5 ether, "");
​
// Alice confirms
vm.prank(alice);
multiSig.confirmTransaction(txId);
​
// Owner revokes Alice, then adds Charlie
vm.prank(deployer);
multiSig.revokeSigningRole(alice);
vm.prank(deployer);
multiSig.grantSigningRole(charlie);
​
// Deployer + Bob confirm (only 2 active signers)
vm.prank(deployer);
multiSig.confirmTransaction(txId);
vm.prank(bob);
multiSig.confirmTransaction(txId);
​
// confirmations == 3 (includes revoked Alice) so execution succeeds
vm.deal(address(multiSig), 1 ether);
vm.prank(deployer);
multiSig.executeTransaction(txId);
}

Recommended Mitigation

- struct Transaction { ... uint256 confirmations; ... }
+ // Remove confirmations counter or recompute it at execution time.
​
- if (txn.confirmations < REQUIRED_CONFIRMATIONS) { ... }
+ uint256 currentConfirmations;
+ for (uint256 i = 0; i < s_signerCount; i++) {
+ address signer = s_signers[i];
+ if (signer != address(0) && s_signatures[txnId][signer]) {
+ currentConfirmations++;
+ }
+ }
+ if (currentConfirmations < REQUIRED_CONFIRMATIONS) { ... }

Support

FAQs

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

Give us feedback!