MultiSig Timelock
First Flight #55
Beginner FriendlyWallet
100 EXP
View all submissions
Previous Next
Submission Details
Impact: medium
Likelihood: high

Signer removal does not invalidate approvals, allowing execution with fewer than 3 active signers

Author Revealed upon completion

Description

  • Normal behavior: After a signer is revoked, any previously submitted confirmations from that account should no longer count toward the required REQUIRED_CONFIRMATIONS threshold (as the system advertises a “3-of-N signers” approval model). Execution should require confirmations from current SIGNING_ROLE holders.

  • Actual behavior: Confirmations are tracked via a cumulative counter (Transaction.confirmations) that is incremented in _confirmTransaction(), but signer revocation (revokeSigningRole()) does not invalidate existing confirmations nor adjust the counter. _executeTransaction() gates execution solely on txn.confirmations >= REQUIRED_CONFIRMATIONS, without verifying that the confirmations belong to current signers. As a result, a transaction can remain executable even after a confirming signer is revoked, including cases where the number of active signers drops below 3.

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
...
@> // No logic for:
// s_signatures[txnId][_account] = false
// decrease s_transactions[txnId].confirmations
// 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);
}
​
function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
​
@> // No check for 3 current signers signed
// CHECKS
// 1. Check if enough confirmations
if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
}
...
}

Risk

Likelihood:

  • Signer set changes (granting and revoking signing roles) are part of the normal multisig lifecycle and are explicitly supported by the contract design.

  • Confirmations are tracked cumulatively and are not recalculated or invalidated when a signer is revoked, so the issue manifests naturally during standard usage without requiring edge-case timing or malicious behavior.

Impact:

  • Transactions can be executed based on confirmations from accounts that are no longer authorized signers, violating the intended “3-of-N active signers” security model.

  • The multisig can reach a weakened governance state where execution is possible even when the number of active signers falls below the documented minimum threshold, undermining signer rotation as a security control.

Proof of Concept

function test_TxExecutableWithFewerThanRequiredActiveSigners() public {
uint256 value = 0.5 ether;
​
// Fund wallet for the transfer (< 1 ETH => no timelock)
vm.deal(address(multiSigTimelock), value);
​
// Add 2 signers (3 total including owner)
multiSigTimelock.grantSigningRole(SIGNER_TWO);
multiSigTimelock.grantSigningRole(SIGNER_THREE);
assertEq(multiSigTimelock.getSignerCount(), 3);
​
// Propose tx
vm.prank(OWNER);
uint256 txnId = multiSigTimelock.proposeTransaction(SPENDER_ONE, OWNER_BALANCE_ONE, hex"");
​
// 3 confirmations
vm.prank(OWNER);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_THREE);
multiSigTimelock.confirmTransaction(txnId);
​
// Revoke signing role
vm.prank(OWNER);
multiSigTimelock.revokeSigningRole(SIGNER_TWO);
​
// Only 2 active signers remain, but confirmations still count as 3
MultiSigTimelock.Transaction memory t = multiSigTimelock.getTransaction(txnId);
assertLt(multiSigTimelock.getSignerCount(), multiSigTimelock.getRequiredConfirmations());
assertEq(t.confirmations, 3);
​
// Execute succeeds
vm.prank(SIGNER_THREE);
multiSigTimelock.executeTransaction(txnId);
​
MultiSigTimelock.Transaction memory t2 = multiSigTimelock.getTransaction(txnId);
assertTrue(t2.executed);
assertEq(SPENDER_ONE.balance, value);
}

Recommended Mitigation

Validate confirmations against the current signer set at execution time

Instead of relying on the stored Transaction.confirmations counter, compute the number of valid confirmations from current SIGNING_ROLE holders when executing:

  • iterate over s_signers[0..s_signerCount-1]

  • count s_signatures[txnId][signer] == true

  • require validCount >= REQUIRED_CONFIRMATIONS

This bounds the loop to at most 5 signers (constant cost) and ensures stale approvals from revoked signers cannot satisfy the threshold.

function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
​
// CHECKS
- // 1. Check if enough confirmations
- if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
- revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
- }
+ // 1. Check if enough *valid* confirmations from current signers
+ uint256 validConfirmations = _countValidConfirmations(txnId);
+ if (validConfirmations < REQUIRED_CONFIRMATIONS) {
+ revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, validConfirmations);
+ }
​
// 2. Check if timelock period has passed
uint256 requiredDelay = _getTimelockDelay(txn.value);
uint256 executionTime = txn.proposedAt + requiredDelay;
if (block.timestamp < executionTime) {
revert MultiSigTimelock__TimelockHasNotExpired(executionTime);
}
...
emit TransactionExecuted(txnId, txn.to, txn.value);
}
​
+ /// @dev Counts confirmations made by *current* signers only (bounded by MAX_SIGNER_COUNT).
+ function _countValidConfirmations(uint256 txnId) internal view returns (uint256 count) {
+ for (uint256 i = 0; i < s_signerCount; i++) {
+ address signer = s_signers[i];
+ if (s_signatures[txnId][signer]) {
+ count++;
+ }
+ }
+ }

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!