feat(contracts-rfq): arbitrary calls without additional native value [SLT-233] #3215
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol
Dismissed
Show dismissed
Hide dismissed
WalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Changes to gas cost
🧾 Summary (50% most significant diffs)
Full diff report 👇
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3215 +/- ##
====================================================
+ Coverage 24.92918% 90.78014% +65.85096%
====================================================
Files 198 60 -138
Lines 13061 1269 -11792
Branches 82 154 +72
====================================================
- Hits 3256 1152 -2104
+ Misses 9523 114 -9409
+ Partials 282 3 -279
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (20)
packages/contracts-rfq/test/mocks/NoOpContract.sol (1)
4-4: Consider removing the solhint-disable commentThe
solhint-disable-next-line no-empty-blockscomment is used to suppress a linter warning about empty contract bodies. However, since the contract isn't empty (it contains areceivefunction), this comment may be unnecessary.Consider removing this line if the linter doesn't raise any warnings without it.
packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol (1)
5-12: LGTM: Function signature aligns with PR objectives. Consider adding documentation.The
fastBridgeTransferReceivedfunction is well-designed to handle arbitrary calls after token transfer, as described in the PR objectives. Thepayablemodifier andcallParamsparameter provide the necessary flexibility.Consider adding NatSpec documentation to clarify:
- The expected behavior when native tokens are sent (i.e., when
msg.value > 0).- The meaning of the
bytes4return value.- Any requirements or expectations for the
callParams.Example:
/// @notice Handles the receipt of tokens via the fast bridge, with support for arbitrary calls. /// @param token The address of the transferred token /// @param amount The amount of tokens transferred /// @param callParams Additional parameters for arbitrary calls /// @return A bytes4 value indicating [DESCRIBE THE MEANING OF THE RETURN VALUE] function fastBridgeTransferReceived( address token, uint256 amount, bytes memory callParams ) external payable returns (bytes4);packages/contracts-rfq/test/mocks/NonPayableRecipient.sol (1)
1-10: Consider expanding test coverage with additional mock contracts.This
NonPayableRecipientmock is valuable for testing the "Arbitrary Call" feature, specifically for the scenario where the recipient contract doesn't implement a payablefastBridgeTransferReceivedfunction. This aligns well with the PR objectives and helps ensure robust error handling.To further enhance test coverage, consider creating additional mock contracts for other scenarios, such as:
- A contract that implements the function correctly (payable and with proper logic).
- A contract that reverts during the function call.
- A contract that implements the function but with high gas consumption.
These additional mocks would allow for comprehensive testing of all scenarios mentioned in the PR objectives, ensuring the robustness of the new feature.
packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol (2)
4-4: Consider using more specific solhint-disable directives.The current
solhint-disabledirective disables all linter rules for the entire file. It's generally better to disable only specific rules or to disable rules for specific lines or functions. This ensures that other potential issues can still be caught by the linter.If you're disabling specific rules, consider replacing this line with more targeted directives, such as:
// solhint-disable-next-line no-empty-blocksplaced directly above the functions that need it.
11-12: LGTM: Correctly implemented mock function with clear documentation.The
fastBridgeTransferReceivedfunction is correctly implemented as a mock for testing purposes. The empty body and the lack of a return value are intentional, as noted in the comment.A minor suggestion to improve the comment:
Consider expanding the comment to explicitly state the expected correct behavior. This could help developers understand the purpose of the mock more quickly. For example:
/// @notice Incorrectly implemented - method does not return anything. /// In a correct implementation, this method should return a boolean value.packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (3)
14-14: LGTM: New error for incorrect recipient return value. Consider adding a comment.The addition of
RecipientIncorrectReturnValueerror is appropriate for handling cases where thedestRecipient.fastBridgeTransferReceived()function returns an incorrect value.Consider adding a brief comment explaining what constitutes an "incorrect" return value for clarity:
+ /// @dev Thrown when the recipient's fastBridgeTransferReceived() function returns a value other than the expected one error RecipientIncorrectReturnValue();
15-15: LGTM: New error for missing recipient return value. Consider adding a comment.The addition of
RecipientNoReturnValueerror is appropriate for handling cases where thedestRecipient.fastBridgeTransferReceived()function doesn't return a value when one is expected.Consider adding a brief comment explaining the scenario when this error is thrown:
+ /// @dev Thrown when the recipient's fastBridgeTransferReceived() function doesn't return a value when one is expected error RecipientNoReturnValue();
6-16: Overall, the new errors enhance error handling for the arbitrary call feature. Consider adding documentation.The three new errors (
CallParamsLengthAboveMax,RecipientIncorrectReturnValue, andRecipientNoReturnValue) are well-aligned with the PR objectives and will improve error handling for the new arbitrary call feature.Consider adding a brief comment block at the beginning of the interface to explain the context of these errors:
/** * @title IFastBridgeV2Errors * @dev Interface for FastBridgeV2 error declarations * @notice This interface includes errors related to the arbitrary call feature, * which allows for invoking a special function hook after token transfer */ interface IFastBridgeV2Errors { // ... (existing code) }packages/contracts-rfq/test/mocks/RecipientMock.sol (2)
6-7: LGTM: Clear contract declaration with appropriate warning.The contract is well-named and correctly implements the
IFastBridgeRecipientinterface. The warning comment is crucial for preventing misuse in production.Consider adding a brief description of the contract's purpose and how it's intended to be used in tests. This can help other developers understand its role more quickly.
11-14: LGTM: Correct implementation of fastBridgeTransferReceived with minor suggestion.The
fastBridgeTransferReceivedfunction is correctly implemented according to theIFastBridgeRecipientinterface. It's properly marked asexternalandpayable, and returns the function selector as expected for a minimal viable implementation.Consider adding underscore prefixes to the unused parameters for clarity:
- function fastBridgeTransferReceived(address, uint256, bytes memory) external payable returns (bytes4) { + function fastBridgeTransferReceived(address _, uint256 _, bytes memory _) external payable returns (bytes4) {This change would make it explicit that these parameters are intentionally unused in this mock implementation.
packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol (3)
6-7: Approved: Clear contract naming and crucial warning comment.The contract name
ExcessiveReturnValueRecipientis descriptive and follows proper naming conventions. The comment explicitly warning against production use is essential and well-placed.Consider adding more details about the contract's purpose in the comment, such as mentioning it's used for testing error handling in the FastBridge system.
11-14: Approved: Correctly implemented mock function for testing error scenarios.The
fastBridgeTransferReceivedfunction is well-implemented for its purpose as a mock to test error handling. It correctly returns excessive data as intended.Consider enhancing the comment to explicitly state the purpose of this incorrect implementation, e.g., "Incorrectly implemented to return excessive data for testing error handling in the FastBridge system."
1-15: Overall assessment: Well-implemented mock contract for testing purposes.This mock contract,
ExcessiveReturnValueRecipient, is well-structured and serves its intended purpose for testing error scenarios in the FastBridge system. Key points:
- Clear warnings against production use.
- Correct implementation of the
receivefunction for accepting ETH.- Intentionally incorrect implementation of
fastBridgeTransferReceivedfor testing.The code quality is good, with descriptive naming and helpful comments. Minor suggestions have been made to enhance the documentation further.
Ensure that comprehensive unit tests are written to utilize this mock contract effectively in testing error handling and edge cases in the FastBridge system.
packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol (2)
11-15: LGTM: Mock function correctly implements incorrect behavior.The
fastBridgeTransferReceivedfunction is properly implemented to return an incorrect value, serving its purpose as a mock for testing error scenarios. The comment clearly indicates that this is an incorrect implementation.Consider adding a
revertstatement with a clear error message if this contract is accidentally used in a non-test environment:function fastBridgeTransferReceived(address, uint256, bytes memory) external payable returns (bytes4) { if (block.chainid != 31337) { // Hardhat's default chain ID revert("IncorrectReturnValueRecipient: Do not use in production"); } // Existing code... }This additional check would provide an extra layer of protection against accidental misuse in a production environment.
1-16: LGTM: Contract effectively serves its purpose as a test mock.The
IncorrectReturnValueRecipientcontract is well-designed to test error scenarios for the new "Arbitrary Call" feature. It correctly implements theIFastBridgeRecipientinterface with intentional errors, which is valuable for thorough testing.Consider adding a brief comment explaining the specific test scenario this mock is designed for. For example:
/// @notice Incorrectly implemented recipient mock for testing purposes. DO NOT USE IN PRODUCTION. /// @dev This mock is specifically designed to test the behavior of the FastBridge contract /// when a recipient returns an incorrect selector value. contract IncorrectReturnValueRecipient is IFastBridgeRecipient { // ... existing code ... }This additional context could help other developers understand the purpose of this mock more quickly.
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (2)
36-36: Consider expanding the documentation forcallParams.The added comment provides a basic description of the
callParamsfield. However, to improve clarity and ease of use, consider expanding the documentation to include:
- The expected format of
callParams.- Any limitations or restrictions on the arbitrary calls.
- How these parameters interact with the
fastBridgeTransferReceived()function mentioned in the PR description.This additional information would help developers understand how to properly use this new feature.
62-62: LGTM: Addition ofcallParamsto BridgeTransactionV2 struct.The inclusion of
bytes callParamsin theBridgeTransactionV2struct is appropriate and consistent with the changes made toBridgeParamsV2. This addition enables the passing of arbitrary call parameters during the bridge transaction, aligning with the PR objectives.However, there's a TODO comment above the struct definition suggesting a potential optimization:
// TODO: consider changing the encoding scheme to prevent spending extra gas on decoding.It might be worth addressing this TODO in a follow-up task to optimize gas usage, especially considering the addition of the new
callParamsfield. Consider creating an issue to track this potential optimization.Would you like me to create a GitHub issue to track the potential encoding scheme optimization mentioned in the TODO comment?
packages/contracts-rfq/test/FastBridgeV2.t.sol (1)
127-138: LGTM. Consider adding test cases for non-emptycallParams.The changes to
createFixturesV2align well with the PR objectives. The addition ofquoteIdandcallParamsas byte arrays provides the necessary structure for testing the new "Arbitrary Call" feature.To ensure comprehensive testing of the new feature:
- Consider adding test cases where
callParamsis non-empty to verify the behavior of arbitrary calls.- It might be beneficial to create helper functions that generate various
callParamsfor different test scenarios.Example:
function generateSampleCallParams() internal pure returns (bytes memory) { // Generate sample call parameters for testing return abi.encodeWithSignature("sampleFunction(uint256,address)", 123, address(0x123)); }packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (1)
Line range hint
55-61: Address the TODO and determine the necessity of the skipped testLines 56-57 include a TODO comment to reevaluate the necessity of
test_getBridgeTransaction_supportsV2if or when the encoding scheme is changed. Currently, the test is being skipped withvm.skip(true);. Consider addressing this TODO by either updating the test to reflect the current encoding scheme or removing it if it is no longer needed.Would you like assistance in updating this test or opening a GitHub issue to track this task?
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
263-264: Consider removing deprecated code for claritySince the gas rebate function is deprecated, consider removing related code (e.g.,
chainGasAmountparameter) to improve maintainability and readability of the contract.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (16)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (8 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol (1 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (2 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (3 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (1 hunks)
- packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol (1 hunks)
- packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol (1 hunks)
- packages/contracts-rfq/test/mocks/NoOpContract.sol (1 hunks)
- packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol (1 hunks)
- packages/contracts-rfq/test/mocks/NonPayableRecipient.sol (1 hunks)
- packages/contracts-rfq/test/mocks/RecipientMock.sol (1 hunks)
🧰 Additional context used
🪛 GitHub Check: Slither
packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol
[warning] 2-2: Incorrect versions of Solidity
Version constraint ^0.8.0 contains known severe issues (https://solidity.readthedocs.io/en/latest/bugs.html)
- FullInlinerNonExpressionSplitArgumentEvaluationOrder
- MissingSideEffectsOnSelectorAccess
- AbiReencodingHeadOverflowWithStaticArrayCleanup
- DirtyBytesArrayToStorage
- DataLocationChangeInInternalOverride
- NestedCalldataArrayAbiReencodingSizeValidation
- SignedImmutables
- ABIDecodeTwoDimensionalArrayMemory
- KeccakCaching.
It is used by:
- ^0.8.0 (contracts/interfaces/IFastBridgeRecipient.sol#2)
- ^0.8.0 (contracts/interfaces/IFastBridgeV2Errors.sol#2)
🔇 Additional comments (34)
packages/contracts-rfq/test/mocks/NoOpContract.sol (1)
5-8: LGTM: NoOpContract implementation is correct and aligns with PR objectivesThe
NoOpContractimplementation is simple and correct:
- The contract name clearly indicates its purpose as a no-operation contract.
- The
receive()function is correctly implemented to accept ETH, which aligns with the PR objectives of allowing arbitrary calls with native value.- The comment above the
receive()function provides a clear explanation of its purpose.This mock contract will be useful for testing the new "Arbitrary Call" feature described in the PR objectives.
packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol (2)
1-1: LGTM: SPDX license identifier is present and correct.The MIT license is appropriate for this interface file.
4-4: LGTM: Interface declaration is clear and follows naming conventions.The
IFastBridgeRecipientinterface is well-named, clearly indicating its purpose in the fast bridge system.packages/contracts-rfq/test/mocks/NonPayableRecipient.sol (2)
1-5: LGTM: File structure and contract declaration are appropriate.The file structure follows best practices with the SPDX license identifier and pragma directive. The contract name
NonPayableRecipientaccurately reflects its purpose as a mock implementation. The comment clearly indicates that this is for testing purposes only and should not be used in production.
6-9: Function implementation aligns with testing requirements.The
fastBridgeTransferReceivedfunction is correctly implemented for its intended purpose as a mock:
- The function signature matches the expected interface for the "Arbitrary Call" feature.
- It's intentionally not marked as
payable, which aligns with the comment and the contract name.- Returning the function selector is appropriate for interface testing.
This implementation will allow testing of scenarios where the recipient contract doesn't properly implement the required interface, which is crucial for thorough testing of the new feature.
packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol (3)
6-7: LGTM: Clear contract naming and crucial documentation.The contract name
NoReturnValueRecipientis descriptive and the accompanying notice comment is essential. It clearly warns that this is an incorrectly implemented mock for testing purposes and should not be used in production. This is excellent practice for preventing misuse of test contracts.
8-9: LGTM: Correctly implementedreceivefunction with clear documentation.The
receivefunction is correctly implemented to accept ETH payments, which is necessary for this mock contract. The comment clearly explains the purpose of this function in the context of the mock.
1-13: Overall assessment: Well-implemented mock contract for testing purposes.This mock contract,
NoReturnValueRecipient, is well-designed for its intended purpose of testing. It correctly implements a scenario where thefastBridgeTransferReceivedfunction doesn't return a value, which is crucial for testing error handling in the main implementation as described in the PR objectives.Key points:
- The contract is clearly marked as a mock and not for production use.
- The
receivefunction allows the contract to accept ETH, as required.- The
fastBridgeTransferReceivedfunction matches the signature described in the PR objectives and intentionally lacks a return value for testing purposes.The implementation aligns well with the PR's goal of introducing the "Arbitrary Call" feature and testing its behavior with different types of recipients.
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (1)
6-6: LGTM: New error for call parameters length validation.The addition of
CallParamsLengthAboveMaxerror is appropriate for the new feature allowing arbitrary calls. It will help in validating the length ofcallParams, ensuring they don't exceed a maximum limit.packages/contracts-rfq/test/mocks/RecipientMock.sol (2)
1-4: LGTM: Appropriate license, Solidity version, and import statement.The contract header is well-structured with a proper license identifier and a recent, secure Solidity version. The import statement correctly references the
IFastBridgeRecipientinterface from a relative path.
8-9: LGTM: Correctly implemented receive function with clear documentation.The
receivefunction is properly implemented to accept ETH, which is necessary for testing scenarios involving ETH transfers. The comment clearly explains its purpose.packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol (2)
1-4: LGTM: Appropriate license, Solidity version, and import statement.The contract header is well-structured with the correct license identifier and a suitable Solidity version. The import statement for the
IFastBridgeRecipientinterface is correctly implemented.
8-9: LGTM: Correctly implementedreceivefunction with appropriate comment.The
receivefunction is properly implemented to allow the contract to accept ETH, which is necessary for testing purposes. The comment clearly explains its purpose.packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol (2)
1-7: LGTM: Contract setup and import are correct.The contract setup, including the license, Solidity version, import statement, and contract declaration, is appropriate for its intended purpose as a mock for testing.
8-9: LGTM: Receive function is correctly implemented.The
receive()function is properly implemented to allow the contract to accept ETH. This is necessary for the mock's functionality in test scenarios.packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
41-41: LGTM: Addition ofcallParamsto BridgeParamsV2 struct.The inclusion of
bytes callParamsin theBridgeParamsV2struct is well-implemented. It allows for flexible data passing for arbitrary calls while maintaining backward compatibility by adding it at the end of the struct. This change aligns with the PR objectives and enhances the functionality of the bridge transaction process.packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol (1)
Line range hint
1-180: Ensure comprehensive test coverage for the new functionality.While the existing tests remain valid and unaffected by the addition of the
callParamsfield, there's a noticeable absence of tests specifically targeting this new functionality. To maintain robust test coverage and ensure the reliability of the new feature, consider the following:
- Add new test cases that explicitly use non-empty
callParamsvalues.- Verify the behavior of the system when
callParamsis provided vs. when it's empty.- Test edge cases and potential error scenarios related to the new
callParamsfunctionality.To assess the current test coverage, you can run the following script:
#!/bin/bash # Description: Analyze test coverage for callParams # Test: Check for usage of callParams in test assertions or function calls rg --type solidity -i '(assert|expect).*callParams|callParams.*\(' packages/contracts-rfq/test/ # Test: Look for any mentions of callParams in test files rg --type solidity -i 'callParams' packages/contracts-rfq/test/If these commands don't yield many results, it suggests that additional tests focusing on
callParamsare needed to ensure comprehensive coverage of the new functionality.packages/contracts-rfq/test/FastBridgeV2.t.sol (1)
Line range hint
1-193: Verify test coverage for the new feature in other test files.The changes to
createFixturesV2provide a good foundation for testing the new "Arbitrary Call" feature. However, it's important to ensure that comprehensive tests are implemented in other test files that use these fixtures.To confirm adequate test coverage, please run the following script:
This will help ensure that the new feature is thoroughly tested across the test suite.
✅ Verification successful
Test coverage for the new feature is adequate.
The
callParamsfield is thoroughly tested across multiple test files, including:
FastBridgeV2.t.solFastBridgeV2.Src.ArbitraryCall.t.solFastBridgeV2.Dst.ArbitraryCall.t.solFastBridgeV2.Encoding.t.solComprehensive test functions cover various scenarios, ensuring the new "Arbitrary Call" feature is well-validated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for test cases using the new callParams field # Search for test cases using callParams echo "Searching for test cases using callParams:" rg --type solidity -i "callParams" packages/contracts-rfq/test/ # Search for new test functions related to arbitrary calls echo "Searching for new test functions related to arbitrary calls:" rg --type solidity -i "test.*arbitrarycall|test.*callparams" packages/contracts-rfq/test/Length of output: 447
Script:
#!/bin/bash # Description: Check for test cases using the new callParams field # Search for test cases using callParams in Solidity files echo "Searching for test cases using callParams:" rg -g "*.sol" -i "callParams" packages/contracts-rfq/test/ # Search for new test functions related to arbitrary calls in Solidity files echo "Searching for new test functions related to arbitrary calls:" rg -g "*.sol" -i "test.*arbitrarycall|test.*callparams" packages/contracts-rfq/test/Length of output: 21341
packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol (3)
10-16: Fixture setup correctly initializescallParamsThe
createFixturesV2()function properly initializes thecallParamsfor both token and ETH transactions. This ensures that the tests are set up with the necessary parameters to validate the newcallParamsfeature.
35-40: Test accurately asserts revert whencallParamslength exceeds maximumThe
test_bridge_token_revert_callParamsLengthAboveMax()function correctly verifies that the contract reverts withCallParamsLengthAboveMaxwhencallParamsexceeds the maximum allowed length. This ensures that length constraints are properly enforced.
42-47: ETH bridge test correctly handles excessivecallParamslengthThe
test_bridge_eth_revert_callParamsLengthAboveMax()function appropriately checks that exceeding the maximumcallParamslength in an ETH bridge transaction results in the expected revert. This maintains consistent behavior across transaction types.packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (1)
55-55: Verify if theviewmodifier should be retainedIn line 55, the function
test_getBridgeTransaction_supportsV2no longer includes theviewmodifier. If the function does not modify state, consider adding theviewmodifier back to this function for clarity and potential gas savings.Apply this diff to add the
viewmodifier:-function test_getBridgeTransaction_supportsV2(IFastBridgeV2.BridgeTransactionV2 memory bridgeTxV2) public { +function test_getBridgeTransaction_supportsV2(IFastBridgeV2.BridgeTransactionV2 memory bridgeTxV2) public view {packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (8)
6-10: Import statements correctly include required mock contracts.The imported mock contracts are appropriately included to test various recipient behaviors.
26-30: Declaration of public address variables for mock recipients is appropriate.The public address variables for the mock recipients are correctly declared, facilitating testing across different scenarios.
32-44:setUpfunction properly initializes and labels mock recipient contracts.The
setUpfunction correctly instantiates each mock recipient contract and assigns labels for clarity during testing.
46-50:setTokenTestRecipientfunction correctly updates token test parameters.This function appropriately updates
userB,tokenParams.to, andtokenTx.destRecipientto the provided recipient address for token tests.
52-56:setEthTestRecipientfunction correctly updates ETH test parameters.This function appropriately updates
userB,ethParams.to, andethTx.destRecipientto the provided recipient address for ETH tests.
58-60:assertEmptyCallParamsfunction ensurescallParamsare empty as expected.The function correctly asserts that
callParamshave zero length, which is essential for tests that require no call parameters.
68-68: Addingvirtualkeyword toexpectBridgeRelayedallows method overriding.Marking
expectBridgeRelayedasvirtualenables derived contracts to override this function, enhancing test flexibility.
226-248: Test functions forNonPayableRecipientcorrectly handle non-payable recipient scenarios.The tests appropriately verify that token transfers succeed to non-payable recipients, while ETH transfers revert as expected due to the recipient's non-payable status.
packages/contracts-rfq/contracts/FastBridgeV2.sol (4)
5-5: Approve the addition of the Address library importThe import of
{Address}from OpenZeppelin is appropriate for using low-level address functions safely, especially sinceAddress.functionCallWithValueis used later in the contract.
13-13: Approve the import of IFastBridgeRecipient interfaceImporting
IFastBridgeRecipientis necessary to interact with the recipient's hook functionfastBridgeTransferReceived.
29-31: Approve the definition of MAX_CALL_PARAMS_LENGTHDefining a maximum length for
callParamshelps prevent potential out-of-gas errors and mitigates denial-of-service attacks by limiting the size of the data. The value2 ** 16 - 1(65535) is reasonable.
53-58: Approve the addition of default parameters in bridge functionAdding default values for
paramsV2ensures backward compatibility when thebridgefunction is called without specifyingparamsV2.
packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol
Outdated
Show resolved
Hide resolved
packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol
Outdated
Show resolved
Hide resolved
| /// @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 | ||
| { | ||
| 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(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding reentrancy protection to external calls
The _checkedCallRecipient function performs an external call to recipient using Address.functionCallWithValue, which could introduce reentrancy vulnerabilities. Although tokens are transferred before this call, adding reentrancy protection enhances security.
Consider adding the nonReentrant modifier by inheriting from OpenZeppelin's ReentrancyGuard and applying it to the relay function.
Apply this diff to add reentrancy protection:
+import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
-contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors {
+contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors, ReentrancyGuard {
...
function relay(bytes memory request, address relayer) public payable
+ nonReentrant
{
...
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
I'm also a bit surprised this isn't non-reenrant. Can you explain why?
There was a problem hiding this comment.
@trajan0x, the relay function is not marked as nonReentrant because it follows the checks-effects-interactions pattern to mitigate reentrancy risks. All state changes are performed before any external calls are made. Specifically, before calling _checkedCallRecipient, the contract updates the bridgeRelayDetails mapping, ensuring that reentrancy vulnerabilities are minimized.
Here's the relevant part of the code:
// Mark bridge transaction as relayed
bridgeRelayDetails[transactionId] = BridgeRelay({
blockNumber: uint48(block.number),
blockTimestamp: uint48(block.timestamp),
relayer: relayer
});
// 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
});
} 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
});
}By updating the bridgeRelayDetails before any external calls, the contract ensures that any attempted reentrancy attacks cannot manipulate critical state variables.
However, adding the nonReentrant modifier to the relay function would provide an additional layer of security. It acts as a safeguard against any unforeseen reentrancy vulnerabilities that might arise from future changes or edge cases not covered by the current implementation.
To add reentrancy protection, you can inherit from OpenZeppelin's ReentrancyGuard and apply the nonReentrant modifier to the relay function:
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors, ReentrancyGuard {
...
function relay(bytes memory request, address relayer) public payable
nonReentrant
{
...
}
}Implementing this change would enhance the contract's security by enforcing non-reentrancy at the function level, ensuring the contract remains robust against potential reentrancy attacks.
There was a problem hiding this comment.
@trajan0x this PR doesn't add any additional risk vectors: legacy ETH transfers are also done through the recipient call. Therefore no additional reenrancy protection was introduced here. I suggest to leave it out of this PR as being a potentially more of a global thing we might or might not want to deal with.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Deploying sanguine-fe with
|
| Latest commit: |
02ba663
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fc715868.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-fbv2-arbitrary-call.sanguine-fe.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (6)
packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol (2)
15-18: LGTM: Improved fixture creation for exclusivity testing.The
createFixturesV2function has been refactored to use separate methods for setting token and ETH test parameters, which improves code organization and readability. This implementation aligns well with the PR objectives for introducing exclusivity-related features.Consider adding a brief comment explaining the purpose of these new methods, e.g.:
function createFixturesV2() public virtual override { // Set up exclusivity parameters for token and ETH tests setTokenTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD); setEthTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD); }
Line range hint
1-19: Overall assessment: Well-implemented test contract for exclusivity features.The
FastBridgeV2DstExclusivityTestcontract effectively sets up the test environment for exclusivity-related features. The code is well-structured, follows good practices, and aligns perfectly with the PR objectives. The introduction of separate methods for setting token and ETH test parameters improves code organization and maintainability.As the project evolves, consider creating a shared utility library for common test setup functions if similar patterns are used across multiple test files. This would further enhance code reusability and maintainability.
packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (2)
107-107: Approve changes with a minor suggestionThe introduction of
setTokenTestExclusivityParams()improves code organization and readability. It aligns well with the PR objectives for the "Arbitrary Call" feature.Consider adding a brief comment explaining the purpose of
setTokenTestExclusivityParams()for better documentation:+ // Set up exclusivity parameters for token bridge test setTokenTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD);
184-184: Approve changes with a minor suggestion for consistencyThe introduction of
setEthTestExclusivityParams()is a good improvement, consistent with the changes made to the token test. It enhances code organization and readability.For consistency with the token test, consider adding a brief comment explaining the purpose of
setEthTestExclusivityParams():+ // Set up exclusivity parameters for ETH bridge test setEthTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD);packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol (2)
17-18: Avoid potential underflow when calculatingexclusivityEndTimeSubtracting
EXCLUSIVITY_PERIOD_ABSfromblock.timestampassumes that the current block timestamp is greater thanEXCLUSIVITY_PERIOD_ABS. While this is generally true, for completeness and to prevent underflows in edge cases (e.g., in test environments with manipulated timestamps), consider adding a check or usingblock.timestamp >= EXCLUSIVITY_PERIOD_ABS.Modify the calculation as follows to ensure safety:
- tokenTx.exclusivityEndTime = block.timestamp - EXCLUSIVITY_PERIOD_ABS; - ethTx.exclusivityEndTime = block.timestamp - EXCLUSIVITY_PERIOD_ABS; + tokenTx.exclusivityEndTime = block.timestamp >= EXCLUSIVITY_PERIOD_ABS ? block.timestamp - EXCLUSIVITY_PERIOD_ABS : 0; + ethTx.exclusivityEndTime = block.timestamp >= EXCLUSIVITY_PERIOD_ABS ? block.timestamp - EXCLUSIVITY_PERIOD_ABS : 0;
11-13: Eliminate redundant parameter setting increateFixturesV2Since the exclusivity parameters are overridden immediately after initialization, consider setting them directly to the desired negative values to simplify the code and improve readability.
Refactor the code as follows:
// Populate the fields using negative exclusivity periods - setTokenTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD_ABS); - setEthTestExclusivityParams(relayerB, EXCLUSIVITY_PERIOD_ABS); - // Override with negative exclusivity period - tokenParamsV2.quoteExclusivitySeconds = -int256(EXCLUSIVITY_PERIOD_ABS); - ethParamsV2.quoteExclusivitySeconds = -int256(EXCLUSIVITY_PERIOD_ABS); + setTokenTestExclusivityParams(relayerA, -int256(EXCLUSIVITY_PERIOD_ABS)); + setEthTestExclusivityParams(relayerB, -int256(EXCLUSIVITY_PERIOD_ABS)); tokenTx.exclusivityEndTime = block.timestamp - EXCLUSIVITY_PERIOD_ABS; ethTx.exclusivityEndTime = block.timestamp - EXCLUSIVITY_PERIOD_ABS;This change reduces redundancy and makes the initialization clearer.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (3 hunks)
🔇 Additional comments (13)
packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol (2)
Line range hint
8-8: LGTM: Constant definition for exclusivity period.The
EXCLUSIVITY_PERIODconstant is well-defined and aligns with the PR objectives. The 60-second value is suitable for testing purposes.
Line range hint
10-13: LGTM: Proper test setup for exclusivity testing.The
setUpfunction correctly overrides the parent class method and advances time by half of theEXCLUSIVITY_PERIOD. This setup is essential for testing exclusivity-related scenarios effectively.packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol (2)
Line range hint
1-33: Summary of changes and their impactThe changes in this file are focused on refactoring the
createFixturesV2function to improve code organization and maintainability. The introduction ofsetTokenTestExclusivityParamsandsetEthTestExclusivityParamsfunctions encapsulates the setup logic for token and ETH transactions, respectively.These changes align with the PR objectives and should enhance the readability of the test setup process. The core test logic remains unchanged, which is a positive aspect of this refactoring.
To ensure the robustness of these changes:
- Verify the implementation of the new functions as suggested earlier.
- Run the full test suite to confirm no regressions have been introduced.
- Consider adding comments to explain the purpose of these new functions if they're not self-explanatory.
Overall, this refactoring appears to be a positive improvement to the test structure.
Line range hint
1-33: Run full test suite to ensure no regressionsWhile the changes appear to be localized to the test setup in
createFixturesV2, it's crucial to verify that they don't introduce any unintended side effects.Please run the entire test suite for the FastBridgeV2 contract to ensure that all tests still pass and no regressions have been introduced. You can use a command similar to:
#!/bin/bash # Description: Run the full test suite for FastBridgeV2 # Test: Execute the test suite forge test --match-contract FastBridgeV2If you encounter any failures, please review and address them before merging this PR.
packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (1)
107-107: Request information about helper functionsThe changes improve code organization by introducing
setTokenTestExclusivityParams()andsetEthTestExclusivityParams(). However, the implementations of these functions are not visible in this file.Could you please provide the implementations of these helper functions or indicate where they are defined? This information would help ensure that the exclusivity parameters are set correctly for both token and ETH tests.
Also applies to: 184-184
packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol (3)
Line range hint
23-27: Tests correctly ensurebridgereverts with incorrect exclusivity parametersThe test function
test_bridge_revert_exclusivityEndTimeZeroappropriately verifies that thebridgefunction reverts whenquoteExclusivitySecondsis set to a negative value equal to-int256(block.timestamp). This ensures the contract handles incorrect exclusivity parameters as expected.
Line range hint
28-32: Consistently handle negative exclusivity periods in testsSimilar to the previous test,
test_bridge_revert_exclusivityPeriodUnderfloweffectively checks for underflow conditions by settingquoteExclusivitySecondsto-int256(block.timestamp + 1). This ensures robustness against potential underflow errors.
Line range hint
23-27: Verify casting of largeblock.timestampvalues to negativeint256Casting
block.timestamptoint256and then negating it may lead to overflows ifblock.timestampexceeds the maximum value ofint256. Although unlikely in the near future, it's good practice to handle potential overflows proactively.Run the following script to check the maximum
int256value and compare it withblock.timestamp:This script ensures that
block.timestampis safely within the bounds ofint256.packages/contracts-rfq/test/FastBridgeV2.t.sol (3)
127-138: Initialization oftokenParamsV2andethParamsV2is correctThe initialization of the
BridgeParamsV2structures with default values is appropriate and aligns with best practices.
166-169: SettingcallParamsinsetTokenTestCallParamsis appropriateThe
setTokenTestCallParamsfunction correctly updatestokenParamsV2.callParamsandtokenTx.callParams.
180-183: SettingcallParamsinsetEthTestCallParamsis appropriateThe
setEthTestCallParamsfunction correctly updatesethParamsV2.callParamsandethTx.callParams.packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol (2)
1-250: Comprehensive and Robust Test ImplementationThe added test contract
FastBridgeV2DstArbitraryCallTestextensively covers various scenarios for theFastBridgeV2functionality, particularly focusing on arbitrary calls to recipient contracts under different conditions. The use of mocks and the thoroughness of edge case handling enhance the reliability of the code. The implementation aligns well with the objectives outlined in the PR summary.
239-239: Verify the correctness ofvm.mockCallRevertdata parameterIn the function
test_relay_eth_noCallParams_revert_recipientReverts, thevm.mockCallRevertis called withdata: "". Even whencallParamsare empty, the recipient's functionfastBridgeTransferReceivedmay still generate specific calldata. Please verify that thedataparameter invm.mockCallRevertaccurately reflects the expected calldata whencallParamsare empty to ensure the mock behaves as intended.To confirm, you can search the codebase for instances where
fastBridgeTransferReceivedis called with emptycallParamsand examine the generated calldata.This will help ensure that the
vm.mockCallRevertin your test accurately simulates the recipient's behavior.
| setTokenTestExclusivityParams(relayerA, EXCLUSIVITY_PERIOD); | ||
| setEthTestExclusivityParams(relayerB, EXCLUSIVITY_PERIOD); |
There was a problem hiding this comment.
💡 Codebase verification
Missing implementation of setEthTestExclusivityParams function
The function setEthTestExclusivityParams is referenced in createFixturesV2 within packages/contracts-rfq/test/FastBridgeV2.t.sol but its implementation was not found. Please implement this function to ensure proper exclusivity setup for ETH transactions.
- File:
packages/contracts-rfq/test/FastBridgeV2.t.sol - Function Reference:
createFixturesV2
🔗 Analysis chain
Approve refactoring, verify new function implementations
The refactoring of createFixturesV2 improves code organization by extracting parameter setup into separate functions. This enhances readability and maintainability.
Please ensure that setTokenTestExclusivityParams and setEthTestExclusivityParams are correctly implemented. Run the following script to verify:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of new functions
# Test: Search for the function implementations
rg --type solidity -A 10 'function setTokenTestExclusivityParams|function setEthTestExclusivityParams' packages/contracts-rfq/test/
Length of output: 297
Script:
#!/bin/bash
# Description: Verify the implementation of new functions
# Search for the function implementations in Solidity files
rg --type solidity -A 10 'function setTokenTestExclusivityParams|function setEthTestExclusivityParams' packages/contracts-rfq/test/
Length of output: 166
Script:
#!/bin/bash
# Description: Verify the implementation of new functions by searching .sol files
# Search for the function implementations in Solidity (.sol) files
rg 'function setTokenTestExclusivityParams|function setEthTestExclusivityParams' --glob '*.sol' -A 10 packages/contracts-rfq/test/
Length of output: 2211
| function test_bridge_token_callParamsLengthMax() public { | ||
| bytes memory callParams = new bytes(2 ** 16 - 1); | ||
| setTokenTestCallParams(callParams); | ||
| test_bridge_token(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated test functions to improve maintainability
The functions test_bridge_token_callParamsLengthMax() and test_bridge_eth_callParamsLengthMax() have identical logic except for the asset type. Consider refactoring them into a single function that accepts a parameter to specify the asset type. This will reduce code duplication and make the tests easier to maintain.
Also applies to: 25-29
| function test_bridge_token_revert_callParamsLengthAboveMax() public { | ||
| bytes memory callParams = new bytes(2 ** 16); | ||
| setTokenTestCallParams(callParams); | ||
| vm.expectRevert(CallParamsLengthAboveMax.selector); | ||
| bridge({caller: userA, msgValue: 0, params: tokenParams, paramsV2: tokenParamsV2}); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated revert test functions to improve maintainability
The functions test_bridge_token_revert_callParamsLengthAboveMax() and test_bridge_eth_revert_callParamsLengthAboveMax() share similar logic. Refactoring them into a single parameterized function can reduce code duplication and improve code maintainability.
Also applies to: 38-43
| bytes memory callParams = new bytes(2 ** 16 - 1); | ||
| setTokenTestCallParams(callParams); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define constants for callParams length limits to enhance readability
Consider defining constants for the maximum allowed callParams length and the value exceeding the maximum. This will improve readability and make future modifications easier.
You can define the constants as:
uint256 constant MAX_CALL_PARAMS_LENGTH = 2 ** 16 - 1;
uint256 constant ABOVE_MAX_CALL_PARAMS_LENGTH = 2 ** 16;Then update the test functions accordingly:
-function test_bridge_token_callParamsLengthMax() public {
- bytes memory callParams = new bytes(2 ** 16 - 1);
+function test_bridge_token_callParamsLengthMax() public {
+ bytes memory callParams = new bytes(MAX_CALL_PARAMS_LENGTH);And similarly for the other functions.
Also applies to: 26-27, 32-33, 39-40
| 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; |
There was a problem hiding this comment.
Ensure safe casting when assigning negative exclusivity periods
When assigning negative values to quoteExclusivitySeconds, directly negating int256(EXCLUSIVITY_PERIOD_ABS) could lead to unexpected behavior if EXCLUSIVITY_PERIOD_ABS exceeds the limits of int256. To prevent potential overflows or underflows, consider safely casting and negating the value.
Apply the following changes for safer casting:
- tokenParamsV2.quoteExclusivitySeconds = -int256(EXCLUSIVITY_PERIOD_ABS);
- ethParamsV2.quoteExclusivitySeconds = -int256(EXCLUSIVITY_PERIOD_ABS);
+ tokenParamsV2.quoteExclusivitySeconds = int256(-EXCLUSIVITY_PERIOD_ABS);
+ ethParamsV2.quoteExclusivitySeconds = int256(-EXCLUSIVITY_PERIOD_ABS);This approach ensures that the negation happens at the int256 level, reducing the risk of overflow.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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; | |
| tokenParamsV2.quoteExclusivitySeconds = int256(-EXCLUSIVITY_PERIOD_ABS); | |
| ethParamsV2.quoteExclusivitySeconds = int256(-EXCLUSIVITY_PERIOD_ABS); | |
| tokenTx.exclusivityEndTime = block.timestamp - EXCLUSIVITY_PERIOD_ABS; | |
| ethTx.exclusivityEndTime = block.timestamp - EXCLUSIVITY_PERIOD_ABS; |
| 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; | ||
| } |
There was a problem hiding this comment.
Potential issue when casting exclusivitySeconds from uint256 to int256
Casting exclusivitySeconds from uint256 to int256 may lead to unexpected behavior if the value exceeds the positive range of int256. Consider changing the type of quoteExclusivitySeconds to uint256 to prevent potential overflows.
Apply this diff to change the type of quoteExclusivitySeconds to uint256:
function setTokenTestExclusivityParams(address relayer, uint256 exclusivitySeconds) public {
tokenParamsV2.quoteRelayer = relayer;
- tokenParamsV2.quoteExclusivitySeconds = int256(exclusivitySeconds);
+ tokenParamsV2.quoteExclusivitySeconds = exclusivitySeconds;
tokenParamsV2.quoteId = bytes.concat("Token:", getMockQuoteId(relayer));
tokenTx.exclusivityRelayer = relayer;
tokenTx.exclusivityEndTime = block.timestamp + exclusivitySeconds;
}Ensure that the quoteExclusivitySeconds field in BridgeParamsV2 is also defined as a uint256.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 setTokenTestExclusivityParams(address relayer, uint256 exclusivitySeconds) public { | |
| tokenParamsV2.quoteRelayer = relayer; | |
| tokenParamsV2.quoteExclusivitySeconds = exclusivitySeconds; | |
| tokenParamsV2.quoteId = bytes.concat("Token:", getMockQuoteId(relayer)); | |
| tokenTx.exclusivityRelayer = relayer; | |
| tokenTx.exclusivityEndTime = block.timestamp + exclusivitySeconds; | |
| } |
| 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; | ||
| } |
There was a problem hiding this comment.
Potential issue when casting exclusivitySeconds from uint256 to int256
Similar to the previous comment, casting exclusivitySeconds from uint256 to int256 can cause issues if the value exceeds int256's positive limit. It's advisable to use uint256 for quoteExclusivitySeconds.
Apply this diff to change the type of quoteExclusivitySeconds to uint256:
function setEthTestExclusivityParams(address relayer, uint256 exclusivitySeconds) public {
ethParamsV2.quoteRelayer = relayer;
- ethParamsV2.quoteExclusivitySeconds = int256(exclusivitySeconds);
+ ethParamsV2.quoteExclusivitySeconds = exclusivitySeconds;
ethParamsV2.quoteId = bytes.concat("ETH:", getMockQuoteId(relayer));
ethTx.exclusivityRelayer = relayer;
ethTx.exclusivityEndTime = block.timestamp + exclusivitySeconds;
}Ensure that the quoteExclusivitySeconds field in BridgeParamsV2 is defined as a uint256.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 setEthTestExclusivityParams(address relayer, uint256 exclusivitySeconds) public { | |
| ethParamsV2.quoteRelayer = relayer; | |
| ethParamsV2.quoteExclusivitySeconds = exclusivitySeconds; | |
| ethParamsV2.quoteId = bytes.concat("ETH:", getMockQuoteId(relayer)); | |
| ethTx.exclusivityRelayer = relayer; | |
| ethTx.exclusivityEndTime = block.timestamp + exclusivitySeconds; | |
| } |
| 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"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify getMockQuoteId function for maintainability
Consider simplifying the getMockQuoteId function to enhance readability and maintainability. Using a mapping or a more concise logic can make the code cleaner.
Here's an example of how you might refactor the function:
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");
- }
+ string memory relayerName;
+ if (relayer == relayerA) {
+ relayerName = "Relayer A";
+ } else if (relayer == relayerB) {
+ relayerName = "Relayer B";
+ } else {
+ relayerName = "unknown relayer";
+ }
+ return bytes(string(abi.encodePacked("created by ", relayerName)));
}This refactoring enhances clarity by reducing repetitive code.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 getMockQuoteId(address relayer) public view returns (bytes memory) { | |
| string memory relayerName; | |
| if (relayer == relayerA) { | |
| relayerName = "Relayer A"; | |
| } else if (relayer == relayerB) { | |
| relayerName = "Relayer B"; | |
| } else { | |
| relayerName = "unknown relayer"; | |
| } | |
| return bytes(string(abi.encodePacked("created by ", relayerName))); | |
| } |
Bundle ReportBundle size has no change ✅ |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/contracts-rfq/contracts/FastBridgeV2.sol (2)
29-31: LGTM: MAX_CALL_PARAMS_LENGTH constant added.The addition of the MAX_CALL_PARAMS_LENGTH constant is a good practice to prevent potential DoS attacks by limiting the size of callParams. The chosen value (65535) seems reasonable.
Consider adding a brief comment explaining the rationale behind this limit, e.g., "Limits callParams size to prevent excessive gas consumption and potential DoS attacks."
357-380: LGTM with suggestion: New _checkedCallRecipient function implemented.The _checkedCallRecipient function is well-implemented with proper checks for return value correctness. However, there's a potential security concern regarding reentrancy.
Consider adding reentrancy protection to this function. You can use OpenZeppelin's
ReentrancyGuardand apply thenonReentrantmodifier to therelayfunction:+import "@openzeppelin/contracts/security/ReentrancyGuard.sol"; -contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { +contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors, ReentrancyGuard { ... function relay(bytes memory request, address relayer) public payable + nonReentrant { ... }This addition will provide an extra layer of security against potential reentrancy attacks during the arbitrary call.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (8 hunks)
🔇 Additional comments (2)
packages/contracts-rfq/contracts/FastBridgeV2.sol (2)
13-13: LGTM: Import statement for IFastBridgeRecipient added.The import statement for the IFastBridgeRecipient interface is correctly placed and necessary for the new arbitrary call functionality.
266-267: LGTM: Comment added for deprecated chainGasAmount.The addition of the comment explaining that chainGasAmount is 0 due to the deprecation of the gas rebate function is helpful for code maintainability and understanding.
| 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)) { |
There was a problem hiding this comment.
LGTM with suggestion: New checks added for callParams and exclusivity period.
The addition of checks for callParams length and exclusivity period is good. However, there's a potential issue with the exclusivityEndTime calculation.
The casting of block.timestamp to int256 could cause overflow issues in the far future. Consider using uint256 instead:
-int256 exclusivityEndTime = int256(block.timestamp) + paramsV2.quoteExclusivitySeconds;
-if (exclusivityEndTime <= 0 || exclusivityEndTime > int256(params.deadline)) {
+uint256 exclusivityEndTime = block.timestamp + paramsV2.quoteExclusivitySeconds;
+if (exclusivityEndTime > params.deadline) {
revert ExclusivityParamsIncorrect();
}This change eliminates the need for the <= 0 check and prevents potential overflow issues.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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)) { | |
| if (paramsV2.callParams.length > MAX_CALL_PARAMS_LENGTH) revert CallParamsLengthAboveMax(); | |
| uint256 exclusivityEndTime = block.timestamp + paramsV2.quoteExclusivitySeconds; | |
| // exclusivityEndTime must be in range (0 .. params.deadline] | |
| if (exclusivityEndTime > params.deadline) { |
There was a problem hiding this comment.
Solidity ^0.8 prevents overflows by trigering hard revert.
There was a problem hiding this comment.
@trajan0x, you're correct. Since Solidity 0.8, integer overflows and underflows cause a revert, so the explicit checks may not be necessary. Thank you for the clarification.
（・・？）
✏️ Learnings added
Learnt from: trajan0x
PR: synapsecns/sanguine#3215
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:145-148
Timestamp: 2024-10-03T16:44:38.362Z
Learning: In Solidity 0.8 and above, integer overflows and underflows cause a hard revert, so explicit overflow checks are unnecessary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| 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)) { |
There was a problem hiding this comment.
Solidity ^0.8 prevents overflows by trigering hard revert.
| /// @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 | ||
| { | ||
| 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(); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm also a bit surprised this isn't non-reenrant. Can you explain why?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/contracts-rfq/contracts/FastBridgeV2.sol (2)
29-31: LGTM: MAX_CALL_PARAMS_LENGTH constant addedThe addition of the MAX_CALL_PARAMS_LENGTH constant with a value of 2^16 - 1 (65535) is appropriate. This sets a reasonable upper limit for the length of callParams, allowing for substantial arbitrary data while preventing excessive gas costs.
Consider adding a comment explaining the rationale behind choosing this specific value for MAX_CALL_PARAMS_LENGTH.
Line range hint
148-153: LGTM with suggestion: New checks added for callParams and exclusivity periodThe addition of checks for callParams length and exclusivity period is good. However, there's a potential issue with the exclusivityEndTime calculation.
Consider using uint256 instead of int256 for the exclusivityEndTime calculation to prevent potential overflow issues in the far future:
-int256 exclusivityEndTime = int256(block.timestamp) + paramsV2.quoteExclusivitySeconds; -if (exclusivityEndTime <= 0 || exclusivityEndTime > int256(params.deadline)) { +uint256 exclusivityEndTime = block.timestamp + paramsV2.quoteExclusivitySeconds; +if (exclusivityEndTime > params.deadline) { revert ExclusivityParamsIncorrect(); }This change eliminates the need for the
<= 0check and prevents potential overflow issues.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (9 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
Learnt from: trajan0x PR: synapsecns/sanguine#3215 File: packages/contracts-rfq/contracts/FastBridgeV2.sol:145-148 Timestamp: 2024-10-03T16:44:38.518Z Learning: In Solidity 0.8 and above, integer overflows and underflows cause a hard revert, so explicit overflow checks are unnecessary.
🔇 Additional comments (3)
packages/contracts-rfq/contracts/FastBridgeV2.sol (3)
5-5: LGTM: Address utility import addedThe addition of the OpenZeppelin Address utility is appropriate. This import suggests new functionality involving external calls or contract checks, which aligns with the arbitrary call feature being implemented.
360-383: LGTM: _checkedCallRecipient function implemented correctlyThe _checkedCallRecipient function is well-implemented:
- It correctly uses Address.functionCallWithValue for the external call, handling ETH transfers.
- Proper checks are in place for the return value, ensuring it matches the expected selector.
- The function reverts with specific error messages for different failure scenarios.
This implementation provides a secure way to make arbitrary calls to recipients while ensuring the correct interface is implemented.
13-13: LGTM: IFastBridgeRecipient interface import addedThe addition of the IFastBridgeRecipient interface import is appropriate for the new arbitrary call feature. This suggests that recipients of fast bridge transfers may need to implement a specific interface.
Could you provide more information about the IFastBridgeRecipient interface? Let's check its implementation:
✅ Verification successful
LGTM: IFastBridgeRecipient interface import added
The addition of the
IFastBridgeRecipientinterface import is appropriate for the new arbitrary call feature. It facilitates interactions with external contracts that implement this interface, ensuring that recipients of fast bridge transfers adhere to the expected structure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the IFastBridgeRecipient interface definition rg -p "interface IFastBridgeRecipient" -A 10Length of output: 359
Script:
#!/bin/bash # Search for contracts implementing IFastBridgeRecipient interface rg -p 'contract .* is .*IFastBridgeRecipient' -A 10Length of output: 1071
Script:
#!/bin/bash # Search for all usages of the fastBridgeTransferReceived function rg -p 'fastBridgeTransferReceived\('Length of output: 1140
Script:
#!/bin/bash # Search for implementations or usages of IFastBridgeRecipient in production contracts rg -p 'IFastBridgeRecipient' packages/contracts-rfq/contracts --ignore-dir=testLength of output: 522
Script:
#!/bin/bash # Search for implementations or usages of IFastBridgeRecipient in production contracts, excluding the test directory rg 'IFastBridgeRecipient' packages/contracts-rfq/contracts --glob '!**/test/**'Length of output: 617
| // 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. | ||
|
|
There was a problem hiding this comment.
Address TODO comment regarding BridgeTransactionV2 struct decoding
The TODO comment indicates that the previous explanation about decoding BridgeTransactionV2 is no longer accurate due to the addition of the variable-length callParams field. This needs to be addressed to ensure correct decoding behavior.
Please update the comment and verify the decoding behavior of the getBridgeTransaction function to account for the new callParams field in BridgeTransactionV2.
| // 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) { | ||
| // 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: pull the tokens from the Relayer to the recipient 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 | ||
| }); |
There was a problem hiding this comment.
LGTM with suggestion: New logic for handling callParams implemented
The implementation of the arbitrary call functionality is well-structured and follows the checks-effects-interactions pattern. However, there's a potential issue with the ERC20 token transfer.
When handling ERC20 tokens with callParams, calling _pullToken(to, token, amount); may incorrectly attempt to transfer tokens from the relayer to the recipient, rather than from the contract to the recipient. Consider using universalTransfer instead:
} else if (token != UniversalTokenLib.ETH_ADDRESS) {
// Arbitrary call requested with ERC20: transfer the tokens first
- _pullToken(to, token, amount);
+ token.universalTransfer(to, amount);This change ensures that the tokens are transferred from the contract to the recipient before making the arbitrary call.
Committable suggestion was skipped due to low confidence.
Description
This is the first of the two PRs for the "Arbitrary Call" (final name pending) feature. The first PR adds the option to invoke a special function hook once the tokens have been transferred to the recipient:
sanguine/packages/contracts-rfq/contracts/interfaces/IFastBridgeRecipient.sol
Lines 4 to 13 in ad70449
The origin sender is given an option to provide
callParamswhen initiating a bridge on the source chain.callParamsis non-empty,destRecipient.fastBridgeTransferReceived()function is called after the tokens are transferred to the recipientmsg.valuefor the hook function call to avoid calling the recipient twice.The transactions with a hook function call will revert in one of these cases (this does not affect the legacy transactions):
fastBridgeTransferReceivedfunction, or doesn't return the magic value (the hook function selector) at the end of the callfastBridgeTransferReceivedcallThe reverts do not put Relayer or Recipient funds at risk, in fact the transaction is only completed if the tokens are transferred to the recipient AND the hook call is successful (as reported by the recipient itself using the magic return value). The relayers are expected to simulate the transaction prior its submission to minimise the chances of the on-chain reverts.
Summary by CodeRabbit
New Features
FastBridgeV2contract to support additional call parameters during bridge transactions.IFastBridgeRecipientfor handling token transfer receipts.Bug Fixes
Tests