Update CalldataVerificationFacet [CalldataVerificationFacet v1.3.0] #1003
Conversation
WalkthroughThe pull request updates the version number and refactors the calldata handling logic in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (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 (
|
lifi-action-bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/Facets/CalldataVerificationFacet.sol (1)
323-381:⚠️ Potential issueRemove duplicate AcrossV3 case handling.
The AcrossV3 case handling is duplicated in the
validateDestinationCalldatafunction. The code block from lines 353-381 is identical to the block from lines 323-350. Remove the duplicate block to maintain clean code and prevent potential maintenance issues.// Case: AcrossV3 if (selector == AcrossFacetV3.startBridgeTokensViaAcrossV3.selector) { (, AcrossFacetV3.AcrossV3Data memory acrossV3Data) = abi.decode( callData.slice(4, callData.length - 4), (ILiFi.BridgeData, AcrossFacetV3.AcrossV3Data) ); return keccak256(dstCalldata) == keccak256(acrossV3Data.message) && keccak256(callTo) == keccak256(abi.encode(acrossV3Data.receiverAddress)); } if ( selector == AcrossFacetV3.swapAndStartBridgeTokensViaAcrossV3.selector ) { (, , AcrossFacetV3.AcrossV3Data memory acrossV3Data) = abi.decode( callData.slice(4, callData.length - 4), ( ILiFi.BridgeData, LibSwap.SwapData[], AcrossFacetV3.AcrossV3Data ) ); return keccak256(dstCalldata) == keccak256(acrossV3Data.message) && keccak256(callTo) == keccak256(abi.encode(acrossV3Data.receiverAddress)); } - // --------------------------------------- - // Case: AcrossV3 - if (selector == AcrossFacetV3.startBridgeTokensViaAcrossV3.selector) { - (, AcrossFacetV3.AcrossV3Data memory acrossV3Data) = abi.decode( - callData.slice(4, callData.length - 4), - (ILiFi.BridgeData, AcrossFacetV3.AcrossV3Data) - ); - - return - keccak256(dstCalldata) == keccak256(acrossV3Data.message) && - keccak256(callTo) == - keccak256(abi.encode(acrossV3Data.receiverAddress)); - } - if ( - selector == - AcrossFacetV3.swapAndStartBridgeTokensViaAcrossV3.selector - ) { - (, , AcrossFacetV3.AcrossV3Data memory acrossV3Data) = abi.decode( - callData.slice(4, callData.length - 4), - ( - ILiFi.BridgeData, - LibSwap.SwapData[], - AcrossFacetV3.AcrossV3Data - ) - ); - return - keccak256(dstCalldata) == keccak256(acrossV3Data.message) && - keccak256(callTo) == - keccak256(abi.encode(acrossV3Data.receiverAddress)); - }
🧹 Nitpick comments (1)
src/Facets/CalldataVerificationFacet.sol (1)
393-396: Consider adding input validation for data length.The internal functions
_extractBridgeDataand_extractSwapDataperform abi.decode operations without validating the minimum required length of the input data. While these functions are internal and the contract includes some validation in public functions, it would be good practice to add explicit length checks.function _extractBridgeData( bytes calldata data ) internal pure returns (ILiFi.BridgeData memory bridgeData) { + require(data.length >= 4, "Invalid data length"); bridgeData = abi.decode(data[4:], (ILiFi.BridgeData)); } function _extractSwapData( bytes calldata data ) internal pure returns (LibSwap.SwapData[] memory swapData) { + require(data.length >= 4, "Invalid data length"); (, swapData) = abi.decode( data[4:], (ILiFi.BridgeData, LibSwap.SwapData[]) ); }Also applies to: 402-408
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Facets/CalldataVerificationFacet.sol(3 hunks)test/solidity/Facets/CalldataVerificationFacet.t.sol(0 hunks)
💤 Files with no reviewable changes (1)
- test/solidity/Facets/CalldataVerificationFacet.t.sol
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
- GitHub Check: generate-tag
🔇 Additional comments (3)
src/Facets/CalldataVerificationFacet.sol (3)
115-124: Consider adding bounds checking in assembly block.The assembly block in
extractNonEVMAddressperforms memory operations without explicit bounds checking. While the current implementation appears safe due to the structure of the input data, it's good practice to add explicit bounds checking to prevent potential vulnerabilities.if (_extractBridgeData(data).hasSourceSwaps) { assembly { + let dataLength := mload(callData) + let requiredLength := add(mload(add(callData, 0x64)), 0x44) + if lt(dataLength, requiredLength) { revert(0, 0) } let offset := mload(add(callData, 0x64)) nonEVMAddress := mload(add(callData, add(offset, 0x24))) } } else { assembly { + let dataLength := mload(callData) + let requiredLength := add(mload(add(callData, 0x44)), 0x44) + if lt(dataLength, requiredLength) { revert(0, 0) } let offset := mload(add(callData, 0x44)) nonEVMAddress := mload(add(callData, add(offset, 0x24))) } }
421-423: Add explicit size check before memory operations in assembly.In the
_compareBytesToBytes32CallTofunction, while there is a length check usingrequire, the assembly block should also include explicit size verification before performing memory operations.assembly { + // Ensure we have enough memory to read 32 bytes + if lt(mload(callTo), 32) { revert(0, 0) } callToAddress := mload(add(callTo, 32)) }
17-17: Version update looks good.The version has been updated from 1.2.0 to 1.3.0, which is appropriate given the removal of StandardizedCallFacet dependency and the simplification of calldata handling logic.
Test Coverage ReportLine Coverage: 80.06% (2241 / 2799 lines) |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
src/Facets/CalldataVerificationFacet.sol (3)
114-123: 🛠️ Refactor suggestionAdd safety checks for assembly operations.
The assembly block performs direct memory manipulation without bounds checking. Consider adding explicit length checks before accessing memory to prevent potential buffer overflows.
Add length validation before assembly operations:
function extractNonEVMAddress( bytes calldata data ) external pure returns (bytes32 nonEVMAddress) { bytes memory callData = data; + require(callData.length >= 0x84, "Insufficient calldata length"); // Non-EVM address is always the first parameter of bridge specific data if (_extractBridgeData(data).hasSourceSwaps) { assembly { let offset := mload(add(callData, 0x64)) + require(lt(offset, sub(mload(callData), 0x24)), "Invalid offset") nonEVMAddress := mload(add(callData, add(offset, 0x24))) } } else { assembly { let offset := mload(add(callData, 0x44)) + require(lt(offset, sub(mload(callData), 0x24)), "Invalid offset") nonEVMAddress := mload(add(callData, add(offset, 0x24))) } } }
351-380:⚠️ Potential issueRemove duplicate AcrossV3 validation block.
The AcrossV3 validation block is duplicated. The code block from lines 351-380 is identical to the one from lines 321-350.
Remove the duplicate code block:
- // --------------------------------------- - // Case: AcrossV3 - if (selector == AcrossFacetV3.startBridgeTokensViaAcrossV3.selector) { - (, AcrossFacetV3.AcrossV3Data memory acrossV3Data) = abi.decode( - callData.slice(4, callData.length - 4), - (ILiFi.BridgeData, AcrossFacetV3.AcrossV3Data) - ); - - return - keccak256(dstCalldata) == keccak256(acrossV3Data.message) && - keccak256(callTo) == - keccak256(abi.encode(acrossV3Data.receiverAddress)); - } - if ( - selector == - AcrossFacetV3.swapAndStartBridgeTokensViaAcrossV3.selector - ) { - (, , AcrossFacetV3.AcrossV3Data memory acrossV3Data) = abi.decode( - callData.slice(4, callData.length - 4), - ( - ILiFi.BridgeData, - LibSwap.SwapData[], - AcrossFacetV3.AcrossV3Data - ) - ); - return - keccak256(dstCalldata) == keccak256(acrossV3Data.message) && - keccak256(callTo) == - keccak256(abi.encode(acrossV3Data.receiverAddress)); - }
419-422: 🛠️ Refactor suggestionAdd bounds checking for assembly operations in _compareBytesToBytes32CallTo.
The assembly block performs direct memory access without proper bounds checking. This could lead to buffer overflows if the input is malformed.
Add explicit bounds checking:
address callToAddress; +require(callTo.length >= 32, "Invalid callTo length for address extraction"); assembly { callToAddress := mload(add(callTo, 32)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Facets/CalldataVerificationFacet.sol(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: generate-tag
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
…ce/contracts into update-CalldataVerificationFacet
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/Facets/CalldataVerificationFacet.sol (1)
351-378:⚠️ Potential issueRemove duplicate AcrossV3 validation block.
The code block for AcrossV3 validation is duplicated. Remove the second occurrence to maintain code cleanliness and prevent potential maintenance issues.
- // --------------------------------------- - // Case: AcrossV3 - if (selector == AcrossFacetV3.startBridgeTokensViaAcrossV3.selector) { - (, AcrossFacetV3.AcrossV3Data memory acrossV3Data) = abi.decode( - data[4:], - (ILiFi.BridgeData, AcrossFacetV3.AcrossV3Data) - ); - - return - keccak256(dstCalldata) == keccak256(acrossV3Data.message) && - keccak256(callTo) == - keccak256(abi.encode(acrossV3Data.receiverAddress)); - } - if ( - selector == - AcrossFacetV3.swapAndStartBridgeTokensViaAcrossV3.selector - ) { - (, , AcrossFacetV3.AcrossV3Data memory acrossV3Data) = abi.decode( - data[4:], - ( - ILiFi.BridgeData, - LibSwap.SwapData[], - AcrossFacetV3.AcrossV3Data - ) - ); - return - keccak256(dstCalldata) == keccak256(acrossV3Data.message) && - keccak256(callTo) == - keccak256(abi.encode(acrossV3Data.receiverAddress)); - }
🧹 Nitpick comments (2)
src/Facets/CalldataVerificationFacet.sol (2)
110-110: Simplify memory allocation.The
callDatavariable is only used in assembly blocks. Consider usingdatadirectly to avoid unnecessary memory allocation.- bytes memory callData = data; + bytes calldata callData = data;
407-428: Add NatSpec documentation for private helper function.The
_compareBytesToBytes32CallTofunction performs critical validation but lacks comprehensive documentation. Consider adding NatSpec comments to explain:
- The purpose of the 20-byte length requirement
- The memory layout assumptions
- The security implications of the comparison
+ /// @notice Compares a bytes array containing an address with a bytes32 value + /// @dev Requires the bytes array to be at least 20 bytes (address length) + /// @param callTo The bytes array containing the address + /// @param callToBytes32 The bytes32 value to compare against + /// @return bool True if the addresses match, false otherwise function _compareBytesToBytes32CallTo( bytes memory callTo, bytes32 callToBytes32 ) private pure returns (bool) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Facets/CalldataVerificationFacet.sol(12 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: 0xDEnYO
PR: lifinance/contracts#1003
File: src/Facets/CalldataVerificationFacet.sol:16-16
Timestamp: 2025-02-19T08:30:20.501Z
Learning: The project does not actively maintain a changelog, and suggestions about adding changelog entries should be avoided.
src/Facets/CalldataVerificationFacet.sol (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#1003
File: src/Facets/CalldataVerificationFacet.sol:16-16
Timestamp: 2025-02-19T08:30:20.501Z
Learning: The project does not actively maintain a changelog, and suggestions about adding changelog entries should be avoided.
🪛 GitHub Check: Olympix Integrated Security
src/Facets/CalldataVerificationFacet.sol
[notice] 249-249:
Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
- GitHub Check: generate-tag
🔇 Additional comments (2)
src/Facets/CalldataVerificationFacet.sol (2)
16-16: LGTM: Version update to 1.3.0.The version update reflects the removal of StandardizedCallFacet support.
249-249: LGTM: Safe downcast to bytes4.The downcast to
bytes4is safe here as it's used for function selector matching, which is always 4 bytes.🧰 Tools
🪛 GitHub Check: Olympix Integrated Security
[notice] 249-249:
Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast
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!!!)