diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index 706859c113..4ad0edaabc 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/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. diff --git a/packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol index 969fbc3220..57f708f9e9 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol +++ b/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 ══════════════════════════════════════════════════════ diff --git a/packages/contracts-rfq/test/FastBridgeV2.Src.FBRV2.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Src.FBRV2.t.sol index 4ed743181b..d8d38d2198 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Src.FBRV2.t.sol +++ b/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 diff --git a/packages/contracts-rfq/test/FastBridgeV2.Src.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Src.t.sol index 14f7cb184c..0ad963895e 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Src.t.sol +++ b/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 { diff --git a/packages/contracts-rfq/test/FastBridgeV2.t.sol b/packages/contracts-rfq/test/FastBridgeV2.t.sol index d4fab0a1ae..167e3196d2 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.t.sol +++ b/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(); } diff --git a/packages/contracts-rfq/test/mocks/WeirdERC20Mock.sol b/packages/contracts-rfq/test/mocks/WeirdERC20Mock.sol new file mode 100644 index 0000000000..f2fdac9f78 --- /dev/null +++ b/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); + } + } +}