H-L02: fix(contracts/ChannelHub): revert on withdrawFromVault failure

[nksazonov]

Description

A node can permanently lock their own vault funds by calling withdrawFromVault(..) with a contract address that:

1. causes the transfer to fail
2. cannot call claimFunds(..) to recover the funds from reclaims

At L304 in withdrawFromVault(..), the node's vault balance is reduced regardless of transfer success. When the transfer fails in _pushFunds(..) (L1322-1324 for ETH, L1342-1344 for ERC20), the funds are moved to _reclaims[contractAddress][token]. However, if contractAddress is a contract without the capability to call claimFunds(..) (no function to make external calls, old contract, simple escrow, etc.), the funds become permanently inaccessible. The core issue is architectural: withdrawFromVault(..) uses the reclaim mechanism designed for adversarial channel operations in a non-adversarial context where it creates unexpected partial-success behavior. The node's vault balance is reduced even when the transfer fails, violating standard atomicity expectations where operations should either succeed completely or fail completely.

Impact

A node operator can permanently lose access to their entire vault balance through withdrawFromVault(..) when the destination address cannot receive the transfer or claim from reclaims. The operation exhibits partial-success behavior: the transaction succeeds (doesn't revert), the vault balance is reduced, but the funds end up in reclaims at an address that may not be able to recover them. This is non-standard behavior that violates atomicity principles - users cannot distinguish between 'transfer succeeded' and 'transfer failed but balance still reduced' without explicitly checking reclaim balances. The maximum impact is the node's entire vault balance across all tokens. While this requires user error (choosing an invalid destination), the protocol's design amplifies this error by using partial-success behavior instead of reverting.

Summary by CodeRabbit

• Refactor

  • Enhanced fund transfer resilience by introducing a non-reverting transfer mechanism; failed transfers are now tracked and recoverable instead of reverting transactions.

• Tests

  • Added comprehensive test coverage for vault deposit and withdrawal operations with ERC20 and native ETH asset support.
  • Added tests for transfer failure scenarios and recovery mechanisms to ensure atomicity and proper error handling.

[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: 19ee8efd-1537-401c-9071-eea01e48790c

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 PR introduces a dual-function approach to fund transfers in ChannelHub: a reverting _pushFunds function for critical paths and a non-reverting _nonRevertingPushFunds function with reclaim-on-failure handling for settlement, closure, and escrow paths. Comprehensive withdrawal tests are added alongside test infrastructure updates.

Changes

Cohort / File(s)	Summary
Core Transfer Logic contracts/src/ChannelHub.sol	Introduced _pushFunds (reverts on failure) and _nonRevertingPushFunds (stores failed transfers in _reclaims mapping) for both native ETH and ERC20 tokens. Replaced multiple _pushFunds calls with _nonRevertingPushFunds in channel settlement/closure and escrow finalization paths where user funds are released.
Vault Withdrawal Tests contracts/test/ChannelHub_IVault.t.sol	New comprehensive test suite covering input validation (zero address, zero amount, insufficient balance), successful withdrawals for both ERC20 and native ETH, event emission verification, full-balance withdrawals, and failure atomicity for both token types with reclaim balance verification.
Test Infrastructure contracts/test/ChannelHub_Base.t.sol, contracts/test/TestChannelHub.sol, contracts/test/ChannelHub_nonRevertingPushFunds.t.sol	Updated test setup to include ETH vault deposit, replaced exposed testing wrapper exposed_pushFunds with exposed_nonRevertingPushFunds, added workaround_setNodeBalance helper, and renamed/updated test contract to target non-reverting variant.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• M-M02: fix(contracts): require zero allocs on close #643: Modifies ChannelHub's close/transition payout behavior to use non-reverting pushFunds with reclaim-on-failure, aligned with settlement path changes in this PR.
• YNU-827: test(contracts): non home chain challenge flows #620: Modifies ChannelHub's escrow/challenge finalization fund-release paths with updated _pushFunds behavior during escrow resolution.
• H-M01: fix(contracts/ChannelHub): remove transferred amount check #636: Modifies ChannelHub's outbound-transfer logic by introducing return-data-based failure handling and reclaim crediting mechanism that parallels this PR's dual-variant approach.

Suggested reviewers

• dimast-x
• philanton
• ihsraham

Poem

🐰 Hop, hop, transfers now take two paths clear—
One reverts when funds disappear,
The other reclaims with graceful care,
No loss of coin in the rabbit's lair! ✨

🚥 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 clearly describes the main change: fixing withdrawFromVault to revert on transfer failure, which directly addresses the architectural issue documented in the PR objectives of preventing permanent fund loss due to partial-success behavior.
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-h-l02

---

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]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

contracts/src/ChannelHub.sol (1)

1388-1392: ⚠️ Potential issue | 🔴 Critical

_nonRevertingPushFunds has a revert path via _trySafeTransfer() that violates its "never reverts" guarantee.

The function's documentation states it is never reverts and is used in adversarial settlement contexts. However, _trySafeTransfer() decodes the low-level call's return data as bool using abi.decode(returnData, (bool)). This operation reverts if the bytes do not represent a canonical boolean value (0 or 1). A non-compliant ERC20 token returning a non-canonical value (e.g., 0xFF) would trigger this revert, violating the critical guarantee on settlement/close paths.

Fix: Decode as uint256 and check equality

-        if (returnData.length >= 32) return abi.decode(returnData, (bool));
+        if (returnData.length >= 32) {
+            return abi.decode(returnData, (uint256)) == 1;
+        }

🤖 Prompt for AI Agents

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

In `@contracts/src/ChannelHub.sol` around lines 1388 - 1392,
_nonRevertingPushFunds relies on _trySafeTransfer to never revert but
_trySafeTransfer currently abi.decode(returnData, (bool)) which can revert on
non-canonical ERC20 returns; update _trySafeTransfer to decode returnData as
uint256 (e.g., abi.decode(returnData, (uint256))) and treat a value of 1 (or any
non-zero convention you choose) as success, also handle empty returnData as
success for tokens that return nothing; keep the same failure path that
increments _reclaims and emits TransferFailed when transfer is considered
unsuccessful so _nonRevertingPushFunds retains its "never reverts" guarantee.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@contracts/src/ChannelHub.sol`:
- Around line 1142-1145: The cooperative/signed payout branches currently call
_nonRevertingPushFunds which converts failures into _reclaims and can
permanently strand funds for users who can't call claimFunds(); instead, for
voluntary releases (e.g., paths reached from withdrawFromChannel, closeChannel,
signed escrow finalization) replace calls to _nonRevertingPushFunds with
_pushFunds so a failing push reverts the transaction and does not move balances
into _reclaims; keep _nonRevertingPushFunds only on explicit
adversarial/timeout/challenge settlement branches where reclaim semantics are
intended, and ensure you still adjust meta.lockedFunds and do not change how
_reclaims is written in those adversarial branches.

---

Outside diff comments:
In `@contracts/src/ChannelHub.sol`:
- Around line 1388-1392: _nonRevertingPushFunds relies on _trySafeTransfer to
never revert but _trySafeTransfer currently abi.decode(returnData, (bool)) which
can revert on non-canonical ERC20 returns; update _trySafeTransfer to decode
returnData as uint256 (e.g., abi.decode(returnData, (uint256))) and treat a
value of 1 (or any non-zero convention you choose) as success, also handle empty
returnData as success for tokens that return nothing; keep the same failure path
that increments _reclaims and emits TransferFailed when transfer is considered
unsuccessful so _nonRevertingPushFunds retains its "never reverts" guarantee.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 477d09f5-ad79-4c45-b1e0-ff845a86626b

📥 Commits

Reviewing files that changed from the base of the PR and between e39b229 and cd0a732.

📒 Files selected for processing (5)

• contracts/src/ChannelHub.sol
• contracts/test/ChannelHub_Base.t.sol
• contracts/test/ChannelHub_IVault.t.sol
• contracts/test/ChannelHub_nonRevertingPushFunds.t.sol
• contracts/test/TestChannelHub.sol