Fix: FastBridgeV2 (Zellic - 01)

packages/contracts-rfq/contracts/FastBridgeV2.sol

@@ -188,16 +188,14 @@ contract FastBridgeV2 is AdminV2, MulticallTarget, IFastBridgeV2, IFastBridgeV2E
         }
         _validateBridgeParams(params, paramsV2, exclusivityEndTime);
 
-        // Transfer tokens to bridge contract. We use the actual transferred amount in case of transfer fees.
-        uint256 originAmount = _takeBridgedUserAsset(params.originToken, params.originAmount);
-
         // Track the amount of origin token owed to protocol.
-        uint256 originFeeAmount = 0;
+        uint256 originAmount = params.originAmount;
+        uint256 protocolFeeAmount = 0;
         if (protocolFeeRate > 0) {
-            originFeeAmount = (originAmount * protocolFeeRate) / FEE_BPS;
+            protocolFeeAmount = (originAmount * protocolFeeRate) / FEE_BPS;
             // The Relayer filling this request will be paid the originAmount after fees.
             // Note: the protocol fees will be accumulated only when the Relayer claims the origin collateral.
-            originAmount -= originFeeAmount;
+            originAmount -= protocolFeeAmount;
         }
 
         // Hash the bridge request and set the initial status to REQUESTED.
@@ -211,7 +209,7 @@ contract FastBridgeV2 is AdminV2, MulticallTarget, IFastBridgeV2, IFastBridgeV2E
                 destToken: params.destToken,
                 originAmount: originAmount,
                 destAmount: params.destAmount,
-                originFeeAmount: originFeeAmount,
+                originFeeAmount: protocolFeeAmount,
                 deadline: params.deadline,
                 // Increment the sender's nonce on every bridge.
                 nonce: senderNonces[params.sender]++,
@@ -227,6 +225,7 @@ contract FastBridgeV2 is AdminV2, MulticallTarget, IFastBridgeV2, IFastBridgeV2E
         bridgeTxDetails[transactionId].status = BridgeStatus.REQUESTED;
         bridgeTxDetails[transactionId].destChainId = params.dstChainId;
 
+        // Emit the events before any external calls.
         emit BridgeRequested({
             transactionId: transactionId,
             sender: params.sender,
@@ -239,6 +238,18 @@ contract FastBridgeV2 is AdminV2, MulticallTarget, IFastBridgeV2, IFastBridgeV2E
             sendChainGas: paramsV2.zapNative != 0
         });
         emit BridgeQuoteDetails(transactionId, paramsV2.quoteId);
+
+        // Transfer the tokens from the user as the last transaction action.
+        address originToken = params.originToken;
+        if (originToken != NATIVE_GAS_TOKEN) {
+            // We need to take the full origin amount from the provided params (that includes `protocolFeeAmount`).
+            uint256 amountToTake = params.originAmount;
+            uint256 balanceBefore = IERC20(originToken).balanceOf(address(this));
+            IERC20(originToken).safeTransferFrom(msg.sender, address(this), amountToTake);
+            uint256 balanceAfter = IERC20(originToken).balanceOf(address(this));
+            // Tokens with fees on transfer (or transferring more than requested) are not supported.
+            if (balanceAfter != balanceBefore + amountToTake) revert AmountIncorrect();
+        }
     }
 
     /// @inheritdoc IFastBridgeV2
@@ -396,8 +407,8 @@ contract FastBridgeV2 is AdminV2, MulticallTarget, IFastBridgeV2, IFastBridgeV2E
         // Accumulate protocol fees if origin fee amount exists.
         address token = request.originToken();
         uint256 amount = request.originAmount();
-        uint256 originFeeAmount = request.originFeeAmount();
-        if (originFeeAmount > 0) protocolFees[token] += originFeeAmount;
+        uint256 protocolFeeAmount = request.originFeeAmount();
+        if (protocolFeeAmount > 0) protocolFees[token] += protocolFeeAmount;
 
         // Emit the event before any external calls.
         emit BridgeDepositClaimed(transactionId, proofRelayer, to, token, amount);
@@ -432,29 +443,6 @@ contract FastBridgeV2 is AdminV2, MulticallTarget, IFastBridgeV2, IFastBridgeV2E
 
     // ═════════════════════════════════════════════ INTERNAL METHODS ══════════════════════════════════════════════════
 
