From eeaf5cda6c14b0059cd361e38e336176f53ac722 Mon Sep 17 00:00:00 2001 From: lumtis Date: Tue, 19 Aug 2025 17:11:46 +0200 Subject: [PATCH] fix: pay gas for settlement withdraw when source and target token are the same --- src/Router.sol | 76 +++++++++++------------- src/swapModules/SwapAlgebra.sol | 67 +++++++++++---------- test/Router.t.sol | 32 +++------- test/mocks/MockSwapModule.sol | 94 +++++++++++++++++++----------- test/swapModules/SwapAlgebra.t.sol | 66 ++++++++++++++++++++- 5 files changed, 205 insertions(+), 130 deletions(-) diff --git a/src/Router.sol b/src/Router.sol index 7db5119..64d7d3e 100644 --- a/src/Router.sol +++ b/src/Router.sol @@ -369,48 +369,42 @@ contract Router is settlementInfo.actualAmount = settlementInfo.wantedAmount; - // Check if source and target ZRC20 are the same - if (intentInfo.zrc20 == settlementInfo.targetZRC20) { - // No swap needed, use original amounts - settlementInfo.amountWithTipOut = intentInfo.amountWithTip; - settlementInfo.tipAfterSwap = wantedTip; - } else { - // Approve swap module to spend tokens - IERC20(intentInfo.zrc20).approve(swapModule, 0); - IERC20(intentInfo.zrc20).approve(swapModule, intentInfo.amountWithTip); - - // Perform swap through swap module - settlementInfo.amountWithTipOut = ISwap(swapModule).swap( - intentInfo.zrc20, - settlementInfo.targetZRC20, - intentInfo.amountWithTip, - settlementInfo.gasZRC20, - settlementInfo.gasFee, - zrc20ToTokenName[intentInfo.zrc20] - ); + // Always call swap module to handle gas fees and potential swaps + // Approve swap module to spend tokens + IERC20(intentInfo.zrc20).approve(swapModule, 0); + IERC20(intentInfo.zrc20).approve(swapModule, intentInfo.amountWithTip); + + // Perform swap through swap module + settlementInfo.amountWithTipOut = ISwap(swapModule).swap( + intentInfo.zrc20, + settlementInfo.targetZRC20, + intentInfo.amountWithTip, + settlementInfo.gasZRC20, + settlementInfo.gasFee, + zrc20ToTokenName[intentInfo.zrc20] + ); - // Calculate slippage difference and adjust tip accordingly - // If swap returns more than expected (surplus), slippageAndFeeCost will be 0 - // Note: Surplus amounts are currently ignored as they are too small to handle - // This will be addressed in future updates - uint256 slippageAndFeeCost = wantedAmountWithTip > settlementInfo.amountWithTipOut - ? wantedAmountWithTip - settlementInfo.amountWithTipOut - : 0; - - // Check if tip covers the slippage and fee costs - if (wantedTip > slippageAndFeeCost) { - // Tip covers all costs, subtract from tip only - settlementInfo.tipAfterSwap = wantedTip - slippageAndFeeCost; - } else { - // Tip doesn't cover costs, use it all and reduce the amount - settlementInfo.tipAfterSwap = 0; - // Calculate how much remaining slippage to cover from the amount - uint256 remainingCost = slippageAndFeeCost - wantedTip; - // Ensure the amount is greater than the remaining cost, otherwise fail - require(settlementInfo.wantedAmount > remainingCost, "Amount insufficient to cover costs after tip"); - // Reduce the actual amount by the remaining cost - settlementInfo.actualAmount = settlementInfo.wantedAmount - remainingCost; - } + // Calculate slippage difference and adjust tip accordingly + // If swap returns more than expected (surplus), slippageAndFeeCost will be 0 + // Note: Surplus amounts are currently ignored as they are too small to handle + // This will be addressed in future updates + uint256 slippageAndFeeCost = wantedAmountWithTip > settlementInfo.amountWithTipOut + ? wantedAmountWithTip - settlementInfo.amountWithTipOut + : 0; + + // Check if tip covers the slippage and fee costs + if (wantedTip > slippageAndFeeCost) { + // Tip covers all costs, subtract from tip only + settlementInfo.tipAfterSwap = wantedTip - slippageAndFeeCost; + } else { + // Tip doesn't cover costs, use it all and reduce the amount + settlementInfo.tipAfterSwap = 0; + // Calculate how much remaining slippage to cover from the amount + uint256 remainingCost = slippageAndFeeCost - wantedTip; + // Ensure the amount is greater than the remaining cost, otherwise fail + require(settlementInfo.wantedAmount > remainingCost, "Amount insufficient to cover costs after tip"); + // Reduce the actual amount by the remaining cost + settlementInfo.actualAmount = settlementInfo.wantedAmount - remainingCost; } return settlementInfo; diff --git a/src/swapModules/SwapAlgebra.sol b/src/swapModules/SwapAlgebra.sol index e262cbc..7bd3bc3 100644 --- a/src/swapModules/SwapAlgebra.sol +++ b/src/swapModules/SwapAlgebra.sol @@ -227,47 +227,54 @@ contract SwapAlgebra is ISwap, Ownable { // Now perform the main swap using Algebra pool for the remaining amount if (amountInForMainSwap > 0) { - // Check if a direct pool exists - bool directPoolExists = algebraPoolExists(tokenIn, tokenOut); - - if (directPoolExists) { - // Swap directly using the Algebra pool - amountOut = swapThroughAlgebraPool(tokenIn, tokenOut, amountInForMainSwap); + // 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); } else { - // Get the intermediary token for this token name - address intermediaryToken = intermediaryTokens[tokenName]; - require(intermediaryToken != address(0), "No intermediary token set for this token name"); + // Check if a direct pool exists + bool directPoolExists = algebraPoolExists(tokenIn, tokenOut); - // Check if both pools exist - bool poolInToInterExists = algebraPoolExists(tokenIn, intermediaryToken); - bool poolInterToOutExists = algebraPoolExists(intermediaryToken, tokenOut); + if (directPoolExists) { + // Swap directly using the Algebra pool + amountOut = swapThroughAlgebraPool(tokenIn, tokenOut, amountInForMainSwap); + } else { + // Get the intermediary token for this token name + address intermediaryToken = intermediaryTokens[tokenName]; + require(intermediaryToken != address(0), "No intermediary token set for this token name"); - require(poolInToInterExists && poolInterToOutExists, "Required Algebra pools do not exist"); + // Check if both pools exist + bool poolInToInterExists = algebraPoolExists(tokenIn, intermediaryToken); + bool poolInterToOutExists = algebraPoolExists(intermediaryToken, tokenOut); - // First hop: tokenIn -> intermediaryToken - uint256 intermediaryAmount = swapThroughAlgebraPool(tokenIn, intermediaryToken, amountInForMainSwap); + require(poolInToInterExists && poolInterToOutExists, "Required Algebra pools do not exist"); - // Second hop: intermediaryToken -> tokenOut - amountOut = swapThroughAlgebraPool(intermediaryToken, tokenOut, intermediaryAmount); + // First hop: tokenIn -> intermediaryToken + uint256 intermediaryAmount = swapThroughAlgebraPool(tokenIn, intermediaryToken, amountInForMainSwap); - emit SwapWithIntermediary(tokenIn, intermediaryToken, tokenOut, amountInForMainSwap, amountOut); - } + // Second hop: intermediaryToken -> tokenOut + amountOut = swapThroughAlgebraPool(intermediaryToken, tokenOut, intermediaryAmount); - // Calculate actual balance change to account for any existing balance - uint256 finalOutBalance = IERC20(tokenOut).balanceOf(address(this)); - uint256 actualAmountOut = finalOutBalance - initialOutBalance; + emit SwapWithIntermediary(tokenIn, intermediaryToken, tokenOut, amountInForMainSwap, amountOut); + } + + // Calculate actual balance change to account for any existing balance + uint256 finalOutBalance = IERC20(tokenOut).balanceOf(address(this)); + uint256 actualAmountOut = finalOutBalance - initialOutBalance; - // Apply slippage check using constant MAX_SLIPPAGE (1%) - uint256 minRequiredAmount = calculateMinAmountOutWithSlippage(amountOut); + // Apply slippage check using constant MAX_SLIPPAGE (1%) + uint256 minRequiredAmount = calculateMinAmountOutWithSlippage(amountOut); - // Verify we received at least the minimum amount expected - require(actualAmountOut >= minRequiredAmount, "Slippage tolerance exceeded"); + // Verify we received at least the minimum amount expected + require(actualAmountOut >= minRequiredAmount, "Slippage tolerance exceeded"); - // Transfer output tokens to sender - IERC20(tokenOut).safeTransfer(msg.sender, actualAmountOut); + // Transfer output tokens to sender + IERC20(tokenOut).safeTransfer(msg.sender, actualAmountOut); - // Update the return value to match what was actually received and transferred - amountOut = actualAmountOut; + // Update the return value to match what was actually received and transferred + amountOut = actualAmountOut; + } } return amountOut; diff --git a/test/Router.t.sol b/test/Router.t.sol index f80e586..d277bcd 100644 --- a/test/Router.t.sol +++ b/test/Router.t.sol @@ -1295,7 +1295,7 @@ contract RouterTest is Test { // No need for additional assertions as the mock would fail if the wrong gas limit was used } - function test_OnCall_SameTokenNoSwap() public { + function test_OnCall_SameTokenWithGasFee() public { // Setup intent contract uint256 sourceChainId = 1; uint256 targetChainId = 2; @@ -1336,9 +1336,8 @@ contract RouterTest is Test { inputToken.mint(address(router), amount + tip); // Gas token goes to the swap module for gas fee gasZRC20.mint(address(swapModule), gasFee); - - // Setup spy on swap module to verify it's not called - vm.record(); + // Input token also goes to swap module for the main swap (same token case) + inputToken.mint(address(swapModule), amount + tip - gasFee); // Setup context IGateway.ZetaChainMessageContext memory context = IGateway.ZetaChainMessageContext({ @@ -1355,30 +1354,13 @@ contract RouterTest is Test { targetChainId, address(inputToken), amount + tip, - tip // Tip should remain unchanged since no swap/slippage + tip - gasFee // Tip should be reduced by gas fee since gas fee is deducted from tip ); // Call onCall vm.prank(address(gateway)); router.onCall(context, address(inputToken), amount + tip, intentPayloadBytes); - // Verify the swap function was not called - (bytes32[] memory reads, bytes32[] memory writes) = vm.accesses(address(swapModule)); - - // Verify no swap was performed - the swap function should not be called - bool swapCalled = false; - bytes4 swapSelector = bytes4(keccak256("swap(address,address,uint256,address,uint256,string)")); - - for (uint256 i = 0; i < reads.length; i++) { - // Check if any read access matches the swap selector storage slot - if (uint256(reads[i]) < 4 && bytes4(uint32(uint256(reads[i]))) == swapSelector) { - swapCalled = true; - break; - } - } - - assertFalse(swapCalled, "Swap function should not be called when source and target ZRC20s are the same"); - // Verify approvals were made to the gateway assertTrue( inputToken.allowance(address(router), address(gateway)) > 0, "Router should approve input token to gateway" @@ -1387,9 +1369,9 @@ contract RouterTest is Test { gasZRC20.allowance(address(router), address(gateway)) > 0, "Router should approve gas ZRC20 to gateway" ); - // Verify actual amount equals wanted amount (no reduction) - // This is implicit in the event emission check above but would be - // even better with an explicit check of the settlement payload + // Verify that the swap function was called (for gas fee handling) + // The swap module should have received the input tokens and handled gas fees + // even though the main swap was skipped due to same token } function test_OnCall_SwapSurplusHandling() public { diff --git a/test/mocks/MockSwapModule.sol b/test/mocks/MockSwapModule.sol index c22b8a8..e443e6a 100644 --- a/test/mocks/MockSwapModule.sol +++ b/test/mocks/MockSwapModule.sol @@ -29,25 +29,39 @@ contract MockSwapModule is ISwap { // Transfer tokens from sender to this contract IERC20(tokenIn).transferFrom(msg.sender, address(this), amountIn); - // Calculate amount out based on slippage settings - uint256 slippageCost = (amountIn * slippage) / 10000; // slippage in basis points (e.g., 100 = 1%) - - // Only account for gas fee if gasZRC20 is provided - uint256 totalCost = slippageCost; - if (gasZRC20 != address(0)) { - totalCost += gasFee; + // 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); } - require(amountIn > totalCost, "Amount insufficient to cover costs after tip"); - - amountOut = amountIn - totalCost; - - // Transfer tokens back to the sender - IERC20(tokenOut).transfer(msg.sender, amountOut); - - // Only transfer gas fee if gasZRC20 is not zero address - if (gasZRC20 != address(0) && gasFee > 0) { - 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; // slippage in basis points (e.g., 100 = 1%) + 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 @@ -68,27 +82,41 @@ contract MockSwapModule is ISwap { // Transfer tokens from sender to this contract IERC20(tokenIn).transferFrom(msg.sender, address(this), amountIn); - // Calculate amount out based on slippage settings - uint256 slippageCost = (amountIn * slippage) / 10000; // slippage in basis points (e.g., 100 = 1%) + // Handle gas fee first (similar to SwapAlgebra) + uint256 amountInForMainSwap = amountIn; - // Only account for gas fee if gasZRC20 is provided - uint256 totalCost = slippageCost; - if (gasZRC20 != address(0)) { - totalCost += gasFee; - } - - require(amountIn > totalCost, "Amount insufficient to cover costs after tip"); - - amountOut = amountIn - totalCost; + 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 tokens back to the sender - IERC20(tokenOut).transfer(msg.sender, amountOut); - - // Only transfer gas fee if gasZRC20 is not zero address - if (gasZRC20 != address(0) && gasFee > 0) { + // 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; // slippage in basis points (e.g., 100 = 1%) + 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; diff --git a/test/swapModules/SwapAlgebra.t.sol b/test/swapModules/SwapAlgebra.t.sol index 9627ac0..396c581 100644 --- a/test/swapModules/SwapAlgebra.t.sol +++ b/test/swapModules/SwapAlgebra.t.sol @@ -516,7 +516,7 @@ contract SwapAlgebraTest is Test { uint256 expectedAmount = 1000 ether; // Calculate min amount based on 1% slippage - uint256 minRequiredAmount = expectedAmount * 99 / 100; + uint256 minRequiredAmount = (expectedAmount * 99) / 100; // Test with amount just below the minimum (should fail) uint256 tooLowAmount = minRequiredAmount - 1; @@ -553,4 +553,68 @@ contract SwapAlgebraTest is Test { assertEq(amountOut, expectedOutput, "Incorrect output amount"); assertEq(outputToken.balanceOf(user), expectedOutput, "Output tokens not received by user"); } + + function test_SwapSameToken() public { + uint256 swapAmount = AMOUNT; + uint256 gasFee = 1 ether; + + // Mint tokens to SwapAlgebra for the same token case + inputToken.mint(address(swapAlgebra), swapAmount); + gasToken.mint(address(swapAlgebra), gasFee); + + // Record initial balances + uint256 initialInputBalance = inputToken.balanceOf(user); + uint256 initialGasBalance = gasToken.balanceOf(user); + + // Test swapping the same token (should skip main swap but handle gas fees) + vm.prank(user); + uint256 amountOut = + swapAlgebra.swap(address(inputToken), address(inputToken), swapAmount, address(gasToken), gasFee); + + // Expected output should be the input amount minus gas fee + uint256 expectedOutput = swapAmount - gasFee; + assertEq(amountOut, expectedOutput, "Incorrect output amount for same token swap"); + + // Check that user received the expected amount (initial balance - input + output) + uint256 expectedFinalBalance = initialInputBalance - swapAmount + expectedOutput; + assertEq(inputToken.balanceOf(user), expectedFinalBalance, "Input tokens not returned correctly"); + + // Check that user received gas tokens + assertEq(gasToken.balanceOf(user), initialGasBalance + gasFee, "Gas tokens not received by user"); + + // Verify that no actual swap was performed (no pool interaction) + // The mock pool should not have been called since it's the same token + } + + function test_SwapSameTokenWithGasToken() public { + uint256 swapAmount = AMOUNT; + uint256 gasFee = 1 ether; + + // Mint tokens to SwapAlgebra for the same token case + inputToken.mint(address(swapAlgebra), swapAmount); + + // Record initial balance + uint256 initialBalance = inputToken.balanceOf(user); + + // Test swapping the same token where the input token is also the gas token + vm.prank(user); + uint256 amountOut = swapAlgebra.swap( + address(inputToken), + address(inputToken), + swapAmount, + address(inputToken), // input token is also gas token + gasFee + ); + + // Expected output should be the input amount minus gas fee + uint256 expectedOutput = swapAmount - gasFee; + assertEq(amountOut, expectedOutput, "Incorrect output amount for same token swap with gas token"); + + // Check that user's balance remains the same (they get back what they sent) + // The gas fee is handled internally by the SwapAlgebra contract + assertEq(inputToken.balanceOf(user), initialBalance, "User balance should remain unchanged"); + + // Verify that no actual swap was performed (no pool interaction) + // The mock pool should not have been called since it's the same token + } }