Description:
The revokeSigningRole() function removes a signer's authority but fails to invalidate their existing confirmations on pending transactions. When a compromised signer is revoked, their prior confirmations still count toward the 3-signature quorum, allowing transactions to execute with "ghost" approvals from unauthorized addresses.
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
uint256 indexToRemove = type(uint256).max;
for (uint256 i = 0; i < s_signerCount; i++) {
if (s_signers[i] == _account) {
indexToRemove = i;
break;
}
}
if (indexToRemove < s_signerCount - 1) {
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);
@>
@>
}
Risk
Likelihood:
-
Reason 1 // Owner detects a compromised or malicious signer and revokes their role - this is a standard operational response
-
Reason 2 // The revoked signer had already confirmed one or more pending transactions before revocation
Impact:
-
1 // Transactions approved by compromised/removed signers can still execute with their "ghost" confirmations
-
2 // Effective quorum drops from 3-of-N to 2-of-(N-1) for any pending transaction the revoked signer confirmed
-
3 // Complete violation of the trust model - the entire purpose of revoking a signer is to remove their authority
-
4 // Potential loss of all contract funds if a malicious transaction was confirmed before revocation
Proof of Concept
Demonstrates a revoked signer's confirmation still counting toward quorum:
function test_RevokedSignerConfirmationPersists() public {
multiSigTimelock.grantSigningRole(SIGNER_TWO);
multiSigTimelock.grantSigningRole(SIGNER_THREE);
multiSigTimelock.grantSigningRole(SIGNER_FOUR);
vm.deal(address(multiSigTimelock), 1 ether);
uint256 txnId = multiSigTimelock.proposeTransaction(ATTACKER, 0.5 ether, "");
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txnId);
multiSigTimelock.revokeSigningRole(SIGNER_TWO);
assertFalse(multiSigTimelock.hasRole(multiSigTimelock.getSigningRole(), SIGNER_TWO));
assertEq(multiSigTimelock.getTransaction(txnId).confirmations, 1);
vm.prank(OWNER);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_THREE);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(OWNER);
multiSigTimelock.executeTransaction(txnId);
assertEq(ATTACKER.balance, 0.5 ether);
}
Recommended Mitigation
Invalidate all confirmations from the revoked signer on pending transactions:
+ event ConfirmationInvalidated(uint256 indexed transactionId, address indexed revokedSigner);
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
+ // Invalidate all confirmations from this signer on pending transactions
+ 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--;
+ emit ConfirmationInvalidated(i, _account);
+ }
+ }
uint256 indexToRemove = type(uint256).max;
for (uint256 i = 0; i < s_signerCount; i++) {
if (s_signers[i] == _account) {
indexToRemove = i;
break;
}
}
if (indexToRemove < s_signerCount - 1) {
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);
}
