Revoked signers' confirmations persist leading to unauthorized transaction execution
Description
-
When a signer is revoked via MultiSigTimelock::revokeSigningRole, their existing confirmations on pending transactions are not removed. These orphaned confirmations continue to count toward the REQUIRED_CONFIRMATIONS threshold.
-
For example: With 5 signers (REQUIRED=3), if a revoked signer confirmed a transaction, it now only needs 2 more confirmations from 4 active signers instead of 3.
With 5 signers (REQUIRED=3), if a revoked signer confirmed a transaction, it now only needs 2 more confirmations from 4 active signers instead of 3.
@> 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:
Depends upon the behavior of signers, likely be revoked in case of maliciousness.
Impact:
-
HIGH. Security dilution over time.
-
A malicious signer can confirm many transactions, get revoked, and their confirmations still reduce the remaining requirement. This can lead to unauthorized execution of transactions with fewer active signers than intended.
Proof of Concept
Deploy the contract with 5 signers.
Propose a transaction and have 3 signers confirm it.
Revoke the signing role of two of the confirming signers.
Execute the transaction successfully with only 1 remaining active signer confirming it, due to the orphaned confirmations from the revoked signers.
Paste the following test case into test/MultiSigTimelock.t.sol to reproduce:
function testRevokedSignerConfirmationsRemainsForPendingTransactions() external grantSigningRoles {
vm.deal(address(multiSigTimelock), OWNER_BALANCE_ONE);
vm.deal(OWNER, OWNER_BALANCE_ONE);
vm.prank(OWNER);
uint256 txnId = multiSigTimelock.proposeTransaction(SPENDER_ONE, OWNER_BALANCE_ONE, hex"");
vm.prank(OWNER);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_THREE);
multiSigTimelock.confirmTransaction(txnId);
multiSigTimelock.revokeSigningRole(SIGNER_TWO);
multiSigTimelock.revokeSigningRole(SIGNER_THREE);
vm.prank(OWNER);
multiSigTimelock.executeTransaction(txnId);
}
Recommended Mitigation
In MultiSigTimelock::revokeSigningRole, iterate through pending transactions and remove the revoked signer's confirmations:
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// CHECKS
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
// Prevent revoking the first signer (would break the multisig), moreover, the first signer is the owner of the contract(wallet)
if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
// Find the index of the account in the array
uint256 indexToRemove = type(uint256).max; // Use max as "not found" indicator
for (uint256 i = 0; i < s_signerCount; i++) {
if (s_signers[i] == _account) {
indexToRemove = i;
break;
}
}
// Gas-efficient array removal: move last element to removed position
if (indexToRemove < s_signerCount - 1) {
// Move the last signer to the position of the removed signer
s_signers[indexToRemove] = s_signers[s_signerCount - 1];
}
// Clear the last position and decrement count
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
+ 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--;
+ }
+ }
}
