Root + Impact

• Root: Revoking a signer through MultiSigTimelock::revokeSigningRole does not invalidate or remove their existing confirmations from pending transactions.

• Impact: Transactions can be executed using approvals from accounts that are no longer authorised signers, violating the multisig security model.

Description

• Normal Behaviour: Once a signer is removed, their authority should be fully revoked, meaning any prior confirmations they provided should no longer count toward the execution quorum.

• Issue: When a signer gets revoked via revokeSigningRole, the contract:

  • removes the signer role,

  • but does not clear the signer's previous confirmations, and

  • does not update the cached confirmation count for existing transactions.

  As a result, confirmations from revoked signers remain valid and can still be used to reach the quorum for execution.

  function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {

  ...

  s_isSigner[_account] = false;

  @> _revokeRole(SIGNING_ROLE, _account);

  // Existing confirmations from `_account` are not removed

  }

  ​

  ...

  ​

  function _executeTransaction(uint256 txnId) internal {

  Transaction storage txn = s_transactions[txnId];

  ​

  @> if (txn.confirmations < REQUIRED_CONFIRMATIONS) {

  revert MultiSigTimelock__InsufficientConfirmations(

  REQUIRED_CONFIRMATIONS,

  txn.confirmations

  );

  }

  // `txn.confirmations` may include approvals from revoked signers

  }

Risk

• Likelihood: Medium

  • Signer revocation is a supported and expected administrative action.

  • Pending transactions may already contain confirmations from the revoked signer

• Impact: High

  • Transactions can be executed based on approvals from unauthorised (revoked) signers.

  • Breaks the core multisig invariant that only current signers can authorise execution.

  • Undermines trust in signer revocation as a security control.

Proof of Concept

• Please add the following test to test/unit/MultiSigTimelockTest.t.sol:

  function test__TransactionGetExecutedEvenAfterRevocationOfItsSigner() public grantSigningRoles {

  vm.prank(OWNER);

  uint256 txnId = multiSigTimelock.proposeTransaction(SPENDER_ONE, 0.5 ether, hex"");

  console2.log("A Transaction gets proposed with id:", txnId);

  ​

  // Funds are sent to `multiSignTimelock`

  vm.deal(address(multiSigTimelock), 0.5 ether);

  ​

  // Confirmations...

  vm.prank(OWNER);

  multiSigTimelock.confirmTransaction(txnId);

  ​

  vm.prank(SIGNER_TWO);

  multiSigTimelock.confirmTransaction(txnId);

  ​

  vm.prank(SIGNER_THREE);

  multiSigTimelock.confirmTransaction(txnId);

  ​

  // Now, OWNER felt that SIGNER_THREE is a bad signer and decided to revoke him using the `revokeSigningRole`

  vm.prank(OWNER);

  multiSigTimelock.revokeSigningRole(SIGNER_THREE);

  ​

  bytes32 SIGNING_ROLE = keccak256("SIGNING_ROLE");

  console2.log("SIGNER_THREE gets revoked, `hasRole(SIGNING_ROLE, SIGNER_THREE)`:", multiSigTimelock.hasRole(SIGNING_ROLE, SIGNER_THREE));

  ​

  // Practically, the transaction shouldn't be executed now, but still, it does

  vm.prank(SIGNER_TWO);

  multiSigTimelock.executeTransaction(txnId);

  }
  
  

• Run the test using:

  forge test --mt test__TransactionGetExecutedEvenAfterRevocationOfItsSigner -vv
  
  

• Logs:

  Ran 1 test for test/unit/MultiSigTimelockTest.t.sol:MultiSigTimeLockTest

  [PASS] test__TransactionGetExecutedEvenAfterRevocationOfItsSigner() (gas: 547832)

  Logs:

  A Transaction gets proposed with id: 0

  SIGNER_THREE gets revoked, `hasRole(SIGNING_ROLE, SIGNER_THREE)`: false

Recommended Mitigation

When revoking a signer, the revokeSigningRole function can also iterate over pending transactions, and:

• remove their signature, and

• decrement the confirmation count accordingly.