-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(fix): adds calldata extraction handling for GenericSwapV3 single swap calls (CallDataVerificationFacet v1.2.0) [CalldataVerificationFacet v1.2.0,IAcrossSpokePool v1.0.0] #767
Conversation
WalkthroughThe changes introduce new audit entries and updates to the audited contracts section in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (3)
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
CodeRabbit Configuration File (
|
…aVerificationFacet
…ontracts into fixCallDataVerificationFacet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/Facets/AcrossFacetV3.sol (1 hunks)
- src/Facets/CalldataVerificationFacet.sol (12 hunks)
- src/Interfaces/IAcrossSpokePool.sol (2 hunks)
- test/solidity/Facets/CalldataVerificationFacet.t.sol (6 hunks)
Additional comments not posted (23)
src/Interfaces/IAcrossSpokePool.sol (2)
2-2
: LGTM!The Solidity version pragma update to
^0.8.17
ensures compatibility with newer features and optimizations.
16-29
: LGTM!The addition of the
depositV3
function expands the functionality of the existingdeposit
method by introducing multiple parameters that enhance its capabilities.src/Facets/AcrossFacetV3.sol (6)
4-18
: LGTM!The imports and contract declaration are necessary for the functionality of the
AcrossFacetV3
contract.
21-25
: LGTM!The immutable storage variables
spokePool
andwrappedNative
are necessary for the functionality of the contract.
29-44
: LGTM!The
AcrossV3Data
struct is necessary for encapsulating the data specific to Across V3.
48-54
: LGTM!The constructor correctly initializes the immutable storage variables
spokePool
andwrappedNative
.
58-102
: LGTM!The external methods
startBridgeTokensViaAcrossV3
andswapAndStartBridgeTokensViaAcrossV3
are necessary for bridging tokens via Across and performing swaps before bridging. They include appropriate modifiers for security and validation.
106-164
: LGTM!The
_startBridge
internal method contains the business logic for the bridge via Across and includes appropriate validation and checks.src/Facets/CalldataVerificationFacet.sol (6)
7-8
: LGTM!The new imports for
AcrossFacetV3
andStargateFacetV2
are necessary for the added functionality and integration with additional bridging protocols.Also applies to: 13-14
73-73
: LGTM!Changing the visibility of the
extractMainParameters
function frompublic
toexternal
can improve gas efficiency when the function is called externally.
107-125
: LGTM!The modifications to the
extractNonEVMAddress
function streamline the extraction process and simplify the code, reducing redundancy.
Line range hint
229-247
: LGTM!The modifications to the
validateCalldata
function enhance clarity and reduce the number of lines of code, improving the efficiency of bridge checks.
408-436
: LGTM!The new functionality for validating calldata from
AcrossFacetV3
andStargateFacetV2
expands the contract's capabilities to handle more complex bridging scenarios.
489-505
: LGTM!The helper functions
_compareBytesToBytes32CallTo
and_bytesToAddress
provide utility for comparing addresses and enhance the contract's robustness.test/solidity/Facets/CalldataVerificationFacet.t.sol (9)
8-11
: LGTM!The new imports are necessary for the new test cases and functionalities introduced in the file.
Also applies to: 16-16, 20-20, 23-25
Line range hint
34-67
: LGTM!The changes to the
setUp
function are necessary to initialize the test environment correctly.
305-332
: LGTM!The test case is well-structured and ensures that the
extractGenericSwapParameters
method correctly reverts on invalid calldata.
334-370
: LGTM!The test case is well-structured and ensures that the
extractGenericSwapParameters
method correctly extracts parameters from minimum valid calldata.
372-423
: LGTM!The test case is well-structured and ensures that the
extractGenericSwapParameters
method correctly extracts parameters from single swap calldata.
749-841
: LGTM!The test case is well-structured and ensures that the
validateDestinationCalldata
method correctly validates StargateV2 destination calldata.
922-1001
: LGTM!The test case is well-structured and ensures that the
validateDestinationCalldata
method correctly validates AcrossV3 destination calldata.
1003-1026
: LGTM!The test case is well-structured and ensures that the
validateDestinationCalldata
method correctly reverts on invalid selector.
1042-1050
: LGTM!The library is simple and provides useful utility functions for creating calldata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (2)
src/Facets/CalldataVerificationFacet.sol (2)
Line range hint
107-135
: ImprovedextractNonEVMAddress
functionThe refactoring of this function enhances its efficiency and flexibility. The addition of the check for
StandardizedCallFacet.standardizedCall
allows for better handling of different call types.However, to improve readability, consider extracting the assembly blocks into separate private functions.
Consider refactoring the assembly blocks into separate private functions:
function extractNonEVMAddress( bytes calldata data ) external pure returns (bytes32 nonEVMAddress) { bytes memory callData; if ( bytes4(data[:4]) == StandardizedCallFacet.standardizedCall.selector ) { // standardizedCall callData = abi.decode(data[4:], (bytes)); } else { callData = data; } // Non-EVM address is always the first parameter of bridge specific data if (_extractBridgeData(data).hasSourceSwaps) { - assembly { - let offset := mload(add(callData, 0x64)) // Get the offset of the bridge specific data - nonEVMAddress := mload(add(callData, add(offset, 0x24))) // Get the non-EVM address - } + nonEVMAddress = _extractNonEVMAddressWithSwaps(callData); } else { - assembly { - let offset := mload(add(callData, 0x44)) // Get the offset of the bridge specific data - nonEVMAddress := mload(add(callData, add(offset, 0x24))) // Get the non-EVM address - } + nonEVMAddress = _extractNonEVMAddressWithoutSwaps(callData); } } +function _extractNonEVMAddressWithSwaps(bytes memory callData) private pure returns (bytes32 nonEVMAddress) { + assembly { + let offset := mload(add(callData, 0x64)) // Get the offset of the bridge specific data + nonEVMAddress := mload(add(callData, add(offset, 0x24))) // Get the non-EVM address + } +} + +function _extractNonEVMAddressWithoutSwaps(bytes memory callData) private pure returns (bytes32 nonEVMAddress) { + assembly { + let offset := mload(add(callData, 0x44)) // Get the offset of the bridge specific data + nonEVMAddress := mload(add(callData, add(offset, 0x24))) // Get the non-EVM address + } +}
Line range hint
338-436
: ExtendedvalidateDestinationCalldata
with new protocolsThe function has been expanded to include validation for StargateV2 and AcrossV3 protocols, enhancing the contract's interoperability. The use of
_compareBytesToBytes32CallTo
for some comparisons is a good step towards standardizing the comparison logic.However, the function is becoming quite long and repetitive. Consider refactoring to improve maintainability:
- Extract the validation logic for each protocol into separate private functions.
- Use a mapping of function selectors to validation functions to reduce the number of if-else statements.
Here's a high-level example of how you might refactor this:
mapping(bytes4 => function(bytes memory, bytes calldata, bytes calldata) returns (bool)) private validationFunctions; constructor() { validationFunctions[AmarokFacet.startBridgeTokensViaAmarok.selector] = _validateAmarok; validationFunctions[StargateFacetV2.startBridgeTokensViaStargate.selector] = _validateStargateV2; // ... add other mappings ... } function validateDestinationCalldata( bytes calldata data, bytes calldata callTo, bytes calldata dstCalldata ) external pure returns (bool isValid) { bytes memory callData = _extractCallData(data); bytes4 selector = abi.decode(callData, (bytes4)); if (validationFunctions[selector] != address(0)) { return validationFunctions[selector](callData, callTo, dstCalldata); } return false; } function _validateAmarok(bytes memory callData, bytes calldata callTo, bytes calldata dstCalldata) private pure returns (bool) { // ... Amarok validation logic ... } function _validateStargateV2(bytes memory callData, bytes calldata callTo, bytes calldata dstCalldata) private pure returns (bool) { // ... StargateV2 validation logic ... } // ... other validation functions ...This refactoring would make the function more modular, easier to maintain, and simpler to extend for future protocols.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/Facets/CalldataVerificationFacet.sol (12 hunks)
Additional comments not posted (3)
src/Facets/CalldataVerificationFacet.sol (3)
73-73
: Improved gas efficiency forextractMainParameters
Changing the function visibility from
public
toexternal
is a good optimization. This change can lead to gas savings when the function is called from outside the contract, asexternal
functions can use calldata directly.
240-247
: Improved efficiency invalidateCalldata
The refactoring of this function brings several improvements:
- Using
keccak256
for comparing bridge names is more gas-efficient than direct string comparison.- The logic has been simplified, improving readability and potentially reducing gas costs.
- The use of
bridgeNameHash
variable improves code clarity.These changes enhance both the performance and maintainability of the function.
7-8
: New bridging protocols integrated and version updatedThe addition of imports for
AcrossFacetV3
andStargateFacetV2
, along with the version update to 1.2.0, indicates significant enhancements to the contract's functionality. These changes expand the contract's capabilities to handle more complex bridging scenarios.To ensure these new imports are utilized correctly, please run the following script:
Also applies to: 13-14, 19-19
Verification successful
Imports Utilized in Contract Logic
The imports for
AcrossFacetV3
andStargateFacetV2
are actively used within theCalldataVerificationFacet
contract. Their integration involves function selectors and data structures, confirming the enhancement of the contract's capabilities to handle complex bridging scenarios.
- Usage of
AcrossFacetV3
andStargateFacetV2
confirmed insrc/Facets/CalldataVerificationFacet.sol
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new imports in the contract # Test: Check for the usage of AcrossFacetV3 and StargateFacetV2 rg --type solidity -e "AcrossFacetV3" -e "StargateFacetV2" src/Facets/CalldataVerificationFacet.solLength of output: 130
Script:
#!/bin/bash # Description: Verify the usage of new imports in the contract # Test: Check for the usage of AcrossFacetV3 and StargateFacetV2 rg -e "AcrossFacetV3" -e "StargateFacetV2" src/Facets/CalldataVerificationFacet.solLength of output: 1028
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/Facets/CalldataVerificationFacet.sol (12 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/Facets/CalldataVerificationFacet.sol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
audit/reports/2024.09.02_CalldataVerificationFacet.pdf
is excluded by!**/*.pdf
Files selected for processing (1)
- audit/auditLog.json (1 hunks)
Additional comments not posted (3)
audit/auditLog.json (3)
19-21
: Changes to auditedContracts section are correct and align with PR objectives.The addition of CalldataVerificationFacet version 1.2.0 associated with audit20240902 is correct and consistent with the PR objectives. The existing entry for StargateFacetV2 is properly maintained.
Line range hint
1-26
: Overall changes to auditLog.json are appropriate and well-structured.The modifications to
auditLog.json
accurately reflect the addition of a new audit for the CalldataVerificationFacet v1.2.0. These changes align well with the PR objectives of implementing calldata extraction handling for GenericSwapV3 single swap calls. The file structure remains consistent, and the new information is properly integrated.
10-15
: New audit entry looks good and aligns with PR objectives.The new audit entry for 02.09.2024 is correctly formatted and includes all necessary information. It's consistent with the PR objectives of implementing calldata extraction for GenericSwapV3 single swap calls in the CallDataVerificationFacet v1.2.0.
To ensure the audit report file exists and the commit hash is valid, please run the following script:
Verification successful
Audit entry verification successful.
The audit entry for "audit20240902" is correct and consistent with the codebase. The audit report file exists, and the commit hash is valid.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of the audit report and the validity of the commit hash # Check if the audit report file exists if [ -f "./audit/reports/2024.09.02_CalldataVerificationFacet.pdf" ]; then echo "Audit report file exists." else echo "Error: Audit report file not found." exit 1 fi # Verify the commit hash if git rev-parse --quiet --verify 374d066a2d4ffd98eb6042cac7a2dd4aa0bf2ff8^{commit}; then echo "Commit hash is valid." else echo "Error: Invalid commit hash." exit 1 fiLength of output: 309
…aVerificationFacet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- audit/auditLog.json (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
audit/auditLog.json (2)
10-16
: LGTM: New audit entry is correctly formatted and consistent.The new audit entry for "audit20240902" is well-structured and contains all the necessary information. It maintains consistency with previous entries in terms of format and auditor details.
Line range hint
1-45
: Summary: Audit log updates align with PR objectivesThe changes to
auditLog.json
are consistent with the PR objectives and the AI-generated summary. The addition of a new audit entry for the CalldataVerificationFacet (version 1.2.0) aligns with the PR's focus on implementing calldata extraction for GenericSwapV3.These updates demonstrate adherence to the security practices outlined in the PR description, specifically the requirement for new contracts to undergo at least one preliminary audit.
Test Coverage ReportLine Coverage: 78.66% (2238 / 2845 lines) |
Which Jira task belongs to this PR?
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)