From b35d71abef0be1ba7ec4f9f03a65326574f2009c Mon Sep 17 00:00:00 2001
From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com>
Date: Wed, 27 Nov 2024 15:59:55 +0000
Subject: [PATCH 01/12] test: add multi-hop tests for `TokenZapV1`

---
 .../contracts-rfq/test/mocks/WETHMock.sol     | 26 +++++++
 .../contracts-rfq/test/zaps/TokenZapV1.t.sol  | 69 ++++++++++++++++++-
 2 files changed, 94 insertions(+), 1 deletion(-)
 create mode 100644 packages/contracts-rfq/test/mocks/WETHMock.sol

diff --git a/packages/contracts-rfq/test/mocks/WETHMock.sol b/packages/contracts-rfq/test/mocks/WETHMock.sol
new file mode 100644
index 0000000000..13dcc4c904
--- /dev/null
+++ b/packages/contracts-rfq/test/mocks/WETHMock.sol
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.20;
+
+import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
+import {Address} from "@openzeppelin/contracts/utils/Address.sol";
+
+/// @notice WETH mock for testing purposes. DO NOT USE IN PRODUCTION.
+contract WETHMock is ERC20 {
+    constructor() ERC20("Mock Wrapped Ether", "Mock WETH") {}
+
+    receive() external payable {
+        deposit();
+    }
+
+    /// @notice We include an empty "test" function so that this contract does not appear in the coverage report.
+    function testWETHMock() external {}
+
+    function withdraw(uint256 amount) external {
+        _burn(msg.sender, amount);
+        Address.sendValue(payable(msg.sender), amount);
+    }
+
+    function deposit() public payable {
+        _mint(msg.sender, msg.value);
+    }
+}
diff --git a/packages/contracts-rfq/test/zaps/TokenZapV1.t.sol b/packages/contracts-rfq/test/zaps/TokenZapV1.t.sol
index 37a07838e2..7e7d013452 100644
--- a/packages/contracts-rfq/test/zaps/TokenZapV1.t.sol
+++ b/packages/contracts-rfq/test/zaps/TokenZapV1.t.sol
@@ -6,6 +6,7 @@ import {TokenZapV1} from "../../contracts/zaps/TokenZapV1.sol";
 
 import {MockERC20} from "../MockERC20.sol";
 import {VaultManyArguments} from "../mocks/VaultManyArguments.sol";
+import {WETHMock} from "../mocks/WETHMock.sol";
 
 import {Test} from "forge-std/Test.sol";
 
