[DO NOT MERGE] yash/refactor/security-upgrade#451
Open
0xpanicError wants to merge 63 commits into
Open
Conversation
- EtherFiAdmin: absolute per-report rebase delta cap (SINGLE_REPORT_REBASE_DELTA_CAP_BPS), independent of elapsedTime, so a long-spanning report cannot move the exchange rate by an outsized absolute amount while passing the annualized APR check. - EtherFiRestaker: add whenNotPaused to all fund-movement functions (previously the pause gated nothing) and add a Guardian fast-halt path (guardianPause). - Blacklister: bound the instant multisig setBlacklistUntil path to MAX_MULTISIG_BLACKLIST_DURATION; document moving permanent blacklist behind the timelock. Tests: blacklist duration bound + restaker pause-gating (passing on fork). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Removing the setBlacklistUntil 90-day cap added earlier. It only guarded against an accidental over-long blacklist, which is operationally recoverable (the multisig can shorten/clear it). It gave no protection against a compromised multisig, which can permanently blacklist via blacklistUser regardless. Net: friction without a real security gain. Reverts Blacklister.sol and Blacklist.t.sol to base. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses Cursor Bugbot review on #445: undelegate() queues withdrawal of all restaked assets (same fund-flow category as queueWithdrawals), so it must also respect the pause for the halt to be a complete/reliable signal. delegateTo is left ungated — it only changes delegation posture and queues/moves no funds. Test extended. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per @0xpanicError's review on #445: Rebase caps — replace the single symmetric 200bps constant with asymmetric, configurable bounds (defaults as proposed): - EtherFiAdmin: negative (slashing) cap on accruedRewards, default 3 bps of TVL (max initial slashing penalty ~2.44 bps), settable behind the operating timelock (maxNegativeRebaseBps, 0 = default). Raise-able during a correlated-slashing event. - LiquidityPool.rebase: positive (reward) upper bound enforced at the share-rate chokepoint, default 25 bps (~1 month at 3% APR), settable behind the operating timelock (maxPositiveRebaseBps, 0 = default). Defense-in-depth independent of the oracle path. Restaker — switch from OZ Pausable to the protocol-wide PausableUntil model: Guardian pauseContractUntil() (auto-expiring, cooldown) + multisig boolean pause; whenNotHalted gates all fund-flow fns (incl. undelegate). Replaces guardianPause(). TestSetup disables both caps for generic suites (large artificial rebases on small TVL); dedicated tests exercise enforcement. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- LiquidityPool: positive rebase cap is now a hard 25 bps constant (MAX_POSITIVE_REBASE_BPS), not governance-configurable (per review: no legitimate single report raises rate >25 bps). Dropped the setter/storage/event/error. Negative (slashing) cap in EtherFiAdmin stays configurable behind the operating timelock. - EtherFiRestaker: use the standard whenNotPaused everywhere by overriding _requireNotPaused() to also check _requireNotPausedUntil() (per review), instead of a bespoke whenNotHalted. - EtherFiRestaker: rename unPauseContract -> unpauseContract (+ interface). - PausableUntil: _pauseUntil falls back to MIN_PAUSE_DURATION when duration is unset, so the Guardian's emergency pause is never a silent no-op that still burns the cooldown (bot finding). - Tests: rewrote the rebase tests that used unrealistically large (5%-100%) rebases on tiny fresh-deploy TVL to use within-cap (<=0.25%) rebases with recomputed assertions; restaker pause tests updated for the rename. Pre-existing ERC721/withdraw fork-harness failures (identical on base) are unrelated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
yash/refactor/natspec
…ble-share-rate yash/fix/remove-acceptable-share-rate
seongyun-ko
changed the title
…re-rate yash/fix/remove-min-share-rate
…o yash/fix/eth-amount-locked
Follow-up to PR #454 review. The fix there (debit totalValueOutOfLp by the eETH amount credited at fulfill, not the ETH paid on claim) had no test that distinguished the two: every withdraw test passes the two amounts equal, so they pass against the buggy code too. - Add test_withdraw_debitsAmountOfEEth_notAmountPaid: pins the divergent down-rebase case (_amount < _amountOfEEth) at the LP boundary where the bug lived. Asserts the debit equals the fulfill-time credit and does not track the smaller ETH paid. - Document _amountOfEEth in the withdraw natspec: it is trusted caller-supplied state, not bounded by Guards 1-3; the checked subtraction is the only LP-local backstop, and the negative-rebase underflow threshold is _amountOfEEth (not _amount). - Fix stale comment in WithdrawEscrowE2E step4 that described the pre-fix behavior (debit by claimable). - Update the stale LiquidityPool.sol:297 _amount reference in the LpRebaseWrnClaimUnderflow finding header.
…th-tests Test + docs: pin withdraw(_amountOfEEth) accounting (follow-up to #454)
yash/fix/eth-amount-locked
…ausing yash/fix/deprecate-oz-pausing
…eentrancy yash/fix/deprecate-oz-pausing-reentrancy
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
0xpanicError
changed the title
yash/fix/deprecate-oz-ownable
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 29c5ad6. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
High Risk
Changes touch eETH/weETH/LiquidityPool accounting, rebase caps, finalized-withdraw semantics, and upgrade storage layout—core TVL and withdrawal paths require careful migration and caller updates.
Overview
This PR is a security-focused refactor of core protocol contracts, deploy scripts, and governance utilities—mostly behavioral and interface changes plus storage-layout preservation for upgrades, not a greenfield rewrite.
Access control & pausing:
eETH,weETH,LiquidityPool,Liquifier,DepositAdapter, and related contracts drop liveOwnableUpgradeableusage in favor ofDeprecatedOZOwnableplaceholders andRoleRegistry-driven roles. Legacybool pausedpaths and many per-contract pause/unpause entrypoints are removed; tokens lean onPausableUntil/ sharedPausable, withpauseUntil()on eETH/weETH gated by super guardian. Deploy scripts stop assertingowner() == timelockand droptransferOwnershipwhere ownership is no longer per-contract.LiquidityPool (high impact): Constructor loses
minAmountForShare;ConstructorAddressesmoves toILiquidityPool.rebaseadds a hardMAX_POSITIVE_REBASE_BPScap on reward increases. Finalized claims usewithdraw(amount, amountOfEEth, rate, shareOfEEth)sototalValueOutOfLpis debited by fulfill-time credit, not payout ETH.minAmountForShareinvariant checks are removed; reentrancy moves to SoladyReentrancyGuardTransientwithDeprecatedOZReentrancyGuardreserving old slots. V2 migration reads legacy locked ETH from__gap_3via assembly.Tokens:
EETHremoves duplicateRolesLibraryinheritance, drops localpausedand operating-multisig pause helpers, and drops__Ownable_init.WeETHaligns pausing withwhenNotPausedon transfers.Liquifier / adapters:
Liquifieruses deprecated OZ pause/reentrancy placeholders + transient guard;pauseContract-style APIs removed from interfaces.DepositAdaptertakes a struct constructor andIDepositAdapter.Governance / rate limiting: New
DeprecatedOZ*layout shims;EtherFiRateLimiteruses customPausable;pauseContract/unPauseContractremoved fromIEtherFiRateLimiter.EarlyAdopterPoolaliases OZPausableto avoid a name clash with ether.fiPausable.Scripts / misc: Bytecode verification updated for new
LiquidityPool/Liquifier/WithdrawRequestNFTconstructor shapes; cumulative rewards deploy comment reflects RoleRegistry-only access.Reviewed by Cursor Bugbot for commit 29c5ad6. Bugbot is set up for automated code reviews on this repo. Configure here.