fix: override transfer to with transfer fee behavior #7264
Conversation
|
yorhodes
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## audit-q3-2025 #7264 +/- ##
=================================================
+ Coverage 76.03% 76.74% +0.71%
=================================================
Files 123 123
Lines 2729 2718 -11
Branches 244 252 +8
=================================================
+ Hits 2075 2086 +11
+ Misses 635 614 -21
+ Partials 19 18 -1
🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughAdds an internal Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant TokenRouter
participant FeeHook as _transferFee()
participant Transfer as _transferTo()
User->>TokenRouter: initiate transfer (calculate fees)
activate TokenRouter
TokenRouter->>FeeHook: _transferFee(feeRecipient, feeAmount)
activate FeeHook
alt default
FeeHook->>Transfer: _transferTo(feeRecipient, feeAmount)
else override (CCTP/Everclear)
FeeHook->>Transfer: transfer from router's wrapped token balance
end
deactivate FeeHook
TokenRouter->>Transfer: _transferTo(user, transferAmount)
deactivate TokenRouter
sequenceDiagram
participant Constructor
participant EverclearEthBridge
Constructor->>EverclearEthBridge: Old: new(..., _scale, ...)
Note over EverclearEthBridge: caller supplied scale
Constructor->>EverclearEthBridge: New: new(...) with SCALE = 1
Note over EverclearEthBridge: scale hardcoded as constant
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (2)solidity/**/*.sol📄 CodeRabbit inference engine (CLAUDE.md)
Files:
solidity/contracts/token/**/*.sol📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (40)
🔇 Additional comments (2)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
solidity/contracts/token/libs/TokenRouter.sol (1)
189-197: Switching to_transferFeefixes the fee black hole on no‑op_transferTobridges. Nice.This cleanly routes fee payouts even when delivery is handled elsewhere (CCTP/Everclear). Please double‑check every router where
_transferTois intentionally no‑op also overrides_transferFeeto avoid silent drops.#!/bin/bash # Find files that override _transferTo with an empty/no-op body, and ensure they also override _transferFee. set -euo pipefail # 1) Collect no-op _transferTo overrides tmp_noop="$(mktemp)" fd -t f -e sol solidity | while read -r f; do if rg -nUP '(?s)function\s+_transferTo\s*\([^)]*\)\s*internal\s+override[^{]*\{\s*(//[^\n]*\n)?\s*\}' "$f" >/dev/null; then echo "$f" >> "$tmp_noop" fi done echo "No-op _transferTo overrides:" cat "$tmp_noop" || true echo # 2) Ensure those files also override _transferFee echo "Files missing _transferFee override:" while read -r f; do rg -nP 'function\s+_transferFee\s*\(' "$f" >/dev/null || echo "$f" done < "$tmp_noop" || true
🧹 Nitpick comments (3)
solidity/contracts/token/libs/TokenRouter.sol (1)
366-384: New_transferFeehook reads well; consider an event (optional).Defaulting to
_transferTokeeps synthetic/collateral routes tidy. If you want easier analytics later, an optionalFeeTransferred(recipient, amount)event here would be handy, but not required.solidity/contracts/token/libs/TokenCollateral.sol (1)
27-28: Clarify the WETHCollateral doc (tiny).Suggest spelling out that routers quote as native (token() == address(0)) while still custodying WETH internally.
- * @dev TokenRouters must have `token() == address(0)` to use this library. + * @dev Intended for routers that custody WETH but quote/route as native ETH: + * token() MUST return address(0) while the router still holds an IWETH instance.solidity/test/token/EverclearTokenBridge.t.sol (1)
404-452: Solid regression: assert fee recipient actually gets paid; make quote deterministic.Nice coverage—this catches the old “fee swallowed by bridge” behavior and proves the fee lands with the recipient contract. One tweak to keep the mud from splashing: quote the fee under the same caller the router/bridge would use, in case the fee calc depends on msg.sender or context.
Suggested changes:
- Prank as the bridge when calling feeContract.quoteTransferRemote.
- Optionally ensure the fee is non‑zero to avoid a vacuous pass.
Based on learnings
- // Get the expected fee from the feeContract - uint256 expectedFeeRecipientFee = feeContract - .quoteTransferRemote(DESTINATION, RECIPIENT, TRANSFER_AMT)[0].amount; + // Get the expected fee from the feeContract as if called by the bridge + vm.prank(address(bridge)); + uint256 expectedFeeRecipientFee = + feeContract.quoteTransferRemote(DESTINATION, RECIPIENT, TRANSFER_AMT)[0].amount; + // Optional: ensure test isn't vacuous + // assertGt(expectedFeeRecipientFee, 0);Also, per repo guidelines, give solhint a quick run to keep everything smelling fresh:
- yarn --cwd solidity lint
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
solidity/contracts/token/TokenBridgeCctpBase.sol(1 hunks)solidity/contracts/token/bridge/EverclearTokenBridge.sol(2 hunks)solidity/contracts/token/libs/TokenCollateral.sol(1 hunks)solidity/contracts/token/libs/TokenRouter.sol(3 hunks)solidity/test/token/EverclearTokenBridge.t.sol(3 hunks)solidity/test/token/TokenBridgeCctp.t.sol(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
solidity/contracts/token/**/*.sol
📄 CodeRabbit inference engine (CLAUDE.md)
Place token bridge contracts (e.g., HypERC20, HypERC20Collateral) under solidity/contracts/token/
Files:
solidity/contracts/token/TokenBridgeCctpBase.solsolidity/contracts/token/libs/TokenRouter.solsolidity/contracts/token/libs/TokenCollateral.solsolidity/contracts/token/bridge/EverclearTokenBridge.sol
solidity/**/*.sol
📄 CodeRabbit inference engine (CLAUDE.md)
Lint Solidity code with solhint via yarn --cwd solidity lint
Files:
solidity/contracts/token/TokenBridgeCctpBase.solsolidity/contracts/token/libs/TokenRouter.solsolidity/test/token/EverclearTokenBridge.t.solsolidity/contracts/token/libs/TokenCollateral.solsolidity/test/token/TokenBridgeCctp.t.solsolidity/contracts/token/bridge/EverclearTokenBridge.sol
🧠 Learnings (2)
📓 Common learnings
Learnt from: yorhodes
PR: hyperlane-xyz/hyperlane-monorepo#6750
File: solidity/contracts/token/bridge/EverclearTokenBridge.sol:212-212
Timestamp: 2025-09-18T15:49:20.478Z
Learning: EverclearTokenBridge sends funds directly to recipients for ERC20 transfers, while EverclearEthBridge sends funds to remote routers that handle WETH unwrapping and ETH delivery. The virtual _getReceiver and _getIntentCalldata functions enable this architectural difference between the two bridge implementations.
📚 Learning: 2025-09-18T15:49:20.478Z
Learnt from: yorhodes
PR: hyperlane-xyz/hyperlane-monorepo#6750
File: solidity/contracts/token/bridge/EverclearTokenBridge.sol:212-212
Timestamp: 2025-09-18T15:49:20.478Z
Learning: EverclearTokenBridge sends funds directly to recipients for ERC20 transfers, while EverclearEthBridge sends funds to remote routers that handle WETH unwrapping and ETH delivery. The virtual _getReceiver and _getIntentCalldata functions enable this architectural difference between the two bridge implementations.
Applied to files:
solidity/test/token/EverclearTokenBridge.t.solsolidity/contracts/token/bridge/EverclearTokenBridge.sol
🔇 Additional comments (7)
solidity/contracts/token/libs/TokenRouter.sol (1)
252-254: Doc nit: fee denomination note is helpful.Stating external fees must be in
token()saves folks from muddy waters later. All good here.solidity/contracts/token/bridge/EverclearTokenBridge.sol (2)
407-416: ETB overriding_transferFeeis the right move.Direct ERC20 transfer from the router balance ensures the fee doesn’t sink when
_transferTois a no‑op. Fits the Everclear split‑delivery model nicely. Based on learnings.
443-454: Verified: all callsites comply with the new constructor signature.Both the test instantiation (line 977) and MockEverclearEthBridge (line 853) correctly invoke the updated constructor with only 3 parameters (_weth, _mailbox, _everclearAdapter) and do not pass a
_scaleargument. The hardcoding of SCALE=1 in the EverclearEthBridge constructor is sound, and no deploy or test code is trying to override it.solidity/test/token/TokenBridgeCctp.t.sol (1)
341-386: Tests nail the fee path behavior.Capturing pre-balances and asserting fee recipient gets paid, user charged
charge, and bridge holds no fee makes the change airtight. Smells like fresh pine.solidity/contracts/token/TokenBridgeCctpBase.sol (1)
369-378: CCTP fee transfer override is spot on.Paying fees via
wrappedToken.safeTransferaligns with burning onlyamount + externalFee. Matches the updated tests and avoids fee residue on the bridge.solidity/test/token/EverclearTokenBridge.t.sol (2)
31-31: Import scope looks good.Pulling in LinearFee just for tests keeps things tidy. No nits here.
849-854: Constructor signature change verified—no lingering 4‑arg calls remain.The swamp's been searched from top to bottom, and it's clean. No old 4‑arg instantiations of
EverclearEthBridgeorMockEverclearEthBridgeleft lurking about—the_scaleparameter's been properly pruned throughout.
Description
Introduces new
internal _transferFeefor warp routes which have no-op_transferToimplementationsImplements overrides in Everclear and CCTP implementations
Drive-by changes
Backward compatibility
Yes
Testing
Unit Tests (which assert fees are transferred to recipients)
Summary by CodeRabbit
New Features
Documentation
Tests