Merge pull request #100 from propeller-heads/router/hr/ENG-4298-min-amount-out-transfer-from

feat: add safeTransferFrom in swap and move core swap logic inside _swapChecked

foundry/src/TychoRouter.sol

@@ -56,7 +56,6 @@ import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol";
 //           ✷✷✷✷✷✷               ✷✷✷✷✷              ✷✷✷✷✷✷✷✷        ✷✷✷✷✷✷      ✷✷✷✷✷✷         ✷✷✷✷✷✷✷✷
 
 error TychoRouter__AddressZero();
-error TychoRouter__AmountZero();
 error TychoRouter__EmptySwaps();
 error TychoRouter__NegativeSlippage(uint256 amount, uint256 minAmount);
 error TychoRouter__AmountInDiffersFromConsumed(
@@ -144,6 +143,103 @@ contract TychoRouter is AccessControl, Dispatcher, Pausable, ReentrancyGuard {
         address receiver,
         bytes calldata swaps
     ) public payable whenNotPaused nonReentrant returns (uint256 amountOut) {
+        IERC20(tokenIn).safeTransferFrom(msg.sender, address(this), amountIn);
+        return _swapChecked(
+            amountIn,
+            tokenIn,
+            tokenOut,
+            minAmountOut,
+            wrapEth,
+            unwrapEth,
+            nTokens,
+            receiver,
+            swaps
+        );
+    }
+
+    /**
+     * @notice Executes a swap operation based on a predefined swap graph, supporting internal token amount splits.
+     *         This function enables multi-step swaps, optional ETH wrapping/unwrapping, and validates the output amount
+     *         against a user-specified minimum.
+     *
+     * @dev
+     * - If `wrapEth` is true, the contract wraps the provided native ETH into WETH and uses it as the sell token.
+     * - If `unwrapEth` is true, the contract converts the resulting WETH back into native ETH before sending it to the receiver.
+     * - For ERC20 tokens, Permit2 is used to approve and transfer tokens from the caller to the router.
+     * - Swaps are executed sequentially using the `_swap` function.
+     * - A fee is deducted from the output token if `fee > 0`, and the remaining amount is sent to the receiver.
+     * - Reverts with `TychoRouter__NegativeSlippage` if the output amount is less than `minAmountOut` and `minAmountOut` is greater than 0.
+     *
+     * @param amountIn The input token amount to be swapped.
+     * @param tokenIn The address of the input token. Use `address(0)` for native ETH
+     * @param tokenOut The address of the output token. Use `address(0)` for native ETH
+     * @param minAmountOut The minimum acceptable amount of the output token. Reverts if this condition is not met. If it's 0, no check is performed.
+     * @param wrapEth If true, wraps the input token (native ETH) into WETH.
+     * @param unwrapEth If true, unwraps the resulting WETH into native ETH and sends it to the receiver.
+     * @param nTokens The total number of tokens involved in the swap graph (used to initialize arrays for internal calculations).
+     * @param receiver The address to receive the output tokens.
+     * @param permitSingle A Permit2 structure containing token approval details for the input token. Ignored if `wrapEth` is true.
+     * @param signature A valid signature authorizing the Permit2 approval. Ignored if `wrapEth` is true.
+     * @param swaps Encoded swap graph data containing details of each swap.
+     *
+     * @return amountOut The total amount of the output token received by the receiver, after deducting fees if applicable.
+     */
+    function swapPermit2(
+        uint256 amountIn,
+        address tokenIn,
+        address tokenOut,
+        uint256 minAmountOut,
+        bool wrapEth,
+        bool unwrapEth,
+        uint256 nTokens,
+        address receiver,
+        IAllowanceTransfer.PermitSingle calldata permitSingle,
+        bytes calldata signature,
+        bytes calldata swaps
+    ) external payable whenNotPaused nonReentrant returns (uint256 amountOut) {
+        // For native ETH, assume funds already in our router. Else, transfer and handle approval.
+        if (tokenIn != address(0)) {
+            permit2.permit(msg.sender, permitSingle, signature);
+            permit2.transferFrom(
+                msg.sender,
+                address(this),
+                uint160(amountIn),
+                permitSingle.details.token
+            );
+        }
+
+        return _swapChecked(
+            amountIn,
+            tokenIn,
+            tokenOut,
+            minAmountOut,
+            wrapEth,
+            unwrapEth,
+            nTokens,
+            receiver,
+            swaps
+        );
+    }
+
+    /**
+     * @notice Internal implementation of the core swap logic shared between swap() and swapPermit2().
+     *
+     * @notice This function centralizes the swap execution logic.
+     * @notice For detailed documentation on parameters and behavior, see the documentation for
+     * swap() and swapPermit2() functions.
+     *
+     */
+    function _swapChecked(
+        uint256 amountIn,
+        address tokenIn,
+        address tokenOut,
+        uint256 minAmountOut,
+        bool wrapEth,
+        bool unwrapEth,
+        uint256 nTokens,
+        address receiver,
+        bytes calldata swaps
+    ) internal returns (uint256 amountOut) {
         if (receiver == address(0)) {
             revert TychoRouter__AddressZero();
         }
@@ -191,70 +287,6 @@ contract TychoRouter is AccessControl, Dispatcher, Pausable, ReentrancyGuard {
         }
     }
 
-    /**
-     * @notice Executes a swap operation based on a predefined swap graph, supporting internal token amount splits.
-     *         This function enables multi-step swaps, optional ETH wrapping/unwrapping, and validates the output amount
-     *         against a user-specified minimum.
-     *
-     * @dev
-     * - If `wrapEth` is true, the contract wraps the provided native ETH into WETH and uses it as the sell token.
-     * - If `unwrapEth` is true, the contract converts the resulting WETH back into native ETH before sending it to the receiver.
-     * - For ERC20 tokens, Permit2 is used to approve and transfer tokens from the caller to the router.
-     * - Swaps are executed sequentially using the `_swap` function.
-     * - A fee is deducted from the output token if `fee > 0`, and the remaining amount is sent to the receiver.
-     * - Reverts with `TychoRouter__NegativeSlippage` if the output amount is less than `minAmountOut` and `minAmountOut` is greater than 0.
-     *
-     * @param amountIn The input token amount to be swapped.
-     * @param tokenIn The address of the input token. Use `address(0)` for native ETH
-     * @param tokenOut The address of the output token. Use `address(0)` for native ETH
-     * @param minAmountOut The minimum acceptable amount of the output token. Reverts if this condition is not met. If it's 0, no check is performed.
-     * @param wrapEth If true, wraps the input token (native ETH) into WETH.
-     * @param unwrapEth If true, unwraps the resulting WETH into native ETH and sends it to the receiver.
-     * @param nTokens The total number of tokens involved in the swap graph (used to initialize arrays for internal calculations).
-     * @param receiver The address to receive the output tokens.
-     * @param permitSingle A Permit2 structure containing token approval details for the input token. Ignored if `wrapEth` is true.
-     * @param signature A valid signature authorizing the Permit2 approval. Ignored if `wrapEth` is true.
-     * @param swaps Encoded swap graph data containing details of each swap.
-     *
-     * @return amountOut The total amount of the output token received by the receiver, after deducting fees if applicable.
-     */
-    function swapPermit2(
-        uint256 amountIn,
-        address tokenIn,
-        address tokenOut,
-        uint256 minAmountOut,
-        bool wrapEth,
-        bool unwrapEth,
-        uint256 nTokens,
-        address receiver,
-        IAllowanceTransfer.PermitSingle calldata permitSingle,
-        bytes calldata signature,
-        bytes calldata swaps
-    ) external payable whenNotPaused returns (uint256 amountOut) {
-        // For native ETH, assume funds already in our router. Else, transfer and handle approval.
-        if (tokenIn != address(0)) {
-            permit2.permit(msg.sender, permitSingle, signature);
-            permit2.transferFrom(
-                msg.sender,
-                address(this),
-                uint160(amountIn),
-                permitSingle.details.token
-            );
-        }
-
-        return swap(
-            amountIn,
-            tokenIn,
-            tokenOut,
-            minAmountOut,
-            wrapEth,
-            unwrapEth,
-            nTokens,
-            receiver,
-            swaps
-        );
-    }
-
     /**
      * @dev Executes sequential swaps as defined by the provided swap graph.
      *

foundry/test/TychoRouter.t.sol

@@ -420,10 +420,10 @@ contract TychoRouterTest is TychoRouterTestSetup {
         // Checks amount out at the end
         uint256 amountIn = 1 ether;
 
-        // Assume Alice has already transferred tokens to the TychoRouter
+        deal(WETH_ADDR, ALICE, amountIn);
-        deal(WETH_ADDR, tychoRouterAddr, amountIn);
-
         vm.startPrank(ALICE);
+        // Approve the tokenIn to be transferred to the router
+        IERC20(WETH_ADDR).approve(address(tychoRouterAddr), amountIn);
 
         bytes memory protocolData = encodeUniswapV2Swap(
             WETH_ADDR, WETH_DAI_POOL, tychoRouterAddr, false
@@ -457,7 +457,44 @@ contract TychoRouterTest is TychoRouterTestSetup {
         vm.stopPrank();
     }
 
-    function testSwapCheckedFailure() public {
+    function testSwapCheckedLessApprovalFailure() public {
+        // Trade 1 WETH for DAI with 1 swap on Uniswap V2
+        // Fails while transferring the tokenIn to the router due to insufficient approval
+        uint256 amountIn = 1 ether;
+
+        deal(WETH_ADDR, ALICE, amountIn);
+        vm.startPrank(ALICE);
+        // Approve less than the amountIn
+        IERC20(WETH_ADDR).approve(address(tychoRouterAddr), amountIn - 1);
+
+        bytes memory protocolData = encodeUniswapV2Swap(
+            WETH_ADDR, WETH_DAI_POOL, tychoRouterAddr, false
+        );
+
+        bytes memory swap = encodeSwap(
+            uint8(0), uint8(1), uint24(0), address(usv2Executor), protocolData
+        );
+        bytes[] memory swaps = new bytes[](1);
+        swaps[0] = swap;
+
+        uint256 minAmountOut = 2600 * 1e18;
+        vm.expectRevert();
+        uint256 amountOut = tychoRouter.swap(
+            amountIn,
+            WETH_ADDR,
+            DAI_ADDR,
+            minAmountOut,
+            false,
+            false,
+            2,
+            ALICE,
+            pleEncode(swaps)
+        );
+
+        vm.stopPrank();
+    }
+
+    function testSwapCheckedNegativeSlippageFailure() public {
         // Trade 1 WETH for DAI with 1 swap on Uniswap V2
         // Does permit2 token approval and transfer
         // Checks amount out at the end and fails
@@ -760,7 +797,9 @@ contract TychoRouterTest is TychoRouterTestSetup {
         // address with the USV2 executor address.
 
         // Tests swapping WETH -> DAI on a USV2 pool without permit2
-        deal(WETH_ADDR, tychoRouterAddr, 1 ether);
+        deal(WETH_ADDR, ALICE, 1 ether);
+        vm.startPrank(ALICE);
+        IERC20(WETH_ADDR).approve(address(tychoRouterAddr), 1 ether);
         uint256 balancerBefore = IERC20(DAI_ADDR).balanceOf(ALICE);
         // Encoded solution generated using `test_split_swap_strategy_encoder_simple_route_no_permit2`
         // but manually replacing the executor address