fix: pay gas for settlement withdraw even when source and target token are the same #43
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThe Router now always invokes the swap module, removing the no-swap branch for same-token cases. SwapAlgebra adds a same-token early exit and direct-pool fast-path, consolidating slippage checks post-swap. MockSwapModule switches to gas-fee-first deduction and adds an overload. Tests are updated/added to cover same-token and gas-fee behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Router
participant SwapModule
participant Pool as DEX/Pool(s)
User->>Router: processIntent(intentInfo, settlementInfo)
Note over Router: Always approve & call swap
Router->>SwapModule: swap(zrc20, targetZRC20, amountWithTip, gasZRC20, gasFee, ...)
alt tokenIn == tokenOut
SwapModule-->>Router: amountWithTipOut (post gas/slippage)
else Direct pool
SwapModule->>Pool: swap tokenIn -> tokenOut
Pool-->>SwapModule: amountOut
SwapModule-->>Router: amountWithTipOut
end
Note over Router: Adjust tipAfterSwap and actualAmount for slippage/fees
Router-->>User: Emit settlement event
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
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
Status, Documentation and Community
|
![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
🧹 Nitpick comments (5)
src/swapModules/SwapAlgebra.sol (1)
244-246: Consider adding event emission for missing intermediary tokenThe require statement correctly validates the intermediary token, but consider emitting an event before reverting to aid in debugging and monitoring when this error occurs in production.
// Get the intermediary token for this token name address intermediaryToken = intermediaryTokens[tokenName]; +if (intermediaryToken == address(0)) { + emit IntermediaryTokenMissing(tokenName, tokenIn, tokenOut); +} require(intermediaryToken != address(0), "No intermediary token set for this token name");Add the event definition near line 49:
event IntermediaryTokenMissing(string indexed tokenName, address indexed tokenIn, address indexed tokenOut);test/Router.t.sol (1)
1339-1340: Consider extracting magic numbers into named constantsThe minted amount calculation
amount + tip - gasFeecould be clearer with a named constant or variable to improve test readability.+// Calculate expected amount after gas fee for same-token case +uint256 expectedSwapModuleAmount = amount + tip - gasFee; // Input token also goes to swap module for the main swap (same token case) -inputToken.mint(address(swapModule), amount + tip - gasFee); +inputToken.mint(address(swapModule), expectedSwapModuleAmount);test/swapModules/SwapAlgebra.t.sol (1)
589-619: Consider adding assertion for zero pool callsWhile the comment mentions "no pool interaction," consider adding an explicit assertion to verify that the mock pool's swap function was not called.
Add a call counter to the MockAlgebraPool contract and verify it remains zero:
// Verify that no actual swap was performed (no pool interaction) // The mock pool should not have been called since it's the same token +MockAlgebraPool pool = MockAlgebraPool(mockAlgebraFactory.poolByPair(address(inputToken), address(inputToken))); +assertEq(pool.swapCallCount(), 0, "Pool swap should not be called for same token");src/Router.sol (1)
389-390: Consider addressing the surplus handling TODOThe comment indicates that surplus amounts are currently ignored. While acknowledged as a future improvement, consider adding a simple event emission to track when surpluses occur for monitoring purposes.
// Note: Surplus amounts are currently ignored as they are too small to handle // This will be addressed in future updates +if (settlementInfo.amountWithTipOut > wantedAmountWithTip) { + emit SwapSurplusDetected( + intentInfo.zrc20, + settlementInfo.targetZRC20, + settlementInfo.amountWithTipOut - wantedAmountWithTip + ); +}Add the event definition near line 105:
event SwapSurplusDetected(address indexed tokenIn, address indexed tokenOut, uint256 surplusAmount);test/mocks/MockSwapModule.sol (1)
78-126: Consider extracting duplicated swap logicThe swap implementation is duplicated between the two function overloads. Consider extracting the common logic to a private function to improve maintainability.
+function _performSwap( + address tokenIn, + address tokenOut, + uint256 amountIn, + address gasZRC20, + uint256 gasFee +) private returns (uint256 amountOut) { + // Transfer tokens from sender to this contract + IERC20(tokenIn).transferFrom(msg.sender, address(this), amountIn); + + // Handle gas fee first (similar to SwapAlgebra) + uint256 amountInForMainSwap = amountIn; + + if (gasFee > 0 && gasZRC20 != address(0)) { + // If tokenIn is not the gas token, we need to account for gas fee + if (tokenIn != gasZRC20) { + // In mock, we just reduce the main swap amount by gas fee + amountInForMainSwap = amountIn - gasFee; + } else { + // If tokenIn is already the gas token, just set aside the needed amount + amountInForMainSwap = amountIn - gasFee; + } + + // Transfer gas fee tokens back to the sender + IERC20(gasZRC20).transfer(msg.sender, gasFee); + } + + // Handle main swap + if (amountInForMainSwap > 0) { + // Check if input and output tokens are the same + if (tokenIn == tokenOut) { + // No swap needed, just return the remaining amount + amountOut = amountInForMainSwap; + } else { + // Calculate amount out based on slippage settings + uint256 slippageCost = (amountInForMainSwap * slippage) / 10000; + amountOut = amountInForMainSwap - slippageCost; + } + + // Transfer tokens back to the sender + IERC20(tokenOut).transfer(msg.sender, amountOut); + } else { + amountOut = 0; + } + + // Allow to set a custom amount out for testing + if (customAmountOut > 0) { + amountOut = customAmountOut; + } + + return amountOut; +} + function swap(address tokenIn, address tokenOut, uint256 amountIn, address gasZRC20, uint256 gasFee) external override returns (uint256 amountOut) { - // [Current implementation] + return _performSwap(tokenIn, tokenOut, amountIn, gasZRC20, gasFee); } function swap(address tokenIn, address tokenOut, uint256 amountIn, address gasZRC20, uint256 gasFee, string memory) external returns (uint256 amountOut) { - // [Current implementation] + return _performSwap(tokenIn, tokenOut, amountIn, gasZRC20, gasFee); }
📜 Review details
Configuration used: .coderabbit.yaml
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 (5)
src/Router.sol(1 hunks)src/swapModules/SwapAlgebra.sol(1 hunks)test/Router.t.sol(4 hunks)test/mocks/MockSwapModule.sol(2 hunks)test/swapModules/SwapAlgebra.t.sol(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.sol
⚙️ CodeRabbit Configuration File
Review the Solidity smart contract code, focusing on security vulnerabilities, gas optimization, best practices, and potential reentrancy issues. Check for proper access controls, input validation, and adherence to established patterns like CEI (Checks-Effects-Interactions).
Files:
test/swapModules/SwapAlgebra.t.solsrc/Router.soltest/mocks/MockSwapModule.soltest/Router.t.solsrc/swapModules/SwapAlgebra.sol
**/test/**/*.sol
⚙️ CodeRabbit Configuration File
Review the Solidity test files, ensuring comprehensive test coverage, proper edge case handling, and correct use of Foundry testing patterns. Check for proper setup, teardown, and assertion patterns.
Files:
test/swapModules/SwapAlgebra.t.soltest/mocks/MockSwapModule.soltest/Router.t.sol
🔇 Additional comments (8)
src/swapModules/SwapAlgebra.sol (2)
262-277: Good refactoring of balance finalization logicThe consolidation of slippage checks and balance calculations after all swap paths is a clean architectural improvement. This ensures consistent handling regardless of whether a direct pool or intermediary path is taken.
230-235: Potential reentrancy vulnerability in same-token transfer pathWhen
tokenIn == tokenOut, the contract transfers tokens directly back to the sender without following the checks-effects-interactions pattern. TheamountOutstate is set after the external transfer call, which could allow reentrancy attacks if the token is malicious or has hooks.Apply this diff to follow the checks-effects-interactions pattern:
// Check if input and output tokens are the same if (tokenIn == tokenOut) { // No swap needed, just transfer the remaining amount back to sender - amountOut = amountInForMainSwap; - IERC20(tokenOut).safeTransfer(msg.sender, amountOut); + uint256 transferAmount = amountInForMainSwap; + amountOut = transferAmount; + IERC20(tokenOut).safeTransfer(msg.sender, transferAmount);Additionally, consider adding a reentrancy guard to the entire
swapfunction for defense in depth.Likely an incorrect or invalid review comment.
test/Router.t.sol (1)
1298-1375: Well-structured test for same-token gas fee handlingThe test correctly validates the gas fee deduction behavior when source and target tokens are identical. The event assertions and balance checks properly verify that gas fees are deducted from the tip as expected.
test/swapModules/SwapAlgebra.t.sol (2)
557-587: Comprehensive test coverage for same-token swapsExcellent addition of test cases for same-token swap scenarios. The tests properly verify that no pool interaction occurs and that gas fees are correctly handled.
519-519: Good fix for operator precedence clarityAdding explicit parentheses improves code clarity and ensures the multiplication is performed before division, preventing potential precision loss.
src/Router.sol (2)
372-385: Clean implementation of unconditional swap routingThe removal of the conditional branch and always routing through the swap module simplifies the code flow and ensures consistent gas fee handling. The double approval pattern (first to 0, then to amount) is a good practice to handle tokens with non-standard approval behaviors.
387-408: Well-implemented slippage and fee cost handlingThe logic correctly handles both scenarios where the tip covers costs and where it doesn't. The require statement provides appropriate protection against insufficient funds.
test/mocks/MockSwapModule.sol (1)
32-47: Good simulation of gas fee handlingThe mock correctly simulates the gas fee deduction logic, making it suitable for testing the Router's behavior with different token scenarios.
Closes #42
The check source == target is done in swap module instead of the router contract
Summary by CodeRabbit