Description
-
Only current signers’ confirmations should count toward the quorum. When a signer is revoked, their prior confirmations must no longer contribute to REQUIRED_CONFIRMATIONS.
-
revokeSigningRole updates membership but does not clear the signer’s prior confirmations, and executeTransaction validates using a pre‑stored counter txn.confirmations without checking whether those confirmations came from currently active signers. This lets “ghost” votes from revoked signers persist and be used to meet quorum.
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
}
function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
}
}
Risk
Likelihood: High
-
Whenever an owner adjusts membership (grant/revoke) between proposal/confirmation and execution, previously revoked signers’ confirmations will still be counted.
-
Common during routine role maintenance, incident response, or team churn - especially in multi‑party treasuries that add/remove signers frequently.
Impact: High
-
Quorum integrity failure: Transactions can execute with fewer than the required number of current signers.
-
Governance attack surface: Owner (or attacker with owner privileges) can grant → confirm → revoke to manufacture quorum, then execute with ghost votes.
Proof of Concept
function testRevokeSigningRoleDoesNotRevokeConfirmation() public {
uint256 txnId = multiSigTimelock.proposeTransaction(SPENDER_ONE, OWNER_BALANCE_ONE, hex"");
console2.log("Transaction ID: ", txnId);
MultiSigTimelock.Transaction memory txn = multiSigTimelock.getTransaction(txnId);
assertEq(txn.confirmations, 0);
console2.log("Initial number of confirmations: ", txn.confirmations);
multiSigTimelock.grantSigningRole(SIGNER_TWO);
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txnId);
txn = multiSigTimelock.getTransaction(txnId);
assertEq(txn.confirmations, 1);
console2.log("Number of confirmations after SIGNER_TWO confirmed: ", txn.confirmations);
multiSigTimelock.revokeSigningRole(SIGNER_TWO);
txn = multiSigTimelock.getTransaction(txnId);
assertEq(txn.confirmations, 1);
console2.log("Number of confirmations after SIGNER_TWO has been revoked: ", txn.confirmations);
}
Output:
[⠊] Compiling...
No files changed, compilation skipped
Ran 1 test for test/unit/MultiSigTimelockTest.t.sol:MultiSigTimeLockTest
[PASS] testRevokeSigningRoleDoesNotRevokeConfirmation() (gas: 228894)
Logs:
Transaction ID: 0
Initial number of confirmations: 0
Number of confirmations after SIGNER_TWO confirmed: 1
Number of confirmations after SIGNER_TWO has been revoked: 1
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.55ms (269.50µs CPU time)
Ran 1 test suite in 14.49ms (2.55ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Recommended Mitigation
function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
- if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
- revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
- }
+ // Recompute active confirmations from the current signer set
+ uint256 activeConfirmations = 0;
+ for (uint256 i = 0; i < s_signerCount; i++) {
+ address signer = s_signers[i];
+ if (signer != address(0) && s_signatures[txnId][signer]) {
+ activeConfirmations++;
+ }
+ }
+ if (activeConfirmations < REQUIRED_CONFIRMATIONS) {
+ revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, activeConfirmations);
+ }
// ... (timelock checks, balance checks, effects/interactions)
- function revokeSigningRole(address _account) external ...
+ function revokeSigningRole(address _account) external ...
{
// ... existing membership updates
+ // (Optional) Invalidate prior confirmations from _account for all pending transactions.
+ // WARNING: Iterating over all transactions is O(n) and may be gas-heavy.
+ for (uint256 t = 0; t < s_transactionCount; t++) {
+ Transaction storage txn = s_transactions[t];
+ if (!txn.executed && s_signatures[t][_account]) {
+ s_signatures[t][_account] = false;
+ // keep 'txn.confirmations' as a cached hint OR remove the cache entirely
+ if (txn.confirmations > 0) {
+ txn.confirmations--;
+ }
+ }
+ }
}
