Root + Impact
Description
Large transactions require multi-day timelocks, but the timelock can be completely bypassed by splitting large amounts into many small transactions (<1 ETH each), which have NO timelock.
function _getTimelockDelay(uint256 value) internal pure returns (uint256) {
uint256 sevenDaysTimeDelayAmount = 100 ether;
uint256 twoDaysTimeDelayAmount = 10 ether;
uint256 oneDayTimeDelayAmount = 1 ether;
if (value >= sevenDaysTimeDelayAmount) {
return SEVEN_DAYS_TIME_DELAY;
} else if (value >= twoDaysTimeDelayAmount) {
return TWO_DAYS_TIME_DELAY;
} else if (value >= oneDayTimeDelayAmount) {
return ONE_DAY_TIME_DELAY;
} else {
@> return NO_TIME_DELAY;
}
}
Risk
Likelihood:
-
Splitting the transaction value among several transactions does not directly violate any protocol terms, as the protocol only enforces timelocks on a single transaction's value.
-
Such a case can occur intentionally to avoid a longer timelock, or unintentionally, as proposed transactions can accumulate over time.
Impact:
-
Complete bypass of the timelock protection
-
High-value transactions can be executed immediately
-
Emergency response window eliminated
-
Timelock becomes security theater
Proof of Concept
Add this snipped of code to the MultiSigTimelockTest.t.sol test file.
function test_timelockBypassWithSplitedTransactions() public grantSigningRoles {
vm.deal(address(multiSigTimelock), OWNER_BALANCE_THREE);
console2.log("SPENDER_ONE balance before: ", SPENDER_ONE.balance);
int256 txnId;
for (uint256 i = 0; i < 10; i++) {
vm.prank(OWNER);
uint256 txnId = multiSigTimelock.proposeTransaction(SPENDER_ONE, 0.5 ether, hex"");
vm.prank(OWNER);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_THREE);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(OWNER);
multiSigTimelock.executeTransaction(txnId);
}
console2.log("SPENDER_ONE balance after: ", SPENDER_ONE.balance);
assertEq(SPENDER_ONE.balance, 5 ether, "SPENDER_ONE should have received 5 ether in total");
}
Even though the example transfers only 5 ether rather than 100, it still uncovers the opportunity to bypass any value eventually.
forge test --mt test_timelockBypassWithSplitedTransactions -vvvv
Recommended Mitigation
Enforce a minimum timelock for all transactions to implement security measures.
+ uint256 private constant MINIMUM_TIMELOCK = 1 hours;
.
.
.
function _getTimelockDelay(uint256 value) internal pure returns (uint256) {
uint256 sevenDaysTimeDelayAmount = 100 ether;
uint256 twoDaysTimeDelayAmount = 10 ether;
uint256 oneDayTimeDelayAmount = 1 ether;
if (value >= sevenDaysTimeDelayAmount) {
return SEVEN_DAYS_TIME_DELAY;
} else if (value >= twoDaysTimeDelayAmount) {
return TWO_DAYS_TIME_DELAY;
} else if (value >= oneDayTimeDelayAmount) {
return ONE_DAY_TIME_DELAY;
} else {
- return NO_TIME_DELAY;
+ return MINIMUM_TIMELOCK;
}
}