test: atomic I3 finalize+lock (Bugbot #470 "Lock finalize not rolled back")

[seongyun-ko]

What this is

Addresses the one remaining open Bugbot finding on #470 — "Lock finalize not rolled back" (Medium) — by making the I3 handler's finalize+lock atomic, exactly like production. Test-only; no src/ changes.

The finding

In WithdrawalSolvencyHandler.finalizeRequests, the lock (addEthAmountLockedForWithdrawal) and the finalize (finalizeRequests) ran as two separate calls. A successful lock followed by a finalize revert orphaned the lock: ethAmountLockedForWithdrawal stayed bumped while lastFinalizedRequestId did not advance, so a later fuzz step could re-lock the same id range and move extra ETH out of the LP. Production never exposes that window — EtherFiAdmin._finalizeWithdrawals (EtherFiAdmin.sol:427-428) runs finalizeRequests then addEthAmountLockedForWithdrawal in one atomic transaction.

The fix

Route both legs through a single external self-call this._finalizeThenLock(target, lockAmount) in production order (finalize → lock), wrapped in try/catch. The shared call frame gives all-or-nothing semantics: if either leg reverts, both state changes roll back — no committed lock can outlive a failed finalize, and no id range is ever locked twice.

• P1 enforcement observation is preserved (success ⇒ bound held; InsufficientLiquidity revert ⇒ bound genuinely violated).
• P1 is still positively/adversarially driven every run by the dedicated probeOverLiquidityLock op, so the non-vacuity gate never depends on the fuzzer stumbling onto an over-bound range here.

Verification

Re-ran the full suite on a mainnet fork — 7/7 pass, with the non-vacuity gates all positively satisfied:

requestsCreated     : 31
requestsFinalized   : 5
requestsClaimed     : 10
finalizeBoundChecks : 5
lockBoundEnforced   : 17

(Fork run done via a local anvil fork node pinned to block 25368000 to dodge a forge-1.5.1 anvil-IPC regression on this machine; the suite itself is unmodified beyond this handler.)

Status of the other #470 Bugbot findings

For the record, the other six are already resolved at the current seongyun/fuzz/security-upgrades HEAD:

• rebase caller / PWQ ctor args / dead snapshot vars — fixed in test: address Bugbot review on #470 (rebase caller, PWQ ctor args, dead vars) #479 (merged).
• "Unset membership manager on fork" (ValidatorStateMachine) — already wired to the real MEMBERSHIP_MANAGER deployed constant (commit 1174666), with an inline comment covering exactly this.
• "Rewards invariants lack non-vacuity" (RewardsDistributor) — the afterInvariant gate + deterministic lifecycle seed are already present (commits 9018e8f / c4cc526).

This PR closes the last one.

---

Note

Low Risk
Test-only invariant handler change; production contracts are untouched.

Overview
The I3 withdrawal solvency fuzz handler no longer runs lock and finalize as separate steps. It now mirrors EtherFiAdmin._finalizeWithdrawals: finalizeRequests first, then addEthAmountLockedForWithdrawal, inside a single this._finalizeThenLock self-call wrapped in try/catch.

That gives production-like all-or-nothing behavior on the fork: if either leg reverts, both roll back, so the handler cannot leave an orphaned lock (bumped ethAmountLockedForWithdrawal without advancing lastFinalizedRequestId) and then re-lock the same id range on a later fuzz step.

P1 checks on successful locks and InsufficientLiquidity reverts are unchanged; adversarial coverage still comes from probeOverLiquidityLock. Test-only change in WithdrawalSolvencyHandler.sol — no src/ changes.

^{Reviewed by Cursor Bugbot for commit d090237. Bugbot is set up for automated code reviews on this repo. Configure here.}

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.