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

Transaction Revert on Execution Failure Leading to Repeated Gas Waste

Author Revealed upon completion

Transaction Revert on Execution Failure Leading to Repeated Gas Waste

Description

The contract implements a strict atomic execution logic in _executeTransaction. If the external interaction fails, the entire transaction is reverted. Consequently, the state update txn.executed = true is rolled back, causing the transaction to remain in a "pending" state executed == false indefinitely. This prevents the transaction from being marked as "failed" and allows for repetitive execution attempts.

function _executeTransaction(uint256 txnId) internal {
@> if (!success) {
@> revert MultiSigTimelock__ExecutionFailed();
}

Risk

Likelihood: Medium
It is highly common for users to propose transactions with incorrect calldata or wrong parameters. Since these transactions never expire, they will stay in the "unexecuted" state forever.

Impact:

There is no direct loss of funds because the revert protects the ETH. However, it causes permanent Gas waste as signers may repeatedly try to execute the "stuck" transaction, and it clutters the contract state with "zombie" entries.

Proof of Concept

Transferring funds to the EthRejector contract triggers a revert on all incoming payments. The execution fails and the status remains false without being updated. Multiple signers attempting to execute the transaction result in continuous reverts and wasted gas fees.

Proof Of Code

EthRejector:

contract EthRejector {
receive() external payable {
revert("Always fails");
}
}
​

place the following code in MultiSigTimelockTest.t.sol:

function test_RevertOnFailedExecutionAndStateRollback() public grantSigningRoles {
vm.deal(address(multiSigTimelock), 10 ether);
vm.prank(OWNER);
uint256 txId = multiSigTimelock.proposeTransaction(address(ethRejector), 1 ether, "0x1234");
​
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txId);
vm.prank(SIGNER_THREE);
multiSigTimelock.confirmTransaction(txId);
vm.prank(SIGNER_FOUR);
multiSigTimelock.confirmTransaction(txId);
​
vm.warp(block.timestamp + 7 days);
​
vm.expectRevert();
vm.prank(OWNER);
multiSigTimelock.executeTransaction(txId);
​
uint256 gasBefore = gasleft();
vm.expectRevert();
vm.prank(SIGNER_FOUR);
multiSigTimelock.executeTransaction(txId);
uint256 gasAfter = gasleft();
​
MultiSigTimelock.Transaction memory txn = multiSigTimelock.getTransaction(txId);
assertEq(txn.executed, false);
console2.log("gas waste:", gasBefore- gasAfter);
}
Logs:
gas waste: 37624

Recommended Mitigation

Eliminate the revert logic by setting executed = true before the external call. If the call fails, emit a TransactionFailed event instead of reverting. this ensures the status is permanently updated on-chain, preventing redundant execution attempts and saving gas for other signers.

function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
​
// 3. Mark as executed BEFORE the external call (prevent reentrancy)
txn.executed = true;
​
// INTERACTIONS
// 4. Execute the transaction
(bool success,) = payable(txn.to).call{value: txn.value}(txn.data);
​
- if (!success) {
- revert MultiSigTimelock__ExecutionFailed();
- }
- emit TransactionExecuted(txnId, txn.to, txn.value);
​
+ if (success) {
+ emit TransactionExecuted(txnId, txn.to, txn.value);
+ } else{
+ emit TransactionFailed(txnId, txn.to, txn.value);
+ }

Support

FAQs

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

Give us feedback!