@@ -16,6 +17,7 @@ contract TokenZapV1Test is Test {
     TokenZapV1 internal tokenZap;
     VaultManyArguments internal vault;
     MockERC20 internal erc20;
+    WETHMock internal weth;
 
     address internal user;
     address internal nativeGasToken = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;
@@ -24,11 +26,13 @@ contract TokenZapV1Test is Test {
         tokenZap = new TokenZapV1();
         vault = new VaultManyArguments();
         erc20 = new MockERC20("TKN", 18);
+        weth = new WETHMock();
 
         user = makeAddr("user");
 
         erc20.mint(address(this), 100 * AMOUNT);
-        deal(address(this), 100 * AMOUNT);
+        deal(address(this), 200 * AMOUNT);
+        weth.deposit{value: 100 * AMOUNT}();
     }
 
     function getVaultPayload(address token, uint256 amount) public view returns (bytes memory) {
@@ -144,6 +148,69 @@ contract TokenZapV1Test is Test {
         test_zap_native_noAmount();
     }
 
+    // ═════════════════════════════════════════════════ MULTIHOPS ═════════════════════════════════════════════════════
+
+    function getZapDataWithdraw(uint256 amount) public view returns (bytes memory) {
+        return tokenZap.encodeZapData(address(weth), abi.encodeCall(WETHMock.withdraw, (amount)), 4);
+    }
+
+    function test_zap_withdraw_depositNative_placeholderZero() public {
+        bytes memory zapDataWithdraw = getZapDataWithdraw(0);
+        bytes memory zapDataDeposit = getZapDataNoAmount(getVaultPayloadNoAmount());
+        weth.transfer(address(tokenZap), AMOUNT);
+        // Do two Zaps in a row
+        bytes4 returnValue = tokenZap.zap(address(weth), AMOUNT, zapDataWithdraw);
+        assertEq(returnValue, tokenZap.zap.selector);
+        returnValue = tokenZap.zap(nativeGasToken, AMOUNT, zapDataDeposit);
+        assertEq(returnValue, tokenZap.zap.selector);
+        // Check that the vault registered the deposit
+        assertEq(vault.balanceOf(user, nativeGasToken), AMOUNT);
+    }
+
+    function test_zap_withdraw_depositNative_placeholderNonZero() public {
+        // Use the approximate amount of tokens as placeholder
+        bytes memory zapDataWithdraw = getZapDataWithdraw(1 ether);
+        bytes memory zapDataDeposit = getZapDataNoAmount(getVaultPayloadNoAmount());
+        weth.transfer(address(tokenZap), AMOUNT);
+        // Do two Zaps in a row
+        bytes4 returnValue = tokenZap.zap(address(weth), AMOUNT, zapDataWithdraw);
+        assertEq(returnValue, tokenZap.zap.selector);
+        returnValue = tokenZap.zap(nativeGasToken, AMOUNT, zapDataDeposit);
+        assertEq(returnValue, tokenZap.zap.selector);
+        // Check that the vault registered the deposit
+        assertEq(vault.balanceOf(user, nativeGasToken), AMOUNT);
+    }
+
+    function test_zap_withdraw_depositNative_placeholderZero_extraTokens() public {
+        // Transfer some extra tokens to the zap contract
+        weth.transfer(address(tokenZap), AMOUNT);
+        // Should not affect the zap
+        test_zap_withdraw_depositNative_placeholderZero();
+    }
+
+    function test_zap_withdraw_depositNative_placeholderZero_extraNative() public {
+        // Transfer some extra native tokens to the zap contract
+        deal(address(tokenZap), AMOUNT);
+        // Should not affect the zap
+        test_zap_withdraw_depositNative_placeholderZero();
+    }
+
+    function test_zap_withdraw_depositNative_placeholderNonZero_extraTokens() public {
+        // Transfer some extra tokens to the zap contract
+        weth.transfer(address(tokenZap), AMOUNT);
+        // Should not affect the zap
+        test_zap_withdraw_depositNative_placeholderNonZero();
+    }
+
+    function test_zap_withdraw_depositNative_placeholderNonZero_extraNative() public {
+        // Transfer some extra native tokens to the zap contract
+        deal(address(tokenZap), AMOUNT);
+        // Should not affect the zap
+        test_zap_withdraw_depositNative_placeholderNonZero();
+    }
+
+    // ═════════════════════════════════════════════════ ENCODING ══════════════════════════════════════════════════════
+
     function test_encodeZapData_roundtrip(address token, uint256 placeholderAmount, uint256 amount) public view {
         bytes memory originalPayload = getVaultPayload(token, placeholderAmount);
         bytes memory expectedPayload = getVaultPayload(token, amount);

From 10c126a5d9d18fea2e8182ab4e0fe9c0b8ea11ef Mon Sep 17 00:00:00 2001
From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com>
Date: Wed, 27 Nov 2024 16:01:46 +0000
Subject: [PATCH 02/12] fix: `TokenZap` should be able to receive ETH

---
 packages/contracts-rfq/contracts/zaps/TokenZapV1.sol | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol b/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
index a754c55f57..5f6d6d3ebd 100644
--- a/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
+++ b/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
@@ -29,6 +29,10 @@ contract TokenZapV1 is IZapRecipient {
     error TokenZapV1__AmountIncorrect();
     error TokenZapV1__PayloadLengthAboveMax();
 
+    /// @notice Allows the contract to receive ETH.
+    /// @dev Leftover ETH can be claimed by anyone, make sure to spend the full balance during the Zaps.
+    receive() external payable {}
+
     /// @notice Performs a Zap action using the specified token and amount. This amount must be previously
     /// transferred to this contract (or supplied as msg.value if the token is native gas token).
     /// @dev The provided ZapData contains the target address and calldata for the Zap action, and must be

From 57f144c8cd8d31bdc77d96077f903f0152877392 Mon Sep 17 00:00:00 2001
From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com>
Date: Wed, 27 Nov 2024 16:16:54 +0000
Subject: [PATCH 03/12] test: existing ETH balance, using lower `msg.value`

---
 .../contracts-rfq/test/zaps/TokenZapV1.t.sol  | 32 +++++++++++++------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/packages/contracts-rfq/test/zaps/TokenZapV1.t.sol b/packages/contracts-rfq/test/zaps/TokenZapV1.t.sol
index 7e7d013452..e5e0577838 100644
--- a/packages/contracts-rfq/test/zaps/TokenZapV1.t.sol
+++ b/packages/contracts-rfq/test/zaps/TokenZapV1.t.sol
@@ -8,6 +8,7 @@ import {MockERC20} from "../MockERC20.sol";
 import {VaultManyArguments} from "../mocks/VaultManyArguments.sol";
 import {WETHMock} from "../mocks/WETHMock.sol";
 
+import {Address} from "@openzeppelin/contracts/utils/Address.sol";
 import {Test} from "forge-std/Test.sol";
 
 // solhint-disable func-name-mixedcase, ordering
@@ -148,6 +149,27 @@ contract TokenZapV1Test is Test {
         test_zap_native_noAmount();
     }
 
+    /// @notice Should be able to use amount lower than msg.value.
+    function test_zap_native_msgValueHigherThanAmount() public {
+        bytes memory zapData = getZapData(getVaultPayload(nativeGasToken, 1 ether));
+        bytes4 returnValue = tokenZap.zap{value: AMOUNT + 1 wei}(nativeGasToken, AMOUNT, zapData);
+        assertEq(returnValue, tokenZap.zap.selector);
+        // Check that the vault registered the deposit
+        assertEq(vault.balanceOf(user, nativeGasToken), AMOUNT);
+        // Note: the extra funds are left in the contract
+        assertEq(address(tokenZap).balance, 1 wei);
+    }
+
+    /// @notice Should be able to utilize both msg.value and existing native balance.
+    function test_zap_native_msgValueLowerThanAmount_extraNative() public {
+        deal(address(tokenZap), 1337);
+        bytes memory zapData = getZapData(getVaultPayload(nativeGasToken, 1 ether));
+        bytes4 returnValue = tokenZap.zap{value: AMOUNT - 1337}(nativeGasToken, AMOUNT, zapData);
+        assertEq(returnValue, tokenZap.zap.selector);
+        // Check that the vault registered the deposit
+        assertEq(vault.balanceOf(user, nativeGasToken), AMOUNT);
+    }
+
     // ═════════════════════════════════════════════════ MULTIHOPS ═════════════════════════════════════════════════════
 
     function getZapDataWithdraw(uint256 amount) public view returns (bytes memory) {
@@ -261,18 +283,10 @@ contract TokenZapV1Test is Test {
         bytes memory originalPayload = getVaultPayload(nativeGasToken, 0);
         bytes memory zapData = getZapData(originalPayload);
 
-        vm.expectRevert(TokenZapV1.TokenZapV1__AmountIncorrect.selector);
+        vm.expectRevert(abi.encodeWithSelector(Address.AddressInsufficientBalance.selector, tokenZap));
         tokenZap.zap{value: 1 ether - 1 wei}(nativeGasToken, 1 ether, zapData);
     }
 
-    function test_zap_native_revert_msgValueHigherThanExpected() public {
-        bytes memory originalPayload = getVaultPayload(nativeGasToken, 0);
-        bytes memory zapData = getZapData(originalPayload);
-
-        vm.expectRevert(TokenZapV1.TokenZapV1__AmountIncorrect.selector);
-        tokenZap.zap{value: 1 ether + 1 wei}(nativeGasToken, 1 ether, zapData);
-    }
-
     function test_encodeZapData_revert_payloadLengthAboveMax() public {
         bytes memory tooLongPayload = new bytes(2 ** 16);
         vm.expectRevert(TokenZapV1.TokenZapV1__PayloadLengthAboveMax.selector);

From 7099b1eb72749cc85f40733b6903cda9dd51ebb3 Mon Sep 17 00:00:00 2001
From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com>
Date: Wed, 27 Nov 2024 16:11:18 +0000
Subject: [PATCH 04/12] fix: `TokenZap` should be able to use previosuly
 acquired ETH

---
 .../contracts/zaps/TokenZapV1.sol              | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol b/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
index 5f6d6d3ebd..f8eec13e27 100644
--- a/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
+++ b/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
@@ -26,7 +26,6 @@ contract TokenZapV1 is IZapRecipient {
 
     address public constant NATIVE_GAS_TOKEN = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;
 
-    error TokenZapV1__AmountIncorrect();
     error TokenZapV1__PayloadLengthAboveMax();
 
     /// @notice Allows the contract to receive ETH.
@@ -34,22 +33,29 @@ contract TokenZapV1 is IZapRecipient {
     receive() external payable {}
 
     /// @notice Performs a Zap action using the specified token and amount. This amount must be previously
-    /// transferred to this contract (or supplied as msg.value if the token is native gas token).
+    /// transferred to this contract (could also be supplied as msg.value if the token is native gas token).
+    /// Note: all funds remaining after the Zap action is performed can be claimed by anyone.
+    /// Make sure to spend the full balance during the Zaps and avoid sending extra funds if a single Zap is performed.
     /// @dev The provided ZapData contains the target address and calldata for the Zap action, and must be
     /// encoded using the encodeZapData function.
     /// @param token        Address of the token to be used for the Zap action.
     /// @param amount       Amount of the token to be used for the Zap action.
-    ///                     Must match msg.value if the token is a native gas token.
     /// @param zapData      Encoded Zap Data containing the target address and calldata for the Zap action.
     /// @return selector    Selector of this function to signal the caller about the success of the Zap action.
     function zap(address token, uint256 amount, bytes calldata zapData) external payable returns (bytes4) {
         // Validate the ZapData format and extract the target address.
         zapData.validateV1();
         address target = zapData.target();
+        uint256 msgValue = msg.value;
+        // Note: we don't check the amount that was transferred to TokenZapV1 (or msg.value for native gas tokens),
+        // so transfering more than `amount` will lead to remaining funds in TokenZapV1, which can be claimed by anyone.
+        // Make sure to send the exact amount for a single Zap or spend the full balance for multiple `zap()` calls.
         if (token == NATIVE_GAS_TOKEN) {
-            // For native gas token (e.g., ETH), verify msg.value matches the expected amount.
+            // For native gas tokens we forward the requested amount to the target contract during the Zap action.
+            // Silimar to ERC20s, we allow to use pre-transferred native tokens for the Zap.
+            msgValue = amount;
             // No approval needed since native token doesn't use allowances.
-            if (msg.value != amount) revert TokenZapV1__AmountIncorrect();
+            // Note: balance check is performed within `Address.functionCallWithValue`.
         } else {
             // For ERC20 tokens, grant unlimited approval to the target if the current allowance is insufficient.
             // This is safe since the contract doesn't custody tokens between zaps.
@@ -63,7 +69,7 @@ contract TokenZapV1 is IZapRecipient {
         bytes memory payload = zapData.payload(amount);
         // Perform the Zap action, forwarding full msg.value to the target contract.
         // Note: this will bubble up any revert from the target contract.
-        Address.functionCallWithValue({target: target, data: payload, value: msg.value});
+        Address.functionCallWithValue({target: target, data: payload, value: msgValue});
         // Return function selector to indicate successful execution
         return this.zap.selector;
     }

From cf3a39e81911f01ca4d5f6ba9be4d49f870ba57b Mon Sep 17 00:00:00 2001
From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com>
Date: Thu, 28 Nov 2024 11:27:06 +0000
Subject: [PATCH 05/12] test: `TokenZap` should be able to do native transfers

---
 .../contracts-rfq/test/zaps/TokenZapV1.t.sol  | 125 ++++++++++++++++++
 1 file changed, 125 insertions(+)

diff --git a/packages/contracts-rfq/test/zaps/TokenZapV1.t.sol b/packages/contracts-rfq/test/zaps/TokenZapV1.t.sol
index e5e0577838..94cf0d9a7f 100644
--- a/packages/contracts-rfq/test/zaps/TokenZapV1.t.sol
+++ b/packages/contracts-rfq/test/zaps/TokenZapV1.t.sol
@@ -5,6 +5,8 @@ import {ZapDataV1} from "../../contracts/libs/ZapDataV1.sol";
 import {TokenZapV1} from "../../contracts/zaps/TokenZapV1.sol";
 
 import {MockERC20} from "../MockERC20.sol";
+import {NonPayableRecipient} from "../mocks/NonPayableRecipient.sol";
+import {RecipientMock} from "../mocks/RecipientMock.sol";
 import {VaultManyArguments} from "../mocks/VaultManyArguments.sol";
 import {WETHMock} from "../mocks/WETHMock.sol";
 
@@ -19,6 +21,8 @@ contract TokenZapV1Test is Test {
     VaultManyArguments internal vault;
     MockERC20 internal erc20;
     WETHMock internal weth;
+    address internal payableMock;
+    address internal nonPayableMock;
 
     address internal user;
     address internal nativeGasToken = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;
@@ -28,6 +32,8 @@ contract TokenZapV1Test is Test {
         vault = new VaultManyArguments();
         erc20 = new MockERC20("TKN", 18);
         weth = new WETHMock();
+        payableMock = address(new RecipientMock());
+        nonPayableMock = address(new NonPayableRecipient());
 
         user = makeAddr("user");
 
@@ -231,6 +237,116 @@ contract TokenZapV1Test is Test {
         test_zap_withdraw_depositNative_placeholderNonZero();
     }
 
+    function test_zap_withdraw_transferNativeEOA_placeholderZero() public {
+        bytes memory zapDataWithdraw = getZapDataWithdraw(0);
+        bytes memory zapDataTransfer = tokenZap.encodeZapData({target: user, payload: "", amountPosition: 0});
+        weth.transfer(address(tokenZap), AMOUNT);
+        // Do two Zaps in a row
+        bytes4 returnValue = tokenZap.zap(address(weth), AMOUNT, zapDataWithdraw);
+        assertEq(returnValue, tokenZap.zap.selector);
+        returnValue = tokenZap.zap(nativeGasToken, AMOUNT, zapDataTransfer);
+        assertEq(returnValue, tokenZap.zap.selector);
+        // Check that the user received the native tokens
+        assertEq(user.balance, AMOUNT);
+    }
+
+    function test_zap_withdraw_transferNativeEOA_placeholderNonZero() public {
+        // Use the approximate amount of tokens as placeholder
+        bytes memory zapDataWithdraw = getZapDataWithdraw(1 ether);
+        bytes memory zapDataTransfer = tokenZap.encodeZapData({target: user, payload: "", amountPosition: 0});
+        weth.transfer(address(tokenZap), AMOUNT);
+        // Do two Zaps in a row
+        bytes4 returnValue = tokenZap.zap(address(weth), AMOUNT, zapDataWithdraw);
+        assertEq(returnValue, tokenZap.zap.selector);
+        returnValue = tokenZap.zap(nativeGasToken, AMOUNT, zapDataTransfer);
+        assertEq(returnValue, tokenZap.zap.selector);
+        // Check that the user received the native tokens
+        assertEq(user.balance, AMOUNT);
+    }
+
+    function test_zap_withdraw_transferNativeEOA_placeholderZero_extraTokens() public {
+        // Transfer some extra tokens to the zap contract
+        weth.transfer(address(tokenZap), AMOUNT);
+        // Should not affect the zap
+        test_zap_withdraw_transferNativeEOA_placeholderZero();
+    }
+
+    function test_zap_withdraw_transferNativeEOA_placeholderZero_extraNative() public {
+        // Transfer some extra native tokens to the zap contract
+        deal(address(tokenZap), AMOUNT);
+        // Should not affect the zap
+        test_zap_withdraw_transferNativeEOA_placeholderZero();
+    }
+
+    function test_zap_withdraw_transferNativeEOA_placeholderNonZero_extraTokens() public {
+        // Transfer some extra tokens to the zap contract
+        weth.transfer(address(tokenZap), AMOUNT);
+        // Should not affect the zap
+        test_zap_withdraw_transferNativeEOA_placeholderNonZero();
+    }
+
+    function test_zap_withdraw_transferNativeEOA_placeholderNonZero_extraNative() public {
+        // Transfer some extra native tokens to the zap contract
+        deal(address(tokenZap), AMOUNT);
+        // Should not affect the zap
+        test_zap_withdraw_transferNativeEOA_placeholderNonZero();
+    }
+
+    function test_zap_withdraw_transferNativeContract_placeholderZero() public {
+        bytes memory zapDataWithdraw = getZapDataWithdraw(0);
+        bytes memory zapDataTransfer = tokenZap.encodeZapData({target: payableMock, payload: "", amountPosition: 0});
+        weth.transfer(address(tokenZap), AMOUNT);
+        // Do two Zaps in a row
+        bytes4 returnValue = tokenZap.zap(address(weth), AMOUNT, zapDataWithdraw);
+        assertEq(returnValue, tokenZap.zap.selector);
+        returnValue = tokenZap.zap(nativeGasToken, AMOUNT, zapDataTransfer);
+        assertEq(returnValue, tokenZap.zap.selector);
+        // Check that the contract received the native tokens
+        assertEq(payableMock.balance, AMOUNT);
+    }
+
+    function test_zap_withdraw_transferNativeContract_placeholderNonZero() public {
+        // Use the approximate amount of tokens as placeholder
+        bytes memory zapDataWithdraw = getZapDataWithdraw(1 ether);
+        bytes memory zapDataTransfer = tokenZap.encodeZapData({target: payableMock, payload: "", amountPosition: 0});
+        weth.transfer(address(tokenZap), AMOUNT);
+        // Do two Zaps in a row
+        bytes4 returnValue = tokenZap.zap(address(weth), AMOUNT, zapDataWithdraw);
+        assertEq(returnValue, tokenZap.zap.selector);
+        returnValue = tokenZap.zap(nativeGasToken, AMOUNT, zapDataTransfer);
+        assertEq(returnValue, tokenZap.zap.selector);
+        // Check that the contract received the native tokens
+        assertEq(payableMock.balance, AMOUNT);
+    }
+
+    function test_zap_withdraw_transferNativeContract_placeholderZero_extraTokens() public {
+        // Transfer some extra tokens to the zap contract
+        weth.transfer(address(tokenZap), AMOUNT);
+        // Should not affect the zap
+        test_zap_withdraw_transferNativeContract_placeholderZero();
+    }
+
+    function test_zap_withdraw_transferNativeContract_placeholderZero_extraNative() public {
+        // Transfer some extra native tokens to the zap contract
+        deal(address(tokenZap), AMOUNT);
+        // Should not affect the zap
+        test_zap_withdraw_transferNativeContract_placeholderZero();
+    }
+
+    function test_zap_withdraw_transferNativeContract_placeholderNonZero_extraTokens() public {
+        // Transfer some extra tokens to the zap contract
+        weth.transfer(address(tokenZap), AMOUNT);
+        // Should not affect the zap
+        test_zap_withdraw_transferNativeContract_placeholderNonZero();
+    }
+
+    function test_zap_withdraw_transferNativeContract_placeholderNonZero_extraNative() public {
+        // Transfer some extra native tokens to the zap contract
+        deal(address(tokenZap), AMOUNT);
+        // Should not affect the zap
+        test_zap_withdraw_transferNativeContract_placeholderNonZero();
+    }
+
     // ═════════════════════════════════════════════════ ENCODING ══════════════════════════════════════════════════════
 
     function test_encodeZapData_roundtrip(address token, uint256 placeholderAmount, uint256 amount) public view {
@@ -287,6 +403,15 @@ contract TokenZapV1Test is Test {
         tokenZap.zap{value: 1 ether - 1 wei}(nativeGasToken, 1 ether, zapData);
     }
 
+    function test_zap_withdraw_transferNative_revert_targetReverted() public {
+        bytes memory zapDataWithdraw = getZapDataWithdraw(0);
+        bytes memory zapDataTransfer = tokenZap.encodeZapData({target: nonPayableMock, payload: "", amountPosition: 0});
+        weth.transfer(address(tokenZap), AMOUNT);
+        tokenZap.zap(address(weth), AMOUNT, zapDataWithdraw);
+        vm.expectRevert(Address.FailedInnerCall.selector);
+        tokenZap.zap(address(weth), AMOUNT, zapDataTransfer);
+    }
+
     function test_encodeZapData_revert_payloadLengthAboveMax() public {
         bytes memory tooLongPayload = new bytes(2 ** 16);
         vm.expectRevert(TokenZapV1.TokenZapV1__PayloadLengthAboveMax.selector);

From 31cbdbd481235f39ba39335c925d16bd12d748e7 Mon Sep 17 00:00:00 2001
From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com>
Date: Thu, 28 Nov 2024 11:30:13 +0000
Subject: [PATCH 06/12] fix: native ETH transfers to EOAs

---
 .../contracts-rfq/contracts/zaps/TokenZapV1.sol   | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol b/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
index f8eec13e27..c7eaef1c8c 100644
--- a/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
+++ b/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
@@ -55,7 +55,7 @@ contract TokenZapV1 is IZapRecipient {
             // Silimar to ERC20s, we allow to use pre-transferred native tokens for the Zap.
             msgValue = amount;
             // No approval needed since native token doesn't use allowances.
-            // Note: balance check is performed within `Address.functionCallWithValue`.
+            // Note: balance check is performed within `Address.sendValue` or `Address.functionCallWithValue` below.
         } else {
             // For ERC20 tokens, grant unlimited approval to the target if the current allowance is insufficient.
             // This is safe since the contract doesn't custody tokens between zaps.
@@ -67,9 +67,16 @@ contract TokenZapV1 is IZapRecipient {
         // Construct the payload for the target contract call with the Zap action.
         // The payload is modified to replace the placeholder amount with the actual amount.
         bytes memory payload = zapData.payload(amount);
-        // Perform the Zap action, forwarding full msg.value to the target contract.
-        // Note: this will bubble up any revert from the target contract.
-        Address.functionCallWithValue({target: target, data: payload, value: msgValue});
+        if (payload.length == 0) {
+            // No payload provided, perform the native gas token transfer to the target.
+            // Note: we avoid using `functionCallWithValue` because target might be an EOA. This will
+            // revert with a generic custom error should the target contract revert on incoming transfer.
+            Address.sendValue({recipient: payable(target), amount: msgValue});
+        } else {
+            // Perform the Zap action, forwarding full msg.value to the target contract.
+            // Note: this will bubble up any revert from the target contract.
+            Address.functionCallWithValue({target: target, data: payload, value: msgValue});
+        }
         // Return function selector to indicate successful execution
         return this.zap.selector;
     }

From d8842887f56cf3095a031ecb8ea939159f9de4a6 Mon Sep 17 00:00:00 2001
From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com>
Date: Thu, 28 Nov 2024 12:55:44 +0000
Subject: [PATCH 07/12] docs: better wording for `msg.value` usage

---
 packages/contracts-rfq/contracts/zaps/TokenZapV1.sol | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol b/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
index c7eaef1c8c..fd61585945 100644
--- a/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
+++ b/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
@@ -34,6 +34,7 @@ contract TokenZapV1 is IZapRecipient {
 
     /// @notice Performs a Zap action using the specified token and amount. This amount must be previously
     /// transferred to this contract (could also be supplied as msg.value if the token is native gas token).
+    /// Zap action will be performed forwarding full `msg.value` for ERC20s or `amount` for native gas token.
     /// Note: all funds remaining after the Zap action is performed can be claimed by anyone.
     /// Make sure to spend the full balance during the Zaps and avoid sending extra funds if a single Zap is performed.
     /// @dev The provided ZapData contains the target address and calldata for the Zap action, and must be
@@ -73,7 +74,7 @@ contract TokenZapV1 is IZapRecipient {
             // revert with a generic custom error should the target contract revert on incoming transfer.
             Address.sendValue({recipient: payable(target), amount: msgValue});
         } else {
-            // Perform the Zap action, forwarding full msg.value to the target contract.
+            // Perform the Zap action, forwarding requested native value to the target contract.
             // Note: this will bubble up any revert from the target contract.
             Address.functionCallWithValue({target: target, data: payload, value: msgValue});
         }

From 40e7e53d49ce47034413291255539ba055407591 Mon Sep 17 00:00:00 2001
From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com>
Date: Thu, 28 Nov 2024 13:17:08 +0000
Subject: [PATCH 08/12] docs: minor fixes

---
 .../contracts/zaps/TokenZapV1.sol             | 32 +++++++++----------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol b/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
index fd61585945..71f04fb62a 100644
--- a/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
+++ b/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
@@ -15,11 +15,11 @@ import {IERC20, SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeE
 import {Address} from "@openzeppelin/contracts/utils/Address.sol";
 
 /// @title TokenZapV1
-/// @notice Facilitates atomic token operations known as "Zaps," allowing to execute predefined actions
-/// on behalf of users like deposits or swaps. Supports ERC20 tokens and native gas tokens (e.g., ETH).
-/// @dev Tokens must be pre-transferred to the contract for execution, with native tokens sent as msg.value.
+/// @notice Facilitates atomic token operations known as "Zaps", allowing the execution of predefined actions
+/// on behalf of users, such as deposits or swaps. Supports ERC20 tokens and native gas tokens (e.g., ETH).
+/// @dev Tokens must be transferred to the contract before execution, native tokens could be provided as `msg.value`.
 /// This contract is stateless and does not hold assets between Zaps; leftover tokens can be claimed by anyone.
-/// Ensure Zaps fully utilize tokens or revert to prevent fund loss.
+/// Ensure that Zaps fully utilize tokens or revert to prevent the loss of funds.
 contract TokenZapV1 is IZapRecipient {
     using SafeERC20 for IERC20;
     using ZapDataV1 for bytes;
@@ -29,12 +29,12 @@ contract TokenZapV1 is IZapRecipient {
     error TokenZapV1__PayloadLengthAboveMax();
 
     /// @notice Allows the contract to receive ETH.
-    /// @dev Leftover ETH can be claimed by anyone, make sure to spend the full balance during the Zaps.
+    /// @dev Leftover ETH can be claimed by anyone. Ensure the full balance is spent during Zaps.
     receive() external payable {}
 
-    /// @notice Performs a Zap action using the specified token and amount. This amount must be previously
-    /// transferred to this contract (could also be supplied as msg.value if the token is native gas token).
-    /// Zap action will be performed forwarding full `msg.value` for ERC20s or `amount` for native gas token.
+    /// @notice Performs a Zap action using the specified token and amount. This amount must have previously been
+    /// transferred to this contract (could also be supplied as msg.value if the token is a native gas token).
+    /// Zap action will be performed forwarding full `msg.value` for ERC20s or `amount` for native gas tokens.
     /// Note: all funds remaining after the Zap action is performed can be claimed by anyone.
     /// Make sure to spend the full balance during the Zaps and avoid sending extra funds if a single Zap is performed.
     /// @dev The provided ZapData contains the target address and calldata for the Zap action, and must be
@@ -48,14 +48,14 @@ contract TokenZapV1 is IZapRecipient {
         zapData.validateV1();
         address target = zapData.target();
         uint256 msgValue = msg.value;
-        // Note: we don't check the amount that was transferred to TokenZapV1 (or msg.value for native gas tokens),
-        // so transfering more than `amount` will lead to remaining funds in TokenZapV1, which can be claimed by anyone.
-        // Make sure to send the exact amount for a single Zap or spend the full balance for multiple `zap()` calls.
+        // Note: we don't check the amount that was transferred to TokenZapV1 (or msg.value for native gas tokens).
+        // Transferring more than `amount` will lead to remaining funds in TokenZapV1, which can be claimed by anyone.
+        // Ensure that you send the exact amount for a single Zap or spend the full balance for multiple `zap()` calls.
         if (token == NATIVE_GAS_TOKEN) {
-            // For native gas tokens we forward the requested amount to the target contract during the Zap action.
-            // Silimar to ERC20s, we allow to use pre-transferred native tokens for the Zap.
+            // For native gas tokens, we forward the requested amount to the target contract during the Zap action.
+            // Similar to ERC20s, we allow using pre-transferred native tokens for the Zap.
             msgValue = amount;
-            // No approval needed since native token doesn't use allowances.
+            // No approval is needed since native tokens don't use allowances.
             // Note: balance check is performed within `Address.sendValue` or `Address.functionCallWithValue` below.
         } else {
             // For ERC20 tokens, grant unlimited approval to the target if the current allowance is insufficient.
@@ -70,11 +70,11 @@ contract TokenZapV1 is IZapRecipient {
         bytes memory payload = zapData.payload(amount);
         if (payload.length == 0) {
             // No payload provided, perform the native gas token transfer to the target.
-            // Note: we avoid using `functionCallWithValue` because target might be an EOA. This will
+            // Note: we avoid using `functionCallWithValue` because the target might be an EOA. This will
             // revert with a generic custom error should the target contract revert on incoming transfer.
             Address.sendValue({recipient: payable(target), amount: msgValue});
         } else {
-            // Perform the Zap action, forwarding requested native value to the target contract.
+            // Perform the Zap action, forwarding the requested native value to the target contract.
             // Note: this will bubble up any revert from the target contract.
             Address.functionCallWithValue({target: target, data: payload, value: msgValue});
         }

From 96cd2cde0a02784ae74f3d30e3e25578a7328fe8 Mon Sep 17 00:00:00 2001
From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com>
Date: Thu, 28 Nov 2024 14:00:23 +0000
Subject: [PATCH 09/12] test: more revert cases around empty target / payload

---
 .../contracts-rfq/test/zaps/TokenZapV1.t.sol  | 71 +++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/packages/contracts-rfq/test/zaps/TokenZapV1.t.sol b/packages/contracts-rfq/test/zaps/TokenZapV1.t.sol
index 94cf0d9a7f..68afd42331 100644
--- a/packages/contracts-rfq/test/zaps/TokenZapV1.t.sol
+++ b/packages/contracts-rfq/test/zaps/TokenZapV1.t.sol
@@ -373,6 +373,11 @@ contract TokenZapV1Test is Test {
 
     // ══════════════════════════════════════════════════ REVERTS ══════════════════════════════════════════════════════
 
+    function getZeroTargetZapData(bytes memory payload, uint16 amountPosition) public pure returns (bytes memory) {
+        // Encode manually as the library checks for zero address
+        return abi.encodePacked(ZapDataV1.VERSION, amountPosition, address(0), payload);
+    }
+
     function test_zap_erc20_revert_notEnoughTokens() public {
         bytes memory zapData = getZapData(getVaultPayload(address(erc20), 0));
         // Transfer tokens to the zap contract first, but not enough
@@ -389,6 +394,43 @@ contract TokenZapV1Test is Test {
         tokenZap.zap(address(erc20), AMOUNT, zapData);
     }
 
+    function test_zap_erc20_revert_targetZeroAddress_emptyPayload() public {
+        bytes memory zapData = getZeroTargetZapData({payload: "", amountPosition: 0});
+        // Transfer tokens to the zap contract first
+        erc20.transfer(address(tokenZap), AMOUNT);
+        vm.expectRevert(TokenZapV1.TokenZapV1__TargetZeroAddress.selector);
+        tokenZap.zap(address(erc20), AMOUNT, zapData);
+    }
+
+    function test_zap_erc20_revert_targetZeroAddress_nonEmptyPayload() public {
+        bytes memory zapData =
+            getZeroTargetZapData({payload: getVaultPayload(address(erc20), 0), amountPosition: 4 + 32 * 2});
+        // Transfer tokens to the zap contract first
+        erc20.transfer(address(tokenZap), AMOUNT);
+        vm.expectRevert(TokenZapV1.TokenZapV1__TargetZeroAddress.selector);
+        tokenZap.zap(address(erc20), AMOUNT, zapData);
+    }
+
+    function test_zap_erc20_revert_targetEOA_nonEmptyPayload() public {
+        bytes memory zapData = tokenZap.encodeZapData({
+            target: user,
+            payload: getVaultPayload(address(erc20), 0),
+            amountPosition: 4 + 32 * 2
+        });
+        // Transfer tokens to the zap contract first
+        erc20.transfer(address(tokenZap), AMOUNT);
+        vm.expectRevert(abi.encodeWithSelector(Address.AddressEmptyCode.selector, user));
+        tokenZap.zap(address(erc20), AMOUNT, zapData);
+    }
+
+    function test_zap_erc20_revert_targetEOA_emptyPayload() public {
+        bytes memory zapData = tokenZap.encodeZapData({target: user, payload: "", amountPosition: 0});
+        // Transfer tokens to the zap contract first
+        erc20.transfer(address(tokenZap), AMOUNT);
+        vm.expectRevert(abi.encodeWithSelector(Address.AddressEmptyCode.selector, user));
+        tokenZap.zap(address(erc20), AMOUNT, zapData);
+    }
+
     function test_zap_native_revert_targetReverted() public {
         bytes memory zapData = getZapData(getVaultPayloadWithRevert());
         vm.expectRevert(VaultManyArguments.VaultManyArguments__SomeError.selector);
@@ -412,6 +454,35 @@ contract TokenZapV1Test is Test {
         tokenZap.zap(address(weth), AMOUNT, zapDataTransfer);
     }
 
+    function test_zap_withdraw_transferNative_revert_targetZeroAddress_emptyPayload() public {
+        bytes memory zapDataWithdraw = getZapDataWithdraw(0);
+        bytes memory zapDataTransfer = getZeroTargetZapData({payload: "", amountPosition: 0});
+        weth.transfer(address(tokenZap), AMOUNT);
+        tokenZap.zap(address(weth), AMOUNT, zapDataWithdraw);
+        vm.expectRevert(TokenZapV1.TokenZapV1__TargetZeroAddress.selector);
+        tokenZap.zap(address(weth), AMOUNT, zapDataTransfer);
+    }
+
+    function test_zap_withdraw_transferNative_revert_targetZeroAddress_nonEmptyPayload() public {
+        bytes memory zapDataWithdraw = getZapDataWithdraw(0);
+        bytes memory payload = getVaultPayloadNoAmount();
+        bytes memory zapDataTransfer = getZeroTargetZapData({payload: payload, amountPosition: uint16(payload.length)});
+        weth.transfer(address(tokenZap), AMOUNT);
+        tokenZap.zap(address(weth), AMOUNT, zapDataWithdraw);
+        vm.expectRevert(TokenZapV1.TokenZapV1__TargetZeroAddress.selector);
+        tokenZap.zap(address(weth), AMOUNT, zapDataTransfer);
+    }
+
+    function test_zap_withdraw_transferNative_revert_targetEOA_nonEmptyPayload() public {
+        bytes memory zapDataWithdraw = getZapDataWithdraw(0);
+        bytes memory zapDataTransfer =
+            tokenZap.encodeZapData({target: user, payload: getVaultPayloadNoAmount(), amountPosition: 0});
+        weth.transfer(address(tokenZap), AMOUNT);
+        tokenZap.zap(address(weth), AMOUNT, zapDataWithdraw);
+        vm.expectRevert(abi.encodeWithSelector(Address.AddressEmptyCode.selector, user));
+        tokenZap.zap(address(weth), AMOUNT, zapDataTransfer);
+    }
+
     function test_encodeZapData_revert_payloadLengthAboveMax() public {
         bytes memory tooLongPayload = new bytes(2 ** 16);
         vm.expectRevert(TokenZapV1.TokenZapV1__PayloadLengthAboveMax.selector);

From 1308f54d0ece203a8da6c20e2ec1b5d484a66d10 Mon Sep 17 00:00:00 2001
From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com>
Date: Thu, 28 Nov 2024 14:01:17 +0000
Subject: [PATCH 10/12] fix: check for empty target

---
 packages/contracts-rfq/contracts/zaps/TokenZapV1.sol | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol b/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
index 71f04fb62a..142524a719 100644
--- a/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
+++ b/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
@@ -27,6 +27,7 @@ contract TokenZapV1 is IZapRecipient {
     address public constant NATIVE_GAS_TOKEN = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;
 
     error TokenZapV1__PayloadLengthAboveMax();
+    error TokenZapV1__TargetZeroAddress();
 
     /// @notice Allows the contract to receive ETH.
     /// @dev Leftover ETH can be claimed by anyone. Ensure the full balance is spent during Zaps.
@@ -47,10 +48,11 @@ contract TokenZapV1 is IZapRecipient {
         // Validate the ZapData format and extract the target address.
         zapData.validateV1();
         address target = zapData.target();
-        uint256 msgValue = msg.value;
+        if (target == address(0)) revert TokenZapV1__TargetZeroAddress();
         // Note: we don't check the amount that was transferred to TokenZapV1 (or msg.value for native gas tokens).
         // Transferring more than `amount` will lead to remaining funds in TokenZapV1, which can be claimed by anyone.
         // Ensure that you send the exact amount for a single Zap or spend the full balance for multiple `zap()` calls.
+        uint256 msgValue = msg.value;
         if (token == NATIVE_GAS_TOKEN) {
             // For native gas tokens, we forward the requested amount to the target contract during the Zap action.
             // Similar to ERC20s, we allow using pre-transferred native tokens for the Zap.

From ec92025d4b29ae6a9caf29b5dfe300a42f576893 Mon Sep 17 00:00:00 2001
From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com>
Date: Thu, 28 Nov 2024 14:02:20 +0000
Subject: [PATCH 11/12] fix: separate the pure native gas token transfer case

---
 packages/contracts-rfq/contracts/zaps/TokenZapV1.sol | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol b/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
index 142524a719..bd58d8f391 100644
--- a/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
+++ b/packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
@@ -39,7 +39,8 @@ contract TokenZapV1 is IZapRecipient {
     /// Note: all funds remaining after the Zap action is performed can be claimed by anyone.
     /// Make sure to spend the full balance during the Zaps and avoid sending extra funds if a single Zap is performed.
     /// @dev The provided ZapData contains the target address and calldata for the Zap action, and must be
-    /// encoded using the encodeZapData function.
+    /// encoded using the encodeZapData function. Native gas token transfers could be done by using empty `payload`,
+    /// this is the only case where target could be an EOA.
     /// @param token        Address of the token to be used for the Zap action.
     /// @param amount       Amount of the token to be used for the Zap action.
     /// @param zapData      Encoded Zap Data containing the target address and calldata for the Zap action.
@@ -70,14 +71,14 @@ contract TokenZapV1 is IZapRecipient {
         // Construct the payload for the target contract call with the Zap action.
         // The payload is modified to replace the placeholder amount with the actual amount.
         bytes memory payload = zapData.payload(amount);
-        if (payload.length == 0) {
-            // No payload provided, perform the native gas token transfer to the target.
+        if (payload.length == 0 && token == NATIVE_GAS_TOKEN) {
+            // Zap Action in a form of native gas token transfer to the target is requested.
             // Note: we avoid using `functionCallWithValue` because the target might be an EOA. This will
             // revert with a generic custom error should the target contract revert on incoming transfer.
             Address.sendValue({recipient: payable(target), amount: msgValue});
         } else {
             // Perform the Zap action, forwarding the requested native value to the target contract.
-            // Note: this will bubble up any revert from the target contract.
+            // Note: this will bubble up any revert from the target contract, and revert if target is EOA.
             Address.functionCallWithValue({target: target, data: payload, value: msgValue});
         }
         // Return function selector to indicate successful execution

From 2305d939b12c383a52d61976d44e88a7e1d73021 Mon Sep 17 00:00:00 2001
From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com>
Date: Thu, 28 Nov 2024 14:03:23 +0000
Subject: [PATCH 12/12] chore: silence solhint warning in the test mock

---
 packages/contracts-rfq/test/mocks/WETHMock.sol | 1 +
 1 file changed, 1 insertion(+)

diff --git a/packages/contracts-rfq/test/mocks/WETHMock.sol b/packages/contracts-rfq/test/mocks/WETHMock.sol
index 13dcc4c904..75b5c4d5dd 100644
--- a/packages/contracts-rfq/test/mocks/WETHMock.sol
+++ b/packages/contracts-rfq/test/mocks/WETHMock.sol
@@ -4,6 +4,7 @@ pragma solidity ^0.8.20;
 import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
 import {Address} from "@openzeppelin/contracts/utils/Address.sol";
 
+// solhint-disable no-empty-blocks
 /// @notice WETH mock for testing purposes. DO NOT USE IN PRODUCTION.
 contract WETHMock is ERC20 {
     constructor() ERC20("Mock Wrapped Ether", "Mock WETH") {}