YNU-760: feat(ChannelHub): reclaim logic on failed transfers#586
YNU-760: feat(ChannelHub): reclaim logic on failed transfers#586
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughReplaces error-based outbound-transfer failures with event-driven reclaim tracking, adds Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Hub as ChannelHub
participant Token as Token/ETHReceiver
participant Reclaim as ReclaimStore
User->>Hub: pushFunds(recipient, token, amount)
activate Hub
Hub->>Token: transfer/call (gas: TRANSFER_GAS_LIMIT)
alt transfer succeeds
Token-->>Hub: success
Hub->>Reclaim: apply payment / update balances
Hub-->>User: operation completes
else transfer fails
Token-->>Hub: failure/revert
Hub->>Reclaim: _reclaims[account][token] += amount
Hub-->>User: emit TransferFailed event
end
deactivate Hub
User->>Hub: claimFunds(token, destination)
activate Hub
Hub->>Reclaim: read reclaimAmount = _reclaims[user][token]
Hub->>Token: transfer reclaimAmount => destination (gas-limited)
Hub->>Reclaim: _reclaims[user][token] = 0
Hub-->>User: emit FundsClaimed event
deactivate Hub
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Summary of ChangesHello @nksazonov, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and security of the ChannelHub contract by addressing critical vulnerabilities related to outbound transfer failures. By implementing a reclaim mechanism and gas limiting for transfers, the protocol can now gracefully handle scenarios like token blacklists, malicious recipient hooks, and gas depletion attacks without reverting transactions or locking funds. This ensures continuous channel operation and protects the Node from potential financial losses, improving the overall stability and reliability of the system. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a crucial security enhancement by implementing a "reclaim" mechanism for failed outbound transfers in the ChannelHub contract. This change prevents transaction reverts on transfer failures, mitigating serious fund-locking and denial-of-service attack vectors. The solution involves limiting gas on outbound transfers and accumulating failed transfer amounts for users to claim later. The implementation is well-documented and accompanied by a comprehensive test suite. I've identified a couple of areas for improvement to enhance the robustness and correctness of the implementation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
contracts/src/ChannelHub.sol (1)
290-302:withdrawFromVaultnow silently succeeds even when the actual transfer fails.With the new
_pushFundsbehavior, if thetoaddress can't receive funds (e.g., blacklisted ERC20 recipient or gas-consuming receiver),_nodeBalancesis decremented and theWithdrawnevent fires, but funds land in_reclaims[to]instead of arriving atto. The caller gets no on-chain signal of failure in the return value—only aTransferFailedevent.This is a behavior change worth documenting explicitly (e.g., in a NatDoc on
withdrawFromVault), so integrators know to watch forTransferFailedalongsideWithdrawn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/ChannelHub.sol` around lines 290 - 302, The withdrawFromVault function currently decrements _nodeBalances and emits Withdrawn even if _pushFunds fails and the funds are routed to _reclaims (signaled by TransferFailed); update the contract docs (NatSpec) for withdrawFromVault to explicitly document this behavior: describe that a successful call only means internal balance was decreased and Withdrawn emitted, but transfers may still fail and result in funds placed in _reclaims, and instruct integrators to monitor TransferFailed events (and optionally reclaims handling) when calling withdrawFromVault; reference withdrawFromVault, _pushFunds, _reclaims, Withdrawn, and TransferFailed in the NatSpec.
🤖 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 310-328: The claimFunds function is marked payable but never reads
msg.value, which lets callers accidentally send ETH that becomes stuck; remove
the payable modifier from the claimFunds declaration so the function cannot
receive native ETH, leaving the rest of the logic (the ETH-transfer branch using
token == address(0) and IERC20 flow) unchanged and ensuring no code relies on
msg.value in claimFunds.
- Around line 1272-1284: The ERC20 gas-limited call in the transfer path can
misinterpret a call to an EOA (no code) as success because (true, "") passes the
current check; update the success logic in the block using TRANSFER_GAS_LIMIT so
that when returndata.length == 0 you also verify the target token address
contains code (i.e., extcodesize > 0) before treating it as a successful
transfer. Concretely, modify the transferSucceeded computation (the code using
address(token).call, returndata, success) to require either decoded true OR
(returndata empty AND token is a contract) — use Address.isContract(token) or an
inline extcodesize check — and leave the reclaim logic (_reclaims[to][token] +=
amount and emit TransferFailed) unchanged.
In `@contracts/test/ChannelHub_pushFunds.t.sol`:
- Around line 98-183: Add comprehensive tests for ChannelHub.claimFunds: create
tests that (1) fund a reclaim via cHub.exposed_pushFunds then call
cHub.claimFunds() as the recipient (destination == msg.sender) for both ETH
(address(0)) and an ERC20 (normalToken) and assert balances changed and
FundsClaimed event emitted; (2) call claimFunds specifying a different
destination (destination != msg.sender) to exercise the blacklisted/third-party
flow and assert funds delivered and event emitted; (3) assert claimFunds reverts
when destination == address(0); (4) assert claimFunds reverts when reclaim
balance is zero; and (5) verify reentrancy protection by funding a malicious
receiver contract that attempts to reenter claimFunds during transfer and assert
the reentrant call fails/does not drain funds. Reference functions and symbols:
claimFunds, FundsClaimed, ChannelHub, cHub, exposed_pushFunds, normalToken,
recipient, TRANSFER_AMOUNT, and any test helpers used for balance assertions.
---
Nitpick comments:
In `@contracts/src/ChannelHub.sol`:
- Around line 290-302: The withdrawFromVault function currently decrements
_nodeBalances and emits Withdrawn even if _pushFunds fails and the funds are
routed to _reclaims (signaled by TransferFailed); update the contract docs
(NatSpec) for withdrawFromVault to explicitly document this behavior: describe
that a successful call only means internal balance was decreased and Withdrawn
emitted, but transfers may still fail and result in funds placed in _reclaims,
and instruct integrators to monitor TransferFailed events (and optionally
reclaims handling) when calling withdrawFromVault; reference withdrawFromVault,
_pushFunds, _reclaims, Withdrawn, and TransferFailed in the NatSpec.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
contracts/SECURITY.mdcontracts/src/ChannelHub.solcontracts/test/ChannelHub_pushFunds.t.solcontracts/test/TestChannelHub.solcontracts/test/mocks/GasConsumingERC20.solcontracts/test/mocks/GasConsumingEthReceiver.solcontracts/test/mocks/NonReturningERC20.solcontracts/test/mocks/RevertingERC20.solcontracts/test/mocks/RevertingEthReceiver.solprotocol-description.md
philanton
left a comment
philanton
left a comment
There was a problem hiding this comment.
So, are we going to allow them withdraw to another address right now?)
Oh, or are we allowing withdrawal even if the sender is not the original owner of these funds? So, like, we could withdraw it ourselves?)) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 1279-1281: The current transfer success check uses "balanceAfter
== balanceBefore - amount" which can underflow and revert if balanceBefore <
amount; change the logic that computes transferSucceeded (using variables
transferSucceeded, balanceBefore, balanceAfter, amount, success) to first ensure
balanceBefore >= amount (e.g., include balanceBefore >= amount in the boolean
expression) before performing the subtraction so the contract doesn't revert and
the subsequent reclaim logic can run.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
contracts/foundry.tomlcontracts/src/ChannelEngine.solcontracts/src/ChannelHub.solcontracts/test/ChannelHub_claimFunds.t.solcontracts/test/ChannelHub_pushFunds.t.solcontracts/test/TestChannelHub.solcontracts/test/mocks/MalformedReturningERC20.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/test/TestChannelHub.sol
|
Having the same question as Anton. But overall approved. |
|
@philanton , @dimast-x , the |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
contracts/src/ChannelHub.sol (1)
321-321: Prefer custom error over string revert for ETH claim failure.This keeps revert handling consistent with the rest of the contract and trims bytecode/gas.
Proposed diff
@@ error IncorrectStateIntent(); error IncorrectChannelStatus(); error ChallengerVersionTooLow(); error OnlyNonHomeEscrowsCanBeChallenged(); + error EthTransferFailed(); @@ if (token == address(0)) { (bool success,) = payable(destination).call{value: amount}(""); - require(success, "ETH transfer failed"); + if (!success) revert EthTransferFailed(); } else { IERC20(token).safeTransfer(destination, amount); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/ChannelHub.sol` at line 321, Replace the string revert in the ETH transfer require with a custom error: declare a new error (e.g., error ETHTransferFailed();) near the top of ChannelHub contract and change the require(success, "ETH transfer failed"); to revert ETHTransferFailed(); (or use if (!success) revert ETHTransferFailed();). This keeps revert handling consistent and reduces bytecode/gas.
🤖 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`:
- Line 321: Replace the string revert in the ETH transfer require with a custom
error: declare a new error (e.g., error ETHTransferFailed();) near the top of
ChannelHub contract and change the require(success, "ETH transfer failed"); to
revert ETHTransferFailed(); (or use if (!success) revert ETHTransferFailed();).
This keeps revert handling consistent and reduces bytecode/gas.
3 participants
Summary by CodeRabbit
New Features
Documentation
Tests