MultiSig Timelock
First Flight #55
Beginner FriendlyWallet
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Timelock bypass for non-ETH assets

s3mvl4d

Description

  • The “dynamic timelock” should delay any high‑value operation from the wallet, whether it sends ETH or performs a token/privileged call via data, so signers have time to detect and stop risky transactions.

  • The timelock duration is computed only from txn.value (ETH sent). Calls with value == 0 but a powerful data payload (e.g., ERC20 transfer, approve, or an admin function on another contract) bypass the timelock entirely and can execute immediately, regardless of the economic value moved.

function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
// Quorum check omitted here for brevity
​
// @> Timelock is computed solely from ETH 'value'
uint256 requiredDelay = _getTimelockDelay(txn.value);
uint256 executionTime = txn.proposedAt + requiredDelay;
if (block.timestamp < executionTime) {
revert MultiSigTimelock__TimelockHasNotExpired(executionTime);
}
​
// @> External call may transfer tokens or invoke privileged logic via 'data'
(bool success,) = payable(txn.to).call{value: txn.value}(txn.data);
if (!success) { revert MultiSigTimelock__ExecutionFailed(); }
}
​
function _getTimelockDelay(uint256 value) internal pure returns (uint256) {
// @> Delay tiers depend only on ETH 'value'
// <1 ETH => 0 delay; 1-10 ETH => 1 day; 10-100 ETH => 2 days; >=100 ETH => 7 days
...
}

Risk

Likelihood: High

  • Whenever the wallet holds ERC20s (treasury tokens, stables) or interacts with external contracts, signers routinely propose data‑only calls (with value == 0). These will always execute immediately after quorum, because the code assigns zero delay to < 1 ETH, including 0.

  • This occurs in normal operations (token payments, approvals, staking, governance calls) and during incidents, making timelock protection ineffective for most non‑ETH value transfers.

Impact: High

  • Governance bypass / asset loss: Large ERC20 transfers or approvals can be executed with no delay, defeating the intended safety window and enabling fast drains if signers are compromised or collude.

  • Policy mismatch: Users assume “high‑value transactions” are delayed; in practice only ETH transfers are delayed, which is misleading and dangerous for treasuries primarily holding tokens.

Proof of Concept

  • Create poc.t.sol under unit directory and copy the code below.

  • Run forge test --mp poc -vv.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import {Test, console2} from "forge-std/Test.sol";
import {MultiSigTimelock} from "src/MultiSigTimelock.sol";
import {ERC20} from "openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
​
contract MultiSigTimeLockTest is Test {
MultiSigTimelock multiSigTimelock;
​
address public immutable RECIPIENT = makeAddr("recipient");
address public immutable SIGNER_TWO = makeAddr("signer_two");
address public immutable SIGNER_THREE = makeAddr("signer_three");
​
function setUp() public {
multiSigTimelock = new MultiSigTimelock();
}
​
function testTimelockBypassForNonETHAssets() public {
// --- Arrange ---
​
// Deploy a mintable mock token and fund the wallet with a large amount
MockERC20 token = new MockERC20();
uint256 bigAmount = 1_000_000 ether;
token.mint(address(multiSigTimelock), bigAmount);
uint256 walletBalance = token.balanceOf(address(multiSigTimelock));
assertEq(walletBalance, bigAmount, "Wallet funded with tokens");
console2.log("Wallet initial token balance:", walletBalance);
​
uint256 recipientBalance = token.balanceOf(RECIPIENT);
assertEq(recipientBalance, 0, "Recipient starts with zero tokens");
console2.log("Recipient initial token balance:", recipientBalance);
​
// Prepare ERC20.transfer(recipient, bigAmount) payload with ZERO ETH value
bytes memory data = abi.encodeWithSignature("transfer(address,uint256)", RECIPIENT, bigAmount);
​
// Propose data-only transaction (value == 0 => no timelock by current logic)
uint256 txnId = multiSigTimelock.proposeTransaction(address(token), 0, data);
​
// Grant two more signers
multiSigTimelock.grantSigningRole(SIGNER_TWO);
multiSigTimelock.grantSigningRole(SIGNER_THREE);
​
// --- Act ---
// Reach quorum
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_THREE);
multiSigTimelock.confirmTransaction(txnId);
​
// Execute immediately WITHOUT waiting (because requiredDelay == 0 for value == 0)
vm.prank(SIGNER_TWO);
multiSigTimelock.executeTransaction(txnId);
​
// --- Assert ---
recipientBalance = token.balanceOf(RECIPIENT);
assertEq(recipientBalance, bigAmount, "Tokens moved instantly");
console2.log("Recipient final token balance:", recipientBalance);
​
walletBalance = token.balanceOf(address(multiSigTimelock));
assertEq(walletBalance, 0, "Wallet drained with no delay");
console2.log("Wallet final token balance:", walletBalance);
​
}
}
​
// Minimal mock token for the test
contract MockERC20 is ERC20 {
constructor() ERC20("Mock", "MCK") {}
​
function mint(address to, uint256 amount) external {
_mint(to, amount);
}
}

Output:

[⠊] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/unit/poc.t.sol:MultiSigTimeLockTest
[PASS] testTimelockBypassForNonETHAssets() (gas: 1360022)
Logs:
Wallet initial token balance: 1000000000000000000000000
Recipient initial token balance: 0
Recipient final token balance: 1000000000000000000000000
Wallet final token balance: 0
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.67ms (505.30µs CPU time)
​
Ran 1 test suite in 14.18ms (1.67ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation

  • Make timelock apply to any nontrivial external call, not just ETH value.

  • Baseline fix (low complexity): enforce a minimum delay for calls with non‑empty data, even when value == 0:

function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
​
- uint256 requiredDelay = _getTimelockDelay(txn.value);
+ uint256 requiredDelay = _getTimelockDelay(txn.value);
+ // Apply at least a 1-day delay to any data-bearing call (token transfers, approvals, admin calls)
+ if (txn.data.length > 0 && requiredDelay < ONE_DAY_TIME_DELAY) {
+ requiredDelay = ONE_DAY_TIME_DELAY;
+ }
​
uint256 executionTime = txn.proposedAt + requiredDelay;
if (block.timestamp < executionTime) {
revert MultiSigTimelock__TimelockHasNotExpired(executionTime);
}
(bool success,) = payable(txn.to).call{value: txn.value}(txn.data);
if (!success) { revert MultiSigTimelock__ExecutionFailed(); }
emit TransactionExecuted(txnId, txn.to, txn.value);
}
Updates

Lead Judging Commences

kelechikizito Lead Judge 28 days ago
Submission Judgement Published
Validated
Assigned finding tags:

No validation of Tx calldata

Appeal created

s3mvl4d Submitter
27 days ago
kelechikizito Lead Judge
26 days ago
kelechikizito Lead Judge
26 days ago
kelechikizito Lead Judge 26 days ago
Submission Judgement Published
Validated
Assigned finding tags:

No validation of Tx calldata

Support

FAQs

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

Give us feedback!