M-H04: feat(contracts/ChannelHub): purge during escrow challenge finalization, count both skip and purge

[nksazonov]

Description

The deposit escrow purge logic is intended to be bounded by MAX_DEPOSIT_ESCROW_PURGE, but the implementation only bounds the number of escrows that are successfully purged, not the number of queue entries scanned. When the loop encounters an already FINALIZED escrow, it advances without increasing purgedCount. As a result, a single purge call can scan an arbitrarily large finalized prefix even when maxToPurge is small, making the actual work performed by the call unbounded.
This becomes exploitable because escrow deposits are stored in one global queue. The timeout finalization path marks disputed escrows as FINALIZED without advancing escrowHead, allowing finalized entries to accumulate in front of the queue until a later purge attempt tries to process them.
An attacker can create many small non-home-chain escrow deposits and cause a large finalized prefix to accumulate at the queue head. Once that happens, the next call to  _purgeEscrowDeposits may need to iterate through all of those finalized entries in a single transaction before it can make progress. If the prefix is large enough, the purge step can run out of gas and revert.
Because _purgeEscrowDeposits is invoked by the protocol’s internal paths, this can cause most normal channel, escrow, and migration transitions to revert.
As a result, liquidity that should have been released can become practically stuck and core protocol workflows can become unavailable.

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Improved escrow queue processing by implementing eager cleanup during finalization operations, reducing queue accumulation and enhancing efficiency.

• Tests

  • Added comprehensive test coverage verifying queue cleanup is triggered across all protocol operations.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 83088ccb-0c24-4e5a-ba0b-0d6a32dbae0c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The pull request redefines the escrow deposit purge queue constraint from MAX_DEPOSIT_ESCROW_PURGE (count-based) to MAX_DEPOSIT_ESCROW_STEPS (step-based), where each loop iteration consumes a step regardless of entry type (skippable or purged). The purge control flow changes to track steps instead of purge count. Eager purge calls are added to finalization paths to process accumulated queue entries earlier. Comprehensive test coverage validates step-budget semantics and purge invocation across all protocol operations.

Changes

Cohort / File(s)	Summary
Core Purge Logic contracts/src/ChannelHub.sol	Renamed constant MAX_DEPOSIT_ESCROW_PURGE to MAX_DEPOSIT_ESCROW_STEPS; refactored _purgeEscrowDeposits(maxSteps) loop to track steps consumed per iteration (including skippable entries) instead of purged count; added eager purge calls in unilateral finalization paths for escrow deposits and withdrawals.
Test Harness contracts/test/TestChannelHub.sol	Updated harness_purgeEscrowDeposits parameter name from maxToPurge to maxSteps to align with new semantics.
Step Semantics Tests contracts/test/ChannelHub_escrowDepositPurge/ChannelHub_purgeEscrowDeposits.t.sol	Renamed test case from test_respectsMaxToPurge_stopsAfterLimit to test_respectsMaxSteps_stopsAfterLimit_allUnlockable; added new test cases validating that skippable entries (DISPUTED, FINALIZED) consume step budget and that purge stops when budget is exhausted before reaching unlockable entries.
Comprehensive Flow Tests contracts/test/ChannelHub_allFlowsInvokePurge.t.sol	New test contract implementing purge-detection mechanism via escrow snapshot and sentinel injection; provides 22 test cases covering channel operations (create, deposit, withdraw, checkpoint, close, challenge), escrow operations (initiate/finalize/challenge in both home-chain and non-home-chain contexts), and public purge invocation; verifies purge is invoked as expected and not invoked in vault operations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• PR #637: Modifies ChannelHub's escrow finalization paths (finalizeEscrowDeposit/finalizeEscrowWithdrawal) and cleanup logic, directly overlapping with eager purge call insertion.
• PR #620: Updates ChannelHub's non-home-chain escrow finalization and funds-release flows, sharing the same finalization function modifications.
• PR #640: Also treats FINALIZED and DISPUTED entries as skippable in the purge loop, reframing the purge constraint semantics similarly.

Suggested reviewers

• philanton
• dimast-x
• ihsraham

Poem

