Submission Details
Severity:
Ambiguous timelock boundary at 10 ETH
Root + Impact
Description
-
In normal scenarios timelock boundaries should be clearly defined and match the docs
-
The problem is that the documentation says "Transactions between 1 ETH and 10 ETH have a timelock of 1 day" and "Transactions between 10 ETH and 100 ETH have a timelock of 2 days". This confuses the users
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; // >= 100 ETH
} else if (value >= twoDaysTimeDelayAmount) {
@> return TWO_DAYS_TIME_DELAY; // >= 10 ETH (includes exactly 10)
} else if (value >= oneDayTimeDelayAmount) {
return ONE_DAY_TIME_DELAY; // >= 1 ETH and < 10 ETH
} else {
return NO_TIME_DELAY; // < 1 ETH
}
}
Risk
Likelihood:
-
Users reading the docs may expect different behavior than implemented
-
No special conditions are needed
Impact:
-
Code behaves differently than documented
-
Users won't expect such behaviour
Proof of Concept
// SPDX-License-Identifier: MIT
import {MultiSigTimelock} from "./MultiSigTimelock.sol";
contract BoundaryTest {
function testBoundaryConditions() external {
MultiSigTimelock wallet = new MultiSigTimelock();
// Test exactly 10 ETH
uint256 delay10ETH = wallet._getTimelockDelay(10 ether);
// Code returns 2 days (TWO_DAYS_TIME_DELAY)
assert(delay10ETH == 48 hours);
// Test exactly 100 ETH
uint256 delay100ETH = wallet._getTimelockDelay(100 ether);
// Code returns 7 days (SEVEN_DAYS_TIME_DELAY)
assert(delay100ETH == 7 days);
// Spec says "100 ETH and above" which matches
}
}
Recommended Mitigation
/**
* @dev This is a role-based multisig rather than a signature-based multisig(albeit, this choice is more gas-intensive). It implements a Timelock feature when executing transactions.
* The contract allows up to five signers, with a minimum of three required to confirm a transaction before it can be executed.
* The timelock duration is determined by the value of the transaction:
* - Transactions below 1 ETH have no timelock.
- * - Transactions between 1 ETH and 10 ETH have a timelock of 1 day.
- * - Transactions between 10 ETH and 100 ETH have a timelock of 2 days.
+ * - Transactions of 1 ETH up to (but not including) 10 ETH have a timelock of 1 day.
+ * - Transactions of 10 ETH up to (but not including) 100 ETH have a timelock of 2 days.
* - Transactions of 100 ETH and above have a timelock of 7 days.
*/
Updates
Lead Judging Commences
kelechikizito 4 days ago
Submission Judgement Published Assigned finding tags:
Timelock Natspec mismatch
Rewards breakdown
nSLOC
205This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.