Implement balance sweeping in pullAmountAndExecute and pullAndExecute functions#73
Implement balance sweeping in pullAmountAndExecute and pullAndExecute functions#73shunkakinoki merged 4 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the pullAmountAndExecute and pullAndExecute functions to automatically sweep any remaining balance back to users after multicall execution. The implementation adds balance checking and transfer logic immediately after the multicall completes, ensuring the router remains stateless and users recover unspent funds.
Key Changes:
- Added automatic balance sweeping in
pullAmountAndExecuteafter multicall execution - Sweep applies to both ERC20 tokens and native ETH
- Emits
Sweepevent when funds are returned to users
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
src/TrailsRouter.sol |
Implements sweep logic in pullAmountAndExecute to return remaining balance to msg.sender after multicall execution |
test/TrailsRouter.t.sol |
Updates test assertions to expect router balance of 0 and users receiving full token/ETH amounts back after execution |
.gas-snapshot |
Records gas cost increases (~10-35k per affected test) due to added sweep operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Router should have no remaining balance (swept back to user) | ||
| assertEq(mockToken.balanceOf(address(router)), 0); | ||
| // User gets their tokens back since multicall didn't consume them | ||
| assertEq(mockToken.balanceOf(user), 1000e18); |
There was a problem hiding this comment.
[nitpick] Inconsistent terminology in test comments: The comments describing the sweep behavior use inconsistent terminology across similar test cases:
- Lines 280, 762: "swept back to user"
- Lines 319, 729, 800: "dust refunded back to user"
Additionally, the term "dust" is misleading since the entire remaining balance is returned, not just dust/residual amounts. For consistency with the implementation (which emits a Sweep event), recommend using "swept back to user" consistently throughout all these tests.
src/TrailsRouter.sol
Outdated
| // Sweep any remaining balance back to msg.sender | ||
| uint256 remaining = _getSelfBalance(token); | ||
| if (remaining > 0) { | ||
| if (token == address(0)) { | ||
| _transferNative(msg.sender, remaining); | ||
| } else { | ||
| _transferERC20(token, msg.sender, remaining); | ||
| } | ||
| emit Sweep(token, msg.sender, remaining); | ||
| } |
There was a problem hiding this comment.
Sweeps entire balance including pre-existing funds: The sweep returns the entire router balance to msg.sender, not just the amount pulled in this transaction. If the router has any pre-existing balance (from accidental transfers, failed transactions, or excess msg.value), the caller will receive those funds as well.
For example:
- Router has 50 tokens from a previous transaction
- User calls
pullAmountAndExecute(token, 100, data)where multicall doesn't consume tokens - User receives 150 tokens (100 they pulled + 50 pre-existing)
If this is intentional (to incentivize cleaning up stuck funds and ensure the router remains stateless), consider documenting this behavior clearly. If not, consider tracking the initial balance before pulling and only sweeping the delta from the current transaction.
| if (token == address(0)) { | ||
| _transferNative(msg.sender, remaining); |
There was a problem hiding this comment.
Sweep failure causes entire transaction to revert: If the native transfer fails (e.g., msg.sender is a contract without receive()/fallback(), or gas exhaustion), the entire transaction reverts, even though the multicall executed successfully. This means:
- Multicall results/effects may be lost
- User cannot complete their transaction without being able to receive the sweep
Previously, the function would succeed and leave funds in the router. Now it fails completely if the sweep cannot be delivered.
Consider one of these approaches:
- Document that callers must be able to receive ETH transfers
- Add a try-catch pattern (requires making transfer logic external)
- Provide a separate
claimSweep()function for users to pull funds later if initial sweep fails - Skip the sweep if it would fail (check if recipient can receive before transferring)
|
|
||
| assertEq(mockToken.balanceOf(address(router)), userBalance); | ||
| assertEq(mockToken.balanceOf(user), 0); | ||
| // Router should have no remaining balance (dust refunded back to user) |
There was a problem hiding this comment.
[nitpick] Inconsistent terminology: Uses "dust refunded" instead of "swept back" like similar tests. For consistency with the implementation's Sweep event and other test comments (lines 280, 762), recommend using "swept back to user" consistently.
| // Router should have no remaining balance (dust refunded back to user) | |
| // Router should have no remaining balance (dust swept back to user) |
|
|
||
| assertEq(address(router).balance, ethAmount); | ||
| assertEq(user.balance, 0); | ||
| // Router should have no remaining balance (dust refunded back to user) |
There was a problem hiding this comment.
[nitpick] Inconsistent terminology: Uses "dust refunded" instead of "swept back" like similar tests. For consistency with the implementation's Sweep event and other test comments (lines 280, 762), recommend using "swept back to user" consistently.
| // Router should have no remaining balance (dust refunded back to user) | |
| // Router should have no remaining balance (dust swept back to user) |
|
|
||
| assertEq(mockToken.balanceOf(address(router)), transferAmount); | ||
| assertEq(mockToken.balanceOf(user), 1000e18 - transferAmount); | ||
| // Router should have no remaining balance (dust refunded back to user) |
There was a problem hiding this comment.
[nitpick] Inconsistent terminology: Uses "dust refunded" instead of "swept back" like the similar test at line 280 (same test scenario). For consistency with the implementation's Sweep event and other test comments, recommend using "swept back to user" consistently.
| // Router should have no remaining balance (dust refunded back to user) | |
| // Router should have no remaining balance (dust swept back to user) |
| if (token == address(0)) { | ||
| _transferNative(msg.sender, remaining); | ||
| } else { | ||
| _transferERC20(token, msg.sender, remaining); | ||
| } | ||
| emit Sweep(token, msg.sender, remaining); |
There was a problem hiding this comment.
Event emitted after external call: The Sweep event is emitted after the external call to _transferNative (which does .call{value:}) or _transferERC20. While not exploitable in this specific case, this violates the Checks-Effects-Interactions pattern where effects (like event emissions) should occur before interactions (external calls).
Consider moving the event emission before the transfer:
if (remaining > 0) {
emit Sweep(token, msg.sender, remaining);
if (token == address(0)) {
_transferNative(msg.sender, remaining);
} else {
_transferERC20(token, msg.sender, remaining);
}
}| if (token == address(0)) { | |
| _transferNative(msg.sender, remaining); | |
| } else { | |
| _transferERC20(token, msg.sender, remaining); | |
| } | |
| emit Sweep(token, msg.sender, remaining); | |
| emit Sweep(token, msg.sender, remaining); | |
| if (token == address(0)) { | |
| _transferNative(msg.sender, remaining); | |
| } else { | |
| _transferERC20(token, msg.sender, remaining); | |
| } |
Enhance the pullAmountAndExecute and pullAndExecute functions to automatically sweep any remaining balance back to the user after executing a multicall. This ensures that users receive any unspent tokens or ETH.