From 7dba739a73321f51e58911bc47c31d93c8d6b97f Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Tue, 1 Oct 2024 19:45:21 +0100 Subject: [PATCH 01/15] feat: add `callParams` --- packages/contracts-rfq/contracts/FastBridgeV2.sol | 7 ++++++- .../contracts/interfaces/IFastBridgeV2.sol | 2 ++ .../test/FastBridgeV2.Dst.Exclusivity.t.sol | 6 ++++-- packages/contracts-rfq/test/FastBridgeV2.t.sol | 15 ++++++++++++--- 4 files changed, 24 insertions(+), 6 deletions(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index 5a764a7c95..266590b340 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -45,7 +45,12 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { function bridge(BridgeParams memory params) external payable { bridge({ params: params, - paramsV2: BridgeParamsV2({quoteRelayer: address(0), quoteExclusivitySeconds: 0, quoteId: bytes("")}) + paramsV2: BridgeParamsV2({ + quoteRelayer: address(0), + quoteExclusivitySeconds: 0, + quoteId: bytes(""), + callParams: bytes("") + }) }); } diff --git a/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol b/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol index 81a67636b5..46d4fcf9c4 100644 --- a/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol @@ -33,10 +33,12 @@ interface IFastBridgeV2 is IFastBridge { /// @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 callParams Parameters for the arbitrary call to the destination recipient (if any) struct BridgeParamsV2 { address quoteRelayer; int256 quoteExclusivitySeconds; bytes quoteId; + bytes callParams; } /// @notice Updated bridge transaction struct to include parameters introduced in FastBridgeV2. diff --git a/packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol index 9d0a414306..a616872f93 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol @@ -11,12 +11,14 @@ contract FastBridgeV2DstExclusivityTest is FastBridgeV2DstTest { tokenParamsV2 = IFastBridgeV2.BridgeParamsV2({ quoteRelayer: relayerA, quoteExclusivitySeconds: int256(EXCLUSIVITY_PERIOD), - quoteId: "" + quoteId: "", + callParams: "" }); ethParamsV2 = IFastBridgeV2.BridgeParamsV2({ quoteRelayer: relayerB, quoteExclusivitySeconds: int256(EXCLUSIVITY_PERIOD), - quoteId: "" + quoteId: "", + callParams: "" }); tokenTx.exclusivityRelayer = relayerA; diff --git a/packages/contracts-rfq/test/FastBridgeV2.t.sol b/packages/contracts-rfq/test/FastBridgeV2.t.sol index 3828076dac..96b987fe7f 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.t.sol @@ -124,9 +124,18 @@ abstract contract FastBridgeV2Test is Test, IFastBridgeV2Errors { function createFixturesV2() public virtual { // Override in tests with exclusivity params - tokenParamsV2 = - IFastBridgeV2.BridgeParamsV2({quoteRelayer: address(0), quoteExclusivitySeconds: 0, quoteId: ""}); - ethParamsV2 = IFastBridgeV2.BridgeParamsV2({quoteRelayer: address(0), quoteExclusivitySeconds: 0, quoteId: ""}); + tokenParamsV2 = IFastBridgeV2.BridgeParamsV2({ + quoteRelayer: address(0), + quoteExclusivitySeconds: 0, + quoteId: bytes(""), + callParams: bytes("") + }); + ethParamsV2 = IFastBridgeV2.BridgeParamsV2({ + quoteRelayer: address(0), + quoteExclusivitySeconds: 0, + quoteId: bytes(""), + callParams: bytes("") + }); tokenTx.exclusivityRelayer = address(0); tokenTx.exclusivityEndTime = block.timestamp; From 6b35618a0bb77f4273f64217fc3dbb8180efa75f Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Wed, 25 Sep 2024 19:36:11 +0100 Subject: [PATCH 02/15] feat: scaffold `IFastBridgeRecipient` --- .../contracts/interfaces/IFastBridgeRecipient.sol | 13 +++++++++++++ .../test/mocks/FastBridgeRecipientMock.sol | 11 +++++++++++ 2 files changed, 24 insertions(+) create mode 100644 packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol create mode 100644 packages/contracts-rfq/test/mocks/FastBridgeRecipientMock.sol diff --git a/packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol b/packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol new file mode 100644 index 0000000000..e76ee865cd --- /dev/null +++ b/packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol @@ -0,0 +1,13 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +interface IFastBridgeRecipient { + function fastBridgeTransferReceived( + address token, + uint256 amount, + bytes memory callParams + ) + external + payable + returns (bytes4); +} diff --git a/packages/contracts-rfq/test/mocks/FastBridgeRecipientMock.sol b/packages/contracts-rfq/test/mocks/FastBridgeRecipientMock.sol new file mode 100644 index 0000000000..8b732e279c --- /dev/null +++ b/packages/contracts-rfq/test/mocks/FastBridgeRecipientMock.sol @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +import {IFastBridgeRecipient} from "../../contracts/interfaces/IFastBridgeRecipient.sol"; + +/// @notice Recipient mock for testing purposes. DO NOT USE IN PRODUCTION. +contract FastBridgeRecipientMock is IFastBridgeRecipient { + function fastBridgeTransferReceived(address, uint256, bytes memory) external payable returns (bytes4) { + return IFastBridgeRecipient.fastBridgeTransferReceived.selector; + } +} From 6dc842cf273cc6d46a3bdccec66dff42c3ffb044 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Tue, 1 Oct 2024 19:55:04 +0100 Subject: [PATCH 03/15] feat: add `callParams` to `BridgeTransactionV2` --- packages/contracts-rfq/contracts/FastBridgeV2.sol | 3 ++- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index 266590b340..0e05f7d948 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -168,7 +168,8 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { nonce: senderNonces[params.sender]++, // increment nonce on every bridge exclusivityRelayer: paramsV2.quoteRelayer, // We checked exclusivityEndTime to be in range (0 .. params.deadline] above, so can safely cast - exclusivityEndTime: uint256(exclusivityEndTime) + exclusivityEndTime: uint256(exclusivityEndTime), + callParams: paramsV2.callParams }) ); bytes32 transactionId = keccak256(request); diff --git a/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol b/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol index 46d4fcf9c4..c4c3751a5b 100644 --- a/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol @@ -59,6 +59,7 @@ interface IFastBridgeV2 is IFastBridge { uint256 nonce; address exclusivityRelayer; uint256 exclusivityEndTime; + bytes callParams; } event BridgeQuoteDetails(bytes32 indexed transactionId, bytes quoteId); From c74672747b7bc67441e07ebff5bb0fe1c98f71ca Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Tue, 1 Oct 2024 20:08:20 +0100 Subject: [PATCH 04/15] test: skip `getBridgeTransaction` V2 test for now --- packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol index 58123fad28..0918c66bf5 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol @@ -47,7 +47,14 @@ contract FastBridgeV2EncodingTest is FastBridgeV2Test { assertEq(decodedTx, bridgeTx); } - function test_getBridgeTransaction_supportsV2(IFastBridgeV2.BridgeTransactionV2 memory bridgeTxV2) public view { + // 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); bytes memory request = abi.encode(bridgeTxV2); IFastBridge.BridgeTransaction memory decodedTx = fastBridge.getBridgeTransaction(request); assertEq(decodedTx, extractV1(bridgeTxV2)); From 543747b2f9d480ef1d488b843ac13f8ace81ce8e Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Wed, 2 Oct 2024 10:26:40 +0100 Subject: [PATCH 05/15] test: add coverage for SRC arbitrary calls --- .../interfaces/IFastBridgeV2Errors.sol | 1 + .../test/FastBridgeV2.Src.ArbitraryCall.t.sol | 48 +++++++++++++++++++ 2 files changed, 49 insertions(+) create mode 100644 packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol diff --git a/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol b/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol index 85f3bacdcf..b1c45bc7c6 100644 --- a/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol +++ b/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.0; interface IFastBridgeV2Errors { error AmountIncorrect(); + error CallParamsLengthAboveMax(); error ChainIncorrect(); error ExclusivityParamsIncorrect(); error MsgValueIncorrect(); diff --git a/packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol new file mode 100644 index 0000000000..44a3b27a2a --- /dev/null +++ b/packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import {FastBridgeV2SrcExclusivityTest} from "./FastBridgeV2.Src.Exclusivity.t.sol"; + +// solhint-disable func-name-mixedcase, ordering +contract FastBridgeV2SrcArbitraryCallTest is FastBridgeV2SrcExclusivityTest { + bytes public constant CALL_PARAMS = abi.encode("Hello, World!"); + + function createFixturesV2() public virtual override { + super.createFixturesV2(); + tokenParamsV2.callParams = CALL_PARAMS; + ethParamsV2.callParams = CALL_PARAMS; + tokenTx.callParams = CALL_PARAMS; + ethTx.callParams = CALL_PARAMS; + } + + // Contract should accept callParams with length up to 2^16 - 1, + // so that the callParams.length is encoded in 2 bytes. + + function test_bridge_token_callParamsLengthMax() public { + bytes memory callParams = new bytes(2 ** 16 - 1); + tokenParamsV2.callParams = callParams; + tokenTx.callParams = callParams; + test_bridge_token(); + } + + function test_bridge_eth_callParamsLengthMax() public { + bytes memory callParams = new bytes(2 ** 16 - 1); + ethParamsV2.callParams = callParams; + ethTx.callParams = callParams; + test_bridge_eth(); + } + + function test_bridge_token_revert_callParamsLengthAboveMax() public { + bytes memory callParams = new bytes(2 ** 16); + tokenParamsV2.callParams = callParams; + vm.expectRevert(CallParamsLengthAboveMax.selector); + bridge({caller: userA, msgValue: 0, params: tokenParams, paramsV2: tokenParamsV2}); + } + + function test_bridge_eth_revert_callParamsLengthAboveMax() public { + bytes memory callParams = new bytes(2 ** 16); + ethParamsV2.callParams = callParams; + vm.expectRevert(CallParamsLengthAboveMax.selector); + bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams, paramsV2: ethParamsV2}); + } +} From 3200f092419d7a5ad89637d66a1c4ab70f003750 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Wed, 2 Oct 2024 10:28:18 +0100 Subject: [PATCH 06/15] feat: check callParams length when bridging --- packages/contracts-rfq/contracts/FastBridgeV2.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index 0e05f7d948..fef6e99acf 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -24,6 +24,9 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { /// @notice Minimum deadline period to relay a requested bridge transaction uint256 public constant MIN_DEADLINE_PERIOD = 30 minutes; + /// @notice Maximum length of accepted callParams + uint256 public constant MAX_CALL_PARAMS_LENGTH = 2 ** 16 - 1; + /// @notice Status of the bridge tx on origin chain mapping(bytes32 => BridgeTxDetails) public bridgeTxDetails; /// @notice Relay details on destination chain @@ -137,6 +140,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { 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(); int256 exclusivityEndTime = int256(block.timestamp) + paramsV2.quoteExclusivitySeconds; // exclusivityEndTime must be in range (0 .. params.deadline] if (exclusivityEndTime <= 0 || exclusivityEndTime > int256(params.deadline)) { From df0a3ab719509342073bc3779582ece57024257b Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Wed, 2 Oct 2024 11:44:26 +0100 Subject: [PATCH 07/15] test: non-payable ETH recipient --- .../contracts-rfq/test/FastBridgeV2.Dst.t.sol | 42 +++++++++++++++++++ .../test/mocks/NonPayableRecipient.sol | 10 +++++ 2 files changed, 52 insertions(+) create mode 100644 packages/contracts-rfq/test/mocks/NonPayableRecipient.sol diff --git a/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol index e441a9f3c4..3f5fbce95a 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.20; import {FastBridgeV2DstBaseTest, IFastBridgeV2} from "./FastBridgeV2.Dst.Base.t.sol"; +import {NonPayableRecipient} from "./mocks/NonPayableRecipient.sol"; // solhint-disable func-name-mixedcase, ordering contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { @@ -17,12 +18,21 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { uint256 chainGasAmount ); + address public nonPayableRecipient; + + function setUp() public virtual override { + super.setUp(); + nonPayableRecipient = address(new NonPayableRecipient()); + vm.label(nonPayableRecipient, "NonPayableRecipient"); + } + function expectBridgeRelayed( IFastBridgeV2.BridgeTransactionV2 memory bridgeTx, bytes32 txId, address relayer ) public + virtual { vm.expectEmit(address(fastBridge)); emit BridgeRelayed({ @@ -107,6 +117,38 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { assertEq(relayerA.balance, LEFTOVER_BALANCE); assertEq(address(fastBridge).balance, 0); } + + // ═══════════════════════════════════════════ NON PAYABLE RECIPIENT ═══════════════════════════════════════════════ + + /// @notice Should not affect the ERC20 transfer + function test_relay_token_nonPayableRecipient() public { + tokenParams.to = nonPayableRecipient; + tokenTx.destRecipient = nonPayableRecipient; + userB = nonPayableRecipient; + test_relay_token(); + } + + function test_relay_token_withRelayerAddress_nonPayableRecipient() public { + tokenParams.to = nonPayableRecipient; + tokenTx.destRecipient = nonPayableRecipient; + userB = nonPayableRecipient; + test_relay_token_withRelayerAddress(); + } + + function test_relay_eth_revert_nonPayableRecipient() public { + ethParams.to = nonPayableRecipient; + ethTx.destRecipient = nonPayableRecipient; + vm.expectRevert(); + relay({caller: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); + } + + function test_relay_eth_withRelayerAddress_revert_nonPayableRecipient() public { + ethParams.to = nonPayableRecipient; + ethTx.destRecipient = nonPayableRecipient; + vm.expectRevert(); + relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); + } + // ══════════════════════════════════════════════════ REVERTS ══════════════════════════════════════════════════════ function test_relay_revert_usedRequestV1() public { diff --git a/packages/contracts-rfq/test/mocks/NonPayableRecipient.sol b/packages/contracts-rfq/test/mocks/NonPayableRecipient.sol new file mode 100644 index 0000000000..1f53dabfd1 --- /dev/null +++ b/packages/contracts-rfq/test/mocks/NonPayableRecipient.sol @@ -0,0 +1,10 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +/// @notice Incorrectly implemented recipient mock for testing purposes. DO NOT USE IN PRODUCTION. +contract NonPayableRecipient { + /// @notice Incorrectly implemented - method is not payable. + function fastBridgeTransferReceived(address, uint256, bytes memory) external pure returns (bytes4) { + return NonPayableRecipient.fastBridgeTransferReceived.selector; + } +} From 5a2205ee58d54e0d128f5f975b79ae1e1be3016c Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Wed, 2 Oct 2024 12:06:40 +0100 Subject: [PATCH 08/15] test: no-op contract recipient --- .../contracts-rfq/test/FastBridgeV2.Dst.t.sol | 55 +++++++++++++++---- .../contracts-rfq/test/mocks/NoOpContract.sol | 8 +++ 2 files changed, 53 insertions(+), 10 deletions(-) create mode 100644 packages/contracts-rfq/test/mocks/NoOpContract.sol diff --git a/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol index 3f5fbce95a..690fc27b66 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.20; import {FastBridgeV2DstBaseTest, IFastBridgeV2} from "./FastBridgeV2.Dst.Base.t.sol"; +import {NoOpContract} from "./mocks/NoOpContract.sol"; import {NonPayableRecipient} from "./mocks/NonPayableRecipient.sol"; // solhint-disable func-name-mixedcase, ordering @@ -18,14 +19,29 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { uint256 chainGasAmount ); + address public noOpRecipient; address public nonPayableRecipient; function setUp() public virtual override { super.setUp(); + noOpRecipient = address(new NoOpContract()); + vm.label(noOpRecipient, "NoOpRecipient"); nonPayableRecipient = address(new NonPayableRecipient()); vm.label(nonPayableRecipient, "NonPayableRecipient"); } + function setTokenTestRecipient(address recipient) public { + userB = recipient; + tokenParams.to = recipient; + tokenTx.destRecipient = recipient; + } + + function setEthTestRecipient(address recipient) public { + userB = recipient; + ethParams.to = recipient; + ethTx.destRecipient = recipient; + } + function expectBridgeRelayed( IFastBridgeV2.BridgeTransactionV2 memory bridgeTx, bytes32 txId, @@ -122,33 +138,52 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { /// @notice Should not affect the ERC20 transfer function test_relay_token_nonPayableRecipient() public { - tokenParams.to = nonPayableRecipient; - tokenTx.destRecipient = nonPayableRecipient; - userB = nonPayableRecipient; + setTokenTestRecipient(nonPayableRecipient); test_relay_token(); } function test_relay_token_withRelayerAddress_nonPayableRecipient() public { - tokenParams.to = nonPayableRecipient; - tokenTx.destRecipient = nonPayableRecipient; - userB = nonPayableRecipient; + setTokenTestRecipient(nonPayableRecipient); test_relay_token_withRelayerAddress(); } function test_relay_eth_revert_nonPayableRecipient() public { - ethParams.to = nonPayableRecipient; - ethTx.destRecipient = nonPayableRecipient; + setEthTestRecipient(nonPayableRecipient); vm.expectRevert(); relay({caller: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); } function test_relay_eth_withRelayerAddress_revert_nonPayableRecipient() public { - ethParams.to = nonPayableRecipient; - ethTx.destRecipient = nonPayableRecipient; + setEthTestRecipient(nonPayableRecipient); vm.expectRevert(); relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); } + // ══════════════════════════════════════════════ NO-OP RECIPIENT ══════════════════════════════════════════════════ + + // Note: in this test, the callParams are not present, and the below test functions succeed. + // Override them in the derived tests where callParams are present to check for a revert. + + function test_relay_token_noOpRecipient_revertWhenCallParamsPresent() public virtual { + setTokenTestRecipient(noOpRecipient); + test_relay_token(); + } + + function test_relay_token_withRelayerAddress_noOpRecipient_revertWhenCallParamsPresent() public virtual { + setTokenTestRecipient(noOpRecipient); + test_relay_token_withRelayerAddress(); + } + + function test_relay_eth_noOpRecipient_revertWhenCallParamsPresent() public virtual { + setEthTestRecipient(noOpRecipient); + test_relay_eth(); + } + + function test_relay_eth_withRelayerAddress_noOpRecipient_revertWhenCallParamsPresent() public virtual { + setEthTestRecipient(noOpRecipient); + test_relay_eth_withRelayerAddress(); + } + // ══════════════════════════════════════════════════ REVERTS ══════════════════════════════════════════════════════ function test_relay_revert_usedRequestV1() public { diff --git a/packages/contracts-rfq/test/mocks/NoOpContract.sol b/packages/contracts-rfq/test/mocks/NoOpContract.sol new file mode 100644 index 0000000000..ae66cfbcce --- /dev/null +++ b/packages/contracts-rfq/test/mocks/NoOpContract.sol @@ -0,0 +1,8 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +// solhint-disable-next-line no-empty-blocks +contract NoOpContract { + /// @notice Mock needs to accept ETH + receive() external payable {} +} From 30ffab5e10adda07ad816061929006d18f7196d8 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Wed, 2 Oct 2024 12:27:51 +0100 Subject: [PATCH 09/15] test: incorrect recipients cases (does not affect base tests) --- .../contracts-rfq/test/FastBridgeV2.Dst.t.sol | 120 ++++++++++++++++++ .../mocks/ExcessiveReturnValueRecipient.sol | 15 +++ .../mocks/IncorrectReturnValueRecipient.sol | 16 +++ .../test/mocks/NoReturnValueRecipient.sol | 13 ++ 4 files changed, 164 insertions(+) create mode 100644 packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol create mode 100644 packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol create mode 100644 packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol diff --git a/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol index 690fc27b66..89ef62af91 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol @@ -2,7 +2,11 @@ pragma solidity ^0.8.20; import {FastBridgeV2DstBaseTest, IFastBridgeV2} from "./FastBridgeV2.Dst.Base.t.sol"; + +import {ExcessiveReturnValueRecipient} from "./mocks/ExcessiveReturnValueRecipient.sol"; +import {IncorrectReturnValueRecipient} from "./mocks/IncorrectReturnValueRecipient.sol"; import {NoOpContract} from "./mocks/NoOpContract.sol"; +import {NoReturnValueRecipient} from "./mocks/NoReturnValueRecipient.sol"; import {NonPayableRecipient} from "./mocks/NonPayableRecipient.sol"; // solhint-disable func-name-mixedcase, ordering @@ -19,13 +23,22 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { uint256 chainGasAmount ); + address public excessiveReturnValueRecipient; + address public incorrectReturnValueRecipient; address public noOpRecipient; + address public noReturnValueRecipient; address public nonPayableRecipient; function setUp() public virtual override { super.setUp(); + excessiveReturnValueRecipient = address(new ExcessiveReturnValueRecipient()); + vm.label(excessiveReturnValueRecipient, "ExcessiveReturnValueRecipient"); + incorrectReturnValueRecipient = address(new IncorrectReturnValueRecipient()); + vm.label(incorrectReturnValueRecipient, "IncorrectReturnValueRecipient"); noOpRecipient = address(new NoOpContract()); vm.label(noOpRecipient, "NoOpRecipient"); + noReturnValueRecipient = address(new NoReturnValueRecipient()); + vm.label(noReturnValueRecipient, "NoReturnValueRecipient"); nonPayableRecipient = address(new NonPayableRecipient()); vm.label(nonPayableRecipient, "NonPayableRecipient"); } @@ -42,6 +55,10 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { ethTx.destRecipient = recipient; } + function assertEmptyCallParams(bytes memory callParams) public pure { + assertEq(callParams.length, 0, "Invalid setup: callParams are not empty"); + } + function expectBridgeRelayed( IFastBridgeV2.BridgeTransactionV2 memory bridgeTx, bytes32 txId, @@ -134,6 +151,76 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { assertEq(address(fastBridge).balance, 0); } + // ═════════════════════════════════════ EXCESSIVE RETURN VALUE RECIPIENT ══════════════════════════════════════════ + + // Note: in this test, the callParams are not present, and the below test functions succeed. + // Override them in the derived tests where callParams are present to check for a revert. + + function test_relay_token_excessiveReturnValueRecipient_revertWhenCallParamsPresent() public virtual { + assertEmptyCallParams(tokenTx.callParams); + setTokenTestRecipient(excessiveReturnValueRecipient); + test_relay_token(); + } + + function test_relay_token_withRelayerAddress_excessiveReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + { + assertEmptyCallParams(tokenTx.callParams); + setTokenTestRecipient(excessiveReturnValueRecipient); + test_relay_token_withRelayerAddress(); + } + + function test_relay_eth_excessiveReturnValueRecipient_revertWhenCallParamsPresent() public virtual { + assertEmptyCallParams(ethTx.callParams); + setEthTestRecipient(excessiveReturnValueRecipient); + test_relay_eth(); + } + + function test_relay_eth_withRelayerAddress_excessiveReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + { + assertEmptyCallParams(ethTx.callParams); + setEthTestRecipient(excessiveReturnValueRecipient); + test_relay_eth_withRelayerAddress(); + } + + // ═════════════════════════════════════ INCORRECT RETURN VALUE RECIPIENT ══════════════════════════════════════════ + + // Note: in this test, the callParams are not present, and the below test functions succeed. + // Override them in the derived tests where callParams are present to check for a revert. + + function test_relay_token_incorrectReturnValueRecipient_revertWhenCallParamsPresent() public virtual { + assertEmptyCallParams(tokenTx.callParams); + setTokenTestRecipient(incorrectReturnValueRecipient); + test_relay_token(); + } + + function test_relay_token_withRelayerAddress_incorrectReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + { + assertEmptyCallParams(tokenTx.callParams); + setTokenTestRecipient(incorrectReturnValueRecipient); + test_relay_token_withRelayerAddress(); + } + + function test_relay_eth_incorrectReturnValueRecipient_revertWhenCallParamsPresent() public virtual { + assertEmptyCallParams(ethTx.callParams); + setEthTestRecipient(incorrectReturnValueRecipient); + test_relay_eth(); + } + + function test_relay_eth_withRelayerAddress_incorrectReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + { + assertEmptyCallParams(ethTx.callParams); + setEthTestRecipient(incorrectReturnValueRecipient); + test_relay_eth_withRelayerAddress(); + } + // ═══════════════════════════════════════════ NON PAYABLE RECIPIENT ═══════════════════════════════════════════════ /// @notice Should not affect the ERC20 transfer @@ -165,25 +252,58 @@ contract FastBridgeV2DstTest is FastBridgeV2DstBaseTest { // Override them in the derived tests where callParams are present to check for a revert. function test_relay_token_noOpRecipient_revertWhenCallParamsPresent() public virtual { + assertEmptyCallParams(tokenTx.callParams); setTokenTestRecipient(noOpRecipient); test_relay_token(); } function test_relay_token_withRelayerAddress_noOpRecipient_revertWhenCallParamsPresent() public virtual { + assertEmptyCallParams(tokenTx.callParams); setTokenTestRecipient(noOpRecipient); test_relay_token_withRelayerAddress(); } function test_relay_eth_noOpRecipient_revertWhenCallParamsPresent() public virtual { + assertEmptyCallParams(ethTx.callParams); setEthTestRecipient(noOpRecipient); test_relay_eth(); } function test_relay_eth_withRelayerAddress_noOpRecipient_revertWhenCallParamsPresent() public virtual { + assertEmptyCallParams(ethTx.callParams); setEthTestRecipient(noOpRecipient); test_relay_eth_withRelayerAddress(); } + // ═════════════════════════════════════════ NO RETURN VALUE RECIPIENT ═════════════════════════════════════════════ + + // Note: in this test, the callParams are not present, and the below test functions succeed. + // Override them in the derived tests where callParams are present to check for a revert. + + function test_relay_token_noReturnValueRecipient_revertWhenCallParamsPresent() public virtual { + assertEmptyCallParams(tokenTx.callParams); + setTokenTestRecipient(noReturnValueRecipient); + test_relay_token(); + } + + function test_relay_token_withRelayerAddress_noReturnValueRecipient_revertWhenCallParamsPresent() public virtual { + assertEmptyCallParams(tokenTx.callParams); + setTokenTestRecipient(noReturnValueRecipient); + test_relay_token_withRelayerAddress(); + } + + function test_relay_eth_noReturnValueRecipient_revertWhenCallParamsPresent() public virtual { + assertEmptyCallParams(ethTx.callParams); + setEthTestRecipient(noReturnValueRecipient); + test_relay_eth(); + } + + function test_relay_eth_withRelayerAddress_noReturnValueRecipient_revertWhenCallParamsPresent() public virtual { + assertEmptyCallParams(ethTx.callParams); + setEthTestRecipient(noReturnValueRecipient); + test_relay_eth_withRelayerAddress(); + } + // ══════════════════════════════════════════════════ REVERTS ══════════════════════════════════════════════════════ function test_relay_revert_usedRequestV1() public { diff --git a/packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol b/packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol new file mode 100644 index 0000000000..9c3be02502 --- /dev/null +++ b/packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +import {IFastBridgeRecipient} from "../../contracts/interfaces/IFastBridgeRecipient.sol"; + +/// @notice Incorrectly implemented recipient mock for testing purposes. DO NOT USE IN PRODUCTION. +contract ExcessiveReturnValueRecipient { + /// @notice Mock needs to accept ETH + receive() external payable {} + + /// @notice Incorrectly implemented - method returns excessive bytes. + function fastBridgeTransferReceived(address, uint256, bytes memory) external payable returns (bytes4, uint256) { + return (IFastBridgeRecipient.fastBridgeTransferReceived.selector, 1337); + } +} diff --git a/packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol b/packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol new file mode 100644 index 0000000000..2bf955da7f --- /dev/null +++ b/packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +import {IFastBridgeRecipient} from "../../contracts/interfaces/IFastBridgeRecipient.sol"; + +/// @notice Incorrectly implemented recipient mock for testing purposes. DO NOT USE IN PRODUCTION. +contract IncorrectReturnValueRecipient is IFastBridgeRecipient { + /// @notice Mock needs to accept ETH + receive() external payable {} + + /// @notice Incorrectly implemented - method returns incorrect value. + function fastBridgeTransferReceived(address, uint256, bytes memory) external payable returns (bytes4) { + // Flip the last bit + return IFastBridgeRecipient.fastBridgeTransferReceived.selector ^ 0x00000001; + } +} diff --git a/packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol b/packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol new file mode 100644 index 0000000000..e10c8b6ded --- /dev/null +++ b/packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol @@ -0,0 +1,13 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +// solhint-disable + +/// @notice Incorrectly implemented recipient mock for testing purposes. DO NOT USE IN PRODUCTION. +contract NoReturnValueRecipient { + /// @notice Mock needs to accept ETH + receive() external payable {} + + /// @notice Incorrectly implemented - method does not return anything. + function fastBridgeTransferReceived(address, uint256, bytes memory) external payable {} +} From 48fad078664d8046cd3b198ed6f5201ada71f7b5 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Wed, 2 Oct 2024 12:38:07 +0100 Subject: [PATCH 10/15] test: define cases for arbitrary call --- .../interfaces/IFastBridgeV2Errors.sol | 3 + .../test/FastBridgeV2.Dst.ArbitraryCall.t.sol | 262 ++++++++++++++++++ ...dgeRecipientMock.sol => RecipientMock.sol} | 6 +- 3 files changed, 270 insertions(+), 1 deletion(-) create mode 100644 packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol rename packages/contracts-rfq/test/mocks/{FastBridgeRecipientMock.sol => RecipientMock.sol} (66%) diff --git a/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol b/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol index b1c45bc7c6..7cc1423a84 100644 --- a/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol +++ b/packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol @@ -11,6 +11,9 @@ interface IFastBridgeV2Errors { error StatusIncorrect(); error ZeroAddress(); + error RecipientIncorrectReturnValue(); + error RecipientNoReturnValue(); + error DeadlineExceeded(); error DeadlineNotExceeded(); error DeadlineTooShort(); diff --git a/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol new file mode 100644 index 0000000000..5ef0b21d26 --- /dev/null +++ b/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol @@ -0,0 +1,262 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import {FastBridgeV2DstExclusivityTest, IFastBridgeV2} from "./FastBridgeV2.Dst.Exclusivity.t.sol"; +import {RecipientMock} from "./mocks/RecipientMock.sol"; + +import {Address} from "@openzeppelin/contracts/utils/Address.sol"; + +// solhint-disable func-name-mixedcase, ordering +contract FastBridgeV2DstArbitraryCallTest is FastBridgeV2DstExclusivityTest { + bytes public constant CALL_PARAMS = abi.encode("Hello, world!"); + bytes public constant REVERT_MSG = "GM, this is a revert"; + + function createFixtures() public virtual override { + // In the inherited tests userB is always used as the recipient of the tokens. + userB = address(new RecipientMock()); + vm.label(userB, "ContractRecipient"); + super.createFixtures(); + } + + function createFixturesV2() public virtual override { + super.createFixturesV2(); + tokenParamsV2.callParams = CALL_PARAMS; + ethParamsV2.callParams = CALL_PARAMS; + tokenTx.callParams = CALL_PARAMS; + ethTx.callParams = CALL_PARAMS; + } + + function setTokenTestCallParams(bytes memory callParams) public { + tokenParamsV2.callParams = callParams; + tokenTx.callParams = callParams; + } + + function setEthTestCallParams(bytes memory callParams) public { + ethParamsV2.callParams = callParams; + ethTx.callParams = callParams; + } + + /// @notice We override the "expect event" function to also check for the arbitrary call + /// made to the token recipient. + function expectBridgeRelayed( + IFastBridgeV2.BridgeTransactionV2 memory bridgeTx, + bytes32 txId, + address relayer + ) + public + virtual + override + { + vm.expectCall({callee: userB, data: getExpectedCalldata(bridgeTx), count: 1}); + super.expectBridgeRelayed(bridgeTx, txId, relayer); + } + + function mockRecipientRevert(IFastBridgeV2.BridgeTransactionV2 memory bridgeTx) public { + vm.mockCallRevert({callee: userB, data: getExpectedCalldata(bridgeTx), revertData: bytes(REVERT_MSG)}); + } + + function getExpectedCalldata(IFastBridgeV2.BridgeTransactionV2 memory bridgeTx) + public + pure + returns (bytes memory) + { + // fastBridgeTransferReceived(token, amount, callParams) + return abi.encodeCall( + RecipientMock.fastBridgeTransferReceived, (bridgeTx.destToken, bridgeTx.destAmount, CALL_PARAMS) + ); + } + + // ═══════════════════════════════════════════════ RECIPIENT EOA ═══════════════════════════════════════════════════ + + function test_relay_token_revert_recipientNotContract() public { + setTokenTestRecipient(userA); + vm.expectRevert(abi.encodeWithSelector(Address.AddressEmptyCode.selector, userA)); + relay({caller: relayerA, msgValue: 0, bridgeTx: tokenTx}); + } + + function test_relay_token_withRelayerAddress_revert_recipientNotContract() public { + setTokenTestRecipient(userA); + vm.expectRevert(abi.encodeWithSelector(Address.AddressEmptyCode.selector, userA)); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: 0, bridgeTx: tokenTx}); + } + + function test_relay_eth_revert_recipientNotContract() public { + setEthTestRecipient(userA); + vm.expectRevert(abi.encodeWithSelector(Address.AddressEmptyCode.selector, userA)); + relay({caller: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); + } + + function test_relay_eth_withRelayerAddress_revert_recipientNotContract() public { + setEthTestRecipient(userA); + vm.expectRevert(abi.encodeWithSelector(Address.AddressEmptyCode.selector, userA)); + relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); + } + + // ═════════════════════════════════════ EXCESSIVE RETURN VALUE RECIPIENT ══════════════════════════════════════════ + + function test_relay_token_excessiveReturnValueRecipient_revertWhenCallParamsPresent() public virtual override { + setTokenTestRecipient(excessiveReturnValueRecipient); + vm.expectRevert(RecipientIncorrectReturnValue.selector); + relay({caller: relayerA, msgValue: 0, bridgeTx: tokenTx}); + } + + function test_relay_token_withRelayerAddress_excessiveReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + override + { + setTokenTestRecipient(excessiveReturnValueRecipient); + vm.expectRevert(RecipientIncorrectReturnValue.selector); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: 0, bridgeTx: tokenTx}); + } + + function test_relay_eth_excessiveReturnValueRecipient_revertWhenCallParamsPresent() public virtual override { + setEthTestRecipient(excessiveReturnValueRecipient); + vm.expectRevert(RecipientIncorrectReturnValue.selector); + relay({caller: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); + } + + function test_relay_eth_withRelayerAddress_excessiveReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + override + { + setEthTestRecipient(excessiveReturnValueRecipient); + vm.expectRevert(RecipientIncorrectReturnValue.selector); + relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); + } + + // ═════════════════════════════════════ INCORRECT RETURN VALUE RECIPIENT ══════════════════════════════════════════ + + function test_relay_token_incorrectReturnValueRecipient_revertWhenCallParamsPresent() public virtual override { + setTokenTestRecipient(incorrectReturnValueRecipient); + vm.expectRevert(RecipientIncorrectReturnValue.selector); + relay({caller: relayerA, msgValue: 0, bridgeTx: tokenTx}); + } + + function test_relay_token_withRelayerAddress_incorrectReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + override + { + setTokenTestRecipient(incorrectReturnValueRecipient); + vm.expectRevert(RecipientIncorrectReturnValue.selector); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: 0, bridgeTx: tokenTx}); + } + + function test_relay_eth_incorrectReturnValueRecipient_revertWhenCallParamsPresent() public virtual override { + setEthTestRecipient(incorrectReturnValueRecipient); + vm.expectRevert(RecipientIncorrectReturnValue.selector); + relay({caller: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); + } + + function test_relay_eth_withRelayerAddress_incorrectReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + override + { + setEthTestRecipient(incorrectReturnValueRecipient); + vm.expectRevert(RecipientIncorrectReturnValue.selector); + relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); + } + + // ══════════════════════════════════════════════ NO-OP RECIPIENT ══════════════════════════════════════════════════ + + function test_relay_token_noOpRecipient_revertWhenCallParamsPresent() public virtual override { + setTokenTestRecipient(noOpRecipient); + vm.expectRevert(Address.FailedInnerCall.selector); + relay({caller: relayerA, msgValue: 0, bridgeTx: tokenTx}); + } + + function test_relay_token_withRelayerAddress_noOpRecipient_revertWhenCallParamsPresent() public virtual override { + setTokenTestRecipient(noOpRecipient); + vm.expectRevert(Address.FailedInnerCall.selector); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: 0, bridgeTx: tokenTx}); + } + + function test_relay_eth_noOpRecipient_revertWhenCallParamsPresent() public virtual override { + setEthTestRecipient(noOpRecipient); + vm.expectRevert(Address.FailedInnerCall.selector); + relay({caller: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); + } + + function test_relay_eth_withRelayerAddress_noOpRecipient_revertWhenCallParamsPresent() public virtual override { + setEthTestRecipient(noOpRecipient); + vm.expectRevert(Address.FailedInnerCall.selector); + relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); + } + + // ═════════════════════════════════════════ NO RETURN VALUE RECIPIENT ═════════════════════════════════════════════ + + function test_relay_token_noReturnValueRecipient_revertWhenCallParamsPresent() public virtual override { + setTokenTestRecipient(noReturnValueRecipient); + vm.expectRevert(RecipientNoReturnValue.selector); + relay({caller: relayerA, msgValue: 0, bridgeTx: tokenTx}); + } + + function test_relay_token_withRelayerAddress_noReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + override + { + setTokenTestRecipient(noReturnValueRecipient); + vm.expectRevert(RecipientNoReturnValue.selector); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: 0, bridgeTx: tokenTx}); + } + + function test_relay_eth_noReturnValueRecipient_revertWhenCallParamsPresent() public virtual override { + setEthTestRecipient(noReturnValueRecipient); + vm.expectRevert(RecipientNoReturnValue.selector); + relay({caller: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); + } + + function test_relay_eth_withRelayerAddress_noReturnValueRecipient_revertWhenCallParamsPresent() + public + virtual + override + { + setEthTestRecipient(noReturnValueRecipient); + vm.expectRevert(RecipientNoReturnValue.selector); + relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); + } + + // ═════════════════════════════════════════════ RECIPIENT REVERTS ═════════════════════════════════════════════════ + + function test_relay_token_revert_recipientReverts() public { + mockRecipientRevert(tokenTx); + vm.expectRevert(REVERT_MSG); + relay({caller: relayerA, msgValue: 0, bridgeTx: tokenTx}); + } + + function test_relay_token_withRelayerAddress_revert_recipientReverts() public { + mockRecipientRevert(tokenTx); + vm.expectRevert(REVERT_MSG); + relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: 0, bridgeTx: tokenTx}); + } + + function test_relay_eth_revert_recipientReverts() public { + mockRecipientRevert(ethTx); + vm.expectRevert(REVERT_MSG); + relay({caller: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); + } + + function test_relay_eth_withRelayerAddress_revert_recipientReverts() public { + mockRecipientRevert(ethTx); + vm.expectRevert(REVERT_MSG); + relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); + } + + function test_relay_eth_noCallParams_revert_recipientReverts() public { + setEthTestCallParams(""); + vm.mockCallRevert({callee: userB, data: "", revertData: bytes(REVERT_MSG)}); + vm.expectRevert("ETH transfer failed"); + 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"); + relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: ethParams.destAmount, bridgeTx: ethTx}); + } +} diff --git a/packages/contracts-rfq/test/mocks/FastBridgeRecipientMock.sol b/packages/contracts-rfq/test/mocks/RecipientMock.sol similarity index 66% rename from packages/contracts-rfq/test/mocks/FastBridgeRecipientMock.sol rename to packages/contracts-rfq/test/mocks/RecipientMock.sol index 8b732e279c..a35d4ac5ec 100644 --- a/packages/contracts-rfq/test/mocks/FastBridgeRecipientMock.sol +++ b/packages/contracts-rfq/test/mocks/RecipientMock.sol @@ -4,7 +4,11 @@ pragma solidity ^0.8.0; import {IFastBridgeRecipient} from "../../contracts/interfaces/IFastBridgeRecipient.sol"; /// @notice Recipient mock for testing purposes. DO NOT USE IN PRODUCTION. -contract FastBridgeRecipientMock is IFastBridgeRecipient { +contract RecipientMock is IFastBridgeRecipient { + /// @notice Mock needs to accept ETH + receive() external payable {} + + /// @notice Minimal viable implementation of the fastBridgeTransferReceived hook. function fastBridgeTransferReceived(address, uint256, bytes memory) external payable returns (bytes4) { return IFastBridgeRecipient.fastBridgeTransferReceived.selector; } From 4ce70d9cf8a20dea565e54ce5aa2c8aea4e6559c Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Wed, 2 Oct 2024 13:37:28 +0100 Subject: [PATCH 11/15] feat: update relay logic with arbitrary calls --- .../contracts-rfq/contracts/FastBridgeV2.sol | 48 ++++++++++++++----- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index fef6e99acf..4aba8ba87a 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -224,18 +224,29 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { address token = transaction.destToken; uint256 amount = transaction.destAmount; - uint256 rebate = chainGasAmount; - if (!transaction.sendChainGas) { - // forward erc20 - rebate = 0; + if (transaction.callParams.length == 0) { + // No arbitrary call requested, so we just transfer the tokens _pullToken(to, token, amount); - } else if (token == UniversalTokenLib.ETH_ADDRESS) { - // lump in gas rebate into amount in native gas token - _pullToken(to, token, amount + rebate); - } else { - // forward erc20 then forward gas rebate in native gas token + } else if (token != UniversalTokenLib.ETH_ADDRESS) { + // Arbitrary call requested with ERC20: transfer the tokens first _pullToken(to, token, amount); - _pullToken(to, UniversalTokenLib.ETH_ADDRESS, rebate); + // Follow up with the hook function call + _checkedCallRecipient({ + recipient: to, + msgValue: 0, + token: token, + amount: amount, + callParams: transaction.callParams + }); + } else { + // Arbitrary call requested with ETH: combine the ETH transfer with the call + _checkedCallRecipient({ + recipient: to, + msgValue: amount, + token: token, + amount: amount, + callParams: transaction.callParams + }); } emit BridgeRelayed( @@ -247,7 +258,8 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { transaction.destToken, transaction.originAmount, transaction.destAmount, - rebate + // chainGasAmount is 0 since the gas rebate function is deprecated + 0 ); } @@ -337,6 +349,20 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { } } + /// @notice Calls the Recipient's hook function with the specified callParams and performs + /// all the necessary checks for the returned value. + function _checkedCallRecipient( + address recipient, + uint256 msgValue, + address token, + uint256 amount, + bytes memory callParams + ) + internal + { + // TODO: implement + } + /// @notice Calculates time since proof submitted /// @dev proof.timestamp stores casted uint40(block.timestamp) block timestamps for gas optimization /// _timeSince(proof) can accomodate rollover case when block.timestamp > type(uint40).max but From ad7044902e50154faac6022df7d6eae756c699db Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Wed, 2 Oct 2024 13:47:20 +0100 Subject: [PATCH 12/15] feat: checked call of the recipient hook function --- packages/contracts-rfq/contracts/FastBridgeV2.sol | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index 4aba8ba87a..b1a8fda395 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -2,6 +2,7 @@ pragma solidity 0.8.24; import {SafeERC20, IERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; +import {Address} from "@openzeppelin/contracts/utils/Address.sol"; import {UniversalTokenLib} from "./libs/UniversalToken.sol"; @@ -9,6 +10,7 @@ import {Admin} from "./Admin.sol"; import {IFastBridge} from "./interfaces/IFastBridge.sol"; import {IFastBridgeV2} from "./interfaces/IFastBridgeV2.sol"; import {IFastBridgeV2Errors} from "./interfaces/IFastBridgeV2Errors.sol"; +import {IFastBridgeRecipient} from "./interfaces/IFastBridgeRecipient.sol"; /// @notice FastBridgeV2 is a contract for bridging tokens across chains. contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { @@ -360,7 +362,18 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { ) internal { - // TODO: implement + 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}); + // Explicit revert if no return data at all + if (returnData.length == 0) revert RecipientNoReturnValue(); + // Check that exactly a single return value was returned + if (returnData.length != 32) revert RecipientIncorrectReturnValue(); + // Return value should be abi-encoded hook function selector + if (bytes32(returnData) != bytes32(IFastBridgeRecipient.fastBridgeTransferReceived.selector)) { + revert RecipientIncorrectReturnValue(); + } } /// @notice Calculates time since proof submitted From 693b432edcbb44a192c58e8e9f6ca02365ef1350 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Thu, 3 Oct 2024 11:07:43 +0100 Subject: [PATCH 13/15] test: refactor state setup --- .../test/FastBridgeV2.Dst.ArbitraryCall.t.sol | 16 +------- .../test/FastBridgeV2.GasBench.Dst.Excl.t.sol | 10 +---- .../test/FastBridgeV2.GasBench.Src.t.sol | 12 +----- .../test/FastBridgeV2.Src.ArbitraryCall.t.sol | 16 +++----- ...astBridgeV2.Src.Exclusivity.Negative.t.sol | 11 ++---- .../test/FastBridgeV2.Src.Exclusivity.t.sol | 13 +------ .../contracts-rfq/test/FastBridgeV2.t.sol | 38 +++++++++++++++++++ 7 files changed, 56 insertions(+), 60 deletions(-) diff --git a/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol index 5ef0b21d26..af4f6235c6 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol @@ -20,20 +20,8 @@ contract FastBridgeV2DstArbitraryCallTest is FastBridgeV2DstExclusivityTest { function createFixturesV2() public virtual override { super.createFixturesV2(); - tokenParamsV2.callParams = CALL_PARAMS; - ethParamsV2.callParams = CALL_PARAMS; - tokenTx.callParams = CALL_PARAMS; - ethTx.callParams = CALL_PARAMS; - } - - function setTokenTestCallParams(bytes memory callParams) public { - tokenParamsV2.callParams = callParams; - tokenTx.callParams = callParams; - } - - function setEthTestCallParams(bytes memory callParams) public { - ethParamsV2.callParams = callParams; - ethTx.callParams = callParams; + setTokenTestCallParams(CALL_PARAMS); + setEthTestCallParams(CALL_PARAMS); } /// @notice We override the "expect event" function to also check for the arbitrary call diff --git a/packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol b/packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol index dc6b5953f5..eae0dffaa1 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol @@ -13,13 +13,7 @@ contract FastBridgeV2DstExclusivityTest is FastBridgeV2DstGasBenchmarkTest { } function createFixturesV2() public virtual override { - tokenParamsV2.quoteRelayer = relayerA; - tokenParamsV2.quoteExclusivitySeconds = int256(EXCLUSIVITY_PERIOD); - ethParamsV2.quoteRelayer = relayerA; - ethParamsV2.quoteExclusivitySeconds = int256(EXCLUSIVITY_PERIOD); - tokenTx.exclusivityRelayer = relayerA; - tokenTx.exclusivityEndTime = block.timestamp + EXCLUSIVITY_PERIOD; - ethTx.exclusivityRelayer = relayerA; - ethTx.exclusivityEndTime = block.timestamp + EXCLUSIVITY_PERIOD; + setTokenTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD); + setEthTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD); } } diff --git a/packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol b/packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol index 916f465475..769266cee5 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol @@ -104,11 +104,7 @@ contract FastBridgeV2GasBenchmarkSrcTest is FastBridgeV2SrcBaseTest { } function test_bridge_token_withExclusivity() public { - tokenParamsV2.quoteRelayer = relayerA; - tokenParamsV2.quoteExclusivitySeconds = int256(EXCLUSIVITY_PERIOD); - tokenParamsV2.quoteId = bytes("Created by Relayer A"); - tokenTx.exclusivityRelayer = relayerA; - tokenTx.exclusivityEndTime = block.timestamp + EXCLUSIVITY_PERIOD; + setTokenTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD); bridge({caller: userA, msgValue: 0, params: tokenParams, paramsV2: tokenParamsV2}); assertEq(fastBridge.bridgeStatuses(getTxId(tokenTx)), IFastBridgeV2.BridgeStatus.REQUESTED); assertEq(srcToken.balanceOf(userA), initialUserBalanceToken - tokenParams.originAmount); @@ -185,11 +181,7 @@ contract FastBridgeV2GasBenchmarkSrcTest is FastBridgeV2SrcBaseTest { } function test_bridge_eth_withExclusivity() public { - ethParamsV2.quoteRelayer = relayerA; - ethParamsV2.quoteExclusivitySeconds = int256(EXCLUSIVITY_PERIOD); - ethParamsV2.quoteId = bytes("Created by Relayer A"); - ethTx.exclusivityRelayer = relayerA; - ethTx.exclusivityEndTime = block.timestamp + EXCLUSIVITY_PERIOD; + setEthTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD); bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams, paramsV2: ethParamsV2}); assertEq(fastBridge.bridgeStatuses(getTxId(ethTx)), IFastBridgeV2.BridgeStatus.REQUESTED); assertEq(userA.balance, initialUserBalanceEth - ethParams.originAmount); diff --git a/packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol index 44a3b27a2a..2b1e83a30c 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol @@ -9,10 +9,8 @@ contract FastBridgeV2SrcArbitraryCallTest is FastBridgeV2SrcExclusivityTest { function createFixturesV2() public virtual override { super.createFixturesV2(); - tokenParamsV2.callParams = CALL_PARAMS; - ethParamsV2.callParams = CALL_PARAMS; - tokenTx.callParams = CALL_PARAMS; - ethTx.callParams = CALL_PARAMS; + setTokenTestCallParams(CALL_PARAMS); + setEthTestCallParams(CALL_PARAMS); } // Contract should accept callParams with length up to 2^16 - 1, @@ -20,28 +18,26 @@ contract FastBridgeV2SrcArbitraryCallTest is FastBridgeV2SrcExclusivityTest { function test_bridge_token_callParamsLengthMax() public { bytes memory callParams = new bytes(2 ** 16 - 1); - tokenParamsV2.callParams = callParams; - tokenTx.callParams = callParams; + setTokenTestCallParams(callParams); test_bridge_token(); } function test_bridge_eth_callParamsLengthMax() public { bytes memory callParams = new bytes(2 ** 16 - 1); - ethParamsV2.callParams = callParams; - ethTx.callParams = callParams; + setEthTestCallParams(callParams); test_bridge_eth(); } function test_bridge_token_revert_callParamsLengthAboveMax() public { bytes memory callParams = new bytes(2 ** 16); - tokenParamsV2.callParams = callParams; + setTokenTestCallParams(callParams); vm.expectRevert(CallParamsLengthAboveMax.selector); bridge({caller: userA, msgValue: 0, params: tokenParams, paramsV2: tokenParamsV2}); } function test_bridge_eth_revert_callParamsLengthAboveMax() public { bytes memory callParams = new bytes(2 ** 16); - ethParamsV2.callParams = callParams; + setEthTestCallParams(callParams); vm.expectRevert(CallParamsLengthAboveMax.selector); bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams, paramsV2: ethParamsV2}); } diff --git a/packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol index 0b19f097f3..4eeacb2f88 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol @@ -8,16 +8,13 @@ contract FastBridgeV2SrcExclusivityNegativeTest is FastBridgeV2SrcTest { uint256 public constant EXCLUSIVITY_PERIOD_ABS = 60 seconds; function createFixturesV2() public virtual override { - tokenParamsV2.quoteRelayer = relayerA; + // Populate the fields using the absolute exclusivity period + setTokenTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD_ABS); + setEthTestExclusivityParams(relayerB, EXCLUSIVITY_PERIOD_ABS); + // Override with negative exclusivity period tokenParamsV2.quoteExclusivitySeconds = -int256(EXCLUSIVITY_PERIOD_ABS); - tokenParamsV2.quoteId = bytes("Created by Relayer A"); - ethParamsV2.quoteRelayer = relayerB; ethParamsV2.quoteExclusivitySeconds = -int256(EXCLUSIVITY_PERIOD_ABS); - ethParamsV2.quoteId = bytes("Created by Relayer B"); - - tokenTx.exclusivityRelayer = relayerA; tokenTx.exclusivityEndTime = block.timestamp - EXCLUSIVITY_PERIOD_ABS; - ethTx.exclusivityRelayer = relayerB; ethTx.exclusivityEndTime = block.timestamp - EXCLUSIVITY_PERIOD_ABS; } diff --git a/packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol b/packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol index 259ef0bacd..230c24778c 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol @@ -8,17 +8,8 @@ contract FastBridgeV2SrcExclusivityTest is FastBridgeV2SrcTest { uint256 public constant EXCLUSIVITY_PERIOD = 60 seconds; function createFixturesV2() public virtual override { - tokenParamsV2.quoteRelayer = relayerA; - tokenParamsV2.quoteExclusivitySeconds = int256(EXCLUSIVITY_PERIOD); - tokenParamsV2.quoteId = bytes("Created by Relayer A"); - ethParamsV2.quoteRelayer = relayerB; - ethParamsV2.quoteExclusivitySeconds = int256(EXCLUSIVITY_PERIOD); - ethParamsV2.quoteId = bytes("Created by Relayer B"); - - 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); } function bridge(address caller, uint256 msgValue, IFastBridge.BridgeParams memory params) public virtual override { diff --git a/packages/contracts-rfq/test/FastBridgeV2.t.sol b/packages/contracts-rfq/test/FastBridgeV2.t.sol index 96b987fe7f..8011f8a8e2 100644 --- a/packages/contracts-rfq/test/FastBridgeV2.t.sol +++ b/packages/contracts-rfq/test/FastBridgeV2.t.sol @@ -163,6 +163,34 @@ abstract contract FastBridgeV2Test is Test, IFastBridgeV2Errors { txV2.nonce = txV1.nonce; } + function setTokenTestCallParams(bytes memory callParams) public { + tokenParamsV2.callParams = callParams; + tokenTx.callParams = callParams; + } + + function setTokenTestExclusivityParams(address relayer, uint256 exclusivitySeconds) public { + tokenParamsV2.quoteRelayer = relayer; + tokenParamsV2.quoteExclusivitySeconds = int256(exclusivitySeconds); + tokenParamsV2.quoteId = bytes.concat("Token:", getMockQuoteId(relayer)); + + tokenTx.exclusivityRelayer = relayer; + tokenTx.exclusivityEndTime = block.timestamp + exclusivitySeconds; + } + + function setEthTestCallParams(bytes memory callParams) public { + ethParamsV2.callParams = callParams; + ethTx.callParams = callParams; + } + + function setEthTestExclusivityParams(address relayer, uint256 exclusivitySeconds) public { + ethParamsV2.quoteRelayer = relayer; + ethParamsV2.quoteExclusivitySeconds = int256(exclusivitySeconds); + ethParamsV2.quoteId = bytes.concat("ETH:", getMockQuoteId(relayer)); + + ethTx.exclusivityRelayer = relayer; + ethTx.exclusivityEndTime = block.timestamp + exclusivitySeconds; + } + function extractV1(IFastBridgeV2.BridgeTransactionV2 memory txV2) public pure @@ -182,6 +210,16 @@ abstract contract FastBridgeV2Test is Test, IFastBridgeV2Errors { txV1.nonce = txV2.nonce; } + function getMockQuoteId(address relayer) public view returns (bytes memory) { + if (relayer == relayerA) { + return bytes("created by Relayer A"); + } else if (relayer == relayerB) { + return bytes("created by Relayer B"); + } else { + return bytes("created by unknown relayer"); + } + } + function getTxId(IFastBridgeV2.BridgeTransactionV2 memory bridgeTx) public pure returns (bytes32) { return keccak256(abi.encode(bridgeTx)); } From 0a222fe650b8ce424a7f6d06d06f6419119712ed Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Thu, 3 Oct 2024 12:18:04 +0100 Subject: [PATCH 14/15] refactor: better comments in `relay` --- packages/contracts-rfq/contracts/FastBridgeV2.sol | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index b1a8fda395..b952afa61d 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -226,11 +226,14 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { 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 transfer the tokens + // 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: transfer the tokens first + // 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({ From 02ba663b3fc9c3cdfc269146de9a5e9c03421d60 Mon Sep 17 00:00:00 2001 From: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com> Date: Mon, 7 Oct 2024 15:04:21 +0100 Subject: [PATCH 15/15] docs: add a TODO note wrt encoding changes --- packages/contracts-rfq/contracts/FastBridgeV2.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/contracts-rfq/contracts/FastBridgeV2.sol b/packages/contracts-rfq/contracts/FastBridgeV2.sol index b952afa61d..1a64515a33 100644 --- a/packages/contracts-rfq/contracts/FastBridgeV2.sol +++ b/packages/contracts-rfq/contracts/FastBridgeV2.sol @@ -127,6 +127,9 @@ 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));