Description:
Confirmations are stored as:
When the owner removes a signer with revokeSigningRole(), the contract revokes the role, but does not:
clear that signer’s existing confirmations on pending transactions, nor
decrement the cached confirmation counts accordingly.
Worse, once a signer is removed, they can no longer call revokeConfirmation() because it is protected by onlyRole(SIGNING_ROLE).
This makes “emergency removal” ineffective against already-confirmed transactions and allows execution under a changed signer set (e.g., 2-of-current-signers can execute a transaction that previously reached 3 confirmations including a now-removed signer).
Impact:
Signer set changes do not provide the expected safety guarantees. A transaction may remain executable even after a signer is removed (including if that signer was removed because they were compromised). This is a governance/safety bypass and can surprise operators who assume “revoking a signer invalidates their votes.”
Proof of Concept:
pragma solidity ^0.8.19;
import {Test} from "forge-std/Test.sol";
import {MultiSigTimelock} from "src/MultiSigTimelock.sol";
contract Receiver {
receive() external payable {}
}
contract MultiSigTimelock_PoC is Test {
MultiSigTimelock internal ms;
address internal OWNER = address(this);
address internal SIGNER_TWO = makeAddr("signer_two");
address internal SIGNER_THREE = makeAddr("signer_three");
address internal SIGNER_FOUR = makeAddr("signer_four");
address internal SIGNER_FIVE = makeAddr("signer_five");
address internal NEW_SIGNER = makeAddr("newSigner");
address internal ATTACKER = makeAddr("attacker");
function setUp() public {
ms = new MultiSigTimelock();
ms.grantSigningRole(SIGNER_TWO);
ms.grantSigningRole(SIGNER_THREE);
ms.grantSigningRole(SIGNER_FOUR);
ms.grantSigningRole(SIGNER_FIVE);
vm.deal(address(ms), 200 ether);
}
function _proposeEthTransfer(address to, uint256 value) internal returns (uint256 txnId) {
txnId = ms.proposeTransaction(to, value, "");
}
function _confirm(uint256 txnId, address signer) internal {
vm.prank(signer);
ms.confirmTransaction(txnId);
}
function test_RemovedSigner_ConfirmationStillCounts_AllowsExecutionWithFewerCurrentSigners() public {
Receiver r = new Receiver();
uint256 txnId = _proposeEthTransfer(address(r), 0.5 ether);
_confirm(txnId, OWNER);
_confirm(txnId, SIGNER_TWO);
_confirm(txnId, SIGNER_THREE);
ms.revokeSigningRole(SIGNER_THREE);
assertFalse(ms.hasRole(ms.getSigningRole(), SIGNER_THREE));
uint256 balBefore = address(r).balance;
ms.executeTransaction(txnId);
assertEq(address(r).balance, balBefore + 0.5 ether);
}
}
Mitigation:
Implement a mechanism to invalidate confirmations when a signer is removed. Add a loop in revokeSigningRole() to clear all confirmations from the removed signer:
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
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--;
}
}
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);
}
Alternatively, implement a signerVersion or epoch system to invalidate old confirmations.
