fix: forward AuctionManager bid revenue + guard WRNFT finalize ordering

[seongyun-ko]

Follow-ups to #385 from the multi-lens audit. Two H-rated findings; minimal-surface fixes.

H-5 · AuctionManager bid revenue trap

Pre-#385, every consumed bid forwarded bid.amount to membershipManagerContractAddress via processAuctionFeeTransfer. That function was deleted in this branch and updateSelectedBidInformation now only flips bid.isActive=false, stranding the consumed-bid ETH in the contract. The new treasury immutable was added but never read.

Fix: rewire updateSelectedBidInformation to forward bid.amount → treasury immediately on consumption. Drop the unused membershipManagerContractAddress immutable (and its constructor param). Emit BidRevenueForwarded for observability.

Bid-cancellation refund is unchanged — _cancelBid still refunds the bidder. The on-chain invariant address(this).balance ≥ Σ(bid.amount for active bids) is preserved.

Stranded today (not handled by this PR): ~0.097 ETH in accumulatedRevenue below the 0.2 ETH auto-forward threshold. That can be left in the contract per the audit conversation and handled later if desired.

H-3 · WithdrawRequestNFT init-order trap

finalizeRequests had no guard that initializeShareRateFreezeUpgrade() had pushed the sentinel checkpoint. If a finalizeRequests(N) slipped in between the proxy upgrade and the init call, it would push (N, rateNow) as the trace's first entry. Every legacy tokenId ≤ N would then lowerLookup to rateNow instead of the 0 sentinel that signals "fall back to live LP rate", silently mispaying every pre-upgrade holder.

Fix: add NotInitialized + a _finalizationRates.length() == 0 precondition to finalizeRequests. The upgrade-ordering trap becomes impossible — finalizeRequests reverts cleanly until the sentinel is in place.

Test changes

• TestSetup.sol pushes the sentinel as part of standard setup (matches post-upgrade reality). Done in both the fresh-deploy path and the fork-upgrade path.
• WithdrawRequestNFTIntrusive gains clearFinalizationRatesForTest() and a matching test-helper _clearFinalizationRatesForTest() so the four tests that explicitly exercise the pre-init transition can reset the trace.
• All new AuctionManager(...) call-sites updated for the dropped constructor param.

Test results

• forge test --match-contract WithdrawRequestNFTTest → 79/80 pass (1 fails due to missing MAINNET_RPC_URL, pre-existing).
• forge test --match-contract AuctionManagerTest → 21/21 pass.
• Full suite: 973 pass / 104 fail; every failure is a pre-existing missing-env (MAINNET_RPC_URL / OP_RPC_URL).

Deployment runbook addition

The upgrade timelock batch must call WithdrawRequestNFT.initializeShareRateFreezeUpgrade() after the new impl is in place. The new guard makes finalizeRequests revert cleanly until this happens, so the failure mode is loud rather than silent.

---

Note

High Risk
Changes treasury ETH routing on bid consumption and gates withdrawal finalization on upgrade ordering—both affect protocol funds and pre-upgrade claim semantics if deployment steps are wrong.

Overview
This PR closes two audit follow-ups: consumed auction bid ETH no longer sits in AuctionManager, and WithdrawRequestNFT.finalizeRequests cannot run before the share-rate-freeze sentinel exists.

AuctionManager: When a bid is consumed via updateSelectedBidInformation, the contract now sends bid.amount to the treasury immutable (with BidRevenueForwarded), replacing the removed membership-manager fee path. The unused membershipManagerContractAddress immutable and constructor argument are dropped; all new AuctionManager(...) sites are updated.

WithdrawRequestNFT: finalizeRequests reverts with NotInitialized while _finalizationRates is empty, so a finalize between proxy upgrade and initializeShareRateFreezeUpgrade() cannot seed a real rate as the first checkpoint and break legacy live-rate fallback.

Tests / deploy hygiene: TestSetup and fork setup call initializeShareRateFreezeUpgrade() by default; intrusive test helpers clear the checkpoint trace so pre-init behavior stays testable.

^{Reviewed by Cursor Bugbot for commit 998a922. 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.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit b0732d2. Configure here.}

[cursor]

reEnterAuction creates insolvency after bid ETH forwarded

Medium Severity

reEnterAuction re-activates a bid without restoring the ETH that updateSelectedBidInformation already forwarded to treasury. If called after consumption, bid.isActive becomes true again while the contract no longer holds bid.amount, breaking the invariant address(this).balance ≥ Σ(active bid amounts). A subsequent _cancelBid would attempt a refund the contract cannot cover.

Additional Locations (1)

• src/AuctionManager.sol#L207-L213

 

^{Reviewed by Cursor Bugbot for commit b0732d2. Configure here.}