feat(contracts-rfq): add version to BridgeTransaction, tight packing [SLT-328] [SLT-273]
#3284
Conversation
WalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
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: |
2c18409
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://808011ee.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-fbv2-version-encoding.sanguine-fe.pages.dev |
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 #3284 +/- ##
===================================================
- Coverage 34.42327% 28.69508% -5.72820%
===================================================
Files 218 184 -34
Lines 13273 12131 -1142
Branches 356 161 -195
===================================================
- Hits 4569 3481 -1088
+ Misses 8414 8366 -48
+ Partials 290 284 -6
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}
packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol
Dismissed
Show dismissed
Hide dismissed
packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol
Dismissed
Show dismissed
Hide dismissed
packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol
Dismissed
Show dismissed
Hide dismissed
packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol
Dismissed
Show dismissed
Hide dismissed
packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol
Dismissed
Show dismissed
Hide dismissed
packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol
Dismissed
Show dismissed
Hide dismissed
packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol
Dismissed
Show dismissed
Hide dismissed
packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol
Dismissed
Show dismissed
Hide dismissed
packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol
Dismissed
Show dismissed
Hide dismissed
packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol
Dismissed
Show dismissed
Hide dismissed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (18)
packages/contracts-rfq/test/FastBridgeV2.GasBench.Encoding.t.sol (1)
16-16: LGTM: Encoding method updated to use BridgeTransactionV2Lib.The change from
abi.encode(tokenTx)toBridgeTransactionV2Lib.encodeV2(tokenTx)aligns with the PR objectives and likely improves gas efficiency. This update is appropriate for a V2 test.Consider adding a comment explaining the gas efficiency improvement, e.g.:
// Use BridgeTransactionV2Lib.encodeV2 for improved gas efficiency bytes memory request = BridgeTransactionV2Lib.encodeV2(tokenTx);packages/contracts-rfq/test/FastBridgeV2.Dst.Base.t.sol (1)
Line range hint
1-51: Summary: Consistent implementation of new BridgeTransactionV2 encodingThe changes in this file consistently implement a new encoding method for
BridgeTransactionV2usingBridgeTransactionV2Lib.encodeV2(). This appears to be part of a larger refactoring effort.To ensure the integrity of this refactoring:
- Verify that all occurrences of
BridgeTransactionV2encoding throughout the codebase have been updated to use the new method.- Ensure that corresponding decoding methods are implemented and tested.
- Update relevant documentation to reflect this change in encoding methodology.
- Consider adding integration tests to verify the compatibility of this encoding change with other components of the system.
packages/contracts-rfq/test/libs/BridgeTransactionV2.t.sol (4)
8-8: Consider addressing linter warnings instead of disabling them.The
solhint-disablecomment is suppressing warnings forfunc-name-mixedcaseandordering. While this can be useful for temporary development, it's generally better to address these issues directly:
- For
func-name-mixedcase: Consider renaming test functions to follow Solidity naming conventions (e.g.,testRoundtripinstead oftest_roundtrip).- For
ordering: Reorganize the contract members to follow the recommended order (type declarations, state variables, events, modifiers, constructors, functions).Addressing these issues can improve code readability and maintainability.
16-38: LGTM: Comprehensive equality check for BridgeTransactionV2.The
assertEqfunction thoroughly compares all fields of theBridgeTransactionV2struct, which is excellent for detailed testing. The use of individualassertEqcalls for each field will provide more specific error messages if a test fails.Consider adding a custom error message parameter to provide context when assertions fail:
function assertEq( IFastBridgeV2.BridgeTransactionV2 memory a, IFastBridgeV2.BridgeTransactionV2 memory b, string memory message ) public pure { assertEq(a.originChainId, b.originChainId, string(abi.encodePacked(message, ": originChainId mismatch"))); // Repeat for other fields... }This would allow test cases to provide more context when calling this function, making debugging easier.
40-58: LGTM: Comprehensive roundtrip test for BridgeTransactionV2.The
test_roundtripfunction thoroughly tests the encoding process by checking each field individually after encoding. This approach ensures that all data is correctly preserved during the encoding process.To improve test coverage and readability, consider:
- Adding explicit test cases with predefined
BridgeTransactionV2structs to cover edge cases.- Using a helper function to reduce repetition in assertions:
function assertEncodedTxEquals(bytes memory encodedTx, IFastBridgeV2.BridgeTransactionV2 memory expectedTx) internal view { assertEq(harness.version(encodedTx), 2); assertEq(harness.originChainId(encodedTx), expectedTx.originChainId); // ... repeat for other fields } function test_roundtrip(IFastBridgeV2.BridgeTransactionV2 memory bridgeTx) public view { bytes memory encodedTx = harness.encodeV2(bridgeTx); assertEncodedTxEquals(encodedTx, bridgeTx); }This refactoring would make the test more maintainable and easier to extend with additional test cases.
60-64: LGTM: Effective roundtrip test for encoding and decoding.The
test_roundtrip_decodeV2function effectively tests both the encoding and decoding processes in a single roundtrip. The use of the customassertEqfunction ensures a thorough comparison of all fields.To enhance the test coverage, consider adding the following:
- Explicit test cases with predefined
BridgeTransactionV2structs to cover edge cases and specific scenarios.- Negative tests to ensure proper handling of invalid encoded data:
function test_decodeV2_invalidData() public { bytes memory invalidEncodedTx = abi.encodePacked(bytes1(0x02), bytes31(0)); vm.expectRevert("Invalid encoded transaction"); harness.decodeV2(invalidEncodedTx); }
- A test for backwards compatibility if there's a previous version of the encoding:
function test_decodeV2_backwardsCompatibility() public { // Assuming there's a V1 encoding function bytes memory v1EncodedTx = harness.encodeV1(/* ... */); vm.expectRevert("Unsupported version"); harness.decodeV2(v1EncodedTx); }These additions would provide more comprehensive coverage of the encoding and decoding functionality.
packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (3)
46-49: LGTM: Renamed parameter for clarity in test_getBridgeTransaction.The renaming of
bridgeTxtobridgeTxV1improves the clarity of the test case, explicitly indicating that it's for V1 transactions. This change is consistent with the support for both V1 and V2 transactions.Consider updating the function name to
test_getBridgeTransactionV1for even better clarity and consistency with the V2 test case naming.
56-60: LGTM: Updated encoding and expectation for V2 transactions.The changes correctly implement the new encoding strategy for V2 transactions using
BridgeTransactionV2Lib.encodeV2. The adjustment ofsendChainGasbased oncallValueaccurately reflects the changes in the V2 transaction structure.Consider extracting the
sendChainGasassignment to a separate line for improved readability:expectedTx = extractV1(bridgeTxV2); expectedTx.sendChainGas = bridgeTxV2.callValue > 0; assertEq(decodedTx, expectedTx);
69-73: LGTM: Renamed parameter for clarity in test_getBridgeTransactionV2_revert_usedRequestV1.The renaming of
bridgeTxtobridgeTxV1improves the clarity of the test case, explicitly indicating that it's using a V1 transaction to test the revert behavior of the V2 function.For consistency with the other test cases, consider updating the encoding to use
abi.encodeexplicitly:bytes memory request = abi.encode(bridgeTxV1);This change would make it clearer that we're using the V1 encoding method for this test case.
packages/contracts-rfq/test/harnesses/BridgeTransactionV2Harness.sol (2)
15-77: LGTM: All getter functions correctly wrap library methods.All getter functions (version through callParams) are implemented correctly, providing a clean interface to their corresponding BridgeTransactionV2Lib methods. The consistent use of calldata for input parameters is gas-efficient.
Consider adding NatSpec documentation to each function to improve code readability and generate comprehensive documentation. This would be particularly helpful for developers using this harness for testing purposes.
1-78: Overall, the BridgeTransactionV2Harness contract is well-implemented.The contract successfully serves its purpose as a test harness for BridgeTransactionV2Lib. All functions are correctly implemented, providing a clean and gas-efficient interface for testing the library's functionality.
To further improve the contract:
- Add NatSpec documentation to the contract and each function.
- Consider grouping related functions (e.g., origin-related, destination-related) using comments or regions for better organization.
- If applicable, add any relevant test helper functions that might be useful for common testing scenarios.
packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol (2)
77-77: LGTM: Updated to use BridgeTransactionV2Lib.encodeV2.The change correctly implements the new encoding method, which aligns with the PR objectives. The function signature remains unchanged, maintaining backward compatibility.
Consider adding a comment explaining the reason for using
encodeV2to improve code readability and maintainability.
Line range hint
1-108: Overall assessment: Consistent and well-implemented changes.The updates in this file consistently implement the use of
BridgeTransactionV2Lib.encodeV2for encodingBridgeTransactionV2structures. These changes align with the PR objectives of enhancing gas efficiency while maintaining backward compatibility. The code remains clean and readable, with only minor suggestions for improvement.Consider adding a comment at the beginning of the file explaining the transition to
BridgeTransactionV2Lib.encodeV2and its benefits, to provide context for future developers working on this code.packages/contracts-rfq/contracts/FastBridgeV2.sol (3)
Line range hint
285-314: Potential Reentrancy Vulnerability inclaimFunctionSimilar to the
refundfunction, theclaimfunction has an external calltoken.universalTransfer(to, amount)at line 311. Emitting the eventBridgeDepositClaimedafter this external call may lead to inconsistencies if a reentrancy attack occurs. Emitting the event before the external call aligns with best practices and enhances security.Apply this diff to move the event emission before the external call:
bridgeTxDetails[transactionId].status = BridgeStatus.RELAYER_CLAIMED; +emit BridgeDepositClaimed(transactionId, bridgeTxDetails[transactionId].proofRelayer, to, token, amount); // update protocol fees if origin fee amount exists address token = request.originToken(); uint256 amount = request.originAmount(); uint256 originFeeAmount = request.originFeeAmount(); if (originFeeAmount > 0) protocolFees[token] += originFeeAmount; // transfer origin collateral less fee to specified address token.universalTransfer(to, amount);
418-430: Use ofblock.timestampfor Time ComparisonsIn
_validateRelayParams,block.timestampis used for time comparisons at lines 424 and 427. While acceptable, it's important to note that miners can manipulate timestamps within a certain range. Ensure that the contract logic can tolerate slight deviations and that critical security decisions aren't solely dependent on precise timestamps.🧰 Tools
🪛 GitHub Check: Slither
[notice] 418-430: Block timestamp
FastBridgeV2._validateRelayParams(bytes,bytes32,address) (contracts/FastBridgeV2.sol#418-430) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > request.deadline() (contracts/FastBridgeV2.sol#424)
- exclRelayer != address(0) && exclRelayer != relayer && block.timestamp <= request.exclusivityEndTime() (contracts/FastBridgeV2.sol#427)
Line range hint
358-374: Gas Optimization in_checkedCallRecipientFunctionThe return data verification logic in
_checkedCallRecipientis thorough but may benefit from gas optimization. Review the necessity of each check to identify potential simplifications that maintain security while reducing gas costs.packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (2)
428-433: Clarify the Invalid Request V2 Test CaseIn the function
test_relay_revert_invalidRequestV2, consider adding comments to explain whyinvalidRequestV2is considered invalid. Providing context improves readability and helps future developers understand the purpose of the test.
465-470: Add Context to Invalid Request V2 with Relayer AddressIn
test_relay_withRelayerAddress_revert_invalidRequestV2, adding a comment to describe whyinvalidRequestV2fails when used with a relayer address would enhance the test's clarity and maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (13 hunks)
- packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.Base.t.sol (3 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Encoding.t.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol (3 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (3 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (3 hunks)
- packages/contracts-rfq/test/harnesses/BridgeTransactionV2Harness.sol (1 hunks)
- packages/contracts-rfq/test/libs/BridgeTransactionV2.t.sol (1 hunks)
🧰 Additional context used
🪛 GitHub Check: Slither
packages/contracts-rfq/contracts/FastBridgeV2.sol
[notice] 100-120: Reentrancy vulnerabilities
Reentrancy in FastBridgeV2.refund(bytes) (contracts/FastBridgeV2.sol#100-120):
External calls:
- token.universalTransfer(to,amount) (contracts/FastBridgeV2.sol#117)
Event emitted after the call(s):
- BridgeDepositRefunded(transactionId,to,token,amount) (contracts/FastBridgeV2.sol#119)
[notice] 100-120: Block timestamp
FastBridgeV2.refund(bytes) (contracts/FastBridgeV2.sol#100-120) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp <= deadline (contracts/FastBridgeV2.sol#109)
[notice] 285-314: Reentrancy vulnerabilities
Reentrancy in FastBridgeV2.claim(bytes,address) (contracts/FastBridgeV2.sol#285-314):
External calls:
- token.universalTransfer(to,amount) (contracts/FastBridgeV2.sol#311)
Event emitted after the call(s):
- BridgeDepositClaimed(transactionId,bridgeTxDetails[transactionId].proofRelayer,to,token,amount) (contracts/FastBridgeV2.sol#313)
[notice] 285-314: Block timestamp
FastBridgeV2.claim(bytes,address) (contracts/FastBridgeV2.sol#285-314) uses timestamp for comparisons
Dangerous comparisons:
- _timeSince(bridgeTxDetails[transactionId].proofBlockTimestamp) <= DISPUTE_PERIOD (contracts/FastBridgeV2.sol#298)
[notice] 418-430: Block timestamp
FastBridgeV2._validateRelayParams(bytes,bytes32,address) (contracts/FastBridgeV2.sol#418-430) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > request.deadline() (contracts/FastBridgeV2.sol#424)
- exclRelayer != address(0) && exclRelayer != relayer && block.timestamp <= request.exclusivityEndTime() (contracts/FastBridgeV2.sol#427)packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol
[warning] 112-117: Assembly usage
BridgeTransactionV2Lib.version(bytes) (contracts/libs/BridgeTransactionV2.sol#112-117) uses assembly
- INLINE ASM (contracts/libs/BridgeTransactionV2.sol#114-116)
[warning] 120-125: Assembly usage
BridgeTransactionV2Lib.originChainId(bytes) (contracts/libs/BridgeTransactionV2.sol#120-125) uses assembly
- INLINE ASM (contracts/libs/BridgeTransactionV2.sol#122-124)
[warning] 128-133: Assembly usage
BridgeTransactionV2Lib.destChainId(bytes) (contracts/libs/BridgeTransactionV2.sol#128-133) uses assembly
- INLINE ASM (contracts/libs/BridgeTransactionV2.sol#130-132)
[warning] 136-141: Assembly usage
BridgeTransactionV2Lib.originSender(bytes) (contracts/libs/BridgeTransactionV2.sol#136-141) uses assembly
- INLINE ASM (contracts/libs/BridgeTransactionV2.sol#138-140)
[warning] 144-149: Assembly usage
BridgeTransactionV2Lib.destRecipient(bytes) (contracts/libs/BridgeTransactionV2.sol#144-149) uses assembly
- INLINE ASM (contracts/libs/BridgeTransactionV2.sol#146-148)
[warning] 152-157: Assembly usage
BridgeTransactionV2Lib.originToken(bytes) (contracts/libs/BridgeTransactionV2.sol#152-157) uses assembly
- INLINE ASM (contracts/libs/BridgeTransactionV2.sol#154-156)
[warning] 160-165: Assembly usage
BridgeTransactionV2Lib.destToken(bytes) (contracts/libs/BridgeTransactionV2.sol#160-165) uses assembly
- INLINE ASM (contracts/libs/BridgeTransactionV2.sol#162-164)
[warning] 168-173: Assembly usage
BridgeTransactionV2Lib.originAmount(bytes) (contracts/libs/BridgeTransactionV2.sol#168-173) uses assembly
- INLINE ASM (contracts/libs/BridgeTransactionV2.sol#170-172)
[warning] 176-181: Assembly usage
BridgeTransactionV2Lib.destAmount(bytes) (contracts/libs/BridgeTransactionV2.sol#176-181) uses assembly
- INLINE ASM (contracts/libs/BridgeTransactionV2.sol#178-180)
[warning] 184-189: Assembly usage
BridgeTransactionV2Lib.originFeeAmount(bytes) (contracts/libs/BridgeTransactionV2.sol#184-189) uses assembly
- INLINE ASM (contracts/libs/BridgeTransactionV2.sol#186-188)
[warning] 192-197: Assembly usage
BridgeTransactionV2Lib.callValue(bytes) (contracts/libs/BridgeTransactionV2.sol#192-197) uses assembly
- INLINE ASM (contracts/libs/BridgeTransactionV2.sol#194-196)
[warning] 200-205: Assembly usage
BridgeTransactionV2Lib.deadline(bytes) (contracts/libs/BridgeTransactionV2.sol#200-205) uses assembly
- INLINE ASM (contracts/libs/BridgeTransactionV2.sol#202-204)
[warning] 208-213: Assembly usage
BridgeTransactionV2Lib.nonce(bytes) (contracts/libs/BridgeTransactionV2.sol#208-213) uses assembly
- INLINE ASM (contracts/libs/BridgeTransactionV2.sol#210-212)
[warning] 216-221: Assembly usage
BridgeTransactionV2Lib.exclusivityRelayer(bytes) (contracts/libs/BridgeTransactionV2.sol#216-221) uses assembly
- INLINE ASM (contracts/libs/BridgeTransactionV2.sol#218-220)
[warning] 224-229: Assembly usage
BridgeTransactionV2Lib.exclusivityEndTime(bytes) (contracts/libs/BridgeTransactionV2.sol#224-229) uses assembly
- INLINE ASM (contracts/libs/BridgeTransactionV2.sol#226-228)
🔇 Additional comments (32)
packages/contracts-rfq/test/FastBridgeV2.GasBench.Encoding.t.sol (2)
4-4: LGTM: New import statement added correctly.The import of
BridgeTransactionV2Libis necessary for the changes in thetest_getBridgeTransactionV2function. The relative path appears to be correct.
Line range hint
1-24: Verify consistency across other test files.The changes in this file are minimal but significant, aligning well with the PR objectives. To ensure consistency across the test suite:
- Check if other test files need similar updates to use
BridgeTransactionV2Lib.- Verify if there are other occurrences of
abi.encode(tokenTx)that should be replaced withBridgeTransactionV2Lib.encodeV2(tokenTx).packages/contracts-rfq/test/FastBridgeV2.Dst.Base.t.sol (3)
4-4: LGTM: New import added for BridgeTransactionV2LibThe import of
BridgeTransactionV2Libis correctly added and is necessary for the changes in therelayandrelayWithAddressfunctions.
48-51: LGTM: Consistent use of new encoding methodThe change in
relayWithAddressis consistent with the modification in therelayfunction, usingBridgeTransactionV2Lib.encodeV2(bridgeTx)instead ofabi.encode(bridgeTx). This ensures consistency in howBridgeTransactionV2is encoded across different relay methods.
35-37: Verify the new encoding method for BridgeTransactionV2The change from
abi.encode(bridgeTx)toBridgeTransactionV2Lib.encodeV2(bridgeTx)looks good. This custom encoding method could potentially optimize gas usage or add version-specific encoding.Please ensure that:
- The
encodeV2method inBridgeTransactionV2Libcorrectly encodes all necessary fields ofBridgeTransactionV2.- The corresponding decoding method is implemented and tested.
- This change is consistent across all relevant parts of the codebase.
You can use the following script to verify the usage of
encodeV2:✅ Verification successful
BridgeTransactionV2Lib.encodeV2 Verified
The custom encoding method
BridgeTransactionV2Lib.encodeV2is used consistently across the codebase, and no instances ofabi.encode(bridgeTx)remain.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of BridgeTransactionV2Lib.encodeV2 # Test: Search for all occurrences of encodeV2. Expect: Consistent usage across the codebase. rg --type solidity -A 3 'BridgeTransactionV2Lib\.encodeV2' # Test: Search for potential missed abi.encode calls for BridgeTransactionV2. Expect: No results. rg --type solidity 'abi\.encode\(\s*bridgeTx\s*\)'Length of output: 179
Script:
#!/bin/bash # Description: Verify the usage of BridgeTransactionV2Lib.encodeV2 # Test: Search for all occurrences of encodeV2 in Solidity files. Expect: Consistent usage across the codebase. rg -g "*.sol" -A 3 'BridgeTransactionV2Lib\.encodeV2' # Test: Search for potential missed abi.encode calls for BridgeTransactionV2 in Solidity files. Expect: No results. rg -g "*.sol" 'abi\.encode\(\s*bridgeTx\s*\)'Length of output: 6221
packages/contracts-rfq/test/libs/BridgeTransactionV2.t.sol (1)
9-14: LGTM: Contract declaration and setup look good.The contract structure and
setUpfunction follow standard testing practices. The use ofinternalfor the harness instance is appropriate, ensuring encapsulation while allowing derived contracts to access it if needed.packages/contracts-rfq/test/FastBridgeV2.Encoding.t.sol (2)
4-5: LGTM: Import statement for BridgeTransactionV2Lib added.The import of
BridgeTransactionV2Libis necessary for the updated encoding functions used in the test cases. This addition aligns with the changes made to support V2 transactions.
64-67: LGTM: Updated encoding for V2 transactions in test_getBridgeTransactionV2.The change to use
BridgeTransactionV2Lib.encodeV2for encoding the request is consistent with the new encoding strategy for V2 transactions. This update ensures that the test case accurately reflects the behavior of the updatedFastBridgeV2contract.packages/contracts-rfq/test/harnesses/BridgeTransactionV2Harness.sol (3)
1-6: LGTM: Contract structure and imports are well-defined.The contract structure, license, Solidity version, and import statement are all appropriate and well-organized. The use of a relative path for imports enhances maintainability.
7-9: LGTM: encodeV2 function correctly wraps the library function.The function signature and implementation are correct, providing a clean interface to the BridgeTransactionV2Lib.encodeV2 method.
11-13: LGTM: decodeV2 function correctly wraps the library function.The function signature and implementation are correct, providing a clean interface to the BridgeTransactionV2Lib.decodeV2 method.
packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol (3)
4-4: LGTM: New import statement added correctly.The import of
BridgeTransactionV2Libis necessary for the updated encoding method used in the contract. The import statement is correctly formatted and placed appropriately in the file.
82-82: LGTM: Updated to use BridgeTransactionV2Lib.encodeV2.The change correctly implements the new encoding method, consistent with the previous updates and aligning with the PR objectives. The function signature remains unchanged, maintaining backward compatibility.
87-87: LGTM: Updated to use BridgeTransactionV2Lib.encodeV2.The change correctly implements the new encoding method, consistent with the previous updates and aligning with the PR objectives. The function signature remains unchanged, maintaining backward compatibility.
packages/contracts-rfq/test/FastBridgeV2.t.sol (2)
254-254: Appropriate Use of BridgeTransactionV2Lib.encodeV2 in getTxIdThe
getTxIdfunction correctly utilizesBridgeTransactionV2Lib.encodeV2(bridgeTx)to generate a consistent transaction ID, ensuring alignment with the encoding used elsewhere.
261-264: New Helper Functions Enhance Test ClarityThe addition of
expectRevertInvalidEncodedTxandexpectRevertUnsupportedVersionfunctions improves test readability and reusability by encapsulating common revert expectations.Also applies to: 265-269
packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol (6)
46-47: Clear and specific error handlingThe error definitions
BridgeTransactionV2__InvalidEncodedTx()andBridgeTransactionV2__UnsupportedVersion(uint16 version)provide precise error messages, enhancing code clarity and debugging efficiency.
51-57: Robust validation of encoded transactionsThe
validateV2function correctly ensures that the encoded transaction meets the minimum length requirement and verifies the version. This preemptive validation enhances security and prevents decoding invalid data.
62-84: Efficient encoding with tight packingThe
encodeV2function effectively uses tight packing to encode theBridgeTransactionV2struct, optimizing gas usage. Splitting the encoding into two parts to avoid thestack-too-deeperror is a practical solution.
89-109: Comprehensive decoding of transaction fieldsThe
decodeV2function systematically decodes each field of the encoded transaction, ensuring all data is accurately retrieved. This methodical approach enhances reliability.
113-117: Appropriate use of inline assembly for data extractionThe functions (
version,originChainId,destChainId,originSender,destRecipient,originToken,destToken,originAmount,destAmount,originFeeAmount,callValue,deadline,nonce,exclusivityRelayer,exclusivityEndTime) utilize inline assembly to extract data efficiently from the tightly packed encoded transaction. Given the need for precise control over calldata manipulation and gas optimization, the use of assembly is justified in this context.Also applies to: 120-125, 128-133, 136-141, 144-149, 152-157, 160-165, 168-173, 176-181, 184-189, 192-197, 200-205, 208-213, 216-221, 224-229
🧰 Tools
🪛 GitHub Check: Slither
[warning] 112-117: Assembly usage
BridgeTransactionV2Lib.version(bytes) (contracts/libs/BridgeTransactionV2.sol#112-117) uses assembly
- INLINE ASM (contracts/libs/BridgeTransactionV2.sol#114-116)
231-234: Efficient slicing of calldata forcallParamsThe
callParamsfunction correctly slices theencodedTxto retrieve the dynamiccallParamsfield, ensuring all remaining data is captured without additional computation.packages/contracts-rfq/contracts/FastBridgeV2.sol (5)
7-7: Import and Usage ofBridgeTransactionV2LibThe inclusion of
BridgeTransactionV2Liband its application tobytesenhances code modularity and encapsulates the bridge transaction logic, improving maintainability and readability.Also applies to: 18-18
66-69: Optimizing Function Parameters withcalldataUpdating function parameters from
bytes memorytobytes calldatainrelay,prove, andclaimfunctions improves gas efficiency by avoiding unnecessary memory allocation. This optimization is beneficial for contract performance.Also applies to: 72-75, 78-81
Line range hint
134-157: Graceful Handling of Transaction Decoding ingetBridgeTransactionEmploying a try-catch block to decode the request using
getBridgeTransactionV2and falling back to V1 decoding ensures backward compatibility. This approach is robust and allows seamless integration of the new V2 structure without disrupting existing functionality.
158-162: Addition ofgetBridgeTransactionV2FunctionIntroducing the
getBridgeTransactionV2function provides a dedicated method to decode V2 bridge transactions. This addition enhances clarity and modularity, promoting better code organization.
Line range hint
179-200: Utilization ofBridgeTransactionV2Lib.encodeV2inbridgeFunctionUpdating the
bridgefunction to useBridgeTransactionV2Lib.encodeV2for encoding requests centralizes the encoding logic. This change improves code reuse and maintainability by leveraging the library's capabilities.packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (2)
434-438: Verify the Placement ofvm.prankCallIn
test_relay_revert_requestV3, ensure that thevm.prankcall is correctly placed and included in the changed lines. Omitting changes in setup functions likevm.prankmay lead to incomplete test configurations.To confirm, run the following script to check the use of
vm.prankin version-related test functions:✅ Verification successful
vm.prank Placement Verified Successfully
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that vm.prank calls are properly placed in version-related tests. # Test: Search for vm.prank calls in test_relay_revert_requestV3 function. # Expect: vm.prank call should be immediately before the relay call. ast-grep --lang solidity --pattern $'function test_relay_revert_requestV3() { $$$ vm.prank($_); $$$ }'Length of output: 226
Script:
#!/bin/bash # Description: Verify that vm.prank calls are properly placed in version-related tests. # Extract lines from the test_relay_revert_requestV3 function rg -A5 'function test_relay_revert_requestV3\(\)' packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol > function_lines.txt # Check if vm.prank is immediately before fastBridge.relay prank_line=$(grep -n 'vm\.prank' function_lines.txt | cut -d: -f1) relay_line=$(grep -n 'fastBridge\.relay' function_lines.txt | cut -d: -f1) if [ "$prank_line" ] && [ "$relay_line" ]; then difference=$((relay_line - prank_line)) if [ "$difference" -eq 1 ]; then echo "vm.prank is correctly placed immediately before fastBridge.relay." else echo "vm.prank is not immediately before fastBridge.relay." fi else echo "Required function calls not found." fiLength of output: 452
471-475: Verify Correct Usage ofvm.prankin V3 TestSimilar to previous observations, in
test_relay_withRelayerAddress_revert_requestV3, confirm that thevm.prankcall is correctly positioned and fully included in the changes. This ensures the test accurately simulates the intended caller context.Use the script below to validate the placement of
vm.prank:✅ Verification successful
vm.prank Correctly Precedes relay Call in test_relay_withRelayerAddress_revert_requestV3
The
vm.prankcall is correctly positioned before therelayfunction call within thetest_relay_withRelayerAddress_revert_requestV3function.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify vm.prank call placement in test_relay_withRelayerAddress_revert_requestV3. # Test: Ensure vm.prank is used correctly in the test function. # Expect: vm.prank call should precede the relay function call. ast-grep --lang solidity --pattern $'function test_relay_withRelayerAddress_revert_requestV3() { $$$ vm.prank($_); $$$ }'Length of output: 245
Script:
#!/bin/bash # Description: Verify the placement of vm.prank in test_relay_withRelayerAddress_revert_requestV3 using ripgrep. # Extract the block of the specific test function rg -A 10 'function test_relay_withRelayerAddress_revert_requestV3\(\)' packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol # Within the extracted block, check the order of vm.prank and relay calls # This assumes the function block does not exceed 10 lines after the function declaration rg -A 10 'function test_relay_withRelayerAddress_revert_requestV3\(\)' packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol | grep -E 'vm\.prank|fastBridge\.relay'Length of output: 895
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (3)
4-4: Proper import ofBridgeTransactionV2LibThe addition of
import {BridgeTransactionV2Lib} from "../contracts/libs/BridgeTransactionV2.sol";correctly incorporates the necessary library for encoding bridge transactions. This ensures that theencodeV2function is available for use within the contract.
41-41: Correct usage ofBridgeTransactionV2Lib.encodeV2The update in the
expectBridgeRequestedfunction to useBridgeTransactionV2Lib.encodeV2(bridgeTx)enhances the encoding of bridge transaction requests. This change aligns with the new encoding standards and ensures consistency across the contract.
919-993: Comprehensive testing of invalid payloads and unsupported versionsThe newly added test functions from
test_prove_revert_requestV1totest_refund_revert_requestV3effectively cover scenarios involving unsupported versions and invalid encoded transactions for theprove,claim, andrefundfunctions. These tests enhance the robustness of the contract by ensuring that it correctly reverts when handling such edge cases.
| function refund(bytes calldata request) external { | ||
| request.validateV2(); | ||
| bytes32 transactionId = keccak256(request); | ||
|
|
||
| BridgeTransactionV2 memory transaction = getBridgeTransactionV2(request); | ||
|
|
||
| if (bridgeTxDetails[transactionId].status != BridgeStatus.REQUESTED) revert StatusIncorrect(); | ||
|
|
||
| if (hasRole(REFUNDER_ROLE, msg.sender)) { | ||
| // Refunder can refund if deadline has passed | ||
| if (block.timestamp <= transaction.deadline) revert DeadlineNotExceeded(); | ||
| } else { | ||
| // Permissionless refund is allowed after REFUND_DELAY | ||
| if (block.timestamp <= transaction.deadline + REFUND_DELAY) revert DeadlineNotExceeded(); | ||
| } | ||
|
|
||
| uint256 deadline = request.deadline(); | ||
| // Permissionless refund is allowed after REFUND_DELAY | ||
| if (!hasRole(REFUNDER_ROLE, msg.sender)) deadline += REFUND_DELAY; | ||
| if (block.timestamp <= deadline) revert DeadlineNotExceeded(); | ||
| // if all checks passed, set to REFUNDED status | ||
| bridgeTxDetails[transactionId].status = BridgeStatus.REFUNDED; | ||
|
|
||
| // transfer origin collateral back to original sender | ||
| address to = transaction.originSender; | ||
| address token = transaction.originToken; | ||
| uint256 amount = transaction.originAmount + transaction.originFeeAmount; | ||
| address to = request.originSender(); | ||
| address token = request.originToken(); | ||
| uint256 amount = request.originAmount() + request.originFeeAmount(); | ||
| token.universalTransfer(to, amount); | ||
|
|
There was a problem hiding this comment.
Potential Reentrancy Vulnerability in refund Function
There's a potential reentrancy issue in the refund function due to the external call token.universalTransfer(to, amount) at line 117. While the state update occurs before the external call, emitting the event BridgeDepositRefunded after the external call could pose risks. It's best practice to emit events before external calls to ensure accurate state representation, even if a reentrancy attack occurs.
Apply this diff to move the event emission before the external call:
bridgeTxDetails[transactionId].status = BridgeStatus.REFUNDED;
+emit BridgeDepositRefunded(transactionId, to, token, amount);
// transfer origin collateral back to original sender
address to = request.originSender();
address token = request.originToken();
uint256 amount = request.originAmount() + request.originFeeAmount();
token.universalTransfer(to, amount);📝 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 refund(bytes calldata request) external { | |
| request.validateV2(); | |
| bytes32 transactionId = keccak256(request); | |
| BridgeTransactionV2 memory transaction = getBridgeTransactionV2(request); | |
| if (bridgeTxDetails[transactionId].status != BridgeStatus.REQUESTED) revert StatusIncorrect(); | |
| if (hasRole(REFUNDER_ROLE, msg.sender)) { | |
| // Refunder can refund if deadline has passed | |
| if (block.timestamp <= transaction.deadline) revert DeadlineNotExceeded(); | |
| } else { | |
| // Permissionless refund is allowed after REFUND_DELAY | |
| if (block.timestamp <= transaction.deadline + REFUND_DELAY) revert DeadlineNotExceeded(); | |
| } | |
| uint256 deadline = request.deadline(); | |
| // Permissionless refund is allowed after REFUND_DELAY | |
| if (!hasRole(REFUNDER_ROLE, msg.sender)) deadline += REFUND_DELAY; | |
| if (block.timestamp <= deadline) revert DeadlineNotExceeded(); | |
| // if all checks passed, set to REFUNDED status | |
| bridgeTxDetails[transactionId].status = BridgeStatus.REFUNDED; | |
| // transfer origin collateral back to original sender | |
| address to = transaction.originSender; | |
| address token = transaction.originToken; | |
| uint256 amount = transaction.originAmount + transaction.originFeeAmount; | |
| address to = request.originSender(); | |
| address token = request.originToken(); | |
| uint256 amount = request.originAmount() + request.originFeeAmount(); | |
| token.universalTransfer(to, amount); | |
| function refund(bytes calldata request) external { | |
| request.validateV2(); | |
| bytes32 transactionId = keccak256(request); | |
| if (bridgeTxDetails[transactionId].status != BridgeStatus.REQUESTED) revert StatusIncorrect(); | |
| uint256 deadline = request.deadline(); | |
| // Permissionless refund is allowed after REFUND_DELAY | |
| if (!hasRole(REFUNDER_ROLE, msg.sender)) deadline += REFUND_DELAY; | |
| if (block.timestamp <= deadline) revert DeadlineNotExceeded(); | |
| // if all checks passed, set to REFUNDED status | |
| bridgeTxDetails[transactionId].status = BridgeStatus.REFUNDED; | |
| emit BridgeDepositRefunded(transactionId, to, token, amount); | |
| // transfer origin collateral back to original sender | |
| address to = request.originSender(); | |
| address token = request.originToken(); | |
| uint256 amount = request.originAmount() + request.originFeeAmount(); | |
| token.universalTransfer(to, amount); |
🧰 Tools
🪛 GitHub Check: Slither
[notice] 100-120: Reentrancy vulnerabilities
Reentrancy in FastBridgeV2.refund(bytes) (contracts/FastBridgeV2.sol#100-120):
External calls:
- token.universalTransfer(to,amount) (contracts/FastBridgeV2.sol#117)
Event emitted after the call(s):
- BridgeDepositRefunded(transactionId,to,token,amount) (contracts/FastBridgeV2.sol#119)
[notice] 100-120: Block timestamp
FastBridgeV2.refund(bytes) (contracts/FastBridgeV2.sol#100-120) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp <= deadline (contracts/FastBridgeV2.sol#109)
There was a problem hiding this comment.
This is a valid, yet niche find. The PR is question doesn't change the way refund (or any other state-chanigng functions) works as far as the contract is concerned, so this is out of scope of the particular PR. This is now tracked as SLT-342 in the internal ticket system.
There was a problem hiding this comment.
@ChiTimesChi, understood. Thank you for tracking this issue in SLT-342.
(_/)
( •_•)
(>🕳 )
✏️ Learnings added
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-10-14T14:45:57.032Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
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.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (14 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.Base.t.sol (3 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (3 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/contracts-rfq/contracts/FastBridgeV2.sol
- packages/contracts-rfq/test/FastBridgeV2.Dst.Base.t.sol
🧰 Additional context used
📓 Learnings (3)
📓 Common learnings
Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118 Timestamp: 2024-10-14T14:45:57.214Z Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (5)
Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.Src.t.sol:919-993 Timestamp: 2024-10-14T14:48:01.520Z Learning: In Solidity test files for FastBridgeV2 (e.g., `packages/contracts-rfq/test/FastBridgeV2.Src.t.sol`), code duplication in test functions is acceptable to maintain readability and maintainability, even if it contradicts DRY principles.Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol:97-97 Timestamp: 2024-10-14T13:23:04.509Z Learning: In `packages/contracts-rfq/test/FastBridgeV2.t.sol`, when testing expected revert behaviors, `abi.encodeWithSelector` is used with custom errors and should not be replaced with `BridgeTransactionV2Lib.encodeV2`.Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56 Timestamp: 2024-10-14T13:24:45.293Z Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118 Timestamp: 2024-10-14T14:45:57.214Z Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56 Timestamp: 2024-10-14T13:24:45.293Z Learning: In Solidity test contracts under the `test` directory, such as `FastBridgeV2.t.sol`, contracts are not deployed, so gas consumption is not a concern.packages/contracts-rfq/test/FastBridgeV2.t.sol (4)
Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56 Timestamp: 2024-10-14T13:24:45.293Z Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56 Timestamp: 2024-10-14T13:24:45.293Z Learning: In Solidity test contracts under the `test` directory, such as `FastBridgeV2.t.sol`, contracts are not deployed, so gas consumption is not a concern.Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.Src.t.sol:919-993 Timestamp: 2024-10-14T14:48:01.520Z Learning: In Solidity test files for FastBridgeV2 (e.g., `packages/contracts-rfq/test/FastBridgeV2.Src.t.sol`), code duplication in test functions is acceptable to maintain readability and maintainability, even if it contradicts DRY principles.Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol:97-97 Timestamp: 2024-10-14T13:23:04.509Z Learning: In `packages/contracts-rfq/test/FastBridgeV2.t.sol`, when testing expected revert behaviors, `abi.encodeWithSelector` is used with custom errors and should not be replaced with `BridgeTransactionV2Lib.encodeV2`.
🔇 Additional comments (9)
packages/contracts-rfq/test/FastBridgeV2.t.sol (6)
4-5: ImportingBridgeTransactionV2Libfor Transaction EncodingThe import statement correctly includes the
BridgeTransactionV2Liblibrary for bridge transaction encoding and decoding, aligning with the updates to the transaction structures.
49-52: Declaration of Internal Byte Arrays for Mock RequestsThe internal byte arrays
mockRequestV1,invalidRequestV2, andmockRequestV3are appropriately declared for use in test scenarios involving different versions of bridge transactions.
79-84: Initialize Mock Requests Appropriately insetUpFunctionThe initialization of
mockRequestV1,invalidRequestV2, andmockRequestV3in thesetUpfunction is correctly timed. ForminginvalidRequestV2beforecreateFixturesV2()andmockRequestV3after ensures that they use the intended parameters for testing.
267-267: UtilizeencodeV2for Consistent Transaction HashingUpdating the
getTxIdfunction to useBridgeTransactionV2Lib.encodeV2(bridgeTx)ensures that the transaction ID is computed using the standardized V2 encoding, enhancing consistency across the bridge transaction processes.
274-276: AddexpectRevertInvalidEncodedTxfor Specific Revert TestingThe
expectRevertInvalidEncodedTxfunction correctly sets up an expectation for reverts due to invalid encoded transactions by referencing the appropriate selector fromBridgeTransactionV2Lib.
278-282: ImplementexpectRevertUnsupportedVersionfor Version Handling TestsThe
expectRevertUnsupportedVersionfunction is properly designed to expect reverts when an unsupported version is encountered, utilizingabi.encodeWithSelectorto pass the version parameter.packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (3)
4-5: ImportingBridgeTransactionV2Libenhances transaction encoding capabilitiesThe addition of the
BridgeTransactionV2Libimport allows the use ofencodeV2, which improves the encoding and decoding processes for bridge transactions. This aligns with the updates made to theFastBridgeV2contract for better efficiency and maintainability.
41-41: UtilizingencodeV2for request encoding ensures version consistencyBy updating the
requestfield to useBridgeTransactionV2Lib.encodeV2(bridgeTx), the code now correctly encodes the bridge transaction according to the new versioning system. This change enhances consistency and prepares the contract for future versions.
918-992: Comprehensive tests added for invalid payload scenariosThe newly added test functions effectively cover cases of unsupported versions and invalid encoded transactions for the
prove,claim, andrefundmethods. These tests ensure that the contract correctly reverts when handling invalid inputs, enhancing the robustness and reliability of the system.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol (3)
1-7: LGTM! Consider adding a brief library description.The library declaration, license, and pragma statements are correct. The import statement is appropriately used.
Consider adding a brief comment above the library declaration to explain its purpose and how it relates to BridgeTransactionV2. This would enhance code readability and maintainability.
59-87: LGTM! Well-implemented encoding function.The
encodeV2function effectively encodes the BridgeTransactionV2 struct by tightly packing the fields. The splitting of encoding into two parts is a good practice to avoid stack-too-deep errors.Consider adding a comment about the return value type, such as:
/// @return The tightly packed encoded transaction as bytes.This would provide clarity on the function's output.
1-238: LGTM! Well-implemented and gas-efficient library.The BridgeTransactionV2Lib is well-structured, gas-efficient, and follows good Solidity development practices. The use of tight packing and inline assembly is appropriate for the use case.
Consider adding natspec comments for all public functions to improve documentation. This would include adding
@paramand@returndescriptions where applicable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (15 hunks)
- packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol (1 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118 Timestamp: 2024-10-14T14:45:57.214Z Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118 Timestamp: 2024-10-14T14:45:57.214Z Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
🪛 GitHub Check: Slither
packages/contracts-rfq/contracts/FastBridgeV2.sol
[notice] 102-126: Reentrancy vulnerabilities
Reentrancy in FastBridgeV2.refund(bytes) (contracts/FastBridgeV2.sol#102-126):
External calls:
- Address.sendValue(address(to),amount) (contracts/FastBridgeV2.sol#120)
- IERC20(token).safeTransfer(to,amount) (contracts/FastBridgeV2.sol#122)
Event emitted after the call(s):
- BridgeDepositRefunded(transactionId,to,token,amount) (contracts/FastBridgeV2.sol#125)
[notice] 102-126: Block timestamp
FastBridgeV2.refund(bytes) (contracts/FastBridgeV2.sol#102-126) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp <= deadline (contracts/FastBridgeV2.sol#111)
[notice] 293-326: Reentrancy vulnerabilities
Reentrancy in FastBridgeV2.claim(bytes,address) (contracts/FastBridgeV2.sol#293-326):
External calls:
- Address.sendValue(address(to),amount) (contracts/FastBridgeV2.sol#320)
- IERC20(token).safeTransfer(to,amount) (contracts/FastBridgeV2.sol#322)
Event emitted after the call(s):
- BridgeDepositClaimed(transactionId,bridgeTxDetails[transactionId].proofRelayer,to,token,amount) (contracts/FastBridgeV2.sol#325)
[notice] 293-326: Block timestamp
FastBridgeV2.claim(bytes,address) (contracts/FastBridgeV2.sol#293-326) uses timestamp for comparisons
Dangerous comparisons:
- _timeSince(bridgeTxDetails[transactionId].proofBlockTimestamp) <= DISPUTE_PERIOD (contracts/FastBridgeV2.sol#306)
[notice] 430-442: Block timestamp
FastBridgeV2._validateRelayParams(bytes,bytes32,address) (contracts/FastBridgeV2.sol#430-442) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > request.deadline() (contracts/FastBridgeV2.sol#436)
- exclRelayer != address(0) && exclRelayer != relayer && block.timestamp <= request.exclusivityEndTime() (contracts/FastBridgeV2.sol#439)
🔇 Additional comments (10)
packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol (6)
8-44: LGTM! Well-structured constant definitions.The constant definitions for VERSION and field offsets are well-organized and clearly commented. The use of tight packing for struct fields is an excellent optimization for gas efficiency.
46-47: LGTM! Well-defined custom errors.The custom error definitions are appropriate and follow good naming conventions. They provide clear and gas-efficient error handling for invalid transactions and unsupported versions.
49-57: LGTM! Robust validation function.The
validateV2function effectively checks the minimum length and version of the encoded transaction. It uses custom errors appropriately and ensures all static fields are present before accessing the version.
89-112: LGTM! Well-structured decoding function.The
decodeV2function effectively decodes the BridgeTransactionV2 struct from the encoded transaction. The use of individual extraction functions for each field promotes code modularity and reusability.
114-120: LGTM! Efficient version extraction.The
versionfunction efficiently extracts the version from the encoded transaction using inline assembly. This approach is appropriate for the required bit manipulation.Note: This approval applies to all similar extraction functions in the library (lines 114-232), as they follow the same pattern of efficient data extraction using inline assembly.
234-237: LGTM! Simple and effective callParams extraction.The
callParamsfunction efficiently extracts the call params from the encoded transaction using a simple slice operation. This approach is appropriate and more readable than using inline assembly for this particular case.packages/contracts-rfq/contracts/FastBridgeV2.sol (4)
7-7: Good use ofBridgeTransactionV2Libfor modularity and maintainabilityThe import and utilization of
BridgeTransactionV2Libenhance code modularity by centralizing the encoding and decoding logic forBridgeTransactionV2. This improves maintainability and reduces code duplication.Also applies to: 18-18
66-66: Optimizing gas usage by changing parameters tocalldataUpdating function parameters from
bytes memorytobytes calldatain functions likerelay,prove,claim, andrefundenhances gas efficiency by avoiding unnecessary memory allocations. This optimization leads to cost savings during contract execution.Also applies to: 72-72, 78-78, 102-102, 140-140, 164-165, 225-225, 293-293
Line range hint
140-156: Ensuring backward compatibility with legacy transaction formatsThe implementation of a try-catch block in
getBridgeTransactionallows the contract to handle both V1 and V2 transaction formats gracefully. By attempting to decode withgetBridgeTransactionV2and falling back to V1 decoding if it fails, you ensure backward compatibility and a smooth transition for users relying on older transaction structures.
Line range hint
187-203: Consistent request encoding usingBridgeTransactionV2Lib.encodeV2Utilizing
BridgeTransactionV2Lib.encodeV2in thebridgefunction ensures consistent and reliable serialization of request data. This approach standardizes the encoding process, reducing potential errors and simplifying future maintenance.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
Line range hint
303-341: Potential Reentrancy Vulnerability inclaimFunctionSimilar to the
refundfunction, theclaimfunction could be vulnerable to reentrancy attacks because external calls toAddress.sendValueorIERC20(token).safeTransferare made before emitting theBridgeDepositClaimedevent. To adhere to the checks-effects-interactions pattern, consider emitting the event before the external calls.Apply this diff to move the event emission before the external calls:
// Note: this is a storage write $.status = BridgeStatus.RELAYER_CLAIMED; +emit BridgeDepositClaimed(transactionId, proofRelayer, to, token, amount); // update protocol fees if origin fee amount exists address token = request.originToken(); uint256 amount = request.originAmount(); uint256 originFeeAmount = request.originFeeAmount(); if (originFeeAmount > 0) protocolFees[token] += originFeeAmount; // transfer origin collateral to specified address (protocol fee was pre-deducted at deposit) if (token == UniversalTokenLib.ETH_ADDRESS) { Address.sendValue(payable(to), amount); } else { IERC20(token).safeTransfer(to, amount); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (15 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118 Timestamp: 2024-10-14T14:45:57.214Z Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
Learnt from: ChiTimesChi PR: synapsecns/sanguine#3284 File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118 Timestamp: 2024-10-14T14:45:57.214Z Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
🪛 GitHub Check: Slither
packages/contracts-rfq/contracts/FastBridgeV2.sol
[notice] 106-131: Reentrancy vulnerabilities
Reentrancy in FastBridgeV2.refund(bytes) (contracts/FastBridgeV2.sol#106-131):
External calls:
- Address.sendValue(address(to),amount) (contracts/FastBridgeV2.sol#125)
- IERC20(token).safeTransfer(to,amount) (contracts/FastBridgeV2.sol#127)
Event emitted after the call(s):
- BridgeDepositRefunded(transactionId,to,token,amount) (contracts/FastBridgeV2.sol#130)
[notice] 106-131: Block timestamp
FastBridgeV2.refund(bytes) (contracts/FastBridgeV2.sol#106-131) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp <= deadline (contracts/FastBridgeV2.sol#116)
[notice] 303-342: Reentrancy vulnerabilities
Reentrancy in FastBridgeV2.claim(bytes,address) (contracts/FastBridgeV2.sol#303-342):
External calls:
- Address.sendValue(address(to),amount) (contracts/FastBridgeV2.sol#336)
- IERC20(token).safeTransfer(to,amount) (contracts/FastBridgeV2.sol#338)
Event emitted after the call(s):
- BridgeDepositClaimed(transactionId,proofRelayer,to,token,amount) (contracts/FastBridgeV2.sol#341)
[notice] 303-342: Block timestamp
FastBridgeV2.claim(bytes,address) (contracts/FastBridgeV2.sol#303-342) uses timestamp for comparisons
Dangerous comparisons:
- _timeSince(proofBlockTimestamp) <= DISPUTE_PERIOD (contracts/FastBridgeV2.sol#322)
[notice] 448-460: Block timestamp
FastBridgeV2._validateRelayParams(bytes,bytes32,address) (contracts/FastBridgeV2.sol#448-460) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > request.deadline() (contracts/FastBridgeV2.sol#454)
- exclRelayer != address(0) && exclRelayer != relayer && block.timestamp <= request.exclusivityEndTime() (contracts/FastBridgeV2.sol#457)
🔇 Additional comments (2)
packages/contracts-rfq/contracts/FastBridgeV2.sol (2)
66-67: Optimizing Gas Usage by Switching tocalldataGreat work on updating the function parameters in
relay,prove, andclaimto usebytes calldatainstead ofbytes memory. This change enhances gas efficiency by reducing unnecessary memory allocations.Also applies to: 72-73, 78-79
Line range hint
194-210: UtilizingBridgeTransactionV2Lib.encodeV2for Consistent EncodingExcellent implementation of
BridgeTransactionV2Lib.encodeV2in thebridgefunction. This promotes consistency and reduces potential errors in encoding bridge transactions.
| function refund(bytes calldata request) external { | ||
| request.validateV2(); | ||
| bytes32 transactionId = keccak256(request); | ||
|
|
||
| BridgeTransactionV2 memory transaction = getBridgeTransactionV2(request); | ||
|
|
||
| BridgeTxDetails storage $ = bridgeTxDetails[transactionId]; | ||
|
|
||
| if ($.status != BridgeStatus.REQUESTED) revert StatusIncorrect(); | ||
|
|
||
| if (hasRole(REFUNDER_ROLE, msg.sender)) { | ||
| // Refunder can refund if deadline has passed | ||
| if (block.timestamp <= transaction.deadline) revert DeadlineNotExceeded(); | ||
| } else { | ||
| // Permissionless refund is allowed after REFUND_DELAY | ||
| if (block.timestamp <= transaction.deadline + REFUND_DELAY) revert DeadlineNotExceeded(); | ||
| } | ||
|
|
||
| uint256 deadline = request.deadline(); | ||
| // Permissionless refund is allowed after REFUND_DELAY | ||
| if (!hasRole(REFUNDER_ROLE, msg.sender)) deadline += REFUND_DELAY; | ||
| if (block.timestamp <= deadline) revert DeadlineNotExceeded(); | ||
| // Note: this is a storage write | ||
| $.status = BridgeStatus.REFUNDED; | ||
|
|
||
| // transfer origin collateral back to original sender | ||
| uint256 amount = transaction.originAmount + transaction.originFeeAmount; | ||
| address to = transaction.originSender; | ||
| address token = transaction.originToken; | ||
|
|
||
| address to = request.originSender(); | ||
| address token = request.originToken(); | ||
| uint256 amount = request.originAmount() + request.originFeeAmount(); | ||
| if (token == UniversalTokenLib.ETH_ADDRESS) { | ||
| Address.sendValue(payable(to), amount); | ||
| } else { | ||
| IERC20(token).safeTransfer(to, amount); | ||
| } | ||
|
|
||
| emit BridgeDepositRefunded(transactionId, transaction.originSender, transaction.originToken, amount); | ||
| emit BridgeDepositRefunded(transactionId, to, token, amount); |
There was a problem hiding this comment.
Potential Reentrancy Vulnerability in refund Function
The refund function may be susceptible to reentrancy attacks due to external calls to Address.sendValue or IERC20(token).safeTransfer before emitting the BridgeDepositRefunded event. To mitigate this risk, it's best practice to update the state and emit events before making external calls.
Apply this diff to move the event emission before the external calls:
// Note: this is a storage write
$.status = BridgeStatus.REFUNDED;
+emit BridgeDepositRefunded(transactionId, to, token, amount);
// transfer origin collateral back to original sender
address to = request.originSender();
address token = request.originToken();
uint256 amount = request.originAmount() + request.originFeeAmount();
if (token == UniversalTokenLib.ETH_ADDRESS) {
Address.sendValue(payable(to), amount);
} else {
IERC20(token).safeTransfer(to, amount);
}📝 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 refund(bytes calldata request) external { | |
| request.validateV2(); | |
| bytes32 transactionId = keccak256(request); | |
| BridgeTransactionV2 memory transaction = getBridgeTransactionV2(request); | |
| BridgeTxDetails storage $ = bridgeTxDetails[transactionId]; | |
| if ($.status != BridgeStatus.REQUESTED) revert StatusIncorrect(); | |
| if (hasRole(REFUNDER_ROLE, msg.sender)) { | |
| // Refunder can refund if deadline has passed | |
| if (block.timestamp <= transaction.deadline) revert DeadlineNotExceeded(); | |
| } else { | |
| // Permissionless refund is allowed after REFUND_DELAY | |
| if (block.timestamp <= transaction.deadline + REFUND_DELAY) revert DeadlineNotExceeded(); | |
| } | |
| uint256 deadline = request.deadline(); | |
| // Permissionless refund is allowed after REFUND_DELAY | |
| if (!hasRole(REFUNDER_ROLE, msg.sender)) deadline += REFUND_DELAY; | |
| if (block.timestamp <= deadline) revert DeadlineNotExceeded(); | |
| // Note: this is a storage write | |
| $.status = BridgeStatus.REFUNDED; | |
| // transfer origin collateral back to original sender | |
| uint256 amount = transaction.originAmount + transaction.originFeeAmount; | |
| address to = transaction.originSender; | |
| address token = transaction.originToken; | |
| address to = request.originSender(); | |
| address token = request.originToken(); | |
| uint256 amount = request.originAmount() + request.originFeeAmount(); | |
| if (token == UniversalTokenLib.ETH_ADDRESS) { | |
| Address.sendValue(payable(to), amount); | |
| } else { | |
| IERC20(token).safeTransfer(to, amount); | |
| } | |
| emit BridgeDepositRefunded(transactionId, transaction.originSender, transaction.originToken, amount); | |
| emit BridgeDepositRefunded(transactionId, to, token, amount); | |
| function refund(bytes calldata request) external { | |
| request.validateV2(); | |
| bytes32 transactionId = keccak256(request); | |
| BridgeTxDetails storage $ = bridgeTxDetails[transactionId]; | |
| if ($.status != BridgeStatus.REQUESTED) revert StatusIncorrect(); | |
| uint256 deadline = request.deadline(); | |
| // Permissionless refund is allowed after REFUND_DELAY | |
| if (!hasRole(REFUNDER_ROLE, msg.sender)) deadline += REFUND_DELAY; | |
| if (block.timestamp <= deadline) revert DeadlineNotExceeded(); | |
| // Note: this is a storage write | |
| $.status = BridgeStatus.REFUNDED; | |
| emit BridgeDepositRefunded(transactionId, to, token, amount); | |
| // transfer origin collateral back to original sender | |
| address to = request.originSender(); | |
| address token = request.originToken(); | |
| uint256 amount = request.originAmount() + request.originFeeAmount(); | |
| if (token == UniversalTokenLib.ETH_ADDRESS) { | |
| Address.sendValue(payable(to), amount); | |
| } else { | |
| IERC20(token).safeTransfer(to, amount); | |
| } |
🧰 Tools
🪛 GitHub Check: Slither
[notice] 106-131: Reentrancy vulnerabilities
Reentrancy in FastBridgeV2.refund(bytes) (contracts/FastBridgeV2.sol#106-131):
External calls:
- Address.sendValue(address(to),amount) (contracts/FastBridgeV2.sol#125)
- IERC20(token).safeTransfer(to,amount) (contracts/FastBridgeV2.sol#127)
Event emitted after the call(s):
- BridgeDepositRefunded(transactionId,to,token,amount) (contracts/FastBridgeV2.sol#130)
[notice] 106-131: Block timestamp
FastBridgeV2.refund(bytes) (contracts/FastBridgeV2.sol#106-131) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp <= deadline (contracts/FastBridgeV2.sol#116)
Description
BridgeTransactionversion is added into the encoded payload, allowing to do small changes for whatever is coming inBridgeTransactionV3.BridgeTransactionstruct is now tightly packed for the encoding. This makes the decoding a bit more efficient, as well as decreases the encoded transaction length. Both of these result in gas savings, making bothrelayandclaim~3k gas cheaper on average.Summary by CodeRabbit
Summary by CodeRabbit
New Features
BridgeTransactionV2Liblibrary for efficient encoding and decoding of bridge transactions.getBridgeTransactionV2for enhanced transaction retrieval.Bug Fixes
Tests