Submission Details
Severity:
The `TimelockController:scheduleBatch` function lets users schedule operations even if their predecessor is unexecuted, contradicting the docs, which state that an operation can't be created until its predecessor is executed.
Description:
```solidity
@> if (!isOperationDone(predecessor) && !isOperationPending(predecessor)) {
revert PredecessorNotExecuted(predecessor);
}
```
This check, is trying to prevent to create an operation if the predecesor have not been executed, but this check is wrong because is saying that if the operation is not done and if it's not pending revert, but if the operation is not done means that it's pending and if the operation is not pending means that's done.
Impact:
Will be posible to create a new operation even if the predecesor is not done, while there is a check trying to prevent that behaviour.
Proof of Concept:
<details><sumarry>Proof of Code</sumarry>
1. Open the `linux terminal`, `wsl` in windows.
2. `nomicfoundation` installation:
- If you have `npm` installed run this command:
- `npm install --save-dev @nomicfoundation/hardhat-foundry`
- If you have `yarn` installed run this command:
- `yarn add --dev @nomicfoundation/hardhat-foundry`
- If you have `pnpm` installed run this command:
- `pnpm add --save-dev @nomicfoundation/hardhat-foundry`
3. open the `hardhat.config.cjs`
- Paste this at the begining of the code:
- `require("@nomicfoundation/hardhat-foundry");`
4. run `npx hardhat init-foundry`
- This task will create a `foundry.toml` file with the right configuration and install `forge-std`
5. In the `test/` forlder create a new folder called `ProofOfCodes`
6. In this `test/ProofOfCodes` folder create a new file and paste the following code
7. To run the test you should run `forge test --mt test_canCreateAOperationAlsoIfPredecesorIsNotDone -vvvv`
```solidity
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
import {Test} from "lib/forge-std/src/Test.sol";
import {TimelockController} from "contracts/core/governance/proposals/TimelockController.sol";
contract TimelockControllerPoC is Test {
TimelockController timelockController;
address admin = makeAddr("admin");
address superUser = makeAddr("superUser");
address user = makeAddr("user");
uint256 constant INITIAL_TIMELOCK_DELAY = 4 days;
bytes32 zeroBytes = bytes32(0);
bytes32 randomNumber = bytes32("323");
bytes32 randomNumber1 = bytes32("232");
address[] targets;
uint256[] values;
bytes[] calldatas;
address[] proposers;
address[] executors;
function setUp() external {
proposers.push(superUser);
executors.push(superUser);
timelockController = new TimelockController(INITIAL_TIMELOCK_DELAY, proposers, executors, admin);
}
function test_canCreateAOperationAlsoIfPredecesorIsNotDone() external {
targets.push(user);
values.push(10);
calldatas.push("");
vm.startPrank(superUser);
bytes32 predecesorId = timelockController.scheduleBatch(targets, values, calldatas, zeroBytes, randomNumber, 5 days);
assertEq(timelockController.isOperationDone(predecesorId), false);
timelockController.scheduleBatch(targets, values, calldatas,predecesorId, randomNumber1, 5 days);
bytes32 actualId = timelockController.hashOperationBatch(targets, values, calldatas,predecesorId, randomNumber1);
vm.stopPrank();
assertEq(timelockController.isOperationDone(predecesorId), false);
assertEq(timelockController.isOperationDone(actualId), false);
}
}
```
</details>
Recommended Mitigation:
- Modify the check, instead of saying `&&` it should say `||` and the second part
should say `isOperationPending(predecessor)` instead of `!isOperationPending(predecessor)`.
```diff
- if (!isOperationDone(predecessor) && !isOperationPending(predecessor)) {
+ if (!isOperationDone(predecessor) && isOperationPending(predecessor)) {
revert PredecessorNotExecuted(predecessor);
}
```
Updates
Lead Judging Commences
inallhonesty 3 months ago
Submission Judgement Published Assigned finding tags:
TimelockController::scheduleBatch checks if predecessor is pending OR executed rather than requiring execution as per comment, allowing scheduling before predecessor executes
inallhonesty 3 months ago
Submission Judgement Published Assigned finding tags:
TimelockController::scheduleBatch checks if predecessor is pending OR executed rather than requiring execution as per comment, allowing scheduling before predecessor executes
Prize pool breakdown
Total prize
77,280 USDC
nSLOC
3,86420 USDC / LOC
74,000
3,280
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.