-    /// @notice Takes the bridged asset from the user into FastBridgeV2 custody. The asset will later be
-    /// claimed by the relayer who completed the relay on the destination chain, or returned to the user
-    /// via the cancel function if no relay is completed.
-    function _takeBridgedUserAsset(address token, uint256 amount) internal returns (uint256 amountTaken) {
-        if (token == NATIVE_GAS_TOKEN) {
-            // For the native gas token, we just need to check that the supplied msg.value is correct.
-            // Supplied `msg.value` is already in FastBridgeV2 custody.
-            if (amount != msg.value) revert MsgValueIncorrect();
-            amountTaken = msg.value;
-        } else {
-            // For ERC20s, token is explicitly transferred from the user to FastBridgeV2.
-            // We don't allow non-zero `msg.value` to avoid extra funds from being stuck in FastBridgeV2.
-            if (msg.value != 0) revert MsgValueIncorrect();
-            // Throw an explicit error if the provided token address is not a contract.
-            if (token.code.length == 0) revert TokenNotContract();
-
-            // Use the balance difference as the amount taken in case of fee on transfer tokens.
-            amountTaken = IERC20(token).balanceOf(address(this));
-            IERC20(token).safeTransferFrom(msg.sender, address(this), amount);
-            amountTaken = IERC20(token).balanceOf(address(this)) - amountTaken;
-        }
-    }
-
     /// @notice Calls the recipient's hook function with the specified zapData and validates
     /// the returned value.
     function _triggerZapWithChecks(address recipient, address token, uint256 amount, bytes calldata zapData) internal {
@@ -500,11 +488,13 @@ contract FastBridgeV2 is AdminV2, MulticallTarget, IFastBridgeV2, IFastBridgeV2E
         internal
         view
     {
+        address originToken = params.originToken;
+        uint256 originAmount = params.originAmount;
         // Check V1 (legacy) params.
         if (params.dstChainId == block.chainid) revert ChainIncorrect();
-        if (params.originAmount == 0 || params.destAmount == 0) revert AmountIncorrect();
+        if (originAmount == 0 || params.destAmount == 0) revert AmountIncorrect();
         if (params.sender == address(0) || params.to == address(0)) revert ZeroAddress();
-        if (params.originToken == address(0) || params.destToken == address(0)) revert ZeroAddress();
+        if (originToken == address(0) || params.destToken == address(0)) revert ZeroAddress();
         if (params.deadline < block.timestamp + MIN_DEADLINE_PERIOD) revert DeadlineTooShort();
 
         // Check V2 params.
@@ -517,6 +507,17 @@ contract FastBridgeV2 is AdminV2, MulticallTarget, IFastBridgeV2, IFastBridgeV2E
         if (exclusivityEndTime < 0 || exclusivityEndTime > int256(params.deadline)) {
             revert ExclusivityParamsIncorrect();
         }
+
+        // Check supplied msg.value.
+        if (originToken == NATIVE_GAS_TOKEN) {
+            // For the native gas token, we just need to check that the supplied msg.value is correct.
+            if (msg.value != originAmount) revert MsgValueIncorrect();
+        } else {
+            // We don't allow non-zero `msg.value` to avoid extra funds from being stuck in FastBridgeV2.
+            if (msg.value != 0) revert MsgValueIncorrect();
+            // Throw an explicit error if the provided token address is not a contract.
+            if (originToken.code.length == 0) revert TokenNotContract();
+        }
     }
 
     /// @notice Validates all parameters required for a relay transaction.

packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol

@@ -53,6 +53,10 @@ abstract contract FastBridgeV2SrcBaseTest is FastBridgeV2Test {
         srcToken.approve(address(fastBridge), type(uint256).max);
         vm.prank(userB);
         srcToken.approve(address(fastBridge), type(uint256).max);
+        // Weird token
+        weirdToken.mint(address(userA), LEFTOVER_BALANCE + tokenParams.originAmount);
+        vm.prank(userA);
+        weirdToken.approve(address(fastBridge), type(uint256).max);
     }
 
     // ══════════════════════════════════════════════════ HELPERS ══════════════════════════════════════════════════════

packages/contracts-rfq/test/FastBridgeV2.Src.FBRV2.t.sol

@@ -19,6 +19,9 @@ contract FastBridgeV2SrcFBRouterV2Test is FastBridgeV2SrcTest {
         srcToken.approve(router, type(uint256).max);
         vm.prank(userB);
         srcToken.approve(router, type(uint256).max);
+        vm.prank(userA);
+        weirdToken.approve(router, type(uint256).max);
+        weirdToken.mint(router, 1);
     }
 
     /// @notice Override bridge function to leverage the old FastBridgeRouterV2 contract

packages/contracts-rfq/test/FastBridgeV2.Src.t.sol

@@ -294,6 +294,20 @@ contract FastBridgeV2SrcTest is FastBridgeV2SrcBaseTest {
         bridge({caller: userA, msgValue: 0, params: tokenParams});
     }
 
+    function test_bridge_revert_weirdToken_amountHigher() public {
+        tokenParams.originToken = address(weirdToken);
+        weirdToken.setTransferToFastBridgeValue(tokenParams.originAmount + 1);
+        vm.expectRevert(AmountIncorrect.selector);
+        bridge({caller: userA, msgValue: 0, params: tokenParams});
+    }
+
+    function test_bridge_revert_weirdToken_amountLower() public {
+        tokenParams.originToken = address(weirdToken);
+        weirdToken.setTransferToFastBridgeValue(tokenParams.originAmount - 1);
+        vm.expectRevert(AmountIncorrect.selector);
+        bridge({caller: userA, msgValue: 0, params: tokenParams});
+    }
+
     // ═══════════════════════════════════════════════════ PROVE ═══════════════════════════════════════════════════════
 
     function checkStatusAndProofAfterProve(bytes32 txId, uint16 expectedProverID, address relayer) public view {

packages/contracts-rfq/test/FastBridgeV2.t.sol

@@ -13,6 +13,7 @@ import {IAdminV2Errors} from "../contracts/interfaces/IAdminV2Errors.sol";
 import {IFastBridgeV2Errors} from "../contracts/interfaces/IFastBridgeV2Errors.sol";
 
 import {MockERC20} from "./mocks/MockERC20.sol";
+import {WeirdERC20Mock} from "./mocks/WeirdERC20Mock.sol";
 
 import {IAccessControl} from "@openzeppelin/contracts/access/IAccessControl.sol";
 import {Test} from "forge-std/Test.sol";
@@ -30,6 +31,7 @@ abstract contract FastBridgeV2Test is Test, IAdminV2Errors, IFastBridgeV2Errors
     FastBridgeV2 public fastBridge;
     MockERC20 public srcToken;
     MockERC20 public dstToken;
+    WeirdERC20Mock public weirdToken;
 
     address public relayerA = makeAddr("Relayer A");
     address public relayerB = makeAddr("Relayer B");
@@ -76,6 +78,7 @@ abstract contract FastBridgeV2Test is Test, IAdminV2Errors, IFastBridgeV2Errors
     function setUp() public virtual {
         srcToken = new MockERC20("SrcToken", 6);
         dstToken = new MockERC20("DstToken", 6);
+        weirdToken = new WeirdERC20Mock("WeirdToken", 6);
         createFixtures();
         mockRequestV1 = abi.encode(extractV1(tokenTx));
         // Invalid V2 request is formed before `createFixturesV2` to ensure it's not using zapData
@@ -84,6 +87,7 @@ abstract contract FastBridgeV2Test is Test, IAdminV2Errors, IFastBridgeV2Errors
         // Mock V3 request is formed after `createFixturesV2` to ensure it's using zapData if needed
         mockRequestV3 = createMockRequestV3(BridgeTransactionV2Lib.encodeV2(ethTx));
         fastBridge = deployFastBridge();
+        weirdToken.setFastBridge(address(fastBridge));
         configureFastBridge();
         mintTokens();
     }

packages/contracts-rfq/test/mocks/WeirdERC20Mock.sol

@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.17;
+
+import {MockERC20} from "./MockERC20.sol";
+
+// solhint-disable no-empty-blocks
+contract WeirdERC20Mock is MockERC20 {
+    uint256 public transferToFastBridgeValue;
+    address public fastBridge;
+
+    constructor(string memory name_, uint8 decimals_) MockERC20(name_, decimals_) {}
+
+    /// @notice We include an empty "test" function so that this contract does not appear in the coverage report.
+    function testWeirdERC20Mock() external {}
+
+    function setFastBridge(address fastBridge_) public {
+        fastBridge = fastBridge_;
+    }
+
+    function setTransferToFastBridgeValue(uint256 value) public {
+        transferToFastBridgeValue = value;
+    }
+
+    function transfer(address to, uint256 value) public virtual override returns (bool) {
+        if (to == fastBridge) {
+            return super.transfer(to, transferToFastBridgeValue);
+        } else {
+            return super.transfer(to, value);
+        }
+    }
+
+    function transferFrom(address from, address to, uint256 value) public virtual override returns (bool) {
+        if (to == fastBridge) {
+            return super.transferFrom(from, to, transferToFastBridgeValue);
+        } else {
+            return super.transferFrom(from, to, value);
+        }
+    }
+}