Active Boosts

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.

Submitted on Feb 23rd 2024 at 08:47:31 UTC by @OxSCSamurai for Boost | Puffer Finance

Report ID: #28660

Report type: Smart Contract

Report severity: Insight

Target: https://etherscan.io/address/0x3C28B7c7Ba1A1f55c9Ce66b263B33B204f2126eA#code

Impacts:

  • 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:

If for any reason it's not clear yet what the bug is, let me clarify for you:

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:

References

https://github.com/PufferFinance/pufETH/blob/3e76d02c7b54323d347c8277327d3877bab591f5/src/Timelock.sol#L255-L262 https://github.com/PufferFinance/pufETH/blob/3e76d02c7b54323d347c8277327d3877bab591f5/src/Timelock.sol#L256

Proof of Concept

TEST1:

https://github.com/PufferFinance/pufETH/blob/3a9f943b3a9133cb2ee9bbf8c39e876c4170ead7/script/DeployPuffETH.s.sol#L95

https://github.com/PufferFinance/pufETH/blob/3a9f943b3a9133cb2ee9bbf8c39e876c4170ead7/script/DeployPuffETH.s.sol#L190

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:

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:

Result: Function does not revert & test passes. Timelock delay of 7 days is accepted.

TEST3 (control test):

Same test as above with the bug fixed, but now using an invalid 6 days for the timelock delay duration:

Result: Function reverts & test fails. Timelock delay of 6 days is not accepted.

One final control test:

TEST4 (control test):

Same test as above with the bug fixed, but now using a valid 8 days for the timelock delay duration:

Result: Function does not revert & test passes. Timelock delay of 8 days is accepted.

Previous28656 - [SC - Insight] Blocking redeemwithdraw from vaultNext28663 - [SC - Low] Deposit of stETH fails due to LIDOs - wei corno...

Last updated

Was this helpful?