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

Timelock Delay Can Be Bypassed for Non-ETH Assets

Author Revealed upon completion

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
}

Support

FAQs

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

Give us feedback!