Root + Impact

• Root: Proposed transactions have no expiration, cancellation, or rejection mechanism and remain executable indefinitely once created.

• Impact: Long-forgotten transactions can be unexpectedly executed in the future — potentially by newly added signers — using current contract funds, causing surprise withdrawals.

Description

• Normal Behaviour: Transactions that fail to reach the required confirmation quorum within a reasonable time frame should be considered rejected or expired, preventing future execution.

• Issue: In MultiSigTimelock, once a transaction is proposed, it is stored permanently and remains executable as long as:

  • it eventually reaches the required number of confirmations, and

  • the timelock period (based on the original proposal time) has elapsed.

  There is no mechanism to:

  • cancel a transaction,

  • mark it as rejected, or

  • enforce a maximum lifetime.

  As a result, partially confirmed transactions can remain dormant for months or years and later be executed unexpectedly.

  function _proposeTransaction(address to, uint256 value, bytes memory data) internal returns (uint256) {

  uint256 transactionId = s_transactionCount;

  ​

  s_transactions[transactionId] = Transaction({

  to: to,

  value: value,

  data: data,

  confirmations: 0,

  proposedAt: block.timestamp,

  executed: false

  @> // There's no deadline at all, letting it stay permanently in the system

  });

  ​

  s_transactionCount++;

  return transactionId;

  }

Risk

• Likelihood: Medium

  • Partially confirmed transactions can easily occur in this system.

  • Signer sets are expected to change over time.

• Impact: High

  • Long-abandoned transactions can be revived and executed without current signers realising the historical context

  • Funds intended for future operations are unexpectedly transferred.

  • Breaks the assumption that unapproved transactions naturally expire.

  • And in the worst case scenario, let's say the transaction contained malicious calldata in some form, due to which the owner didn't approve it. But now, he can't reject it either; letting enough time for the attacker to somehow reach to those signers and get the confirmation through (either by hacking or force blackmailing)

  Well, this worst case might sound hypothetical, but it is indeed possible as well. Cuz when we are aware of some issue, we get rid of it, not keeping the possibility alive for a long time.

Proof of Concept

• Add the following test to test/unit/MultiSigTimelockTest.t.sol. Please also read the comments, as they describe the attack/failure scenario.

  function test__StaleTransactionExecutedAfterLongDelay() public grantSigningRoles {

  // Owner proposes a transaction, and funds get added as well

  vm.prank(OWNER);

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

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

  ​

  // Only two confirmations (below quorum)

  vm.prank(OWNER);

  multiSigTimelock.confirmTransaction(txnId);

  ​

  vm.prank(SIGNER_TWO);

  multiSigTimelock.confirmTransaction(txnId);

  ​

  // Owner tries to execute the transaction, but it fails as the quorum is not reached

  vm.prank(OWNER);

  vm.expectRevert();

  multiSigTimelock.executeTransaction(txnId);

  ​

  // No more confirmation came through, and around a year passed

  vm.warp(block.timestamp + 365 days);

  ​

  // At this point, everyone considers that transaction as dead

  // and in the meantime, a lot of transactions were to be proposed and rejected

  // Let's say there was another transaction of 0.5 ether

  ​

  // But due to some reason, a new signer is added by revoking the previous one

  vm.prank(OWNER);

  multiSigTimelock.revokeSigningRole(SIGNER_FIVE);

  ​

  address naiveSigner = makeAddr("naive_signer");

  vm.prank(OWNER);

  multiSigTimelock.grantSigningRole(naiveSigner);

  ​

  // And this new signer didn't have any historical context

  // Thus, unknowingly confirms the old transaction, and executes it

  vm.startPrank(naiveSigner);

  multiSigTimelock.confirmTransaction(txnId);

  multiSigTimelock.executeTransaction(txnId);

  vm.stopPrank();

  ​

  // To everyone's surprise, this year-old transaction suddenly woke from being dead,

  // and ate up the funds of the current proposed transaction

  }
  
  

• Run the test using:

  forge test --mt test__StaleTransactionExecutedAfterLongDelay

Recommended Mitigation

Introduce a mechanism to limit the lifetime of proposed transactions or eradicate stale transactions.

Possible approaches:

• Option 1 — Proposal Expiry (Most Preferred)

  Enforce a maximum execution window after proposal:

  + uint256 private constant MAX_TRANSACTION_LIFETIME = 14 days; // Let's say

  ​

  ...

  ​

  function _executeTransaction(uint256 txnId) internal {

  ...

  ​

  + require(

  + block.timestamp <= txn.proposedAt + MAX_TRANSACTION_LIFETIME,

  + "Transaction expired"

  + );

  ​

  ...

  }

  ​
  
  

• Option 2 — Explicit Cancellation

  Allow the owner or a quorum of signers to cancel pending transactions.

• Option 3 — Reset on Signer Changes (Least Preferred)

  Invalidate all pending transactions when the signer set is modified.