Audit fixes [GasZipFacet v2.0.2] #910
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Facets/GasZipFacet.sol (1)
21-22: Consistent use ofMAX_CHAINID_LENGTH_ALLOWED.
Defining the same constant in bothGasZipFacetandGasZipPeripheryis beneficial if they operate independently, but consider extracting it into a shared library or a global constants file to avoid duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Facets/GasZipFacet.sol(3 hunks)src/Libraries/LibAsset.sol(1 hunks)src/Periphery/GasZipPeriphery.sol(2 hunks)src/Periphery/Permit2Proxy.sol(2 hunks)src/Periphery/ReceiverAcrossV3.sol(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Periphery/GasZipPeriphery.sol (3)
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:4-14
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In `GasZipPeriphery.sol`, `LibUtil` and `Validatable` are used, so ensure not to suggest their removal in future reviews.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:77-77
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In `GasZipPeriphery.sol`, prefer to avoid making more expensive function calls like calling the contract's own functions with `msg.value` after a swap, in order to save gas.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:57-62
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In the `GasZipPeriphery` contract, it's acceptable to let low-level calls like `liFiDEXAggregator.call` fail without explicit error handling, as failing the entire transaction is sufficient and saves gas.
🔇 Additional comments (9)
src/Periphery/GasZipPeriphery.sol (3)
17-17: Simplified inheritance may introduce reentrancy exposure.
By removing ReentrancyGuard, the contract is no longer protected against potential reentrancy attacks. If malicious contracts or untrusted external calls are possible via liFiDEXAggregator, consider reinstating reentrancy protections in high-risk functions.
23-23: Good practice: Unified constant for chain ID limitations.
Introducing MAX_CHAINID_LENGTH_ALLOWED enhances maintainability and readability and ensures consistency across the codebase.
105-105: Validate usage of new chain-length check.
The new check if (length > MAX_CHAINID_LENGTH_ALLOWED) is straightforward. Ensure that edge cases (e.g., exactly 32 chain IDs) are covered by your tests.
src/Periphery/ReceiverAcrossV3.sol (1)
108-108: Improved token approval logic.
Using safeApproveWithRetry helps circumvent non-zero allowance errors. This is generally efficient but verify that the executor contract does not rely on any leftover approval after execution.
src/Facets/GasZipFacet.sol (3)
27-27: Renamed constant clarifies non-EVM usage.
Changing NON_EVM_RECEIVER_IDENTIFIER to NON_EVM_ADDRESS is clearer. Confirm that references in tests and other contracts have been updated accordingly.
109-109: Receiver address validation for EVM vs. non-EVM flows.
The conditional ensures EVM addresses differ from the NON_EVM_ADDRESS placeholder. Double-check that bridging to non-EVM addresses is tested thoroughly, especially for receivers with unusual address formats.
135-135: Same chain ID length check.
The logic aligns with GasZipPeriphery. Ensure consistent unit testing covers boundary conditions (e.g., 1, 32, and more than 32 chain IDs).
src/Libraries/LibAsset.sol (1)
70-70: Streamlined ERC20 approval using forceApprove.
Replacing the two-step approval process with SafeERC20.forceApprove is more gas-efficient and concise. Just confirm the underlying token adheres to expected ERC20 behaviors (e.g., some tokens do not allow setting non-zero allowances without resetting to 0 first).
src/Periphery/Permit2Proxy.sol (1)
Line range hint 204-218: Variable rename improves clarity.
Renaming permit to tokenPermissionsHash in getPermit2MsgHash accurately conveys the variable’s role, reducing potential confusion. The updated usage is consistent and clear.
Test Coverage ReportLine Coverage: 78.74% (2200 / 2794 lines) |
lifi-action-bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
audit/reports/report-cantinacode-lifi-1216.pdfis excluded by!**/*.pdf
📒 Files selected for processing (1)
audit/auditLog.json(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: generate-tag
- GitHub Check: analyze
🔇 Additional comments (4)
audit/auditLog.json (4)
108-113: Note the change in auditorThe audit entry
audit20250110is performed by Cantina (security firm) while all other audits are by Sujith Somraaj. This diversity in auditors is a good practice for getting different perspectives on the codebase.
247-255: Verify RelayerCelerIM version jumpsThe RelayerCelerIM shows significant version jumps and parallel versions:
- v1.0.3 → v2.1.0 (major version jump)
- v2.1.1 added same day as v2.1.0
This could indicate breaking changes. Ensure proper migration documentation exists.
✅ Verification successful
Version jumps are audit-driven, not functionality-breaking
The version changes (1.0.3 → 2.1.0 → 2.1.1) are all part of the same audit cycle (audit20250109) and don't represent breaking changes. The core functionality of the RelayerCelerIM contract remains consistent across versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for migration guides or breaking change documentation fd -e md -e txt | xargs rg -l "migration|upgrade|breaking.?change" -i # Check commit messages for breaking changes git log --oneline -p -- "**/RelayerCelerIM.sol" | grep -B 5 -A 5 "BREAKING"Length of output: 155
Script:
#!/bin/bash # Find RelayerCelerIM implementation files fd RelayerCelerIM # Look for any docs or comments mentioning RelayerCelerIM versions rg -i "RelayerCelerIM.*(1\.0\.3|2\.1\.[01])" -A 5 -B 5 # Check if there are any version tags git tag | grep -i "relayer.*celer"Length of output: 262
Script:
#!/bin/bash # Check documentation content cat docs/RelayerCelerIM.md # Look for version-related comments or changes in implementation cat src/Periphery/RelayerCelerIM.sol # Check test file for version-specific tests cat test/solidity/Periphery/RelayerCelerIM.t.solLength of output: 32239
209-215: Verify Permit2Proxy version sequenceMultiple versions of Permit2Proxy were audited in close succession:
- v1.0.1 by Cantina (audit20250110)
- v1.0.2 by Sujith (audit20250109_3)
Ensure that v1.0.2 incorporates the findings from both audits.
193-199: Verify version ordering for LiFiDEXAggregatorThe version sequence for LiFiDEXAggregator appears unusual:
- v1.5.0 → v1.6.0 (expected)
- v1.5.1 added after v1.6.0 (potential concern)
This could indicate a hotfix (v1.5.1) to the v1.5.x branch while v1.6.0 development continued separately.
✅ Verification successful
Version ordering for LiFiDEXAggregator is valid
The audit IDs (audit20250109_2 and audit20250109_3) show these versions were audited sequentially on January 9, 2025, indicating an intentional hotfix pattern where v1.5.1 was created to address issues in the v1.5.x branch while v1.6.0 development continued separately. This is a common and valid versioning practice.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check git history to understand the version sequence git log --oneline -p -- "**/LiFiDEXAggregator.sol" | grep -B 5 -A 5 "version"Length of output: 0
Script:
#!/bin/bash # First, find the LiFiDEXAggregator file fd LiFiDEXAggregator.sol # Then check the audit logs for version information cat audit/auditLog.json | jq '.[] | select(. | tostring | contains("1.5")) as $item' # Look for version-related commits git log --oneline | grep -i "version\|1.5\|1.6"Length of output: 381
Script:
#!/bin/bash # Check the current version in the file cat src/Periphery/LiFiDEXAggregator.sol | grep -A 1 -B 1 "version" # Get a better view of the audit log structure cat audit/auditLog.json | jq '."LiFiDEXAggregator"' # Look at the file history git log --oneline src/Periphery/LiFiDEXAggregator.solLength of output: 249
lifi-action-bot
changed the title
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!!!)