28660 - [SC - Insight] pufETHsrcTimelock_setDelay - L State constant M...
pufETH/src/Timelock::_setDelay() - L256: State constant MINIMUM_DELAY is incorrectly treated as an invalid minimum delay value, as can be seen here where newDelay <= MINIMUM_DELAY is used instead of newDelay < MINIMUM_DELAY.
Contract fails to deliver promised returns, but doesn't lose value
Description
Brief/Intro
Summary:
The hardcoded constant value for timelock minimum delay is incorrectly treated as an invalid timelock delay value.
Impact in scope:
Contract fails to deliver promised returns, but doesn't lose value
In this context the promised "return" is related to the hardcoded minimum timelock delay value of 7 days.
Vulnerability Details
Buggy function:
function _setDelay(uint256 newDelay) internal {
if (newDelay <= MINIMUM_DELAY) {
revert InvalidDelay(newDelay);
}
emit DelayChanged(delay, newDelay);
delay = newDelay;
}
If for any reason it's not clear yet what the bug is, let me clarify for you:
/**
* @notice Minimum delay enforced by the contract
*/
uint256 public constant MINIMUM_DELAY = 7 days;
It should be clear enough from the above that the value for MINIMUM_DELAY is a VALID minimum value for the timelock delay period. Therefore the <= in newDelay <= MINIMUM_DELAY should be <, because a newDelay value of 7 days should be 100% valid, but anything LESS than 7 days is INVALID, therefore we should use newDelay < MINIMUM_DELAY instead, so that it translates to all values of newDelay where newDelay >= MINIMUM_DELAY, are all VALID values.
Impact Details
Can never change the timelock delay value to the hardcoded VALID minimum delay period of 7 days as is intended protocol functionality because the buggy function was called during constructor/deployment time, as well as called whenever they want to change the delay again after deployment.
The lowest possible value(in days) for the timelock delay is 8 days, contrary to the "promised" minimum delay of 7 days.
Investors/VCs, regulators, users, protocol DAO/governance, multisigs, external integrating projects and external devs will all expect the timelock's minimum possible delay to be 7 days(as promised by the hardcoded MINIMUM_DELAY constant), but this is not the case. Anyone who plans/strategizes or builds according to this expectation, will be disappointed to learn that a delay of 7 days is not possible.
it seems there might have been some confusion during protocol tests and test setup and especially during deployment contract setup & testing as to "weird" behaviour when a minimum delay of 7 days was selected, so the response was to modify it to 7 days + 1 in order to bypass the "weird"/revert behaviour... when in fact it was simply the bug in the internal _setDelay() function.
Recommendations:
Ensure that the hardcoded value of constant MINIMUM_DELAY is treated correctly throughout the codebase. Here correctly means that 7 days timelock delay is still a valid value to be used in the protocol and that only anything strictly less than 7 days is invalid.
Ensure that during tests setup & testing that assumptions are properly validated. It seems the intention here was to deploy with 7 days as the initial timelock delay value, but your tests/deployment transactions reverted, due to this bug. The tests/deploy script was then modified to bypass this revert, by using a modified initial timelock delay value of 7 days + 1.
Correct understanding of minimum & maximum boundaries in terms of valid vs invalid values is crucial, and the implementation of this correct understanding must be consistently applied throughout the codebase.
BUGFIX:
function _setDelay(uint256 newDelay) internal {
- if (newDelay <= MINIMUM_DELAY) {
+ if (newDelay < MINIMUM_DELAY) {
revert InvalidDelay(newDelay);
}
emit DelayChanged(delay, newDelay);
delay = newDelay;
}
With the bug not yet fixed(newDelay <= MINIMUM_DELAY) and the protocol tests modified to use 7 days instead of (0 or 7 days + 1) as per above code snippets, we get an InvalidDelay error/revert in the test results using forge command ETH_RPC_URL=https://mainnet.infura.io/v3/YOUR_KEY forge test --contracts script/DeployPuffETH.s.sol -vvvvv:
├─ [2801070] DeployPuffETH::run()
│ ├─ [0] VM::startBroadcast(<pk>)
│ │ └─ ← ()
│ ├─ [2092564] → new AccessManager@0x652Fa863F2476bE5606f0611b622AE51f411BF05
│ │ ├─ emit RoleGranted(roleId: 0, account: 0xC4a2E012024d4ff28a4E2334F58D4Cc233EB1FE1, delay: 0, since: 1702902095 [1.702e9], newMember: true)
│ │ └─ ← 10323 bytes of code
│ ├─ [337052] → new NoImplementation@0x844FA36d460c4F0c4e14d900Ad9B294f223936c7
│ │ └─ ← 1683 bytes of code
│ ├─ [56850] → new PufferDepositor@0x1bA145F1BE82d7285FFfAee5f79dc7744ffD0a9F
│ │ ├─ emit Upgraded(implementation: NoImplementation: [0x844FA36d460c4F0c4e14d900Ad9B294f223936c7])
│ │ └─ ← 163 bytes of code
│ ├─ [0] VM::label(PufferDepositor: [0x1bA145F1BE82d7285FFfAee5f79dc7744ffD0a9F], "PufferDepositor")
│ │ └─ ← ()
│ ├─ [56850] → new PufferVault@0x989e351E99b9c7Ff8a31042521C772b0ECED39D3
│ │ ├─ emit Upgraded(implementation: NoImplementation: [0x844FA36d460c4F0c4e14d900Ad9B294f223936c7])
│ │ └─ ← 163 bytes of code
│ ├─ [0] VM::label(PufferVault: [0x989e351E99b9c7Ff8a31042521C772b0ECED39D3], "PufferVault")
│ │ └─ ← ()
│ ├─ [23096] → new <unknown>@0x9696768d5e2B611BD89181D54AeB3259Bab9616F
│ │ └─ ← 0 bytes of code
│ └─ ← InvalidDelay(604800 [6.048e5])
└─ ← InvalidDelay(604800 [6.048e5])
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.81s
Ran 5 test suites in 5.43s: 9 tests passed, 3 failed, 0 skipped (12 total tests)
Failing tests:
Encountered 1 failing test in test/Integration/PufferTest.integration.t.sol:PufferTest
[FAIL. Reason: setup failed: ] setUp() (gas: 0)
Encountered 1 failing test in test/unit/PufETH.t.sol:PufETHTest
[FAIL. Reason: setup failed: ] setUp() (gas: 0)
Encountered 1 failing test in test/unit/Timelock.t.sol:TimelockTest
[FAIL. Reason: setup failed: ] setUp() (gas: 0)
Encountered a total of 3 failing tests, 9 tests succeeded
Result: Function reverts & test fails. Timelock delay of 7 days is not accepted.
TEST2:
Same test as above, but now with the bug fixed(newDelay < MINIMUM_DELAY) and using a valid 7 days for the timelock delay duration:
