From 36a42bdbcf599a2d712008d6eb50ebcc4acb90b0 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Wed, 2 Oct 2024 15:32:52 +0100 Subject: [PATCH 01/21] feat: add `callValue` to bridge params --- packages/contracts-rfq/contracts/FastBridgeV2.sol | 1 + packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol | 3 +++ packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol | 2 ++ packages/contracts-rfq/test/FastBridgeV2.t.sol | 2 ++ 4 files changed, 8 insertions(+) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index 1a64515a33..8d1e604a44 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -54,6 +54,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { quoteRelayer: address(0), quoteExclusivitySeconds: 0, quoteId: bytes(""), + callValue: 0, callParams: bytes("") }) }); diff --git a/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol b/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol index c4c3751a5b..e11b4cd505 100644 --- a/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol @@ -30,14 +30,17 @@ interface IFastBridgeV2 is IFastBridge { /// for backwards compatibility. /// Note: quoteRelayer and quoteExclusivitySeconds are either both zero (indicating no exclusivity) /// or both non-zero (indicating exclusivity for the given period). + /// Note: callValue > 0 could NOT be used with destToken = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE (ETH_ADDRESS) /// @param quoteRelayer Relayer that provided the quote for the transaction /// @param quoteExclusivitySeconds Period of time the quote relayer is guaranteed exclusivity after user's deposit /// @param quoteId Unique quote identifier used for tracking the quote + /// @param callValue ETH value to send to the recipient (if any) /// @param callParams Parameters for the arbitrary call to the destination recipient (if any) struct BridgeParamsV2 { address quoteRelayer; int256 quoteExclusivitySeconds; bytes quoteId; + uint256 callValue; bytes callParams; } diff --git a/packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol index a616872f93..7926b3d362 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol @@ -12,12 +12,14 @@ contract FastBridgeV2DstExclusivityTest is FastBridgeV2DstTest { quoteRelayer: relayerA, quoteExclusivitySeconds: int256(EXCLUSIVITY_PERIOD), quoteId: "", + callValue: 0, callParams: "" }); ethParamsV2 = IFastBridgeV2.BridgeParamsV2({ quoteRelayer: relayerB, quoteExclusivitySeconds: int256(EXCLUSIVITY_PERIOD), quoteId: "", + callValue: 0, callParams: "" }); diff --git a/packages/contracts-rfq/test/FastBridgeV2.t.sol b/packages/contracts-rfq/test/FastBridgeV2.t.sol index 8011f8a8e2..af77ec05d8 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.t.sol @@ -128,12 +128,14 @@ abstract contract FastBridgeV2Test is Test, IFastBridgeV2Errors { quoteRelayer: address(0), quoteExclusivitySeconds: 0, quoteId: bytes(""), + callValue: 0, callParams: bytes("") }); ethParamsV2 = IFastBridgeV2.BridgeParamsV2({ quoteRelayer: address(0), quoteExclusivitySeconds: 0, quoteId: bytes(""), + callValue: 0, callParams: bytes("") }); From 919a0a94277e0af1b8ce9bf45a9e8d03a3735658 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Wed, 2 Oct 2024 16:07:50 +0100 Subject: [PATCH 02/21] feat: add `callValue` to bridge tx V2 --- packages/contracts-rfq/contracts/FastBridgeV2.sol | 2 +- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol | 2 +- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol | 2 +- packages/contracts-rfq/test/FastBridgeV2.t.sol | 2 -- 4 files changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index 8d1e604a44..20ef419645 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -173,7 +173,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { originAmount: originAmount, destAmount: params.destAmount, originFeeAmount: originFeeAmount, - sendChainGas: params.sendChainGas, + callValue: 0, // TODO: fix deadline: params.deadline, nonce: senderNonces[params.sender]++, // increment nonce on every bridge exclusivityRelayer: paramsV2.quoteRelayer, diff --git a/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol b/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol index e11b4cd505..a1485d3f31 100644 --- a/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol @@ -57,7 +57,7 @@ interface IFastBridgeV2 is IFastBridge { uint256 originAmount; // amount in on origin bridge less originFeeAmount uint256 destAmount; uint256 originFeeAmount; - bool sendChainGas; + uint256 callValue; // ETH value to send to the recipient (if any) - replaces V1's sendChainGas flag uint256 deadline; // user specified deadline for destination relay uint256 nonce; address exclusivityRelayer; diff --git a/packages/contracts-rfq/test/FastBridgeV2.Src.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Src.t.sol index c2a5902133..1b55ad4ef9 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Src.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Src.t.sol @@ -42,7 +42,7 @@ contract FastBridgeV2SrcTest is FastBridgeV2SrcBaseTest { destToken: bridgeTx.destToken, originAmount: bridgeTx.originAmount, destAmount: bridgeTx.destAmount, - sendChainGas: bridgeTx.sendChainGas + sendChainGas: bridgeTx.callValue > 0 }); } diff --git a/packages/contracts-rfq/test/FastBridgeV2.t.sol b/packages/contracts-rfq/test/FastBridgeV2.t.sol index af77ec05d8..9ac073146f 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.t.sol @@ -160,7 +160,6 @@ abstract contract FastBridgeV2Test is Test, IFastBridgeV2Errors { txV2.originAmount = txV1.originAmount; txV2.destAmount = txV1.destAmount; txV2.originFeeAmount = txV1.originFeeAmount; - txV2.sendChainGas = txV1.sendChainGas; txV2.deadline = txV1.deadline; txV2.nonce = txV1.nonce; } @@ -207,7 +206,6 @@ abstract contract FastBridgeV2Test is Test, IFastBridgeV2Errors { txV1.originAmount = txV2.originAmount; txV1.destAmount = txV2.destAmount; txV1.originFeeAmount = txV2.originFeeAmount; - txV1.sendChainGas = txV2.sendChainGas; txV1.deadline = txV2.deadline; txV1.nonce = txV2.nonce; } From 867d5d9ca8e953fd5acd85a1368fa7c3ab78628c Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Thu, 3 Oct 2024 12:25:49 +0100 Subject: [PATCH 03/21] test: add cases for SRC calls with `callValue` --- .../interfaces/IFastBridgeV2Errors.sol | 1 + .../test/FastBridgeV2.Src.ArbitraryCall.t.sol | 53 +++++++++++++++++++ .../contracts-rfq/test/FastBridgeV2.t.sol | 10 ++++ 3 files changed, 64 insertions(+) diff --git a/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol b/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol index 7cc1423a84..f40b760c30 100644 --- a/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol +++ b/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol @@ -7,6 +7,7 @@ interface IFastBridgeV2Errors { error ChainIncorrect(); error ExclusivityParamsIncorrect(); error MsgValueIncorrect(); + error NativeTokenCallValueNotSupported(); error SenderIncorrect(); error StatusIncorrect(); error ZeroAddress(); diff --git a/packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol index 2b1e83a30c..5b6e84b732 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol @@ -6,6 +6,7 @@ import {FastBridgeV2SrcExclusivityTest} from "./FastBridgeV2.Src.Exclusivity.t.s // solhint-disable func-name-mixedcase, ordering contract FastBridgeV2SrcArbitraryCallTest is FastBridgeV2SrcExclusivityTest { bytes public constant CALL_PARAMS = abi.encode("Hello, World!"); + uint256 public constant CALL_VALUE = 1_337_420; function createFixturesV2() public virtual override { super.createFixturesV2(); @@ -41,4 +42,56 @@ contract FastBridgeV2SrcArbitraryCallTest is FastBridgeV2SrcExclusivityTest { vm.expectRevert(CallParamsLengthAboveMax.selector); bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams, paramsV2: ethParamsV2}); } + + // ══════════════════════════════════════ WITH CALL VALUE, NO CALL PARAMS ══════════════════════════════════════════ + + function test_bridge_token_withCallValue_noCallParams() public { + setTokenTestCallParams(""); + setTokenTestCallValue(CALL_VALUE); + test_bridge_token(); + } + + function test_bridge_token_diffSender_withCallValue_noCallParams() public { + setTokenTestCallParams(""); + setTokenTestCallValue(CALL_VALUE); + test_bridge_token_diffSender(); + } + + function test_bridge_eth_withCallValue_noCallParams_revert() public { + setEthTestCallParams(""); + setEthTestCallValue(CALL_VALUE); + vm.expectRevert(NativeTokenCallValueNotSupported.selector); + bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams, paramsV2: ethParamsV2}); + } + + function test_bridge_eth_diffSender_withCallValue_noCallParams_revert() public { + setEthTestCallParams(""); + setEthTestCallValue(CALL_VALUE); + vm.expectRevert(NativeTokenCallValueNotSupported.selector); + bridge({caller: userB, msgValue: ethParams.originAmount, params: ethParams, paramsV2: ethParamsV2}); + } + + // ═══════════════════════════════════════ WITH CALL VALUE & CALL PARAMS ═══════════════════════════════════════════ + + function test_bridge_token_withCallValue_withCallParams() public { + setTokenTestCallValue(CALL_VALUE); + test_bridge_token(); + } + + function test_bridge_token_diffSender_withCallValue_withCallParams() public { + setTokenTestCallValue(CALL_VALUE); + test_bridge_token_diffSender(); + } + + function test_bridge_eth_withCallValue_withCallParams_revert() public { + setEthTestCallValue(CALL_VALUE); + vm.expectRevert(NativeTokenCallValueNotSupported.selector); + bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams, paramsV2: ethParamsV2}); + } + + function test_bridge_eth_diffSender_withCallValue_withCallParams_revert() public { + setEthTestCallValue(CALL_VALUE); + vm.expectRevert(NativeTokenCallValueNotSupported.selector); + bridge({caller: userB, msgValue: ethParams.originAmount, params: ethParams, paramsV2: ethParamsV2}); + } } diff --git a/packages/contracts-rfq/test/FastBridgeV2.t.sol b/packages/contracts-rfq/test/FastBridgeV2.t.sol index 9ac073146f..2b9bce809f 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.t.sol @@ -169,6 +169,11 @@ abstract contract FastBridgeV2Test is Test, IFastBridgeV2Errors { tokenTx.callParams = callParams; } + function setTokenTestCallValue(uint256 callValue) public { + tokenParamsV2.callValue = callValue; + tokenTx.callValue = callValue; + } + function setTokenTestExclusivityParams(address relayer, uint256 exclusivitySeconds) public { tokenParamsV2.quoteRelayer = relayer; tokenParamsV2.quoteExclusivitySeconds = int256(exclusivitySeconds); @@ -183,6 +188,11 @@ abstract contract FastBridgeV2Test is Test, IFastBridgeV2Errors { ethTx.callParams = callParams; } + function setEthTestCallValue(uint256 callValue) public { + ethParamsV2.callValue = callValue; + ethTx.callValue = callValue; + } + function setEthTestExclusivityParams(address relayer, uint256 exclusivitySeconds) public { ethParamsV2.quoteRelayer = relayer; ethParamsV2.quoteExclusivitySeconds = int256(exclusivitySeconds); From c93c403e77dc7af05a4b5885a825949a2b4f60ba Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Thu, 3 Oct 2024 12:49:27 +0100 Subject: [PATCH 04/21] refactor: simplify tests further --- .../test/FastBridgeV2.Dst.Exclusivity.t.sol | 23 ++----------- .../contracts-rfq/test/FastBridgeV2.Dst.t.sol | 32 ++++++++++--------- 2 files changed, 20 insertions(+), 35 deletions(-) diff --git a/packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol index 7926b3d362..ba66ae53c4 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol @@ -1,32 +1,15 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.20; -import {FastBridgeV2DstTest, IFastBridgeV2} from "./FastBridgeV2.Dst.t.sol"; +import {FastBridgeV2DstTest} from "./FastBridgeV2.Dst.t.sol"; // solhint-disable func-name-mixedcase, ordering contract FastBridgeV2DstExclusivityTest is FastBridgeV2DstTest { uint256 public constant EXCLUSIVITY_PERIOD = 60 seconds; function createFixturesV2() public virtual override { - tokenParamsV2 = IFastBridgeV2.BridgeParamsV2({ - quoteRelayer: relayerA, - quoteExclusivitySeconds: int256(EXCLUSIVITY_PERIOD), - quoteId: "", - callValue: 0, - callParams: "" - }); - ethParamsV2 = IFastBridgeV2.BridgeParamsV2({ - quoteRelayer: relayerB, - quoteExclusivitySeconds: int256(EXCLUSIVITY_PERIOD), - quoteId: "", - callValue: 0, - callParams: "" - }); - - tokenTx.exclusivityRelayer = relayerA; - tokenTx.exclusivityEndTime = block.timestamp + EXCLUSIVITY_PERIOD; - ethTx.exclusivityRelayer = relayerB; - ethTx.exclusivityEndTime = block.timestamp + EXCLUSIVITY_PERIOD; + setTokenTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD); + setEthTestExclusivityParams(relayerB, EXCLUSIVITY_PERIOD); } // ═══════════════════════════════════════════════ RELAY: TOKEN ════════════════════════════════════════════════════ diff --git a/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol index 89ef62af91..1463efa2e0 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol @@ -89,15 +89,25 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { assertEq(relayer, expectedRelayer); } + function checkTokenBalances(address recipient, address relayCaller) public view { + assertEq(dstToken.balanceOf(recipient), tokenParams.destAmount); + assertEq(dstToken.balanceOf(relayCaller), LEFTOVER_BALANCE); + assertEq(dstToken.balanceOf(address(fastBridge)), 0); + } + + function checkEthBalances(address recipient, address relayCaller) public view { + assertEq(recipient.balance, ethParams.destAmount); + assertEq(relayCaller.balance, LEFTOVER_BALANCE); + assertEq(address(fastBridge).balance, 0); + } + /// @notice RelayerA completes the ERC20 bridge request function test_relay_token() public { bytes32 txId = getTxId(tokenTx); expectBridgeRelayed(tokenTx, txId, relayerA); relay({caller: relayerA, msgValue: 0, bridgeTx: tokenTx}); checkRelayedViews({txId: txId, expectedRelayer: relayerA}); - assertEq(dstToken.balanceOf(userB), tokenParams.destAmount); - assertEq(dstToken.balanceOf(relayerA), LEFTOVER_BALANCE); - assertEq(dstToken.balanceOf(address(fastBridge)), 0); + checkTokenBalances({recipient: userB, relayCaller: relayerA}); } /// @notice RelayerB completes the ERC20 bridge request, using relayerA's address @@ -106,9 +116,7 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { expectBridgeRelayed(tokenTx, txId, relayerA); relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: 0, bridgeTx: tokenTx}); checkRelayedViews({txId: txId, expectedRelayer: relayerA}); - assertEq(dstToken.balanceOf(userB), tokenParams.destAmount); - assertEq(dstToken.balanceOf(relayerB), LEFTOVER_BALANCE); - assertEq(dstToken.balanceOf(address(fastBridge)), 0); + checkTokenBalances({recipient: userB, relayCaller: relayerB}); } /// @notice RelayerB completes the ETH bridge request @@ -117,9 +125,7 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { expectBridgeRelayed(ethTx, txId, relayerB); relay({caller: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); checkRelayedViews({txId: txId, expectedRelayer: relayerB}); - assertEq(userB.balance, ethParams.destAmount); - assertEq(relayerB.balance, LEFTOVER_BALANCE); - assertEq(address(fastBridge).balance, 0); + checkEthBalances({recipient: userB, relayCaller: relayerB}); } /// @notice RelayerA completes the ETH bridge request, using relayerB's address @@ -128,9 +134,7 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { expectBridgeRelayed(ethTx, txId, relayerB); relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); checkRelayedViews({txId: txId, expectedRelayer: relayerB}); - assertEq(userB.balance, ethParams.destAmount); - assertEq(relayerA.balance, LEFTOVER_BALANCE); - assertEq(address(fastBridge).balance, 0); + checkEthBalances({recipient: userB, relayCaller: relayerA}); } /// @notice RelayerA completes the ETH bridge request, using relayerB's address @@ -146,9 +150,7 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { assertEq(recordedBlockNumber, 987_654_321); assertEq(recordedBlockTimestamp, 123_456_789); assertEq(recordedRelayer, relayerB); - assertEq(userB.balance, ethParams.destAmount); - assertEq(relayerA.balance, LEFTOVER_BALANCE); - assertEq(address(fastBridge).balance, 0); + checkEthBalances({recipient: userB, relayCaller: relayerA}); } // ═════════════════════════════════════ EXCESSIVE RETURN VALUE RECIPIENT ══════════════════════════════════════════ From 957524679f2d2f3c85068db55e42d1e489ceb3ef Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Thu, 3 Oct 2024 13:21:57 +0100 Subject: [PATCH 05/21] test: add cases for DST relays with `callValue` --- .../test/FastBridgeV2.Dst.ArbitraryCall.t.sol | 101 +++++++++- .../contracts-rfq/test/FastBridgeV2.Dst.t.sol | 175 +++++++++++++++++- 2 files changed, 274 insertions(+), 2 deletions(-) diff --git a/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol index af4f6235c6..d48420dba0 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol @@ -1,7 +1,8 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.20; -import {FastBridgeV2DstExclusivityTest, IFastBridgeV2} from "./FastBridgeV2.Dst.Exclusivity.t.sol"; +import {IFastBridgeV2} from "../contracts/interfaces/IFastBridgeV2.sol"; +import {FastBridgeV2DstExclusivityTest} from "./FastBridgeV2.Dst.Exclusivity.t.sol"; import {RecipientMock} from "./mocks/RecipientMock.sol"; import {Address} from "@openzeppelin/contracts/utils/Address.sol"; @@ -98,6 +99,28 @@ contract FastBridgeV2DstArbitraryCallTest is FastBridgeV2DstExclusivityTest { relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: 0, bridgeTx: tokenTx}); } + function test_relay_token_withCallValue_excessiveReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + override + { + setTokenTestCallValue(CALL_VALUE); + setTokenTestRecipient(excessiveReturnValueRecipient); + vm.expectRevert(RecipientIncorrectReturnValue.selector); + relay({caller: relayerA, msgValue: CALL_VALUE, bridgeTx: tokenTx}); + } + + function test_relay_token_withRelayerAddressCallValue_excessiveReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + override + { + setTokenTestCallValue(CALL_VALUE); + setTokenTestRecipient(excessiveReturnValueRecipient); + vm.expectRevert(RecipientIncorrectReturnValue.selector); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: CALL_VALUE, bridgeTx: tokenTx}); + } + function test_relay_eth_excessiveReturnValueRecipient_revertWhenCallParamsPresent() public virtual override { setEthTestRecipient(excessiveReturnValueRecipient); vm.expectRevert(RecipientIncorrectReturnValue.selector); @@ -132,6 +155,28 @@ contract FastBridgeV2DstArbitraryCallTest is FastBridgeV2DstExclusivityTest { relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: 0, bridgeTx: tokenTx}); } + function test_relay_token_withCallValue_incorrectReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + override + { + setTokenTestCallValue(CALL_VALUE); + setTokenTestRecipient(incorrectReturnValueRecipient); + vm.expectRevert(RecipientIncorrectReturnValue.selector); + relay({caller: relayerA, msgValue: CALL_VALUE, bridgeTx: tokenTx}); + } + + function test_relay_token_withRelayerAddressCallValue_incorrectReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + override + { + setTokenTestCallValue(CALL_VALUE); + setTokenTestRecipient(incorrectReturnValueRecipient); + vm.expectRevert(RecipientIncorrectReturnValue.selector); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: CALL_VALUE, bridgeTx: tokenTx}); + } + function test_relay_eth_incorrectReturnValueRecipient_revertWhenCallParamsPresent() public virtual override { setEthTestRecipient(incorrectReturnValueRecipient); vm.expectRevert(RecipientIncorrectReturnValue.selector); @@ -162,6 +207,24 @@ contract FastBridgeV2DstArbitraryCallTest is FastBridgeV2DstExclusivityTest { relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: 0, bridgeTx: tokenTx}); } + function test_relay_token_withCallValue_noOpRecipient_revertWhenCallParamsPresent() public virtual override { + setTokenTestCallValue(CALL_VALUE); + setTokenTestRecipient(noOpRecipient); + vm.expectRevert(Address.FailedInnerCall.selector); + relay({caller: relayerA, msgValue: CALL_VALUE, bridgeTx: tokenTx}); + } + + function test_relay_token_withRelayerAddressCallValue_noOpRecipient_revertWhenCallParamsPresent() + public + virtual + override + { + setTokenTestCallValue(CALL_VALUE); + setTokenTestRecipient(noOpRecipient); + vm.expectRevert(Address.FailedInnerCall.selector); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: CALL_VALUE, bridgeTx: tokenTx}); + } + function test_relay_eth_noOpRecipient_revertWhenCallParamsPresent() public virtual override { setEthTestRecipient(noOpRecipient); vm.expectRevert(Address.FailedInnerCall.selector); @@ -192,6 +255,28 @@ contract FastBridgeV2DstArbitraryCallTest is FastBridgeV2DstExclusivityTest { relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: 0, bridgeTx: tokenTx}); } + function test_relay_token_withCallValue_noReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + override + { + setTokenTestCallValue(CALL_VALUE); + setTokenTestRecipient(noReturnValueRecipient); + vm.expectRevert(RecipientNoReturnValue.selector); + relay({caller: relayerA, msgValue: CALL_VALUE, bridgeTx: tokenTx}); + } + + function test_relay_token_withRelayerAddressCallValue_noReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + override + { + setTokenTestCallValue(CALL_VALUE); + setTokenTestRecipient(noReturnValueRecipient); + vm.expectRevert(RecipientNoReturnValue.selector); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: CALL_VALUE, bridgeTx: tokenTx}); + } + function test_relay_eth_noReturnValueRecipient_revertWhenCallParamsPresent() public virtual override { setEthTestRecipient(noReturnValueRecipient); vm.expectRevert(RecipientNoReturnValue.selector); @@ -222,6 +307,20 @@ contract FastBridgeV2DstArbitraryCallTest is FastBridgeV2DstExclusivityTest { relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: 0, bridgeTx: tokenTx}); } + function test_relay_token_withCallValue_revert_recipientReverts() public { + setTokenTestCallValue(CALL_VALUE); + mockRecipientRevert(tokenTx); + vm.expectRevert(REVERT_MSG); + relay({caller: relayerA, msgValue: CALL_VALUE, bridgeTx: tokenTx}); + } + + function test_relay_token_withRelayerAddressCallValue_revert_recipientReverts() public { + setTokenTestCallValue(CALL_VALUE); + mockRecipientRevert(tokenTx); + vm.expectRevert(REVERT_MSG); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: CALL_VALUE, bridgeTx: tokenTx}); + } + function test_relay_eth_revert_recipientReverts() public { mockRecipientRevert(ethTx); vm.expectRevert(REVERT_MSG); diff --git a/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol index 1463efa2e0..a3a0f95556 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol @@ -23,6 +23,8 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { uint256 chainGasAmount ); + uint256 public constant CALL_VALUE = 1_337_420; + address public excessiveReturnValueRecipient; address public incorrectReturnValueRecipient; address public noOpRecipient; @@ -67,6 +69,7 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { public virtual { + uint256 chainGasAmount = bridgeTx.destToken == ETH_ADDRESS ? 0 : bridgeTx.callValue; vm.expectEmit(address(fastBridge)); emit BridgeRelayed({ transactionId: txId, @@ -77,7 +80,7 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { destToken: bridgeTx.destToken, originAmount: bridgeTx.originAmount, destAmount: bridgeTx.destAmount, - chainGasAmount: 0 + chainGasAmount: chainGasAmount }); } @@ -153,6 +156,28 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { checkEthBalances({recipient: userB, relayCaller: relayerA}); } + // ══════════════════════════════════════════ RELAYS WITH CALL VALUE ═══════════════════════════════════════════════ + + function test_relay_token_withCallValue() public { + setTokenTestCallValue(CALL_VALUE); + bytes32 txId = getTxId(tokenTx); + expectBridgeRelayed(tokenTx, txId, relayerA); + relay({caller: relayerA, msgValue: CALL_VALUE, bridgeTx: tokenTx}); + checkRelayedViews({txId: txId, expectedRelayer: relayerA}); + checkTokenBalances({recipient: userB, relayCaller: relayerA}); + assertEq(userB.balance, CALL_VALUE); + } + + function test_relay_token_withRelayerAddressCallValue() public { + setTokenTestCallValue(CALL_VALUE); + bytes32 txId = getTxId(tokenTx); + expectBridgeRelayed(tokenTx, txId, relayerA); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: CALL_VALUE, bridgeTx: tokenTx}); + checkRelayedViews({txId: txId, expectedRelayer: relayerA}); + checkTokenBalances({recipient: userB, relayCaller: relayerB}); + assertEq(userB.balance, CALL_VALUE); + } + // ═════════════════════════════════════ EXCESSIVE RETURN VALUE RECIPIENT ══════════════════════════════════════════ // Note: in this test, the callParams are not present, and the below test functions succeed. @@ -173,6 +198,26 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { test_relay_token_withRelayerAddress(); } + function test_relay_token_withCallValue_excessiveReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + { + assertEmptyCallParams(tokenTx.callParams); + setTokenTestCallValue(CALL_VALUE); + setTokenTestRecipient(excessiveReturnValueRecipient); + test_relay_token_withCallValue(); + } + + function test_relay_token_withRelayerAddressCallValue_excessiveReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + { + assertEmptyCallParams(tokenTx.callParams); + setTokenTestCallValue(CALL_VALUE); + setTokenTestRecipient(excessiveReturnValueRecipient); + test_relay_token_withRelayerAddressCallValue(); + } + function test_relay_eth_excessiveReturnValueRecipient_revertWhenCallParamsPresent() public virtual { assertEmptyCallParams(ethTx.callParams); setEthTestRecipient(excessiveReturnValueRecipient); @@ -208,6 +253,26 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { test_relay_token_withRelayerAddress(); } + function test_relay_token_withCallValue_incorrectReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + { + assertEmptyCallParams(tokenTx.callParams); + setTokenTestCallValue(CALL_VALUE); + setTokenTestRecipient(incorrectReturnValueRecipient); + test_relay_token_withCallValue(); + } + + function test_relay_token_withRelayerAddressCallValue_incorrectReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + { + assertEmptyCallParams(tokenTx.callParams); + setTokenTestCallValue(CALL_VALUE); + setTokenTestRecipient(incorrectReturnValueRecipient); + test_relay_token_withRelayerAddressCallValue(); + } + function test_relay_eth_incorrectReturnValueRecipient_revertWhenCallParamsPresent() public virtual { assertEmptyCallParams(ethTx.callParams); setEthTestRecipient(incorrectReturnValueRecipient); @@ -236,6 +301,20 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { test_relay_token_withRelayerAddress(); } + function test_relay_token_withCallValue_nonPayableRecipient_revert() public { + setTokenTestCallValue(CALL_VALUE); + setTokenTestRecipient(nonPayableRecipient); + vm.expectRevert(); + relay({caller: relayerA, msgValue: CALL_VALUE, bridgeTx: tokenTx}); + } + + function test_relay_token_withRelayerAddressCallValue_nonPayableRecipient_revert() public { + setTokenTestCallValue(CALL_VALUE); + setTokenTestRecipient(nonPayableRecipient); + vm.expectRevert(); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: CALL_VALUE, bridgeTx: tokenTx}); + } + function test_relay_eth_revert_nonPayableRecipient() public { setEthTestRecipient(nonPayableRecipient); vm.expectRevert(); @@ -265,6 +344,20 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { test_relay_token_withRelayerAddress(); } + function test_relay_token_withCallValue_noOpRecipient_revertWhenCallParamsPresent() public virtual { + assertEmptyCallParams(tokenTx.callParams); + setTokenTestCallValue(CALL_VALUE); + setTokenTestRecipient(noOpRecipient); + test_relay_token_withCallValue(); + } + + function test_relay_token_withRelayerAddressCallValue_noOpRecipient_revertWhenCallParamsPresent() public virtual { + assertEmptyCallParams(tokenTx.callParams); + setTokenTestCallValue(CALL_VALUE); + setTokenTestRecipient(noOpRecipient); + test_relay_token_withRelayerAddressCallValue(); + } + function test_relay_eth_noOpRecipient_revertWhenCallParamsPresent() public virtual { assertEmptyCallParams(ethTx.callParams); setEthTestRecipient(noOpRecipient); @@ -294,6 +387,23 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { test_relay_token_withRelayerAddress(); } + function test_relay_token_withCallValue_noReturnValueRecipient_revertWhenCallParamsPresent() public virtual { + assertEmptyCallParams(tokenTx.callParams); + setTokenTestCallValue(CALL_VALUE); + setTokenTestRecipient(noReturnValueRecipient); + test_relay_token_withCallValue(); + } + + function test_relay_token_withRelayerAddressCallValue_noReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + { + assertEmptyCallParams(tokenTx.callParams); + setTokenTestCallValue(CALL_VALUE); + setTokenTestRecipient(noReturnValueRecipient); + test_relay_token_withRelayerAddressCallValue(); + } + function test_relay_eth_noReturnValueRecipient_revertWhenCallParamsPresent() public virtual { assertEmptyCallParams(ethTx.callParams); setEthTestRecipient(noReturnValueRecipient); @@ -362,4 +472,67 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { vm.expectRevert(ZeroAddress.selector); relayWithAddress({caller: relayerA, relayer: address(0), msgValue: 0, bridgeTx: tokenTx}); } + + function test_relay_token_withCallValue_revert_zeroCallValue() public { + setTokenTestCallValue(CALL_VALUE); + vm.expectRevert(MsgValueIncorrect.selector); + relay({caller: relayerA, msgValue: 0, bridgeTx: tokenTx}); + } + + function test_relay_token_withCallValue_revert_lowerCallValue() public { + setTokenTestCallValue(CALL_VALUE); + vm.expectRevert(MsgValueIncorrect.selector); + relay({caller: relayerA, msgValue: CALL_VALUE - 1, bridgeTx: tokenTx}); + } + + function test_relay_token_withCallValue_revert_higherCallValue() public { + setTokenTestCallValue(CALL_VALUE); + vm.expectRevert(MsgValueIncorrect.selector); + relay({caller: relayerA, msgValue: CALL_VALUE + 1, bridgeTx: tokenTx}); + } + + function test_relay_token_withRelayerAddressCallValue_revert_zeroCallValue() public { + setTokenTestCallValue(CALL_VALUE); + vm.expectRevert(MsgValueIncorrect.selector); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: 0, bridgeTx: tokenTx}); + } + + function test_relay_token_withRelayerAddressCallValue_revert_lowerCallValue() public { + setTokenTestCallValue(CALL_VALUE); + vm.expectRevert(MsgValueIncorrect.selector); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: CALL_VALUE - 1, bridgeTx: tokenTx}); + } + + function test_relay_token_withRelayerAddressCallValue_revert_higherCallValue() public { + setTokenTestCallValue(CALL_VALUE); + vm.expectRevert(MsgValueIncorrect.selector); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: CALL_VALUE + 1, bridgeTx: tokenTx}); + } + + function test_relay_eth_withCallValue_revert_notSupported() public { + setEthTestCallValue(CALL_VALUE); + // Neither destAmount, CALL_VALUE, nor destAmount + CALL_VALUE should work here + vm.expectRevert(NativeTokenCallValueNotSupported.selector); + relay({caller: relayerB, msgValue: CALL_VALUE, bridgeTx: ethTx}); + vm.expectRevert(NativeTokenCallValueNotSupported.selector); + relay({caller: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); + vm.expectRevert(NativeTokenCallValueNotSupported.selector); + relay({caller: relayerB, msgValue: ethParams.destAmount + CALL_VALUE, bridgeTx: ethTx}); + } + + function test_relay_eth_withRelayerAddressCallValue_revert_notSupported() public { + setEthTestCallValue(CALL_VALUE); + // Neither destAmount, CALL_VALUE, nor destAmount + CALL_VALUE should work here + vm.expectRevert(NativeTokenCallValueNotSupported.selector); + relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: CALL_VALUE, bridgeTx: ethTx}); + vm.expectRevert(NativeTokenCallValueNotSupported.selector); + relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); + vm.expectRevert(NativeTokenCallValueNotSupported.selector); + relayWithAddress({ + caller: relayerA, + relayer: relayerB, + msgValue: ethParams.destAmount + CALL_VALUE, + bridgeTx: ethTx + }); + } } From 7872f2a6d547d32ad1ed641b4d7484cf6231c505 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Thu, 3 Oct 2024 13:29:04 +0100 Subject: [PATCH 06/21] feat: support `callValue` in `bridge()` --- .../contracts-rfq/contracts/FastBridgeV2.sol | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index 20ef419645..970bff70af 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -147,6 +147,9 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { if (params.originToken == address(0) || params.destToken == address(0)) revert ZeroAddress(); if (params.deadline < block.timestamp + MIN_DEADLINE_PERIOD) revert DeadlineTooShort(); if (paramsV2.callParams.length > MAX_CALL_PARAMS_LENGTH) revert CallParamsLengthAboveMax(); + if (paramsV2.callValue != 0 && params.destToken == UniversalTokenLib.ETH_ADDRESS) { + revert NativeTokenCallValueNotSupported(); + } int256 exclusivityEndTime = int256(block.timestamp) + paramsV2.quoteExclusivitySeconds; // exclusivityEndTime must be in range (0 .. params.deadline] if (exclusivityEndTime <= 0 || exclusivityEndTime > int256(params.deadline)) { @@ -173,7 +176,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { originAmount: originAmount, destAmount: params.destAmount, originFeeAmount: originFeeAmount, - callValue: 0, // TODO: fix + callValue: paramsV2.callValue, deadline: params.deadline, nonce: senderNonces[params.sender]++, // increment nonce on every bridge exclusivityRelayer: paramsV2.quoteRelayer, @@ -185,17 +188,17 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { bytes32 transactionId = keccak256(request); bridgeTxDetails[transactionId].status = BridgeStatus.REQUESTED; - emit BridgeRequested( - transactionId, - params.sender, - request, - params.dstChainId, - params.originToken, - params.destToken, - originAmount, - params.destAmount, - params.sendChainGas - ); + emit BridgeRequested({ + transactionId: transactionId, + sender: params.sender, + request: request, + destChainId: params.dstChainId, + originToken: params.originToken, + destToken: params.destToken, + originAmount: originAmount, + destAmount: params.destAmount, + sendChainGas: paramsV2.callValue != 0 + }); emit BridgeQuoteDetails(transactionId, paramsV2.quoteId); } From ad7dd4c304bd62ed70c4b97de5d9317358c7c19b Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Thu, 3 Oct 2024 13:49:54 +0100 Subject: [PATCH 07/21] feat: support `callValue` in `relay()` --- .../contracts-rfq/contracts/FastBridgeV2.sol | 61 ++++++++++--------- 1 file changed, 31 insertions(+), 30 deletions(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index 970bff70af..a5887761b0 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -228,51 +228,52 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { bridgeRelayDetails[transactionId] = BridgeRelay({blockNumber: uint48(block.number), blockTimestamp: uint48(block.timestamp), relayer: relayer}); - // transfer tokens to recipient on destination chain and gas rebate if requested + // transfer tokens to recipient on destination chain and do an arbitrary call if requested address to = transaction.destRecipient; address token = transaction.destToken; uint256 amount = transaction.destAmount; // All state changes have been done at this point, can proceed to the external calls. // This follows the checks-effects-interactions pattern to mitigate potential reentrancy attacks. - if (transaction.callParams.length == 0) { - // No arbitrary call requested, so we just pull the tokens from the Relayer to the recipient, - // or transfer ETH to the recipient (if token is ETH_ADDRESS) - _pullToken(to, token, amount); - } else if (token != UniversalTokenLib.ETH_ADDRESS) { - // Arbitrary call requested with ERC20: pull the tokens from the Relayer to the recipient first - _pullToken(to, token, amount); - // Follow up with the hook function call - _checkedCallRecipient({ - recipient: to, - msgValue: 0, - token: token, - amount: amount, - callParams: transaction.callParams - }); + if (token == UniversalTokenLib.ETH_ADDRESS) { + // For ETH non-zero callValue is not allowed + if (transaction.callValue != 0) revert NativeTokenCallValueNotSupported(); + // Check that the correct msg.value was sent + if (msg.value != amount) revert MsgValueIncorrect(); } else { - // Arbitrary call requested with ETH: combine the ETH transfer with the call + // For ERC20s, we check that the correct msg.value was sent + if (msg.value != transaction.callValue) revert MsgValueIncorrect(); + // We need to pull the tokens from the Relayer to the recipient first before performing an + // optional post-transfer arbitrary call. + _pullToken(to, token, amount); + } + + if (transaction.callParams.length != 0) { + // Arbitrary call requested, perform it while supplying full msg.value to the recipient _checkedCallRecipient({ recipient: to, - msgValue: amount, + msgValue: msg.value, token: token, amount: amount, callParams: transaction.callParams }); + } else if (msg.value != 0) { + // No arbitrary call requested, but msg.value was sent. This is either a relay with ETH, + // or a non-zero callValue request with an ERC20. In both cases, transfer the ETH to the recipient. + Address.sendValue(payable(to), msg.value); } - emit BridgeRelayed( - transactionId, - relayer, - to, - transaction.originChainId, - transaction.originToken, - transaction.destToken, - transaction.originAmount, - transaction.destAmount, - // chainGasAmount is 0 since the gas rebate function is deprecated - 0 - ); + emit BridgeRelayed({ + transactionId: transactionId, + relayer: relayer, + to: to, + originChainId: transaction.originChainId, + originToken: transaction.originToken, + destToken: transaction.destToken, + originAmount: transaction.originAmount, + destAmount: transaction.destAmount, + chainGasAmount: transaction.callValue + }); } /// @inheritdoc IFastBridgeV2 From 5ca5ad1979b5cb60a67fefa4e6699b6cf50f979d Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Thu, 3 Oct 2024 13:52:19 +0100 Subject: [PATCH 08/21] test: update revert message for failed ETH transfers --- .../contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol index d48420dba0..1bd47fba4b 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol @@ -336,14 +336,14 @@ contract FastBridgeV2DstArbitraryCallTest is FastBridgeV2DstExclusivityTest { function test_relay_eth_noCallParams_revert_recipientReverts() public { setEthTestCallParams(""); vm.mockCallRevert({callee: userB, data: "", revertData: bytes(REVERT_MSG)}); - vm.expectRevert("ETH transfer failed"); + vm.expectRevert(Address.FailedInnerCall.selector); relay({caller: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); } function test_relay_eth_withRelayerAddress_noCallParams_revert_recipientReverts() public { setEthTestCallParams(""); vm.mockCallRevert({callee: userB, data: "", revertData: bytes(REVERT_MSG)}); - vm.expectRevert("ETH transfer failed"); + vm.expectRevert(Address.FailedInnerCall.selector); relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); } } From e8355c327618ec34ed799f413c67091fe22c4dbd Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Thu, 3 Oct 2024 13:53:45 +0100 Subject: [PATCH 09/21] refactor: always forward full msg.value for the hook call --- packages/contracts-rfq/contracts/FastBridgeV2.sol | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index a5887761b0..15c1431f89 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -250,13 +250,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { if (transaction.callParams.length != 0) { // Arbitrary call requested, perform it while supplying full msg.value to the recipient - _checkedCallRecipient({ - recipient: to, - msgValue: msg.value, - token: token, - amount: amount, - callParams: transaction.callParams - }); + _checkedCallRecipient({recipient: to, token: token, amount: amount, callParams: transaction.callParams}); } else if (msg.value != 0) { // No arbitrary call requested, but msg.value was sent. This is either a relay with ETH, // or a non-zero callValue request with an ERC20. In both cases, transfer the ETH to the recipient. @@ -366,7 +360,6 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { /// all the necessary checks for the returned value. function _checkedCallRecipient( address recipient, - uint256 msgValue, address token, uint256 amount, bytes memory callParams @@ -376,7 +369,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { bytes memory hookData = abi.encodeCall(IFastBridgeRecipient.fastBridgeTransferReceived, (token, amount, callParams)); // This will bubble any revert messages from the hook function - bytes memory returnData = Address.functionCallWithValue({target: recipient, data: hookData, value: msgValue}); + bytes memory returnData = Address.functionCallWithValue({target: recipient, data: hookData, value: msg.value}); // Explicit revert if no return data at all if (returnData.length == 0) revert RecipientNoReturnValue(); // Check that exactly a single return value was returned From eb2bbb8025778b635569c768e234df48c8327b3f Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Thu, 3 Oct 2024 14:01:39 +0100 Subject: [PATCH 10/21] refactor: use `_pullToken` only in `bridge()` --- .../contracts-rfq/contracts/FastBridgeV2.sol | 34 +++++++++---------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index 15c1431f89..b2653bd401 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -156,8 +156,8 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { revert ExclusivityParamsIncorrect(); } // transfer tokens to bridge contract - // @dev use returned originAmount in request in case of transfer fees - uint256 originAmount = _pullToken(address(this), params.originToken, params.originAmount); + /// @dev use returned originAmount in request in case of transfer fees + uint256 originAmount = _pullToken(params.originToken, params.originAmount); // track amount of origin token owed to protocol uint256 originFeeAmount; @@ -243,13 +243,15 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { } else { // For ERC20s, we check that the correct msg.value was sent if (msg.value != transaction.callValue) revert MsgValueIncorrect(); - // We need to pull the tokens from the Relayer to the recipient first before performing an + // We need to transfer the tokens from the Relayer to the recipient first before performing an // optional post-transfer arbitrary call. - _pullToken(to, token, amount); + IERC20(token).safeTransferFrom(msg.sender, to, amount); } if (transaction.callParams.length != 0) { // Arbitrary call requested, perform it while supplying full msg.value to the recipient + // Note: if token has a fee on transfers, the recipient will have received less than `amount`. + // This is a very niche edge case and should be handled by the recipient contract. _checkedCallRecipient({recipient: to, token: token, amount: amount, callParams: transaction.callParams}); } else if (msg.value != 0) { // No arbitrary call requested, but msg.value was sent. This is either a relay with ETH, @@ -334,25 +336,21 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { return abi.decode(request, (BridgeTransactionV2)); } - /// @notice Pulls a requested token from the user to the requested recipient. - /// @dev Be careful of re-entrancy issues when msg.value > 0 and recipient != address(this) - function _pullToken(address recipient, address token, uint256 amount) internal returns (uint256 amountPulled) { - if (token != UniversalTokenLib.ETH_ADDRESS) { + /// @notice Pulls a requested token from the user to this contract. + function _pullToken(address token, uint256 amount) internal returns (uint256 amountPulled) { + if (token == UniversalTokenLib.ETH_ADDRESS) { + // For ETH we just need to check that the supplied msg.value is correct + if (amount != msg.value) revert MsgValueIncorrect(); + amountPulled = msg.value; + } else { token.assertIsContract(); // Record token balance before transfer - amountPulled = IERC20(token).balanceOf(recipient); + amountPulled = IERC20(token).balanceOf(address(this)); // Token needs to be pulled only if msg.value is zero // This way user can specify WETH as the origin asset - IERC20(token).safeTransferFrom(msg.sender, recipient, amount); + IERC20(token).safeTransferFrom(msg.sender, address(this), amount); // Use the difference between the recorded balance and the current balance as the amountPulled - amountPulled = IERC20(token).balanceOf(recipient) - amountPulled; - } else { - // Otherwise, we need to check that ETH amount matches msg.value - if (amount != msg.value) revert MsgValueIncorrect(); - // Transfer value to recipient if not this address - if (recipient != address(this)) token.universalTransfer(recipient, amount); - // We will forward msg.value in the external call later, if recipient is not this contract - amountPulled = msg.value; + amountPulled = IERC20(token).balanceOf(address(this)) - amountPulled; } } From c50042adbfee3c4a02ea6517cf2325b694160fd2 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Thu, 3 Oct 2024 14:19:27 +0100 Subject: [PATCH 11/21] refactor: isolate validation of relay params --- .../contracts-rfq/contracts/FastBridgeV2.sol | 45 +++++++++++-------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index b2653bd401..513ee34a9f 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -203,27 +203,10 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { } /// @inheritdoc IFastBridgeV2 - // TODO: reduce cyclomatic complexity alongside arbitrary call - // solhint-disable-next-line code-complexity function relay(bytes memory request, address relayer) public payable { - if (relayer == address(0)) revert ZeroAddress(); - // Check if the transaction has already been relayed bytes32 transactionId = keccak256(request); - if (bridgeRelays(transactionId)) revert TransactionRelayed(); - // Decode the transaction and check that it could be relayed on this chain BridgeTransactionV2 memory transaction = getBridgeTransactionV2(request); - if (transaction.destChainId != uint32(block.chainid)) revert ChainIncorrect(); - // Check the deadline for relay to happen - if (block.timestamp > transaction.deadline) revert DeadlineExceeded(); - // Check the exclusivity period, if it is still ongoing - // forgefmt: disable-next-item - if ( - transaction.exclusivityRelayer != address(0) && - transaction.exclusivityRelayer != relayer && - block.timestamp <= transaction.exclusivityEndTime - ) { - revert ExclusivityPeriodNotPassed(); - } + _validateRelayParams(transaction, transactionId, relayer); // mark bridge transaction as relayed bridgeRelayDetails[transactionId] = BridgeRelay({blockNumber: uint48(block.number), blockTimestamp: uint48(block.timestamp), relayer: relayer}); @@ -389,4 +372,30 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { delta = uint40(block.timestamp) - proofBlockTimestamp; } } + + /// @notice Performs all the necessary checks for a relay to happen. + function _validateRelayParams( + BridgeTransactionV2 memory transaction, + bytes32 transactionId, + address relayer + ) + internal + view + { + if (relayer == address(0)) revert ZeroAddress(); + // Check if the transaction has already been relayed + if (bridgeRelays(transactionId)) revert TransactionRelayed(); + if (transaction.destChainId != uint32(block.chainid)) revert ChainIncorrect(); + // Check the deadline for relay to happen + if (block.timestamp > transaction.deadline) revert DeadlineExceeded(); + // Check the exclusivity period, if it is still ongoing + // forgefmt: disable-next-item + if ( + transaction.exclusivityRelayer != address(0) && + transaction.exclusivityRelayer != relayer && + block.timestamp <= transaction.exclusivityEndTime + ) { + revert ExclusivityPeriodNotPassed(); + } + } } From 9fb4461b2df27625fdefbd9a3c7dfbf42f1558f0 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Thu, 3 Oct 2024 14:52:02 +0100 Subject: [PATCH 12/21] refactor: isolate validation of the bridge params --- .../contracts-rfq/contracts/FastBridgeV2.sol | 47 ++++++++++++------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index 513ee34a9f..c4483b183f 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -137,24 +137,10 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { } /// @inheritdoc IFastBridgeV2 - // TODO: reduce cyclomatic complexity alongside arbitrary call - // solhint-disable-next-line code-complexity function bridge(BridgeParams memory params, BridgeParamsV2 memory paramsV2) public payable { - // check bridge params - if (params.dstChainId == block.chainid) revert ChainIncorrect(); - if (params.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 (params.deadline < block.timestamp + MIN_DEADLINE_PERIOD) revert DeadlineTooShort(); - if (paramsV2.callParams.length > MAX_CALL_PARAMS_LENGTH) revert CallParamsLengthAboveMax(); - if (paramsV2.callValue != 0 && params.destToken == UniversalTokenLib.ETH_ADDRESS) { - revert NativeTokenCallValueNotSupported(); - } int256 exclusivityEndTime = int256(block.timestamp) + paramsV2.quoteExclusivitySeconds; - // exclusivityEndTime must be in range (0 .. params.deadline] - if (exclusivityEndTime <= 0 || exclusivityEndTime > int256(params.deadline)) { - revert ExclusivityParamsIncorrect(); - } + _validateBridgeParams(params, paramsV2, exclusivityEndTime); + // transfer tokens to bridge contract /// @dev use returned originAmount in request in case of transfer fees uint256 originAmount = _pullToken(params.originToken, params.originAmount); @@ -373,6 +359,35 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { } } + /// @notice Performs all the necessary checks for a bridge to happen. + /// @dev There's no good way to refactor this function to reduce cyclomatic complexity due to + /// the number of checks that need to be performed, so we skip the code-complexity rule here. + // solhint-disable-next-line code-complexity + function _validateBridgeParams( + BridgeParams memory params, + BridgeParamsV2 memory paramsV2, + int256 exclusivityEndTime + ) + internal + view + { + // Check V1 (legacy) params + if (params.dstChainId == block.chainid) revert ChainIncorrect(); + if (params.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 (params.deadline < block.timestamp + MIN_DEADLINE_PERIOD) revert DeadlineTooShort(); + // Check V2 params + if (paramsV2.callParams.length > MAX_CALL_PARAMS_LENGTH) revert CallParamsLengthAboveMax(); + if (paramsV2.callValue != 0 && params.destToken == UniversalTokenLib.ETH_ADDRESS) { + revert NativeTokenCallValueNotSupported(); + } + // exclusivityEndTime must be in range (0 .. params.deadline] + if (exclusivityEndTime <= 0 || exclusivityEndTime > int256(params.deadline)) { + revert ExclusivityParamsIncorrect(); + } + } + /// @notice Performs all the necessary checks for a relay to happen. function _validateRelayParams( BridgeTransactionV2 memory transaction, From 156e33382371751ee00174470127fb8f95402107 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Tue, 8 Oct 2024 10:54:39 +0100 Subject: [PATCH 13/21] docs: could -> can --- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol b/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol index a1485d3f31..54dbb5f3f6 100644 --- a/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol @@ -30,7 +30,7 @@ interface IFastBridgeV2 is IFastBridge { /// for backwards compatibility. /// Note: quoteRelayer and quoteExclusivitySeconds are either both zero (indicating no exclusivity) /// or both non-zero (indicating exclusivity for the given period). - /// Note: callValue > 0 could NOT be used with destToken = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE (ETH_ADDRESS) + /// Note: callValue > 0 can NOT be used with destToken = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE (ETH_ADDRESS) /// @param quoteRelayer Relayer that provided the quote for the transaction /// @param quoteExclusivitySeconds Period of time the quote relayer is guaranteed exclusivity after user's deposit /// @param quoteId Unique quote identifier used for tracking the quote @@ -70,7 +70,7 @@ interface IFastBridgeV2 is IFastBridge { /// @notice Initiates bridge on origin chain to be relayed by off-chain relayer, with the ability /// to provide temporary exclusivity fill rights for the quote relayer. /// @param params The parameters required to bridge - /// @param paramsV2 The parameters for exclusivity fill rights (optional, could be left empty) + /// @param paramsV2 The parameters for exclusivity fill rights (optional, can be left empty) function bridge(BridgeParams memory params, BridgeParamsV2 memory paramsV2) external payable; /// @notice Relays destination side of bridge transaction by off-chain relayer From 2b77b4af8091c58e54c93fb8a28b37ed292939d3 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Tue, 8 Oct 2024 11:05:09 +0100 Subject: [PATCH 14/21] test: enable the backwards compatibility encoding test --- .../test/FastBridgeV2.Encoding.t.sol | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol index 0918c66bf5..21d32876fc 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol @@ -47,17 +47,15 @@ contract FastBridgeV2EncodingTest is FastBridgeV2Test { assertEq(decodedTx, bridgeTx); } - // The addition of variable length field (callParams) in BridgeTransactionV2 breaks the compatibility - // with the original BridgeTransaction struct. - // Solidity's abi.encode(bridgeTxV2) will use the first 32 bytes to encode the data offset for the whole struct, - // which is ALWAYS equal to 32 (data starts right after the offset). This is weird, but it is what it is. - // https://ethereum.stackexchange.com/questions/152971/abi-encode-decode-mystery-additional-32-byte-field-uniswap-v2 - function test_getBridgeTransaction_supportsV2(IFastBridgeV2.BridgeTransactionV2 memory bridgeTxV2) public { - // TODO: reevaluate the necessity of this test if/when the encoding scheme is changed - vm.skip(true); + /// @notice We expect all the V1 fields except for `sendChainGas` to match. + /// `sendChainGas` is replaced with `callValue` in V2, therefore we expect non-zero `callValue` + /// to match `sendChainGas = true` in V1 + function test_getBridgeTransaction_supportsV2(IFastBridgeV2.BridgeTransactionV2 memory bridgeTxV2) public view { bytes memory request = abi.encode(bridgeTxV2); IFastBridge.BridgeTransaction memory decodedTx = fastBridge.getBridgeTransaction(request); - assertEq(decodedTx, extractV1(bridgeTxV2)); + IFastBridge.BridgeTransaction memory expectedTx = extractV1(bridgeTxV2); + expectedTx.sendChainGas = bridgeTxV2.callValue > 0; + assertEq(decodedTx, expectedTx); } function test_getBridgeTransactionV2(IFastBridgeV2.BridgeTransactionV2 memory bridgeTxV2) public view { From 271f59da3150e905ba28977b94a9bfa51f05647a Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Tue, 8 Oct 2024 11:08:28 +0100 Subject: [PATCH 15/21] fix: getBridgeTransaction partial support for V2 structs --- .../contracts-rfq/contracts/FastBridgeV2.sol | 33 +++++++++++++++---- .../contracts/interfaces/IFastBridge.sol | 2 +- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index c4483b183f..0da1b511da 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -127,13 +127,32 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { } /// @inheritdoc IFastBridge - function getBridgeTransaction(bytes memory request) external pure returns (BridgeTransaction memory) { - // TODO: the note below isn't true anymore with the BridgeTransactionV2 struct - // since the variable length `callParams` was added. This needs to be fixed/acknowledged. - - // Note: when passing V2 request, this will decode the V1 fields correctly since the new fields were - // added as the last fields of the struct and hence the ABI decoder will simply ignore the extra data. - return abi.decode(request, (BridgeTransaction)); + /// @dev This method is added to achieve backwards compatibility with decoding requests into V1 structs: + /// - `callValue` is partially reported as a zero/non-zero flag + /// - `callParams` is ignored + /// In order to process all kinds of requests use getBridgeTransactionV2 instead. + function getBridgeTransaction(bytes memory request) external view returns (BridgeTransaction memory) { + // Try decoding into V2 struct first. This will revert if V1 struct is passed + try this.getBridgeTransactionV2(request) returns (BridgeTransactionV2 memory txV2) { + // Note: we entirely ignore the callParams field, as it was not present in V1 + return BridgeTransaction({ + originChainId: txV2.originChainId, + destChainId: txV2.destChainId, + originSender: txV2.originSender, + destRecipient: txV2.destRecipient, + originToken: txV2.originToken, + destToken: txV2.destToken, + originAmount: txV2.originAmount, + destAmount: txV2.destAmount, + originFeeAmount: txV2.originFeeAmount, + sendChainGas: txV2.callValue != 0, + deadline: txV2.deadline, + nonce: txV2.nonce + }); + } catch { + // Fallback to V1 struct + return abi.decode(request, (BridgeTransaction)); + } } /// @inheritdoc IFastBridgeV2 diff --git a/packages/contracts-rfq/contracts/interfaces/IFastBridge.sol b/packages/contracts-rfq/contracts/interfaces/IFastBridge.sol index b691dfb5b4..033d602f76 100644 --- a/packages/contracts-rfq/contracts/interfaces/IFastBridge.sol +++ b/packages/contracts-rfq/contracts/interfaces/IFastBridge.sol @@ -97,7 +97,7 @@ interface IFastBridge { /// @notice Decodes bridge request into a bridge transaction /// @param request The bridge request to decode - function getBridgeTransaction(bytes memory request) external pure returns (BridgeTransaction memory); + function getBridgeTransaction(bytes memory request) external view returns (BridgeTransaction memory); /// @notice Checks if the dispute period has passed so bridge deposit can be claimed /// @param transactionId The transaction id associated with the encoded bridge transaction to check From 2317a588c1fa3202c378ab05157562e1a13fa34c Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Tue, 8 Oct 2024 12:26:01 +0100 Subject: [PATCH 16/21] test: add clarifications about expected reverts --- .../contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol index 1bd47fba4b..1fb3ed8148 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol @@ -195,6 +195,8 @@ contract FastBridgeV2DstArbitraryCallTest is FastBridgeV2DstExclusivityTest { // ══════════════════════════════════════════════ NO-OP RECIPIENT ══════════════════════════════════════════════════ + // Note: in these tests NoOpRecipient doesn't implement hook function, so we expect a generic OZ library revert. + function test_relay_token_noOpRecipient_revertWhenCallParamsPresent() public virtual override { setTokenTestRecipient(noOpRecipient); vm.expectRevert(Address.FailedInnerCall.selector); @@ -336,6 +338,8 @@ contract FastBridgeV2DstArbitraryCallTest is FastBridgeV2DstExclusivityTest { function test_relay_eth_noCallParams_revert_recipientReverts() public { setEthTestCallParams(""); vm.mockCallRevert({callee: userB, data: "", revertData: bytes(REVERT_MSG)}); + // Note: OZ library doesn't bubble the revert message for just sending ETH + // (as opposed to doing an external hook call). Therefore we expect a generic library revert. vm.expectRevert(Address.FailedInnerCall.selector); relay({caller: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); } @@ -343,6 +347,8 @@ contract FastBridgeV2DstArbitraryCallTest is FastBridgeV2DstExclusivityTest { function test_relay_eth_withRelayerAddress_noCallParams_revert_recipientReverts() public { setEthTestCallParams(""); vm.mockCallRevert({callee: userB, data: "", revertData: bytes(REVERT_MSG)}); + // Note: OZ library doesn't bubble the revert message for just sending ETH + // (as opposed to doing an external hook call). Therefore we expect a generic library revert. vm.expectRevert(Address.FailedInnerCall.selector); relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); } From d575cbec1e0853d82d1501947a595e4ab69bbe25 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Thu, 10 Oct 2024 10:33:52 +0100 Subject: [PATCH 17/21] test: more incorrect relay scenarios --- .../contracts-rfq/test/FastBridgeV2.Dst.t.sol | 40 +++++++++++++++++-- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol index 3f9b88ad02..047ad6bec2 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol @@ -555,18 +555,52 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { relay({caller: relayerA, msgValue: tokenParams.destAmount, bridgeTx: tokenTx}); } + function test_relay_token_withRelayerAddress_revert_approvedZero() public { + vm.prank(relayerB); + dstToken.approve(address(fastBridge), 0); + vm.expectRevert(); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: 0, bridgeTx: tokenTx}); + } + + function test_relay_token_withRelayerAddress_revert_approvedNotEnough() public { + vm.prank(relayerB); + dstToken.approve(address(fastBridge), tokenParams.destAmount - 1); + vm.expectRevert(); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: 0, bridgeTx: tokenTx}); + } + + function test_relay_token_withRelayerAddress_revert_nonZeroMsgValue() public { + vm.expectRevert(); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: tokenParams.destAmount, bridgeTx: tokenTx}); + } + function test_relay_eth_revert_lowerMsgValue() public { vm.expectRevert(); - relay({caller: relayerA, msgValue: ethParams.destAmount - 1, bridgeTx: ethTx}); + relay({caller: relayerB, msgValue: ethParams.destAmount - 1, bridgeTx: ethTx}); } function test_relay_eth_revert_higherMsgValue() public { vm.expectRevert(); - relay({caller: relayerA, msgValue: ethParams.destAmount + 1, bridgeTx: ethTx}); + relay({caller: relayerB, msgValue: ethParams.destAmount + 1, bridgeTx: ethTx}); } function test_relay_eth_revert_zeroMsgValue() public { vm.expectRevert(); - relay({caller: relayerA, msgValue: 0, bridgeTx: ethTx}); + relay({caller: relayerB, msgValue: 0, bridgeTx: ethTx}); + } + + function test_relay_eth_withRelayerAddress_revert_lowerMsgValue() public { + vm.expectRevert(); + relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: ethParams.destAmount - 1, bridgeTx: ethTx}); + } + + function test_relay_eth_withRelayerAddress_revert_higherMsgValue() public { + vm.expectRevert(); + relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: ethParams.destAmount + 1, bridgeTx: ethTx}); + } + + function test_relay_eth_withRelayerAddress_revert_zeroMsgValue() public { + vm.expectRevert(); + relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: 0, bridgeTx: ethTx}); } } From 189931c968e016c2f9d1f431ad8aebace124f6d7 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Thu, 10 Oct 2024 10:43:21 +0100 Subject: [PATCH 18/21] refactor: better naming for `_pullToken`, docs --- .../contracts-rfq/contracts/FastBridgeV2.sol | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index e1b1431260..b5e41386f4 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -162,7 +162,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { // transfer tokens to bridge contract /// @dev use returned originAmount in request in case of transfer fees - uint256 originAmount = _pullToken(params.originToken, params.originAmount); + uint256 originAmount = _takeBridgedUserAsset(params.originToken, params.originAmount); // track amount of origin token owed to protocol uint256 originFeeAmount; @@ -324,22 +324,24 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { return abi.decode(request, (BridgeTransactionV2)); } - /// @notice Pulls a requested token from the user to this contract. - function _pullToken(address token, uint256 amount) internal returns (uint256 amountPulled) { + /// @notice Takes the bridged asset from the user into FastBridgeV2 custody. It will be later + /// claimed by the relayer who completed the relay on destination chain, or refunded back to the user, + /// should no one complete the relay. + function _takeBridgedUserAsset(address token, uint256 amount) internal returns (uint256 amountTaken) { if (token == UniversalTokenLib.ETH_ADDRESS) { - // For ETH we just need to check that the supplied msg.value is correct + // For ETH 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(); - amountPulled = msg.value; + 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. token.assertIsContract(); - // Token needs to be pulled only if msg.value is zero - // This way user can specify WETH as the origin asset if (msg.value != 0) revert MsgValueIncorrect(); - // Record token balance before transfer - amountPulled = IERC20(token).balanceOf(address(this)); + amountTaken = IERC20(token).balanceOf(address(this)); IERC20(token).safeTransferFrom(msg.sender, address(this), amount); - // Use the difference between the recorded balance and the current balance as the amountPulled - amountPulled = IERC20(token).balanceOf(address(this)) - amountPulled; + // Use the balance difference as the amount taken in case of fee on transfer tokens. + amountTaken = IERC20(token).balanceOf(address(this)) - amountTaken; } } From 8c426627b5ccb06969426ff510daaedc303131ac Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Thu, 10 Oct 2024 12:24:22 +0100 Subject: [PATCH 19/21] refactor: remove unnecessary cast --- packages/contracts-rfq/contracts/FastBridgeV2.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index b5e41386f4..995150abf4 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -422,7 +422,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { if (relayer == address(0)) revert ZeroAddress(); // Check if the transaction has already been relayed if (bridgeRelays(transactionId)) revert TransactionRelayed(); - if (transaction.destChainId != uint32(block.chainid)) revert ChainIncorrect(); + if (transaction.destChainId != block.chainid) revert ChainIncorrect(); // Check the deadline for relay to happen if (block.timestamp > transaction.deadline) revert DeadlineExceeded(); // Check the exclusivity period, if it is still ongoing From 07822144acd4fb3afe73f7b235d94763de99d15e Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Thu, 10 Oct 2024 12:25:40 +0100 Subject: [PATCH 20/21] refactor: read from stack, not memory where possible --- packages/contracts-rfq/contracts/FastBridgeV2.sol | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index 995150abf4..1861691c1e 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -220,17 +220,18 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { address to = transaction.destRecipient; address token = transaction.destToken; uint256 amount = transaction.destAmount; + uint256 callValue = transaction.callValue; // All state changes have been done at this point, can proceed to the external calls. // This follows the checks-effects-interactions pattern to mitigate potential reentrancy attacks. if (token == UniversalTokenLib.ETH_ADDRESS) { // For ETH non-zero callValue is not allowed - if (transaction.callValue != 0) revert NativeTokenCallValueNotSupported(); + if (callValue != 0) revert NativeTokenCallValueNotSupported(); // Check that the correct msg.value was sent if (msg.value != amount) revert MsgValueIncorrect(); } else { // For ERC20s, we check that the correct msg.value was sent - if (msg.value != transaction.callValue) revert MsgValueIncorrect(); + if (msg.value != callValue) revert MsgValueIncorrect(); // We need to transfer the tokens from the Relayer to the recipient first before performing an // optional post-transfer arbitrary call. IERC20(token).safeTransferFrom(msg.sender, to, amount); @@ -253,10 +254,10 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { to: to, originChainId: transaction.originChainId, originToken: transaction.originToken, - destToken: transaction.destToken, + destToken: token, originAmount: transaction.originAmount, - destAmount: transaction.destAmount, - chainGasAmount: transaction.callValue + destAmount: amount, + chainGasAmount: callValue }); } From 432c34e8d784644e9343d3f78bad0827b9fcf13c Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Thu, 10 Oct 2024 12:28:32 +0100 Subject: [PATCH 21/21] fix: emit the event prior to the external calls --- .../contracts-rfq/contracts/FastBridgeV2.sol | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index 1861691c1e..b77e68a0c9 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -222,6 +222,19 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { uint256 amount = transaction.destAmount; uint256 callValue = transaction.callValue; + // Emit the event before any external calls + emit BridgeRelayed({ + transactionId: transactionId, + relayer: relayer, + to: to, + originChainId: transaction.originChainId, + originToken: transaction.originToken, + destToken: token, + originAmount: transaction.originAmount, + destAmount: amount, + chainGasAmount: callValue + }); + // All state changes have been done at this point, can proceed to the external calls. // This follows the checks-effects-interactions pattern to mitigate potential reentrancy attacks. if (token == UniversalTokenLib.ETH_ADDRESS) { @@ -247,18 +260,6 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { // or a non-zero callValue request with an ERC20. In both cases, transfer the ETH to the recipient. Address.sendValue(payable(to), msg.value); } - - emit BridgeRelayed({ - transactionId: transactionId, - relayer: relayer, - to: to, - originChainId: transaction.originChainId, - originToken: transaction.originToken, - destToken: token, - originAmount: transaction.originAmount, - destAmount: amount, - chainGasAmount: callValue - }); } /// @inheritdoc IFastBridgeV2