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

Missing Function to Cancel/Delete Proposed Transactions

Author Revealed upon completion

Description

  • The contract allows proposing and executing transactions but does not provide any mechanism to cancel or delete a proposed transaction that hasn't been executed yet.

  • Once a transaction is proposed, it remains in the contract's storage forever, even if it becomes obsolete or unwanted. This could lead to storage bloat and potential confusion about which transactions are active.

function proposeTransaction(address to, uint256 value, bytes calldata data)
external
nonReentrant
noneZeroAddress(to)
onlyOwner
returns (uint256)
{
return _proposeTransaction(to, value, data);
}
​
@> // No function to cancel or delete a proposed transaction exists
@> // Transactions remain in storage forever after proposal

Risk

Likelihood:

  • When a transaction is proposed but later determined to be unnecessary or incorrect

  • Storage accumulates old, obsolete transactions that will never be executed

Impact:

  • Storage bloat over time with unused transactions

  • Potential confusion about which transactions are actively being considered

  • No way to clean up malicious or erroneous transaction proposals

  • Minor gas inefficiency from unused storage

Proof of Concept

function testCannotCancelProposedTransaction() public {
// Propose a transaction
uint256 txId = multiSig.proposeTransaction(recipient, 1 ether, "");
// Later realize this transaction is wrong or unwanted
// No function exists to cancel it!
// The transaction remains in storage forever
MultiSigTimelock.Transaction memory txn = multiSig.getTransaction(txId);
assertEq(txn.to, recipient); // Still exists in storage
// Only option is to never confirm/execute it, but it clutters storage
}

Recommended Mitigation

+ event TransactionCancelled(uint256 indexed transactionId);
+
+ error MultiSigTimelock__CannotCancelExecutedTransaction();
​
+ /**
+ * @dev Function to cancel a proposed transaction before execution
+ * @param txnId The ID of the transaction to cancel
+ */
+ function cancelTransaction(uint256 txnId)
+ external
+ nonReentrant
+ onlyOwner
+ transactionExists(txnId)
+ notExecuted(txnId)
+ {
+ Transaction storage txn = s_transactions[txnId];
+
+ // Mark as executed to prevent future execution
+ txn.executed = true;
+
+ // Optional: Reset confirmations to save gas on storage refunds
+ txn.confirmations = 0;
+
+ emit TransactionCancelled(txnId);
+ }

==================================================================================

[INFO] No Upgrade Mechanism or Emergency Pause Functionality

Description

  • The contract does not implement any upgrade mechanism (such as proxy pattern) or emergency pause functionality.

  • If a critical vulnerability is discovered after deployment, there is no way to pause operations or upgrade the contract logic to fix the issue. All funds would need to be migrated to a new contract manually.

@> // No pause functionality
@> // No upgrade mechanism
@> // No emergency withdrawal function
​
contract MultiSigTimelock is Ownable, AccessControl, ReentrancyGuard {
// ... contract code ...
}

Risk

Likelihood:

  • When a critical vulnerability is discovered post-deployment

  • When the contract needs to be upgraded with new features or fixes

  • During emergency situations requiring immediate action

Impact:

  • No ability to pause contract operations during an emergency

  • Cannot upgrade contract logic if vulnerabilities are found

  • Must deploy entirely new contract and migrate funds manually

  • Users must trust that no critical bugs exist since there's no recovery mechanism

Proof of Concept

function testNoPauseOrUpgradeMechanism() public {
// If a vulnerability is discovered, there's no way to pause
// No emergency stop mechanism exists
// Can only deploy new contract and migrate funds
// This requires coordinating all signers and moving funds
}

Recommended Mitigation

+ import {Pausable} from "@openzeppelin/contracts/utils/Pausable.sol";
​
- contract MultiSigTimelock is Ownable, AccessControl, ReentrancyGuard {
+ contract MultiSigTimelock is Ownable, AccessControl, ReentrancyGuard, Pausable {
​
+ bytes32 private constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
+
+ constructor() Ownable(msg.sender) {
+ // ... existing constructor code ...
+ _grantRole(PAUSER_ROLE, msg.sender);
+ }
​
+ function pause() external onlyRole(PAUSER_ROLE) {
+ _pause();
+ }
​
+ function unpause() external onlyRole(PAUSER_ROLE) {
+ _unpause();
+ }
​
function proposeTransaction(address to, uint256 value, bytes calldata data)
external
nonReentrant
+ whenNotPaused
noneZeroAddress(to)
onlyOwner
returns (uint256)
{
return _proposeTransaction(to, value, data);
}
​
function executeTransaction(uint256 txnId)
external
nonReentrant
+ whenNotPaused
onlyRole(SIGNING_ROLE)
transactionExists(txnId)
notExecuted(txnId)
{
_executeTransaction(txnId);
}
​
+ /**
+ * @dev Emergency function to recover funds if contract is paused
+ * Requires consensus from all current signers
+ */
+ function emergencyWithdraw(address payable to)
+ external
+ whenPaused
+ onlyOwner
+ {
+ // Could add additional signature requirements here
+ uint256 balance = address(this).balance;
+ (bool success,) = to.call{value: balance}("");
+ require(success, "Emergency withdrawal failed");
+ }

Support

FAQs

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

Give us feedback!