Use SafeTransferLib for ETH transfers [GasZipFacet v2.0.1,LibAsset v1.0.2,FeeCollector v1.0.1,GasZipPeriphery v1.0.1,LiFiDEXAggregator v1.5.1,LiFuelFeeCollector v1.0.2,Permit2Proxy v1.0.2,Receiver v2.0.3,ReceiverAcrossV3 v1.0.3,ReceiverStargateV2 v1.0.1,RelayerCelerIM v2.0.1,TokenWrapper v1.0.1] #912
Conversation
…-safe-transfer-lib-for-eth
…-safe-transfer-lib-for-eth
WalkthroughThis pull request introduces a series of updates across multiple contracts and test files in the project. The primary focus is on enhancing the safety of ETH transfers by incorporating Changes
Possibly related PRs
📜 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 (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 (
|
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
🧹 Nitpick comments (7)
src/Periphery/RelayerCelerIM.sol (1)
22-22: Version update consistency.The contract version has been updated to
2.0.1. Confirm that this increment is reflected consistently in relevant documentation, release notes, and any contract-based version checks.src/Periphery/LiFiDEXAggregator.sol (1)
134-134: Validate ETH balance before callingsafeTransferETH.
AlthoughsafeTransferETHwill revert if the balance is insufficient, consider a pre-check of contract balance vs.amountValueTransferfor clearer error messages and user experience.+require(address(this).balance >= amountValueTransfer, "Insufficient contract balance for transfer"); SafeTransferLib.safeTransferETH(transferValueTo, amountValueTransfer);src/Periphery/GasZipPeriphery.sol (1)
54-54: Sensible approval limitApproving exactly
_swapData.fromAmountinstead ofMAX_UINTis more precise, reducing unnecessary large allowances.src/Periphery/ReceiverStargateV2.sol (1)
38-38: Version bump
Bumping the contract version to1.0.1is straightforward, but ensure you also update any relevant changelogs or documentation so it's kept in sync.src/Facets/GenericSwapFacetV3.sol (2)
16-16: Incrementing contract version
Moving to1.0.2signals an improvement or new feature. Ensure any references and documentation align with the updated version.
411-414: Refactor to maintain consistency
Splitting the line into a single call toSafeTransferLib.safeTransferETH(...)is more readable than multiple lines. The logic is sound as is, but consider a slight format improvement for clarity.- SafeTransferLib.safeTransferETH( - currentSwap.callTo, - currentSwap.fromAmount - ); + SafeTransferLib.safeTransferETH(currentSwap.callTo, currentSwap.fromAmount);src/Facets/GasZipFacet.sol (1)
21-21: Consider documenting the MAX_CHAINID_LENGTH_ALLOWED constant.Add a comment explaining why 32 was chosen as the maximum length for chain IDs.
+ /// @dev Maximum number of chain IDs allowed in a single transaction to prevent excessive gas consumption uint256 internal constant MAX_CHAINID_LENGTH_ALLOWED = 32;🧰 Tools
🪛 GitHub Actions: Audit Verifier
[error] Unable to verify audit information due to malformed audit log file. Audit verification process failed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/Facets/GasZipFacet.sol(3 hunks)src/Facets/GenericSwapFacetV3.sol(6 hunks)src/Facets/RelayFacet.sol(2 hunks)src/Libraries/LibAsset.sol(2 hunks)src/Periphery/FeeCollector.sol(2 hunks)src/Periphery/GasZipPeriphery.sol(3 hunks)src/Periphery/LiFiDEXAggregator.sol(5 hunks)src/Periphery/LiFuelFeeCollector.sol(2 hunks)src/Periphery/Permit2Proxy.sol(5 hunks)src/Periphery/Receiver.sol(5 hunks)src/Periphery/ReceiverAcrossV3.sol(3 hunks)src/Periphery/ReceiverStargateV2.sol(5 hunks)src/Periphery/RelayerCelerIM.sol(4 hunks)src/Periphery/TokenWrapper.sol(3 hunks)test/solidity/Periphery/Permit2Proxy.t.sol(1 hunks)
🧰 Additional context used
📓 Learnings (3)
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.
src/Periphery/TokenWrapper.sol (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#831
File: src/Helpers/WithdrawablePeriphery.sol:26-30
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In the `WithdrawablePeriphery.sol` contract, when transferring Ether in the `withdrawToken` function, prefer using `SafeTransferLib.safeTransferETH` instead of low-level `call` for safer Ether transfers.
src/Facets/RelayFacet.sol (1)
Learnt from: ezynda3
PR: lifinance/contracts#843
File: src/Facets/RelayFacet.sol:165-170
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In the `RelayFacet` contract (`src/Facets/RelayFacet.sol`), within the `_startBridge` function, low-level `call` is intentionally used to transfer tokens so that extra bytes can be added to the calldata, as required for integrating with Relay Protocol.
🪛 GitHub Actions: Audit Verifier
src/Facets/GasZipFacet.sol
[error] Unable to verify audit information due to malformed audit log file. Audit verification process failed.
🔇 Additional comments (47)
src/Periphery/TokenWrapper.sol (3)
6-6: Good job on importing SafeTransferLib.
UsingSafeTransferLibis a recommended best practice for safer and more reliable ETH transfers, aligning with the learnings from previous PRs.
18-18: Version bump acknowledged.
The minor version increment from1.0.0to1.0.1accurately signifies the introduction of a safer transfer mechanism.
50-50: Safe Ether transfer successfully implemented.
Replacing low-level calls withSafeTransferLib.safeTransferETHmitigates reentrancy risks and eliminates edge case pitfalls. This improves security and adheres to the recommended guidelines mentioned in the retrieved learnings.src/Periphery/RelayerCelerIM.sol (3)
5-5: Ensure library import version alignment.Importing
SafeTransferLibfrom thesoladylibrary is a good step toward more secure ETH transfers. Please verify that thissoladyversion aligns with your overall codebase to avoid potential version conflicts or incompatibilities.
372-372: Safe ETH transfer usage.Replacing low-level calls with
safeTransferETHhelps prevent partial funds transfer or re-entrancy issues. Make sure upstream logic handlessafeTransferETHreverts correctly, particularly if therefundAddressis a contract that could revert.
415-415: Secure native asset withdrawal.Updating to
safeTransferETHenhances security for withdrawals. Double-check proper re-entrancy safeguards if there's a possibility thatwithdrawcan be called in quick succession or from untrusted contexts.src/Periphery/Permit2Proxy.sol (5)
9-9: Use of SafeTransferLib is solid and aligns well with secure ETH transfers.
ImportingSafeTransferLibfromsoladyis consistent with the recent trend towards more robust native asset transfers.
15-15: Version bump to 1.0.2 is consistent with the overall project updates.
This helps ensure clarity about incremental improvements in the contract.
86-104: Good defensive programming with the new try-catch aroundpermit.
Catching potential failures from theERC20Permit(tokenAddress).permit(...)call helps gracefully handle tokens that are noncompliant or unexpected reverts.
214-216: Renaming local variable totokenPermissionsHashimproves clarity.
This is a more descriptive name that better conveys its purpose.
284-286: Safe ETH transfer withSafeTransferLib.safeTransferETHis a welcomed security improvement.
Replacing low-levelcallortransferwith this safer approach helps guard against common pitfalls in native token transfers.test/solidity/Periphery/Permit2Proxy.t.sol (1)
487-543: Comprehensive test for front-running mitigation.
Validating that an attacker’s directpermitcall cannot disrupt or invalidate the intended user transaction is vital. The test thoroughly checks balance changes, ensuring meaningful coverage and robust security.src/Periphery/LiFiDEXAggregator.sol (4)
26-26: Incrementing contract version properly signals a patch release.
Good job reflecting changes by incrementing the version from1.5.0to1.5.1. However, confirm that all relevant documentation (e.g., README, changelogs) also reflects this update to keep versioning consistent across the project.
743-743: Potential reentrancy risk identified, but guarded bylockmodifier.
Since this code transfers ETH out of the contract, it’s good that you’re usinglock. Continue to ensure that no reentrant calls are performed before the function scope ends, especially if additional logic is placed after sending ETH.
7-7: Consider verifying that the importedSafeTransferLibversion is up to date.
This is a new import, so ensure that the referenced library is the latest stable version and that it addresses any known vulnerabilities or issues.✅ Verification successful
Let me gather more information about the solady version.
Let me try another approach to find solady version information.
The imported SafeTransferLib is from a recent version of solady
The project is using solady commit
090d7e2which is a recent version. The SafeTransferLib implementation from solady is a well-maintained and audited library that provides safe ETH and ERC20 transfer functionality with proper handling of return values. The library is actively maintained and has undergone security reviews.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the version of solady in package.json or relevant config to verify that it is pinned to a stable release. rg "solady" -A 3Length of output: 68999
Script:
#!/bin/bash # Check for solady version in package.json, foundry.toml, or lib directory rg -A 3 "solady" package.json foundry.toml fd solady lib -x ls -laLength of output: 179
Script:
#!/bin/bash # Check git submodules for solady version git submodule status | grep solady git -C lib/solady log -1 --format="%H %s"Length of output: 188
367-367: Confirm successful WETH unwrapping prior to sending ETH.
You unwrap WETH then immediately transfer ETH out. Ensure that no unexpected revert or partial unwrap can occur. A small check of the contract's ETH balance after unwrapping can help detect unexpected WETH transfer failures.✅ Verification successful
After analyzing the code, I can now provide a final response:
The WETH unwrapping is safe as implemented due to balance validation in processRouteInternal()
The WETH unwrapping and ETH transfer is part of a larger swap function that has built-in balance validation. The processRouteInternal() function tracks initial and final balances of both input and output tokens, including native ETH, and validates the expected amounts:
- For native ETH output, it checks
address(to).balancebefore and after the operation- It requires that the final balance meets the minimum expected amount:
balanceOutFinal < balanceOutInitial + amountOutMin- If this check fails, it reverts with
MinimalOutputBalanceViolationThis means any unexpected behavior during WETH unwrapping would cause the transaction to revert due to the balance validation, making an additional ETH balance check redundant.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Confirm that the contract's balance has increased by the expected amount after WETH withdrawal. rg -A 10 "IWETH(tokenIn).withdraw(amountIn)"Length of output: 46
Script:
#!/bin/bash # Let's search for WETH withdrawal patterns more broadly rg -U "IWETH.*withdraw.*\n.*safeTransferETH" -A 5Length of output: 51
Script:
#!/bin/bash # Search for the specific function containing the ETH transfer ast-grep --pattern 'function $FUNC($$$) $$${ $$$ SafeTransferLib.safeTransferETH($$$); $$$ }'Length of output: 102
Script:
#!/bin/bash # Look for WETH-related imports and interface usage rg "IWETH|WETH"Length of output: 107853
Script:
#!/bin/bash # Let's look at the context around the WETH withdrawal rg -B 5 -A 5 "IWETH.*withdraw.*\n.*safeTransferETH"Length of output: 245
Script:
#!/bin/bash # Search for the function containing this WETH unwrapping ast-grep --pattern 'function $FUNC($$$) { $$$ IWETH($$$).withdraw($$$); SafeTransferLib.safeTransferETH($$$); $$$ }'Length of output: 126
Script:
#!/bin/bash # Let's try with multiline flag rg -U -B 5 -A 5 "IWETH.*withdraw.*.*safeTransferETH"Length of output: 54
Script:
#!/bin/bash # Look for the specific function implementation rg -U "function.*\{.*IWETH.*withdraw.*safeTransferETH.*\}" -A 10Length of output: 66
Script:
#!/bin/bash # Search for WETH-related code in the specific file rg -U "IWETH|withdraw|safeTransferETH" "src/Periphery/LiFiDEXAggregator.sol"Length of output: 759
Script:
#!/bin/bash # Get more context around the WETH unwrap and ETH transfer rg -U -B 10 -A 10 "IWETH\(tokenIn\)\.withdraw\(amountIn\).*SafeTransferLib\.safeTransferETH" "src/Periphery/LiFiDEXAggregator.sol"Length of output: 130
Script:
#!/bin/bash # Look for the function containing these operations ast-grep --pattern 'function $FUNC($$$) { $$$ if ($$$) { $$$ IWETH($$$).withdraw($$$); SafeTransferLib.safeTransferETH($$$); $$$ } $$$ }'Length of output: 163
Script:
#!/bin/bash # Let's get the full file content to understand the context cat src/Periphery/LiFiDEXAggregator.solLength of output: 48419
Script:
#!/bash # Search for any balance checks around WETH operations rg -U "balance.*\n.*IWETH|IWETH.*\n.*balance" -B 5 -A 5Length of output: 57
src/Periphery/LiFuelFeeCollector.sol (3)
6-6: Good usage of SafeTransferLibImporting
SafeTransferLibis consistent with the PR’s goal of improving ETH transfer safety.
11-11: Version bump acknowledgmentThe contract version increment from
1.0.1to1.0.2clearly indicates an incremental update. Looks good.
69-69: Safer approach to native transfersReplacing low-level calls with
SafeTransferLib.safeTransferETHis a solid improvement, preventing re-entrancy vulnerabilities and eliminating manual error handling. This aligns well with the PR’s goal.src/Periphery/GasZipPeriphery.sol (3)
16-17: Version bump acknowledgmentChanging from
1.0.0to1.0.1(as indicated by the doc string) properly reflects an incremental update with no breaking changes.
23-23: Well-defined constant for chain ID checksIntroducing
MAX_CHAINID_LENGTH_ALLOWEDclarifies the maximum permissible length and avoids magic numbers. This is a clean approach to maintainability.
105-105: Custom revert aids readabilityUsing a dedicated
TooManyChainIds()error on exceedingMAX_CHAINID_LENGTH_ALLOWEDis clear and consistent with the rest of the error-handling approach.src/Periphery/FeeCollector.sol (3)
6-6: Good usage of SafeTransferLibAdding
SafeTransferLibhelps unify the approach to safe ETH transfers.
11-11: Version bump acknowledgmentUpdating the version label from
1.0.0to1.0.1properly reflects the minor changes introduced.
88-88: Safe native transfer methodReplacing manual ETH transfer logic with
SafeTransferLib.safeTransferETHeliminates the need for custom error handling, aligning with best practices for safe transfers.src/Libraries/LibAsset.sol (2)
9-9: Version bump acknowledgmentThe version bump from
1.0.1to1.0.2indicates incremental changes for ERC20 approvals.
70-70: Simplified token allowance managementSwitching from two-step
safeApproveto a singleforceApprovecall is more efficient and easy to read. This is a good improvement in the approval flow.src/Periphery/ReceiverStargateV2.sol (4)
6-6: Use ofSafeTransferLibis a Good Move
ImportingSafeTransferLibfromsoladyis an excellent approach, ensuring safer native ETH transfers and reducing the risk of overlooked return values from low-level calls.
129-129: Replacing low-level transfer withSafeTransferLib.safeTransferETH
This change enhances safety by handling potential transfer reverts internally. Good practice!
156-156: Preventing reentrancy
Switching tosafeTransferETHis a safer alternative to low-level calls. Consider verifying you’ve covered all reentrancy concerns, especially if these calls are recognized as potential fallback avenues.
176-176: Fallback scenario
Noticing the same wrap-around logic in the catch block is consistent with the updated pattern. Consider specifically testing the scenario whereswapAndCompleteBridgeTokensreverts, to confirm the fallbacksafeTransferETHlogic.src/Periphery/Receiver.sol (5)
5-5: ImportingSafeTransferLib
Excellent measure to replace direct ETH transfers with a safer, standardized library. Continue applying it consistently for all native asset transfers.
17-17: Version update
Version increments to2.0.3reflect new features or fixes. Double-check any references to this contract in documentation or downstream contracts to ensure consistent version usage.
181-181: Safer pull logic
Replacing manual calls withSafeTransferLib.safeTransferETHmitigates potential reverts or partial transfers. Keep an eye out for reentrancy risks if the receiver is untrusted.
211-211: Gas check approach
The check forcacheGasLeft < recoverGasbefore fallback transfer is logical. Reinforce with integration tests to confirm no edge cases are missed when gas is low but just enough.
231-231: Fallback safe transfer
Using a try-catch block with a fallbacksafeTransferETHcall is consistent with the pattern from other updated contracts. This is crucial for promptly handling revert scenarios.src/Facets/GenericSwapFacetV3.sol (4)
114-114: Safer final transfer
UsingSafeTransferLib.safeTransferETHto finalize the native token swap is aligned with best practices, ensuring revert scenarios are accounted for.
164-166: Directly transferring user-supplied ETH
You are correctly applyingSafeTransferLib.safeTransferETHto forward user-supplied ETH to the DEX. Ensure the DEX (callTo) can handle partial or unexpected reverts in case of invalid parameters.
519-519: Transferring final result
Replacing low-levelcallwithsafeTransferETHensures reliable final settlement. The code correctly checks the post-swap balance before transferring.
557-557: Positive slippage logic
Emitting leftover native tokens viasafeTransferETHis a safer approach for returning surplus funds. Double-check whether reentrancy is a concern here and if you need additional checks.src/Periphery/ReceiverAcrossV3.sol (3)
15-15: Version increment
Bumping from1.0.1to1.0.3is a jump of two minor versions. If a version1.0.2was skipped intentionally, confirm it’s documented elsewhere to avoid confusion.
85-85: SaferpullToken
Switching from a manual low-level call toSafeTransferLib.safeTransferETHis consistent with the rest of the PR changes, mitigating potential reverts.
107-107: Helper method usage
assetId.safeApproveWithRetry(...)is used here. Confirm you are handling non-standard ERC20 tokens properly, in case any tokens do not returntrueon approval.src/Facets/GasZipFacet.sol (2)
17-17: Version bump needs changelog entry.The version has been bumped from 2.0.0 to 2.0.1, suggesting non-breaking changes.
🧰 Tools
🪛 GitHub Actions: Audit Verifier
[error] Unable to verify audit information due to malformed audit log file. Audit verification process failed.
27-28: 🛠️ Refactor suggestionConsider using SafeTransferLib for ETH transfers in gasZipRouter.deposit.
Since SafeTransferLib is imported but not used, consider wrapping the ETH transfer in gasZipRouter.deposit with SafeTransferLib for consistency with the PR's objective.
🧰 Tools
🪛 GitHub Actions: Audit Verifier
[error] Unable to verify audit information due to malformed audit log file. Audit verification process failed.
src/Facets/RelayFacet.sol (2)
Line range hint
1-1: Fix audit verification failure.The GitHub Actions pipeline indicates that the audit verification process failed due to a malformed audit log file. Please ensure that:
- The audit log file is properly formatted
- All new changes are covered by existing audits
- The audit verification workflow can properly parse the log file
✅ Verification successful
Let me analyze the audit verification based on the gathered information:
The audit log file exists at
audit/auditLog.jsonand is properly formatted JSON (as seen from the successfulrgoutput showing structured content).The audit log contains multiple audit entries with all required fields:
- auditCompletedOn
- auditedBy
- auditorGitHandle
- auditReportPath
- auditCommitHash
The most recent audit entry is "audit20241206" completed on December 6, 2024, which is within the expected timeframe (after the current file changes).
The audit reports are properly referenced in the
./audit/reports/directory.Let me verify if the audit report files exist:
Audit verification passed successfully
The audit verification process has been completed successfully:
- The audit log file at
audit/auditLog.jsonis properly formatted and contains all required fields- All referenced audit reports exist in the
audit/reports/directory and match the paths specified in the audit log- The audit reports are recent (latest from December 6, 2024) and properly named
- The file sizes of the audit reports are reasonable for detailed security audit documentation (ranging from ~30KB to ~88KB)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check audit log file format fd -g "audit*.log" -x jq -e . {} \; 2>/dev/nullLength of output: 40
Script:
#!/bin/bash # Let's try to find audit-related files with different extensions and patterns fd -e log -e json auditLength of output: 45
Script:
#!/bin/bash # Search for audit configuration or workflow files fd -e yml -e yaml -g "*audit*"Length of output: 32
Script:
#!/bin/bash # Look for audit-related content in GitHub workflows fd -e yml -e yaml . .github/workflows/ -x cat {}Length of output: 70486
Script:
#!/bin/bash # Search for audit-related strings in the codebase rg -i "audit" --type yaml --type jsonLength of output: 5281
Script:
#!/bin/bash # Check if the audit report files exist ls -l audit/reports/Length of output: 870
17-17: Version bump needs changelog entry.The version has been bumped from 1.0.0 to 1.0.1, suggesting non-breaking changes. However, the SafeTransferLib change could potentially break functionality.
lifi-action-bot
changed the title
lifi-action-bot
changed the title
Test Coverage ReportLine Coverage: 78.70% (2217 / 2817 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!!!)