H-M01: fix(contracts/ChannelHub): remove transferred amount check

[nksazonov]

Description

The _pushFunds function uses a balance-checking pattern to verify ERC20 transfer success by comparing balances before and after the transfer. However, when success == true but the balance check fails (balanceAfter != balanceBefore - amount), the protocol incorrectly credits the full amount to _reclaims even though the tokens have already been transferred to the recipient. This creates a double-spend vulnerability where the recipient receives tokens twice: once from the actual transfer and once from claiming the reclaim. The vulnerability is triggered when the contract's balance increases during the transfer operation, which occurs with ERC777 tokens when the recipient's tokensReceived hook donates tokens back to ChannelHub. The protocol explicitly supports ERC777 tokens (evidenced by the L150 comment about 'ERC777 hooks' in gas limit calculation), making this an intended use case. The vulnerability affects all operations calling _pushFunds(...): withdrawFromVault(..), closeChannel(..), finalizeEscrowDeposit(..), finalizeEscrowWithdrawal(..), and challenge processing.

Impact

A malicious node operator can deploy a contract with a malicious tokensReceived hook that donates tokens back to ChannelHub during withdrawals. This causes the balance check to fail even though the transfer succeeded, resulting in the full amount being credited to reclaims. The attacker can then claim these funds again via claimFunds, effectively receiving 2x the withdrawn amount while only 1x is deducted from their vault accounting. By repeating this attack, the attacker can drain their entire vault balance plus an equal amount from the protocol's reserves, creating an insolvency situation where other users cannot withdraw their full balances. The cost is minimal (1-2 wei per withdrawal) and the profit is 100% of the withdrawn amount. Since the ChannelHub holds tokens for all nodes in a shared balance, the accounting shortfall created by this exploit affects all other users of the same token, effectively allowing the attacker to steal funds that belong to other nodes.

Summary by CodeRabbit

• Bug Fixes

  • Enhanced ERC20 transfer failure detection to properly handle edge-case token behaviors that could previously cause incorrect transaction processing, improving protocol robustness.

• Documentation

  • Updated security guidelines to detail ERC20 transfer failure mechanisms and potential attack vectors.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8cf30001-87cf-4462-ba26-bc13e02cbbf9

📥 Commits

Reviewing files that changed from the base of the PR and between 3e0a27e and e368985.

📒 Files selected for processing (4)

• contracts/SECURITY.md
• contracts/src/ChannelHub.sol
• contracts/test/ChannelHub_pushFunds.t.sol
• contracts/test/mocks/DonatingERC20.sol

---

📝 Walkthrough

Walkthrough

The PR replaces ERC20 transfer failure detection in ChannelHub's _pushFunds from balance-delta checking to return-value checking via a new _trySafeTransfer helper. Documentation clarifies two failure vectors: gas depletion and donation-back double-spend. New tests and a DonatingERC20 mock validate the updated approach.

Changes

Cohort / File(s)	Summary
Transfer Failure Detection Implementation contracts/src/ChannelHub.sol	Replaced balance-delta verification with return-value-based detection via new _trySafeTransfer(...) internal function. On transfer failure, credits _reclaims[to][token] and emits TransferFailed event. Handles empty returns, boolean-encoded returns, and non-standard short return data according to return-data semantics.
Test Coverage & Mocks contracts/test/ChannelHub_pushFunds.t.sol, contracts/test/mocks/DonatingERC20.sol	Added test for ERC20 returning false on transfer failure (test_accumulatesReclaims_whenERC20ReturnsFalse) and test for donation-back edge case (test_succeeds_whenERC777DonatesBack). Introduced DonatingERC20 mock contract that donates tokens back to ChannelHub during transfers to validate no false reclaim credit occurs.
Security Documentation contracts/SECURITY.md	Expanded ERC777 gas depletion attack documentation with explicit description of donation-back double-spend vector and clarified that protocol uses return-value checking (not balance-delta checking) for transfer success determination.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant ChannelHub
    participant Token
    
    rect rgba(100, 149, 237, 0.5)
    note over Caller,Token: New Return-Value-Based Transfer Failure Detection
    Caller->>ChannelHub: _pushFunds(to, token, amount)
    ChannelHub->>ChannelHub: _trySafeTransfer(token, to, amount)
    ChannelHub->>Token: transfer{gas: TRANSFER_GAS_LIMIT}(to, amount)
    
    alt Transfer Success (returns true)
        Token-->>ChannelHub: return true
        ChannelHub-->>Caller: Proceed (no reclaim credit)
    else Transfer Failure (returns false or reverts)
        Token-->>ChannelHub: return false or revert
        ChannelHub->>ChannelHub: _reclaims[to][token] += amount
        ChannelHub->>ChannelHub: emit TransferFailed(...)
        ChannelHub-->>Caller: Failure handled, reclaim credited
    end
    end
    
    rect rgba(255, 165, 0, 0.5)
    note over Caller,Token: Donation-Back Edge Case (Handled Correctly)
    Caller->>ChannelHub: _pushFunds(to, token, amount)
    ChannelHub->>Token: transfer{gas: TRANSFER_GAS_LIMIT}(to, amount)
    Token->>Token: Execute transfer to recipient
    Token->>Token: Donate tokens back to ChannelHub
    Token-->>ChannelHub: return true
    ChannelHub-->>Caller: Transfer succeeds (no false reclaim)
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• YNU-760: feat(ChannelHub): reclaim logic on failed transfers #586: Modifies ChannelHub's outbound ERC20 transfer failure handling with gas-capped low-level transfer semantics and reclaim crediting on failures.

Suggested reviewers

• ihsraham
• philanton
• dimast-x

Poem

🐰 A hop and a bound, returns now reign supreme,
No balance-checks needed, just return values gleam—
Donations may dance back, but we're wise to the trick,
Safe transfers at last, swift and slick!
✨🥕

🚥 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 and specifically identifies the main change: removing the transferred amount check from ChannelHub's _pushFunds function to fix a vulnerability.
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-m01

---

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.