Skip to content

fix(pwq): blacklist gate on PriorityWithdrawalQueue claim#441

Merged
0xpanicError merged 2 commits into
pankaj/feat/security-upgradesfrom
seongyun/feat/pr385-review-fixes
May 28, 2026
Merged

fix(pwq): blacklist gate on PriorityWithdrawalQueue claim#441
0xpanicError merged 2 commits into
pankaj/feat/security-upgradesfrom
seongyun/feat/pr385-review-fixes

Conversation

@seongyun-ko
Copy link
Copy Markdown
Contributor

@seongyun-ko seongyun-ko commented May 28, 2026

Summary

Addresses H-3 from the multi-lens audit of PR #385.

PriorityWithdrawalQueue._claimWithdraw now gates on blacklister.nonBlacklisted(request.user). Previously anyone could claim on behalf of request.user, including when that user became blacklisted between finalization and claim — proceeds would flow to a sanctioned address via a non-blacklisted accomplice. Both claimWithdraw and batchClaimWithdraw now fail-closed.

Disposition of other audit findings

  • H-1 (executeTasks permissionless) — intended, documented in the existing source comments.
  • H-2 (AuctionManager treasury hard-coded as immutable, no setter) — acked, not fixed. The simple version of the setter introduces a storage collision with the pre-deprecation DEPRECATED_admin slot on the deployed proxy: post-upgrade, the new treasury storage slot reads as the old admin EOA (non-zero), so the defensive address(0) revert doesn't fire and bid revenue would silently leak. A correct fix requires a one-shot reinitializer batched into the upgrade upgradeToAndCall, which expands scope materially. Deferring to a follow-up.
  • H-4 (storage-layout diff for all upgraded contracts) — ops checklist item, not code.
  • M-1 (invalidate→re-validate double-lock per security reviewer) — closed as not-a-bug. invalidateRequest reverts on finalized requests (WRN.sol:352) and EtherFiAdmin._validateWithdrawals sums only valid requests for the lock amount. The "double-lock" path isn't reachable.
  • M-3 (per-pauser cooldown) — current per-contract isolation already meets the constraint that the same pauser can pause multiple contracts back-to-back. No fix.
  • M-5 (per-address rate-limit grief) — downgraded to Low. Drain cost ≈ bucket cap, not free dust. Not a practical attack vector.
  • M-6 (WRN escrow check tightening) — dropped. Both old and new check would always pass in any reachable state (lock invariant guarantees ethAmountLockedForWithdrawal >= request.amountOfEEth at claim entry). Cosmetic change, no real underflow path.
  • M-7 / M-8 — intentional design (rate freeze at finalization; user stops earning rewards once request submitted).

Changes by file

File Change
src/withdrawals/PriorityWithdrawalQueue.sol Add IBlacklister public immutable blacklister; thread through constructor with zero-check; add blacklister.nonBlacklisted(request.user) at top of _claimWithdraw.
test/TestSetup.sol, test/PriorityWithdrawalQueue.t.sol, test/integration-tests/*, test/fork-tests/UpgradeStorageIntegrity.t.sol Thread blacklister arg through PWQ constructor call sites.
script/upgrades/priority-queue/transactionsPriorityQueue.s.sol Placeholder address(0) for blacklister in PWQ constructor; operator substitutes real address pre-deploy for bytecode-match verification.

Test plan

  • forge build clean (only pre-existing lint warnings)
  • Run forge test --match-path test/PriorityWithdrawalQueue.t.sol against a mainnet fork — requires MAINNET_RPC_URL
  • Add new test test_claimWithdraw_revertsForBlacklistedRequestUser (PWQ) — flagged by testing reviewer as priority follow-up
  • Add new test test_batchClaimWithdraw_revertsEntireBatchOnBlacklistedUser (PWQ) — pin batch failure semantics

Note

Medium Risk
Touches withdrawal payout and whitelist access with a new immutable constructor arg; mis-deployed blacklister address would brick the implementation, but behavior aligns with existing Blacklister usage elsewhere in the stack.

Overview
PriorityWithdrawalQueue now wires in IBlacklister as a constructor immutable (non-zero required) and uses it for sanctions gating.

Whitelist flows (onlyWhitelisted) call blacklister.nonBlacklisted(msg.sender) so a compromised VIP can be blocked by blacklist before ops removes whitelist.

Claims (_claimWithdraw, including batch) call blacklister.nonBlacklisted(request.user) so a third party cannot claim finalized ETH to a user who was blacklisted after fulfillment.

Constructor arity changes are reflected across tests, fork/integration upgrades, and the priority-queue upgrade script (bytecode verification still uses a placeholder address(0) for blacklister until ops substitutes the deployed address).

Reviewed by Cursor Bugbot for commit 6a5dd7a. Bugbot is set up for automated code reviews on this repo. Configure here.

@chatgpt-codex-connector
Copy link
Copy Markdown

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

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Fix All in Cursor

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

Reviewed by Cursor Bugbot for commit 2a96fcf. Configure here.

Comment thread script/upgrades/priority-queue/transactionsPriorityQueue.s.sol
@seongyun-ko seongyun-ko requested a review from 0xpanicError May 28, 2026 12:16
Addresses H-3 from the multi-lens audit of PR #385.

PriorityWithdrawalQueue.claimWithdraw was callable on behalf of any
`request.user`, including a sanctioned address that became blacklisted between
request finalization and claim — proceeds would flow to the blacklisted user via
a non-blacklisted accomplice. Add blacklister as an immutable, wire it through
the constructor with a zero-check, and call `blacklister.nonBlacklisted(request.user)`
at the top of `_claimWithdraw` so both `claimWithdraw` and `batchClaimWithdraw`
fail-closed on sanctioned recipients. Cancel path is already covered transitively
via eETH's own `nonBlacklisted` on transfer.

H-2 — bid-revenue treasury setter was acked rather than implemented.
Storage-collision risk with the pre-deprecation DEPRECATED_admin slot makes
the safe fix non-trivial; deferred.

Test/script call-site updates thread the new PWQ blacklister arg.
@seongyun-ko seongyun-ko force-pushed the seongyun/feat/pr385-review-fixes branch from 2a96fcf to 662d851 Compare May 28, 2026 12:30
@seongyun-ko seongyun-ko changed the title fix(pwq,wrnft): blacklist gate on PWQ claim + tighten WRN escrow check fix(pwq): blacklist gate on PriorityWithdrawalQueue claim May 28, 2026
@0xpanicError
Copy link
Copy Markdown

I dont see the rationale to have blacklister on priority queue as it is already whitelist gated.

@0xpanicError 0xpanicError merged commit 87bc3d6 into pankaj/feat/security-upgrades May 28, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants