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

Timelock Delay Can Be Bypassed for Non-ETH Assets

howiecht

Timelock Delay Can Be Bypassed for Non-ETH Assets

Description

  • The protocol implements a dynamic timelock delay based on the transaction's ETH value. However, this mechanism can be bypassed when transferring ERC20 tokens or interacting with other contracts via the data parameter. Since these transactions typically carry a value of 0 ETH, they are assigned NO_TIME_DELAY (0 seconds), rendering the timelock ineffective for all non-ETH assets.

Risk

Likelihood:

  • All token-based transactions will naturally bypass the delay logic.

Impact:

  • The timelock is a critical security feature intended to provide a reaction window; bypassing it for tokens leaves the majority of the contract's potential assets unprotected.

Proof of Concept

// add to test/unit/MultiSigTimelockTest.t.sol
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
contract MockToken is ERC20 {
constructor() ERC20("Mock Token", "MTK") {
_mint(msg.sender, 100e18);
}
}
​
function testCanPassTimelockDelayExecuteTransaction() public grantSigningRoles {
// ARRANGE
MockToken token = new MockToken();
token.transfer(address(multiSigTimelock), 100e18);
​
vm.deal(address(multiSigTimelock), OWNER_BALANCE_TWO);
// Transferring 100 tokens, but 'value' passed to proposeTransaction will be 0
bytes memory data = abi.encodeWithSelector(ERC20.transfer.selector, SPENDER_ONE, 100e18);
​
vm.prank(OWNER);
uint256 txnId = multiSigTimelock.proposeTransaction(address(token), 0, data);
​
// ACT
vm.prank(OWNER);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_THREE);
multiSigTimelock.confirmTransaction(txnId);
​
// This executes IMMEDIATELY because txnId.value == 0, bypassing the intended safety delay
vm.prank(OWNER);
multiSigTimelock.executeTransaction(txnId);
​
// ASSERT
assertEq(token.balanceOf(SPENDER_ONE), 100e18);
}

Recommended Mitigation

Modify the delay calculation logic to account for transactions containing data payloads. A simple solution is to apply a mandatory minimum delay whenever data.length > 0.

function _getTimelockDelay(uint256 value, bytes memory data) internal pure returns (uint256) {
+ if (data.length > 0) {
+ return ANOTHER_TIME_DELAY; // Or another safe default for arbitrary calls
+ }
uint256 sevenDaysTimeDelayAmount = 100 ether;
// ... rest of logic
}
Updates

Lead Judging Commences

kelechikizito Lead Judge 4 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!