Skip to content

fix: bound the deposit-refund backlog by collapsing refunds per address#397

Open
matthias-wright wants to merge 9 commits into
audit-may-2026from
m/withdrawal-overflow
Open

fix: bound the deposit-refund backlog by collapsing refunds per address#397
matthias-wright wants to merge 9 commits into
audit-may-2026from
m/withdrawal-overflow

Conversation

@matthias-wright

Copy link
Copy Markdown
Collaborator

Builds on #380 (which builds on #377, #276, and #275).

Addresses #362.

Changes:

  • finalizer/src/actor.rs — deposit_refund_key and its wrappers (refunded_deposit_key, invalid_signature_refund_key, malformed_deposit_refund_key, invalid_deposit_tax_key) no longer take a deposit index; key is now (tag, address).
  • finalizer/src/actor.rs — removed the now-unused deposit_index param from push_invalid_deposit_withdrawals and updated the three deposit-processing call sites and queue_deposit_refund's key construction. queue_deposit_refund keeps the param (still used in the parse-failure critical-error log).
  • finalizer/src/actor.rs — made queue_deposit_refund and DepositRejectionReason pub(crate) for testing.
  • finalizer/src/tests/deposit_refunds.rs (new, registered in tests/mod.rs) — many_invalid_deposits_to_one_address_collapse_to_one_entry (50 same-address refunds → one summed entry, one index consumed, incremental root == full rebuild) and distinct_addresses_and_reasons_stay_separate (no over-merging).

@sebastian-osec

Copy link
Copy Markdown

I think this is a good partial fix for the deposit-refund amplification case.

The new keying correctly collapses many rejected deposits to the same (reason tag, withdrawal address) into one pending refund entry, with the amount summed.

However, I do not think this fully closes #362 as written. The broader invariant is that the withdrawal cap should bound epoch-boundary finalizer work, not only the number of withdrawals emitted in the block.

get_withdrawals_for_epoch_with_limits still calls get_for_epoch_by_kind, which collects the full per-kind epoch queue into a Vec before applying .take(max_...). Epoch rollover also still calls reschedule_withdrawal_epoch, which moves all remaining withdrawals and rebuilds the withdrawal SSZ tree.

There is also still an attacker-controlled way to avoid this specific collapse: the merge key includes withdrawal_address, and withdrawal_credentials comes from the deposit request. So many invalid deposits with many distinct valid Eth1 withdrawal addresses still create many ready refund entries. The second new test confirms distinct addresses remain separate, which is correct for accounting, but means the backlog is bounded by distinct addresses rather than by the configured withdrawal cap.

I think the queue still needs capped-prefix iteration without materializing the full ready epoch, and rollover/SSZ updates need to avoid work proportional to the full overflow backlog.

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