Refund extra tokens, reset approvals #13
Conversation
WalkthroughThe bridge contract now imports ERC20, expands TransactionInitiated to include developerFee and protocolFee, and refines initiateTransaction for both native and ERC20 paths with explicit pre/post balance tracking and refund handling. _distributeFees returns (totalFeeAmount, protocolFee). Tests add a MockRefundTarget and new refund-focused cases and event expectations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Bridge as UniversalBridgeV1
participant Target as Forward/Refund Target
participant Dev as Dev Fee Recipient
participant Proto as Protocol Fee Recipient
rect rgb(240,248,255)
note over User,Bridge: Native token path
User->>Bridge: initiateTransaction(value: ETH, data)
Bridge->>Bridge: _distributeFees() -> totalFee, protocolFee
Bridge->>Dev: Transfer developerFee (ETH)
Bridge->>Proto: Transfer protocolFee (ETH)
Bridge->>Target: call{value: sendValue}(data)
alt Target refunds ETH
Target-->>Bridge: refundAmount (ETH)
end
Bridge-->>User: refund leftover ETH (if any)
Bridge-->>User: emit TransactionInitiated(..., developerFee, protocolFee, ...)
end
sequenceDiagram
autonumber
actor User
participant Bridge as UniversalBridgeV1
participant Token as ERC20
participant Target as Forward/Refund Target
participant Dev as Dev Fee Recipient
participant Proto as Protocol Fee Recipient
rect rgb(245,255,245)
note over User,Bridge: ERC20 path
User->>Bridge: initiateTransaction(token, amount, data, value: ETH?)
Bridge->>Bridge: _distributeFees() -> totalFee, protocolFee
Bridge->>Dev: Transfer developerFee (token or ETH path-specific)
Bridge->>Proto: Transfer protocolFee
Bridge->>Token: transferFrom(User, Bridge, tokenAmount)
Bridge->>Token: approve(Target, tokenAmount)
Bridge->>Target: call{value: msg.value}(data)
Target-->>Bridge: refundAmount (token, optional)
Bridge->>Token: approve(Target, 0)
Bridge-->>User: refund leftover tokens and ETH (if any)
Bridge-->>User: emit TransactionInitiated(..., developerFee, protocolFee, ...)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
src/UniversalBridgeV1.sol (1)
206-213: Forward exactly req.tokenAmount for native tokens; refund any excess msg.value.Current logic forwards
msg.value - totalFeeAmount. If the caller overpays, the surplus also gets forwarded toforwardAddress, which may unintentionally capture funds. Safer: forward exactlyreq.tokenAmountand refund the rest in the post-call refund block you already have.- uint256 sendValue = msg.value - totalFeeAmount; - - if (sendValue < req.tokenAmount) { - revert UniversalBridgeMismatchedValue(req.tokenAmount, sendValue); - } - _call(req.forwardAddress, sendValue, req.callData); // calldata empty for direct transfer + uint256 maxSendValue = msg.value - totalFeeAmount; + if (maxSendValue < req.tokenAmount) { + revert UniversalBridgeMismatchedValue(req.tokenAmount, maxSendValue); + } + // Forward exactly the requested tokenAmount; any surplus is refunded below. + _call(req.forwardAddress, req.tokenAmount, req.callData); // calldata empty for direct transfer
🧹 Nitpick comments (8)
test/utils/MockRefundTarget.sol (2)
16-24: Prefer SafeTransferLib over direct ERC20 calls in tests, too.Directly calling
ERC20(token).transfer/transferFromwill fail with non-standard tokens (USDT-style, no return bool). Even in tests, using SafeTransferLib mirrors prod behavior and avoids brittle assumptions.Apply:
-import "lib/solady/src/tokens/ERC20.sol"; +import "lib/solady/src/utils/SafeTransferLib.sol"; @@ - require(ERC20(tokenAddress).transferFrom(msg.sender, address(this), tokenAmount), "Token transfer failed"); + SafeTransferLib.safeTransferFrom(tokenAddress, msg.sender, address(this), tokenAmount); @@ - require(ERC20(tokenAddress).transfer(receiver, netAmount), "Transfer to receiver failed"); + SafeTransferLib.safeTransfer(tokenAddress, receiver, netAmount); @@ - require(ERC20(tokenAddress).transfer(msg.sender, refundAmount), "Refund transfer failed"); + SafeTransferLib.safeTransfer(tokenAddress, msg.sender, refundAmount);Also applies to: 27-38
30-37: Validate refundAmount ≤ tokenAmount to avoid accidental underflow.If
refundAmount > tokenAmount, subtraction reverts with an arithmetic error. Make intent explicit and fail fast with a clear reason.- uint256 netAmount = tokenAmount - refundAmount; + require(refundAmount <= tokenAmount, "invalid refund"); + uint256 netAmount = tokenAmount - refundAmount;Apply to both ERC20 and native branches.
Also applies to: 51-60
test/UniversalBridgeV1.t.sol (3)
84-85: Strengthen refund-path assertions.You already validate balances. Consider also:
- Asserting the mock target’s
RefundLogemission (topics/data) to ensure the intermediate accounting was correct.- Verifying ERC20 allowance reset back to 0 after the spender call.
Example snippets:
@@ - bridge.initiateTransaction{ value: sendValueWithFees }(req, _signature); + bridge.initiateTransaction{ value: sendValueWithFees }(req, _signature); + // Optional: assert refund event from mock target (topics simplified) + // vm.expectEmit(false, false, false, true, address(mockRefundTarget)); + // emit MockRefundTarget.RefundLog(sender, receiver, NATIVE_TOKEN, sendValue, refundAmount, "native refund test"); @@ - bridge.initiateTransaction(req, _signature); + bridge.initiateTransaction(req, _signature); + // Optional: allowance must be cleared + assertEq(mockERC20.allowance(address(bridge), address(mockRefundTarget)), 0, "allowance not reset");If helpful, I can push exact
vm.expectEmitcalls wired to the mock’s ABI.Also applies to: 116-125, 623-671, 672-724
487-517: Overpay path is untested. Consider explicit overpayment behavior.Right now only the “underpay” case reverts. Please add a test that overpays
msg.valueand confirms whether the surplus is (a) forwarded or (b) refunded. This protects UX and documents intended semantics.I can add a test once we settle on the contract behavior (see contract comment below).
161-203: Minor: redundant comment in ERC20 refund test.The note “including refund that will come back” is misleading—approval need only cover
tokenAmount + fees; the refund is pulled from the bridge after it receives tokens and grants allowance. No action needed; just mentioning to avoid confusion.Also applies to: 422-464
src/UniversalBridgeV1.sol (3)
318-350: Cap developerFeeBps and validate input locally.We cap protocol fee bps, but not developer fee bps. Extremely large dev bps will revert indirectly; better to fail early with a clear error and shared semantics.
function _distributeFees( @@ - ) private returns (uint256, uint256) { + ) private returns (uint256, uint256) { + // Align with BPS semantics (max 100%) + if (developerFeeBps > 10_000) { + revert UniversalBridgeInvalidFeeBps(); + } @@ uint256 protocolFee = (tokenAmount * protocolFeeBps) / 10_000; uint256 developerFee = (tokenAmount * developerFeeBps) / 10_000; uint256 totalFeeAmount = protocolFee + developerFee;Optional: also enforce
totalFeeAmount <= tokenAmountto preempt surprising UX on native flows.
139-149: Operational observability: consider Pause/Unpause and Restrict/Unrestrict events.Emitting events on
pauseandrestrictAddresschanges eases monitoring and incident response.I can add minimal events (
Paused(bool),AddressRestricted(address,bool)) if you want.Also applies to: 143-145
165-173: Future-proofing: include fee guards in the signed payload.Protocol fee bps and dev fee bps can change between signing and execution, causing unexpected reverts or outcomes. Consider adding a “maxTotalFeeBps” or explicit fee bps to the EIP-712 struct to freeze expectations per request.
Also applies to: 281-316
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/UniversalBridgeV1.sol(7 hunks)test/UniversalBridgeV1.t.sol(7 hunks)test/utils/MockRefundTarget.sol(1 hunks)
🔇 Additional comments (6)
test/utils/MockRefundTarget.sol (1)
63-81: Nice: flexible fallback-driven test target.The
fallbackthat decodes inputs and routes by sentinel NATIVE address keeps the mock compact and expressive for multiple scenarios. Good test ergonomics.test/UniversalBridgeV1.t.sol (1)
15-27: Event shape updates look correct and are asserted.Including
developerFeeandprotocolFeein the emitted event and asserting their values increases observability of fee accounting. LGTM.Also applies to: 449-463
src/UniversalBridgeV1.sol (4)
196-205: Good: ETH pre-balance snapshot that excludes current msg.value.
contractEthBalanceBefore = address(this).balance - msg.value;is the right baseline for later refund calculations. Nice attention to edge cases.
219-239: ERC20 path looks sound: pull, bounded approve, call, zero-out approve, refund deltas.Nice, minimal approvals and post-call refunds cover the primary footguns for spenders and non-spenders. No changes requested.
241-247: ETH refund block is correct and reentrancy-safe.Comparing after/before balances using the pre-captured baseline ensures only net inflows are refunded. With
nonReentrant, external refund calls are safe. Good.
249-259: Event accounting is accurate.Emitting
developerFee = totalFeeAmount - protocolFeeandprotocolFeeseparately is consistent and testable. LGTM.
| (uint256 totalFeeAmount, uint256 protocolFee) = _distributeFees( | ||
| req.tokenAddress, | ||
| req.tokenAmount, | ||
| req.developerFeeRecipient, | ||
| req.developerFeeBps | ||
| ); | ||
|
|
There was a problem hiding this comment.
Enforce non-zero developerFeeRecipient when developerFeeBps > 0.
If developerFeeBps > 0 and the recipient is zero, native fees would be burned (ETH to address(0) succeeds), and some ERC20s may revert. Add an explicit guard.
@@ function initiateTransaction
- (uint256 totalFeeAmount, uint256 protocolFee) = _distributeFees(
+ if (req.developerFeeBps != 0 && req.developerFeeRecipient == address(0)) {
+ revert UniversalBridgeZeroAddress();
+ }
+ (uint256 totalFeeAmount, uint256 protocolFee) = _distributeFees(
req.tokenAddress,
req.tokenAmount,
req.developerFeeRecipient,
req.developerFeeBps
);Also applies to: 318-350
🤖 Prompt for AI Agents
In src/UniversalBridgeV1.sol around lines 199-205 (and likewise apply the same
fix to the block around lines 318-350), add an explicit require that when
req.developerFeeBps > 0 then req.developerFeeRecipient != address(0) before
calling _distributeFees; update the revert message to be descriptive (e.g.,
"developer fee set but recipient is zero address") so the function fails early
and avoids burning native ETH or triggering ERC20 reverts.
|
|
||
| // distribute fees | ||
| uint256 totalFeeAmount = _distributeFees( | ||
| (uint256 totalFeeAmount, uint256 protocolFee) = _distributeFees( |
There was a problem hiding this comment.
_distributeFees should return developerFee as well, so that we can use it for event
Summary by CodeRabbit
New Features
Tests