feat(contracts-rfq): arbitrary call with value [SLT-233] [SLT-318] #3246
Conversation
WalkthroughThe changes in this pull request focus on enhancing 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
|
Deploying sanguine-fe with
|
| Latest commit: |
432c34e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://15ab7326.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-fbv2-arbitrary-call-val.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3246 +/- ##
====================================================
+ Coverage 30.68788% 90.77404% +60.08616%
====================================================
Files 447 60 -387
Lines 29787 1279 -28508
Branches 82 159 +77
====================================================
- Hits 9141 1161 -7980
+ Misses 19797 114 -19683
+ Partials 849 4 -845
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Changes to gas cost
🧾 Summary (50% most significant diffs)
Full diff report 👇
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (8)
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (1)
10-10: LGTM! Consider grouping related errors.The new error
NativeTokenCallValueNotSupportedis well-named and consistent with the interface's style. It clearly describes a specific condition related to native token call values.Consider grouping this error with other related errors (if any) for better organization. For instance, it could be placed near
MsgValueIncorrectif they are conceptually related.packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (2)
Line range hint
1-1024: Consider updating tests for newsendChainGasbehavior.While the existing test suite is comprehensive, the change in
sendChainGaslogic may require updates to existing tests or the addition of new ones. This ensures that the new behavior is properly validated.Consider adding or modifying tests to specifically check:
- Cases where
callValueis zero butsendChainGaswas previously true.- Cases where
callValueis non-zero butsendChainGaswas previously false.- Edge cases around very small non-zero
callValueamounts.This will help ensure that the new logic is working as intended across all scenarios.
Line range hint
1-1024: Summary: Significant change insendChainGaslogic requires careful verification.The main change in this file is the modification of the
sendChainGaslogic in theexpectBridgeRequestedfunction. This change could have far-reaching effects on how cross-chain gas costs are handled. Key points to address:
- Verify that this change aligns with the intended behavior across the entire codebase.
- Update existing tests or add new ones to cover the new
sendChainGasbehavior thoroughly.- Ensure that all stakeholders are aware of this change and its potential implications on gas handling in cross-chain transactions.
Consider documenting this change clearly in the contract's documentation or comments, explaining the rationale behind linking
sendChainGastocallValueinstead of using a separate flag. This will help future developers understand the design decision.packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
60-60: Clarify Comment RegardingcallValueReplacementIn the comment for
callValueinBridgeTransactionV2, it says: "replaces V1's sendChainGas flag". SincesendChainGaswas a boolean, andcallValueis auint256, consider updating the comment to provide more clarity on howcallValuereplacessendChainGas.You can revise the comment as follows:
- uint256 callValue; // ETH value to send to the recipient (if any) - replaces V1's sendChainGas flag + uint256 callValue; // ETH value to send to the recipient (if any); replaces V1's sendChainGas flag by specifying the amount of ETH to sendpackages/contracts-rfq/contracts/FastBridgeV2.sol (2)
366-389: Use ofblock.timestampin validation checks.While the use of
block.timestampfor validation in_validateBridgeParamsis acceptable, be cautious of potential miner manipulation within a small range. Ensure that any reliance on timestamps does not introduce vulnerabilities or allow exploits due to slight timestamp discrepancies.🧰 Tools
🪛 GitHub Check: Slither
[notice] 366-389: Block timestamp
FastBridgeV2._validateBridgeParams(IFastBridge.BridgeParams,IFastBridgeV2.BridgeParamsV2,int256) (contracts/FastBridgeV2.sol#366-389) uses timestamp for comparisons
Dangerous comparisons:
- params.deadline < block.timestamp + MIN_DEADLINE_PERIOD (contracts/FastBridgeV2.sol#379)
- exclusivityEndTime <= 0 || exclusivityEndTime > int256(params.deadline) (contracts/FastBridgeV2.sol#386)
392-415: Time-based validation in_validateRelayParams.Similarly, consider the implications of using
block.timestampin_validateRelayParams. Minor adjustments by miners could affect time-sensitive logic. Review whether this could impact the security or functionality of the relay process.🧰 Tools
🪛 GitHub Check: Slither
[notice] 392-415: Block timestamp
FastBridgeV2._validateRelayParams(IFastBridgeV2.BridgeTransactionV2,bytes32,address) (contracts/FastBridgeV2.sol#392-415) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > transaction.deadline (contracts/FastBridgeV2.sol#405)
- transaction.exclusivityRelayer != address(0) && transaction.exclusivityRelayer != relayer && block.timestamp <= transaction.exclusivityEndTime (contracts/FastBridgeV2.sol#409-411)packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (2)
304-309: Specify expected revert reason for clearer test diagnostics.Including the expected revert reason improves test precision and aids in debugging.
311-316: Consider specifying the expected revert reason for better clarity in test assertions.This enhances the test's expressiveness and robustness.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (7 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 (7 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (14 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (3 hunks)
🧰 Additional context used
🪛 GitHub Check: Slither
packages/contracts-rfq/contracts/FastBridgeV2.sol
[notice] 366-389: Block timestamp
FastBridgeV2._validateBridgeParams(IFastBridge.BridgeParams,IFastBridgeV2.BridgeParamsV2,int256) (contracts/FastBridgeV2.sol#366-389) uses timestamp for comparisons
Dangerous comparisons:
- params.deadline < block.timestamp + MIN_DEADLINE_PERIOD (contracts/FastBridgeV2.sol#379)
- exclusivityEndTime <= 0 || exclusivityEndTime > int256(params.deadline) (contracts/FastBridgeV2.sol#386)
[notice] 392-415: Block timestamp
FastBridgeV2._validateRelayParams(IFastBridgeV2.BridgeTransactionV2,bytes32,address) (contracts/FastBridgeV2.sol#392-415) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > transaction.deadline (contracts/FastBridgeV2.sol#405)
- transaction.exclusivityRelayer != address(0) && transaction.exclusivityRelayer != relayer && block.timestamp <= transaction.exclusivityEndTime (contracts/FastBridgeV2.sol#409-411)
🔇 Additional comments (53)
packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol (2)
4-4: Verify the impact of removing the IFastBridgeV2 import.The import statement for
IFastBridgeV2has been removed. Please ensure that this interface is not used directly in this test file and that all necessary types and functions are still accessible through theFastBridgeV2DstTestimport.To confirm that the removal of the import doesn't break any functionality, run the following script:
11-12: Approve changes to createFixturesV2 and verify new methods.The refactoring of
createFixturesV2to usesetTokenTestExclusivityParamsandsetEthTestExclusivityParamsimproves code organization and readability. This change simplifies the setup process for test fixtures.To ensure the new methods correctly set up the test environment, please verify their implementation:
✅ Verification successful
Verified the implementation of new methods.
The methods
setTokenTestExclusivityParamsandsetEthTestExclusivityParamsare correctly defined and utilized within the test suite (packages/contracts-rfq/test/FastBridgeV2.t.sol). This ensures the test fixtures are set up as intended, confirming the refactoring increateFixturesV2improves code organization without introducing issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of new methods # Test: Check the implementation of setTokenTestExclusivityParams and setEthTestExclusivityParams rg --type solidity -A 10 '(setTokenTestExclusivityParams|setEthTestExclusivityParams)' packages/contracts-rfq/test/FastBridgeV2.Dst.t.solLength of output: 580
Script:
#!/bin/bash # Description: Search for the implementation of setTokenTestExclusivityParams and setEthTestExclusivityParams in Solidity files # Find all .sol files and search for the method implementations fd --extension sol | xargs rg '(setTokenTestExclusivityParams|setEthTestExclusivityParams)'Length of output: 1713
Script:
#!/bin/bash # Description: Extract implementations of setTokenTestExclusivityParams and setEthTestExclusivityParams # Extract the function implementations from Solidity files rg -A 10 'function setTokenTestExclusivityParams' packages/contracts-rfq/test/**/*.sol rg -A 10 'function setEthTestExclusivityParams' packages/contracts-rfq/test/**/*.solLength of output: 947
Script:
#!/bin/bash # Description: Comprehensive search for setTokenTestExclusivityParams and setEthTestExclusivityParams implementations across all Solidity files # Find all .sol files and search for the function implementations without restricting the search to specific directories fd --extension sol | xargs rg 'function\s+setTokenTestExclusivityParams\s*\(' fd --extension sol | xargs rg 'function\s+setEthTestExclusivityParams\s*\('Length of output: 443
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1)
45-45: Verify the impact of changingsendChainGaslogic.The condition for setting
sendChainGashas been changed frombridgeTx.sendChainGastobridgeTx.callValue > 0. This modification alters when chain gas is considered to be sent, potentially affecting cross-chain transaction behavior.To ensure this change doesn't introduce unintended side effects, please run the following verification:
Review the results to confirm that this change aligns with the intended behavior across the codebase.
✅ Verification successful
Change to
sendChainGasinFastBridgeV2.Src.t.solis correctly implemented.
The alteration frombridgeTx.sendChainGastobridgeTx.callValue > 0is localized toFastBridgeV2.Src.t.soland does not affect other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all usages of BridgeRequested event and sendChainGas rg --type solidity -e 'emit BridgeRequested' -e 'sendChainGas'Length of output: 95
Script:
#!/bin/bash # Search for all usages of BridgeRequested event and sendChainGas in .sol files rg -g "*.sol" -e 'emit BridgeRequested' -e 'sendChainGas'Length of output: 7534
packages/contracts-rfq/test/FastBridgeV2.Src.ArbitraryCall.t.sol (9)
9-9: Definition ofCALL_VALUEis appropriate and follows conventions.The constant
CALL_VALUEis correctly defined and adheres to Solidity naming conventions.
48-52: Testtest_bridge_token_withCallValue_noCallParamscorrectly validates token bridging with call value and no call parameters.The test function appropriately sets up the scenario where a call value is provided without call parameters for token bridging and invokes
test_bridge_token()as expected.
54-58: Testtest_bridge_token_diffSender_withCallValue_noCallParamscorrectly handles different sender scenario with call value and no call parameters.The function correctly tests token bridging with a different sender, providing a call value without call parameters.
60-65: Testtest_bridge_eth_withCallValue_noCallParams_revertaccurately expects revert when bridging ETH with call value and no call parameters.The test correctly expects a revert with
NativeTokenCallValueNotSupported.selectorwhen attempting to bridge ETH providing a call value without call parameters, ensuring the contract behaves as intended under these conditions.
67-72: Testtest_bridge_eth_diffSender_withCallValue_noCallParams_revertcorrectly tests revert scenario with different sender.This test ensures that bridging ETH with a different sender, providing a call value without call parameters, correctly reverts with the expected error selector.
76-79: Testtest_bridge_token_withCallValue_withCallParamseffectively tests token bridging with both call value and call parameters.The function successfully tests the scenario where both a call value and call parameters are provided for token bridging, validating the contract's handling of this case.
81-84: Testtest_bridge_token_diffSender_withCallValue_withCallParamsappropriately handles different sender with call value and call parameters.The test accurately assesses token bridging with a different sender when both call value and call parameters are provided.
86-90: Testtest_bridge_eth_withCallValue_withCallParams_revertcorrectly expects revert when bridging ETH with call value and call parameters.The test ensures that providing both call value and call parameters while bridging ETH correctly results in a revert with
NativeTokenCallValueNotSupported.selector, as the contract does not support this operation.
92-96: Testtest_bridge_eth_diffSender_withCallValue_withCallParams_revertaccurately tests revert scenario with different sender.This function validates that bridging ETH with a different sender, providing both call value and call parameters, correctly triggers a revert with the expected error selector.
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (2)
43-43:⚠️ Potential issueData Type Suggestion: Use
uint256forquoteExclusivitySecondsIn the
BridgeParamsV2struct,quoteExclusivitySecondsis declared asint256. Since time durations cannot be negative, consider changing its type touint256to reflect that it should only represent non-negative values.You can apply the following change:
- int256 quoteExclusivitySeconds; + uint256 quoteExclusivitySeconds;Likely invalid or redundant comment.
60-60: Consistency Check BetweencallValueParametersEnsure that
callValueinBridgeTransactionV2aligns withcallValueinBridgeParamsV2. It is important to maintain consistency between these structs to prevent mismatches during the bridging process.You can run the following script to verify the usage of
callValueacross the codebase:✅ Verification successful
Consistency Verified for
callValueParametersThe
callValuefield in bothBridgeTransactionV2andBridgeParamsV2structs is consistently defined asuint256. This ensures alignment between the two structs, preventing potential mismatches during the bridging process.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of `callValue` in structs and function parameters. # Find all declarations of `callValue` rg --type solidity 'uint256 callValue' # Find all usages of `callValue` in functions rg --type solidity -A3 'function.*\((.*?)\)' | rg 'callValue'Length of output: 168
Script:
#!/bin/bash # Description: Verify consistent usage of `callValue` in structs and function parameters by searching within .sol files. # Find all declarations of `callValue` in .sol files rg 'uint256\s+callValue' --glob '*.sol' # Find all usages of `callValue` in function parameters within .sol files rg 'function\s+\w+\s*\(([^)]*callValue[^)]*)\)' --glob '*.sol'Length of output: 801
Script:
#!/bin/bash # Description: Locate and extract `callValue` definitions within BridgeTransactionV2 and BridgeParamsV2 structs. # Find BridgeTransactionV2 struct definition and extract callValue rg 'struct\s+BridgeTransactionV2\s*\{' --glob '*.sol' -A10 | rg 'uint256\s+callValue' # Find BridgeParamsV2 struct definition and extract callValue rg 'struct\s+BridgeParamsV2\s*\{' --glob '*.sol' -A10 | rg 'uint256\s+callValue'Length of output: 429
packages/contracts-rfq/test/FastBridgeV2.t.sol (4)
131-132: Initialization ofcallValueandcallParamsfortokenParamsV2The
callValueis correctly initialized to0, andcallParamsis set to an empty byte array. This ensures default values are properly set for token transactions in tests.
138-139: Initialization ofcallValueandcallParamsforethParamsV2The
callValueandcallParamsare correctly initialized for ETH transactions, matching the token parameters.
172-175: Implementation ofsetTokenTestCallValueThe function
setTokenTestCallValuecorrectly updatestokenParamsV2.callValueandtokenTx.callValuewith the providedcallValue. This allows for dynamic testing of call values in token transactions.
191-194: Implementation ofsetEthTestCallValueThe function
setEthTestCallValuecorrectly updatesethParamsV2.callValueandethTx.callValuewith the providedcallValue. This facilitates testing of call values in ETH transactions.packages/contracts-rfq/test/FastBridgeV2.Dst.ArbitraryCall.t.sol (2)
4-5: Imports updated appropriatelyThe updated import statements correctly include the necessary interfaces and base test contracts required for these tests.
339-339: Confirm the use ofAddress.FailedInnerCall.selectorLines 339 and 346 use
vm.expectRevert(Address.FailedInnerCall.selector);when expecting a revert. Verify that this selector correctly corresponds to the expected revert reason and is used consistently where appropriate.#!/bin/bash # Description: Ensure that Address.FailedInnerCall.selector is correctly used and defined. # Check where Address.FailedInnerCall.selector is defined and used. rg --type solidity 'Address\.FailedInnerCall\.selector' -A 2Also applies to: 346-346
packages/contracts-rfq/contracts/FastBridgeV2.sol (13)
57-57: Initialization of default parameters inBridgeParamsV2.Adding
callValue: 0andcallParams: bytes("")ensures that default values are properly set whenparamsV2is not provided. This maintains backward compatibility and prevents potential issues with uninitialized parameters.
142-143: Refactoring validation logic into_validateBridgeParamsfunction.Abstracting the parameter validation into
_validateBridgeParamsenhances code readability and maintainability by consolidating checks in a single location.
145-146: Secure token transfer using_pullToken.Utilizing
_pullTokento transfer tokens ensures that tokens are securely pulled to the contract, which is crucial for safeguarding assets and preventing unauthorized transfers.
165-165: IncludingcallValueinBridgeTransactionV2encoding.Incorporating
callValuewhen encoding the bridge transaction ensures that the specified value is correctly relayed to the destination chain, enhancing the contract's functionality.
177-187: Enhanced event emission with named parameters inBridgeRequested.Using named parameters in the
emit BridgeRequestedstatement improves clarity and readability of the emitted events, making it easier to track and debug transactions.
195-200: Refactoring relay logic into_validateRelayParamsfunction.Moving validation logic into
_validateRelayParamsreduces redundancy and aligns with the single-responsibility principle, improving code organization.
207-211: Validatingmsg.valuefor ETH transfers in relays.The checks ensure that when transferring ETH,
callValuemust be zero andmsg.valuemust match the expected amount, preventing incorrect fund transfers and enhancing security.
213-217: Validatingmsg.valuefor ERC20 transfers and secure token forwarding.By confirming that
msg.valuematchestransaction.callValueand securely transferring ERC20 tokens from the relayer to the recipient, the contract upholds the integrity of the relay process.
220-229: Handling of arbitrary calls and ETH transfers post-relay.The logic correctly manages scenarios where
callParamsare provided, executing an arbitrary call with the suppliedcallValue. It also appropriately transfers ETH to the recipient whenmsg.valueis non-zero and nocallParamsare present.
231-241: Detailed event emission inBridgeRelayed.Emitting comprehensive details in the
BridgeRelayedevent, including parameters likechainGasAmount, facilitates easier monitoring and auditing of bridge transactions.
308-314: Improved token pull logic in_pullTokenfunction.The refactored
_pullTokenmethod now pulls tokens directly to the contract, simplifying the transfer process and mitigating potential reentrancy risks. The balance checks account for tokens with transfer fees, ensuring accurate tracking of transferred amounts.Also applies to: 317-317, 320-320, 322-322
339-341: Secure external call implementation in_checkedCallRecipient.Using
Address.functionCallWithValueto perform external calls while forwardingmsg.valueensures safe interaction with recipient contracts. The additional checks onreturnDataconfirm that the recipient contract returns the expected value, enhancing reliability.
374-415: Centralization of validation logic enhances maintainability.By centralizing parameter validations in
_validateBridgeParamsand_validateRelayParams, the codebase becomes more maintainable and easier to update, adhering to best practices.🧰 Tools
🪛 GitHub Check: Slither
[notice] 392-415: Block timestamp
FastBridgeV2._validateRelayParams(IFastBridgeV2.BridgeTransactionV2,bytes32,address) (contracts/FastBridgeV2.sol#392-415) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > transaction.deadline (contracts/FastBridgeV2.sol#405)
- transaction.exclusivityRelayer != address(0) && transaction.exclusivityRelayer != relayer && block.timestamp <= transaction.exclusivityEndTime (contracts/FastBridgeV2.sol#409-411)packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (20)
26-26: Definition ofCALL_VALUEconstant is appropriate.The addition of the
CALL_VALUEconstant sets a consistent value for use in tests involving call values.
72-72: Update ofchainGasAmountcalculation is correct.The conditional assignment of
chainGasAmountbased on whetherdestTokenisETH_ADDRESSensures proper handling of call values for different token types.
83-83: Emission ofchainGasAmountinBridgeRelayedevent is appropriate.Including
chainGasAmountin the event emission provides better transparency and tracking of the relayed transactions.
95-99: Addition ofcheckTokenBalancesfunction enhances code readability.The function encapsulates balance assertions for token transfers, improving test maintainability.
101-105: Addition ofcheckEthBalancesfunction improves balance checks for ETH transfers.This refactoring promotes code reuse and clarity in tests involving ETH balances.
113-113: Refactoring to usecheckTokenBalancesimproves test consistency.Using the new helper function streamlines balance checks in the test.
122-122: Refactoring to usecheckTokenBalancesenhances code consistency in tests.This promotes uniformity across token-related test cases.
131-131: Refactoring to usecheckEthBalancesenhances test code readability.Utilizing the helper function for ETH balances simplifies the test code.
140-140: Update to usecheckEthBalancesfunction in the test is appropriate.This change improves maintainability and clarity of balance assertions.
156-156: Use ofcheckEthBalancesin test enhances code consistency.Consistent use of helper functions across tests aids in readability.
161-169: Addition oftest_relay_token_withCallValueenhances test coverage for token relays with call value.The test correctly sets up call value scenarios and verifies expected behaviors.
171-178: Addition oftest_relay_token_withRelayerAddressCallValueextends tests to cover relayer addresses with call values.This ensures comprehensive testing of relays involving both call values and specified relayer addresses.
476-480: Addition oftest_relay_token_withCallValue_revert_zeroCallValueincreases test coverage for invalid call value scenarios.The test correctly verifies that zero
msg.valuewhen a call value is expected causes a revert.
482-486: Addition oftest_relay_token_withCallValue_revert_lowerCallValueensures proper validation of insufficientmsg.value.This enhances the contract's robustness by validating
msg.value.
489-493: Addition oftest_relay_token_withCallValue_revert_higherCallValuevalidates rejection of excessivemsg.value.The test ensures that overpayment is correctly handled.
495-499: Addition oftest_relay_token_withRelayerAddressCallValue_revert_zeroCallValuechecks for correct revert whenmsg.valueis zero.This test adds necessary validation for relays with relayer addresses.
501-505: Addition oftest_relay_token_withRelayerAddressCallValue_revert_lowerCallValuevalidates handling of insufficientmsg.value.Ensures consistency in how call value discrepancies are handled.
507-511: Addition oftest_relay_token_withRelayerAddressCallValue_revert_higherCallValueensures excessmsg.valueis rejected.This contributes to the integrity of the relay process.
513-522: Addition oftest_relay_eth_withCallValue_revert_notSupportedconfirms that call values are not supported for ETH relays.Validates that ETH transfers do not accept additional call values.
524-537: Addition oftest_relay_eth_withRelayerAddressCallValue_revert_notSupportedensures ETH relays do not accept call values with relayer addresses.This test secures the behavior of ETH relay functions against unsupported scenarios.
ChiTimesChi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (1)
50-52: Use Correct NatSpec Tags for Internal CommentsThe comments starting at line 50 should use the
@devtag instead of@noticesince they describe internal implementation details rather than user-facing documentation. This improves clarity for developers.Apply this diff to adjust the comments:
- /// @notice We expect all the V1 fields except for `sendChainGas` to match. - /// `sendChainGas` is replaced with `callValue` in V2, therefore we expect non-zero `callValue` - /// to match `sendChainGas = true` in V1 + /// @dev We expect all the V1 fields except for `sendChainGas` to match. + /// `sendChainGas` is replaced with `callValue` in V2, therefore we expect non-zero `callValue` + /// to match `sendChainGas = true` in V1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (7 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridge.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (1 hunks)
🧰 Additional context used
🔇 Additional comments (12)
packages/contracts-rfq/contracts/interfaces/IFastBridge.sol (1)
100-100: Approved: Function modifier change frompuretoviewThe change from
puretoviewfor thegetBridgeTransactionfunction is appropriate if the implementation needs to read contract state. This modification allows for more flexibility in the function's implementation while maintaining its non-state-modifying nature.To ensure consistency, please verify that the implementation of this function in the concrete contract matches this interface change. Run the following script to check the implementation:
packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (3)
53-53: Appropriate Addition ofviewModifierThe addition of the
viewmodifier to thetest_getBridgeTransaction_supportsV2function correctly indicates that it does not modify state, enhancing code clarity and adhering to Solidity best practices.
57-57: Correct Translation ofcallValuetosendChainGasThe assignment
expectedTx.sendChainGas = bridgeTxV2.callValue > 0;accurately maps the V2callValueto the V1sendChainGasfield. This ensures that a non-zerocallValuein V2 corresponds tosendChainGasbeingtruein V1, maintaining consistency between versions.
56-56:⚠️ Potential issueVerify the Definition of
extractV1FunctionThe function
extractV1is called on line 56 but is not defined within this file. Ensure thatextractV1is properly defined or imported to prevent compilation errors.Run the following script to check for the definition of
extractV1:✅ Verification successful
extractV1Function Definition VerifiedThe
extractV1function is defined inpackages/contracts-rfq/test/FastBridgeV2.t.soland is properly imported inFastBridgeV2.Encoding.t.sol:
FastBridgeV2.Encoding.t.solimportsFastBridgeV2.t.sol, ensuring access toextractV1.No compilation errors related to the
extractV1function definition are expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the function `extractV1` is defined in the codebase. # Test: Search for the definition of `extractV1`. # Expect: At least one definition found. rg --type solidity 'function\s+extractV1' --glob '*.sol'Length of output: 91
Script:
#!/bin/bash # Description: Verify that the function `extractV1` is defined in the codebase. # Test: Search for the definition of `extractV1` within Solidity files. # Expect: At least one definition found. rg 'function\s+extractV1' --glob '*.sol'Length of output: 159
Script:
#!/bin/bash # Description: Check if 'FastBridgeV2.Encoding.t.sol' imports the file where 'extractV1' is defined. # Extract import statements from 'packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol' rg 'import\s+' packages/contracts-rfq/test/FastBridgeV2.Encoding.t.solLength of output: 169
packages/contracts-rfq/contracts/FastBridgeV2.sol (8)
57-57: DefaultcallValueset to 0 for backward compatibilityThe
callValue: 0initialization ensures that when thebridgefunction is called withoutparamsV2, the defaultcallValueis zero, maintaining backward compatibility.
130-155: Backward compatibility handling ingetBridgeTransactionThe implementation of
getBridgeTransactioncorrectly attempts to decode the request into a V2 struct, and falls back to V1 if decoding fails. This approach maintains backward compatibility and ensures that both V1 and V2 requests are handled properly.
164-165: AdjustedoriginAmountto account for transfer feesThe code correctly updates
originAmountusing the returned value from_pullTokento handle tokens with transfer fees. This ensures that the actual amount received by the contract is used in subsequent calculations.
184-184: IncludedcallValueinBridgeTransactionV2encodingAdding
callValue: paramsV2.callValueto theBridgeTransactionV2struct ensures that the call value is accurately captured in the encodedrequest. This is important for relayers to process the transaction correctly.
196-206: UpdatedBridgeRequestedevent with new parametersThe
BridgeRequestedevent now includes updated fields, and correctly computessendChainGasbased onparamsV2.callValue != 0. This provides better transparency and aligns the event data with the updated transaction parameters.
226-248: Relay function correctly handles ETH and ERC20 tokens withcallValueThe
relayfunction appropriately distinguishes between ETH and ERC20 tokens, enforcing thatcallValueis zero for ETH transfers and performing necessary validations onmsg.value. The function follows the checks-effects-interactions pattern, enhancing security.
327-341:_pullTokenfunction accounts for tokens with transfer feesThe implementation of
_pullTokencorrectly calculatesamountPulledby comparing the contract's balance before and after the transfer. This handles tokens that may have transfer fees or differing transfer behaviors, ensuring accurate accounting.
358-361: Secure external call with explicit return value validationThe
_checkedCallRecipientfunction safely calls the recipient'sfastBridgeTransferReceivedfunction usingAddress.functionCallWithValueand explicitly checks that the correct selector is returned. This adds a layer of security by ensuring the recipient contract acknowledges the call properly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (12)
packages/contracts-rfq/contracts/FastBridgeV2.sol (4)
Line range hint
161-206: Approved: Enhancements to thebridgefunctionThe changes to the
bridgefunction improve code organization, readability, and functionality:
- The addition of
_validateBridgeParamscentralizes parameter validation, enhancing maintainability.- Replacing
_pullTokenwith_takeBridgedUserAssetprovides a more descriptive name for the operation.- The
BridgeRequestedevent now includes more detailed information, improving traceability.Suggestion for improvement:
Consider using named parameters for theBridgeRequestedevent emission to enhance readability and reduce the risk of parameter order mistakes in future modifications.Example of using named parameters for the event:
- emit BridgeRequested({ - transactionId: transactionId, - sender: params.sender, - request: request, - destChainId: params.dstChainId, - originToken: params.originToken, - destToken: params.destToken, - originAmount: originAmount, - destAmount: params.destAmount, - sendChainGas: paramsV2.callValue != 0 - }); + emit BridgeRequested({ + transactionId: transactionId, + sender: params.sender, + request: request, + destChainId: params.dstChainId, + originToken: params.originToken, + destToken: params.destToken, + originAmount: originAmount, + destAmount: params.destAmount, + sendChainGas: paramsV2.callValue != 0 + });
214-260: Approved: Enhancements to therelayfunction with improved securityThe changes to the
relayfunction significantly improve its functionality and security:
- The addition of
_validateRelayParamscentralizes parameter validation, enhancing maintainability.- New logic for handling
callValueandcallParamssupports the added functionality from the PR objectives.- Explicit checks for correct
msg.valueimprove security by ensuring the expected amount is sent.- The
BridgeRelayedevent now includes more detailed information, improving traceability.Suggestion for improvement:
Consider adding a check for the maximum allowedcallValueto prevent potential issues with extremely large values.Add a constant for the maximum allowed
callValueand a check in the_validateRelayParamsfunction:uint256 public constant MAX_CALL_VALUE = 1000 ether; // Adjust as needed function _validateRelayParams( BridgeTransactionV2 memory transaction, bytes32 transactionId, address relayer ) internal view { // ... existing checks ... if (transaction.callValue > MAX_CALL_VALUE) revert CallValueTooHigh(); }
327-344: Approved: New_takeBridgedUserAssetfunction with robust asset handlingThe new
_takeBridgedUserAssetfunction provides a robust mechanism for taking bridged assets from the user:
- It correctly handles both ETH and ERC20 tokens.
- Includes checks for correct
msg.value, improving security.- Accounts for potential transfer fees in ERC20 tokens, ensuring accurate accounting.
Suggestion for gas optimization:
Consider using unchecked arithmetic for the balance difference calculation, as it's unlikely to overflow.Optimize the balance difference calculation:
- amountTaken = IERC20(token).balanceOf(address(this)) - amountTaken; + unchecked { + amountTaken = IERC20(token).balanceOf(address(this)) - amountTaken; + }
384-411: Approved: Comprehensive_validateBridgeParamsfunctionThe new
_validateBridgeParamsfunction provides a thorough validation of bridge parameters:
- It covers both V1 and V2 parameters, ensuring backwards compatibility.
- Includes checks for chain IDs, amounts, addresses, deadlines, and exclusivity parameters.
- The validations enhance the security and robustness of the bridging process.
Suggestion for improvement:
Consider grouping related checks together and adding comments to improve readability.Reorganize the checks for better readability:
function _validateBridgeParams( BridgeParams memory params, BridgeParamsV2 memory paramsV2, int256 exclusivityEndTime ) internal view { // Chain validation if (params.dstChainId == block.chainid) revert ChainIncorrect(); // Amount validation if (params.originAmount == 0 || params.destAmount == 0) revert AmountIncorrect(); // Address validation if (params.sender == address(0) || params.to == address(0)) revert ZeroAddress(); if (params.originToken == address(0) || params.destToken == address(0)) revert ZeroAddress(); // Deadline validation if (params.deadline < block.timestamp + MIN_DEADLINE_PERIOD) revert DeadlineTooShort(); // V2 specific validations if (paramsV2.callParams.length > MAX_CALL_PARAMS_LENGTH) revert CallParamsLengthAboveMax(); if (paramsV2.callValue != 0 && params.destToken == UniversalTokenLib.ETH_ADDRESS) { revert NativeTokenCallValueNotSupported(); } // Exclusivity validation if (exclusivityEndTime <= 0 || exclusivityEndTime > int256(params.deadline)) { revert ExclusivityParamsIncorrect(); } }packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (8)
304-309: Specify the expected error invm.expectRevertfor clearer test failuresIn
test_relay_token_withCallValue_nonPayableRecipient_revert, consider specifying the expected error invm.expectRevert()to ensure the test only passes when the specific error is thrown. This improves the test's precision and helps in identifying unintended passes.Apply this diff to enhance the test:
function test_relay_token_withCallValue_nonPayableRecipient_revert() public { setTokenTestCallValue(CALL_VALUE); setTokenTestRecipient(nonPayableRecipient); - vm.expectRevert(); + vm.expectRevert(bytes("RecipientCannotReceiveEther()")); relay({caller: relayerA, msgValue: CALL_VALUE, bridgeTx: tokenTx}); }
311-317: Include the specific error invm.expectRevertfor precise testingIn
test_relay_token_withRelayerAddressCallValue_nonPayableRecipient_revert, specifying the expected error invm.expectRevert()enhances test accuracy by ensuring it only passes when the intended error occurs.Apply this diff to improve the test clarity:
function test_relay_token_withRelayerAddressCallValue_nonPayableRecipient_revert() public { setTokenTestCallValue(CALL_VALUE); setTokenTestRecipient(nonPayableRecipient); - vm.expectRevert(); + vm.expectRevert(bytes("RecipientCannotReceiveEther()")); relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: CALL_VALUE, bridgeTx: tokenTx}); }
558-563: Add expected error invm.expectRevertfor better test validationIn
test_relay_token_withRelayerAddress_revert_approvedZero, consider adding the expected error tovm.expectRevert()to ensure the test fails only when the specific approval error occurs.Apply this diff:
function test_relay_token_withRelayerAddress_revert_approvedZero() public { vm.prank(relayerB); dstToken.approve(address(fastBridge), 0); - vm.expectRevert(); + vm.expectRevert("ERC20: transfer amount exceeds allowance"); relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: 0, bridgeTx: tokenTx}); }
565-571: Specify expected error invm.expectRevertto enhance test robustnessIn
test_relay_token_withRelayerAddress_revert_approvedNotEnough, including the expected error invm.expectRevert()makes the test more reliable by confirming it only passes on the correct failure condition.Apply this diff:
function test_relay_token_withRelayerAddress_revert_approvedNotEnough() public { vm.prank(relayerB); dstToken.approve(address(fastBridge), tokenParams.destAmount - 1); - vm.expectRevert(); + vm.expectRevert("ERC20: transfer amount exceeds allowance"); relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: 0, bridgeTx: tokenTx}); }
572-576: Define expected error invm.expectRevertfor accurate test outcomesIn
test_relay_token_withRelayerAddress_revert_nonZeroMsgValue, specifying the expected error invm.expectRevert()ensures that the test accurately checks for the intended revert condition.Apply this change:
function test_relay_token_withRelayerAddress_revert_nonZeroMsgValue() public { - vm.expectRevert(); + vm.expectRevert(MsgValueIncorrect.selector); relayWithAddress({caller: relayerB, relayer: relayerA, msgValue: tokenParams.destAmount, bridgeTx: tokenTx}); }
579-591: Include expected error messages invm.expectRevertfor precise testingIn the ETH relay tests (
test_relay_eth_revert_lowerMsgValue,test_relay_eth_revert_higherMsgValue, andtest_relay_eth_revert_zeroMsgValue), specifying the expected error enhances test clarity and ensures they only pass when the correct error is thrown.Example diff for
test_relay_eth_revert_lowerMsgValue:function test_relay_eth_revert_lowerMsgValue() public { - vm.expectRevert(); + vm.expectRevert(MsgValueIncorrect.selector); relay({caller: relayerB, msgValue: ethParams.destAmount - 1, bridgeTx: ethTx}); }Repeat similar changes for the other two functions.
592-604: Specify expected errors invm.expectRevertfor consistent test behaviorIn the ETH relay tests with relayer address (
test_relay_eth_withRelayerAddress_revert_lowerMsgValue,test_relay_eth_withRelayerAddress_revert_higherMsgValue, andtest_relay_eth_withRelayerAddress_revert_zeroMsgValue), adding the expected error tovm.expectRevert()ensures tests are robust and only pass on the correct error.Example diff for
test_relay_eth_withRelayerAddress_revert_lowerMsgValue:function test_relay_eth_withRelayerAddress_revert_lowerMsgValue() public { - vm.expectRevert(); + vm.expectRevert(MsgValueIncorrect.selector); relayWithAddress({caller: relayerA, relayer: relayerB, msgValue: ethParams.destAmount - 1, bridgeTx: ethTx}); }Apply similar adjustments to the other functions.
26-27: Consider using a descriptive name forCALL_VALUEWhile
CALL_VALUEis functional, a more descriptive constant name could improve readability. For instance,DEFAULT_CALL_VALUEorTEST_CALL_VALUEmight better convey its purpose in tests.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (7 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (15 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol
🧰 Additional context used
🔇 Additional comments (4)
packages/contracts-rfq/contracts/FastBridgeV2.sol (4)
57-57: LGTM: Addition ofcallValuetoBridgeParamsV2The addition of the
callValuefield to theBridgeParamsV2struct enhances the flexibility of bridging operations, allowing for native value transfers on the destination chain. This change aligns well with the PR objectives.
361-361: LGTM: Updated_checkedCallRecipientto forwardmsg.valueThe addition of
value: msg.valueto thefunctionCallWithValuecall in_checkedCallRecipientis correct and consistent with the new functionality for handlingcallValue. This change allows the forwarding of native tokens to the recipient's hook function when required.
413-437: Approved: Robust_validateRelayParamsfunctionThe new
_validateRelayParamsfunction provides comprehensive validation for relay parameters:
- It checks the relayer address, ensuring it's not the zero address.
- Verifies that the transaction hasn't been relayed already.
- Validates the destination chain ID.
- Checks the deadline for the relay.
- Handles the exclusivity period correctly.
These validations significantly enhance the security and correctness of the relay process. The function is well-structured and covers all necessary aspects of parameter validation.
130-155: Approved: EnhancedgetBridgeTransactionwith V2 supportThe function has been successfully updated to support both V1 and V2 transaction structures, maintaining backwards compatibility. This is a good approach for handling the transition to the new structure.
However, note that there's a subtle change in behavior:
- The
sendChainGasfield is now derived from whethercallValueis non-zero, rather than being explicitly set.Ensure that this change in behavior is documented and that all dependent systems are updated accordingly.
To verify the impact of this change, we can search for usages of
getBridgeTransaction:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
packages/contracts-rfq/contracts/FastBridgeV2.sol (6)
57-57: LGTM! Consider adding documentation forcallValue.The addition of
callValueto theBridgeParamsV2struct enhances the flexibility of bridging operations. This change aligns well with the PR objectives.Consider adding a brief comment explaining the purpose and usage of
callValueto improve code clarity.
130-155: LGTM! Consider a minor optimization for V1 transactions.The implementation of
getBridgeTransactioneffectively handles both V1 and V2 structures, ensuring backward compatibility. The use of a try-catch block for decoding is appropriate.Consider adding a version byte to the encoded request to avoid unnecessary try-catch for V1 transactions. This could slightly improve performance for older transaction formats.
Line range hint
161-206: LGTM! Consider enhancing error handling for_takeBridgedUserAsset.The refactoring of the
bridgefunction improves code organization and adds support for new functionality. The use of_validateBridgeParamsand_takeBridgedUserAssetenhances the robustness of the bridging process.Consider wrapping the call to
_takeBridgedUserAssetin a try-catch block to handle potential errors more gracefully, especially for tokens with unusual behavior.
214-262: LGTM! Consider a gas optimization for ETH transfers.The refactoring of the
relayfunction improves code organization and adds support forcallValueand arbitrary calls. The implementation correctly handles different token types and follows good security practices.For ETH transfers without
callParams, consider using a low-levelcallinstead ofAddress.sendValueto save gas. However, ensure proper checks are in place to handle potential failure.
329-347: LGTM! Consider adding a sanity check for the transferred amount.The
_takeBridgedUserAssetfunction is well-implemented, handling both ETH and ERC20 tokens correctly. It properly accounts for potential transfer fees in ERC20 tokens.Consider adding a sanity check to ensure that
amountTakenis not zero after the transfer, which could indicate a failed transfer for certain token implementations.
386-413: LGTM! Consider grouping related checks.The
_validateBridgeParamsfunction performs comprehensive validation of both V1 and V2 parameters. The checks are thorough and cover all necessary aspects.Consider grouping related checks together (e.g., all address checks, all amount checks) to improve readability and maintainability of the function.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (7 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
415-439: LGTM! Comprehensive validation of relay parameters.The
_validateRelayParamsfunction performs thorough checks on all relevant aspects of the relay operation. The validation of the exclusivity period is particularly well-implemented.The logical ordering of checks, starting with the most basic ones, is a good practice that can help in early rejection of invalid requests.
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/contracts-rfq/test/FastBridgeV2.t.sol (1)
Line range hint
159-171: Consider initializingcallValueandcallParamsinsetStorageBridgeTxV2.The
setStorageBridgeTxV2function currently does not assign values to the newly addedcallValueandcallParamsfields in thetxV2struct. This omission might lead to uninitialized fields during testing, potentially causing unexpected behavior.Apply the following changes to include these fields:
function setStorageBridgeTxV2( IFastBridgeV2.BridgeTransactionV2 storage txV2, IFastBridge.BridgeTransaction memory txV1 ) internal { txV2.originChainId = txV1.originChainId; txV2.destChainId = txV1.destChainId; txV2.originSender = txV1.originSender; txV2.destRecipient = txV1.destRecipient; txV2.originToken = txV1.originToken; txV2.destToken = txV1.destToken; txV2.originAmount = txV1.originAmount; txV2.destAmount = txV1.destAmount; txV2.originFeeAmount = txV1.originFeeAmount; txV2.deadline = txV1.deadline; txV2.nonce = txV1.nonce; + txV2.callValue = 0; + txV2.callParams = bytes(""); }Alternatively, if
callValueandcallParamsare available intxV1, copy them accordingly:+ txV2.callValue = txV1.callValue; + txV2.callParams = txV1.callParams;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
packages/contracts-rfq/test/FastBridgeV2.t.sol (3)
131-133: Initialization of new fields inBridgeParamsV2is correctly implemented.The
callValueandcallParamsfields are properly initialized in bothtokenParamsV2andethParamsV2, ensuring that the new parameters are correctly set up for testing purposes.Also applies to: 138-140
172-175: MethodsetTokenTestCallValuecorrectly updates test parameters.The function appropriately sets the
callValuein bothtokenParamsV2andtokenTx, maintaining consistency in the test environment for token transactions.
191-194: MethodsetEthTestCallValuecorrectly updates test parameters.This function accurately sets the
callValuein bothethParamsV2andethTx, ensuring consistency for ETH transactions in the tests.
Description
This PR finishes the work started in #3215 by adding the functionality to request a native value to be supplied on the destination chain, when relaying the request.
The requested "call value" will be attached to the recipient hook function call (if
callParamsis a non-empty payload), or transferred as a separate native transfer after the tokens are sent.Summary by CodeRabbit
New Features
Bug Fixes
Tests