🐰 Step by step, the queue advances clear,
No longer counting what was purged so dear,
But every hop—skip, finalize, or release—
Consumes a step toward escrow peace!
With eager hands we cleanse the queue,
Testing flows both old and new.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: adding purge calls during escrow finalization and updating the counting mechanism to include both skip and purge operations.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/audit-m-h04

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[nksazonov]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧹 Nitpick comments (2)

contracts/test/ChannelHub_allFlowsInvokePurge.t.sol (1)

24-62: Please add migration paths to this suite.

initiateMigration() and finalizeMigration() still reach _purgeEscrowDeposits() via _applyEffects() in contracts/src/ChannelHub.sol, so they are part of the same regression surface. Right now a future change could stop invoking purge there without tripping this “all flows” suite.

Also applies to: 284-567

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@contracts/test/ChannelHub_allFlowsInvokePurge.t.sol` around lines 24 - 62,
The test suite is missing migration flows that also trigger _purgeEscrowDeposits
via _applyEffects; add cases that call initiateMigration() and
finalizeMigration() (both home and non-home variants if applicable) to the
ChannelHub_allFlowsInvokePurge.t.sol tests so the sentinel injection
(_snapshotAndInjectSentinel()) is applied before each migration action and
_assertPurgeInvoked() is asserted afterward, ensuring initiateMigration and
finalizeMigration are covered alongside the other flows that must invoke
_purgeEscrowDeposits in ChannelHub.sol.

contracts/src/ChannelHub.sol (1)

386-395: Align the exposed terminology with the new step budget.

This loop now budgets inspected entries, not successful purges. Renaming/documenting the remaining exposed maxToPurge surface as maxSteps would make the new behavior much clearer for callers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@contracts/src/ChannelHub.sol` around lines 386 - 395, The public-facing
semantics changed to budget inspected entries rather than successful purges, so
update the exposed parameter and any related references from the old name (e.g.,
maxToPurge) to maxSteps and adjust documentation/comments accordingly;
specifically, in the _purgeEscrowDeposits function signature and any external
callers/events/ABI docs that reference maxToPurge, rename the parameter to
maxSteps (or add a clear comment that maxToPurge is now a step budget) and
ensure all mentions (function name _purgeEscrowDeposits, escrowHead,
_escrowDepositIds usage) reflect the new terminology to avoid confusion for
callers.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@contracts/src/ChannelHub.sol`:
- Around line 386-395: The public-facing semantics changed to budget inspected
entries rather than successful purges, so update the exposed parameter and any
related references from the old name (e.g., maxToPurge) to maxSteps and adjust
documentation/comments accordingly; specifically, in the _purgeEscrowDeposits
function signature and any external callers/events/ABI docs that reference
maxToPurge, rename the parameter to maxSteps (or add a clear comment that
maxToPurge is now a step budget) and ensure all mentions (function name
_purgeEscrowDeposits, escrowHead, _escrowDepositIds usage) reflect the new
terminology to avoid confusion for callers.

In `@contracts/test/ChannelHub_allFlowsInvokePurge.t.sol`:
- Around line 24-62: The test suite is missing migration flows that also trigger
_purgeEscrowDeposits via _applyEffects; add cases that call initiateMigration()
and finalizeMigration() (both home and non-home variants if applicable) to the
ChannelHub_allFlowsInvokePurge.t.sol tests so the sentinel injection
(_snapshotAndInjectSentinel()) is applied before each migration action and
_assertPurgeInvoked() is asserted afterward, ensuring initiateMigration and
finalizeMigration are covered alongside the other flows that must invoke
_purgeEscrowDeposits in ChannelHub.sol.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8b687ec9-1fb7-41da-96c3-310689d40040

📥 Commits

Reviewing files that changed from the base of the PR and between b4c6077 and d3a6e26.

📒 Files selected for processing (4)

• contracts/src/ChannelHub.sol
• contracts/test/ChannelHub_allFlowsInvokePurge.t.sol
• contracts/test/ChannelHub_escrowDepositPurge/ChannelHub_purgeEscrowDeposits.t.sol
• contracts/test/TestChannelHub.sol