introduces a gas-optimized periphery contract for same-chain swaps (GenericSwapper v1.0.0) #710
Conversation
WalkthroughThis pull request introduces a comprehensive update to the LiFi protocol's infrastructure, focusing on enhancing token swapping capabilities and network configurations. The changes include adding a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 7
🧹 Nitpick comments (24)
src/Periphery/GenericSwapper.sol (5)
36-39: Consider more flexible aggregator checks.
The modifier only allows calls to a single aggregator. If, in the future, more aggregator addresses are whitelisted, you may need a structure for multiple aggregator addresses.
41-55: Double-check logic for multiple swap call recipients.
By restricting calls to either dexAggregator or feeCollector, this ensures security but might hamper flexibility for aggregator expansions. Keep in mind if additional addresses must be supported.
78-86: Enforce consistent revert messages.
Currently, you catch and revert using LibUtil.revertWith(res). Consider appending a standardized prefix or ensure consistent logging across the codebase.
330-337: Validate recognized poolType values.
The swap function uses an if/else chain for pool types 0 to 5. Consider adding an explicit revert for unexpected pool types or grouping them more systematically.
Line range hint
493-513: Sanity check on lastCalledPool mechanism.
Storing lastCalledPool to detect whether the right pool is calling the callback is good. Still, consider events or other checks to ensure no race conditions if multiple swaps are executed in parallel.test/solidity/Periphery/GenericSwapper.t.sol (2)
66-75: Consider negative path testing.
In tests such as test_CanExecuteSingleSwapNativeToERC20_V4, add explicit negative or boundary checks: insufficient funds, reverts, etc. This helps ensure contract resilience.
482-487: Confirm aggregator approval sequences.
When verifying the aggregator approvals, confirm that existing allowances are reset to zero first. This test snippet is good but consider additional checks on partial allowances.src/Periphery/LiFiDEXAggregator.sol (1)
529-541: Use typed checks for callback usage.
In uniswapV3SwapCallback, you cast msg.sender to lastCalledPool. This is fine for security, but consider typed addresses or a more robust factory check if expanded for multiple pools.test/solidity/Facets/GenericSwapFacetV3.t.sol (5)
70-75: Confirm block number replay stability.
Tests rely on forked block 19834820. Ensure these tests remain stable if the chain data changes or for local block reorg. Use pinned data or mocking for true stability.
229-230: Message consistency in events.
You fire LiFiGenericSwapCompleted with fromAssetId and toAssetId but store them as constants in some places. Ensure consistent usage of these addresses across events if you add additional logging in the future.
Line range hint
332-337: Corner case coverage.
The test_WillRevertIfSlippageIsTooHighSingleERC20ToERC20 is good, but consider also testing the scenario where minAmountOut is significantly higher than possible, forcing an immediate revert.
Line range hint
680-688: Maintain the whitelisting procedure.
After removing the DEX from the whitelist, you revert if used. This test is correct. It might be worthwhile to also test re-adding the DEX to confirm no stale states remain.
163-166: Naming consistency.
vm.label(ADDRESS_WETH, "WETH_TOKEN") is consistent with labeling DAI_TOKEN and so forth. Ensure all tokens have consistent naming in logs to avoid confusion in large test sets.script/deploy/facets/DeployGenericSwapFacetV3.s.sol (1)
26-29: Consider making config path configurableThe hardcoded path to global.json could be problematic in different deployment environments.
Consider making it configurable through environment variables:
string memory globalConfigPath = string.concat( root, - "/config/global.json" + vm.envOr("CONFIG_PATH", string("/config/global.json")) );deployments/bsc.diamond.staging.json (1)
72-75: Consider adding name and version for better traceabilityThe facet entry with address
0xA269cb81E6bBB86683558e449cb1bAFFdb155Bfchas empty name and version fields. This could make tracking and maintenance more difficult.docs/GenericSwapper.md (2)
9-10: Highlight security implications of approval requirementsThe documentation should more prominently warn about the security implications of not checking token approvals. Consider adding a dedicated "Security Considerations" section.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...chain swaps. It will not emit any events and it can only use the LI.FI DEX Aggregato...(COMMA_COMPOUND_SENTENCE)
[style] ~9-~9: Reusing ‘It’ could be redundant. Try omitting the pronoun.
Context: ... swaps. It will not emit any events and it can only use the LI.FI DEX Aggregator to ex...(SUBJECT_DROP)
58-59: Fix grammatical error in method descriptionThe description should read "This method is used to execute a swap from ERC20 to any other token..."
🧰 Tools
🪛 LanguageTool
[uncategorized] ~58-~58: Possible missing preposition found.
Context: ...od is used to execute a swap from ERC20 any other token (ERC20 or native) incl. a p...(AI_HYDRA_LEO_MISSING_TO)
test/solidity/utils/TestHelpers.sol (1)
82-111: Consider parameterizing hardcoded values in _getFeeCollectorSwapDataThe function uses hardcoded addresses (ADDRESS_USDC) and amounts (defaultNativeFeeCollectionAmount, defaultUSDCFeeCollectionAmount). Consider making these configurable parameters for better flexibility in testing different scenarios.
config/global.json (1)
115-115: Standardize Metis native address formatThe Metis native address uses mixed case. While technically valid due to the checksum, it's recommended to use a consistent format across all addresses.
- "metis": "0xDeadDeAddeAddEAddeadDEaDDEAdDeaDDeAD0000", + "metis": "0xdeaddeaddeaddeaddeaddeaddeaddeaddead0000"test/solidity/Periphery/ReceiverStargateV2.t.sol (2)
Line range hint
89-95: Add test cases for edge cases in pullTokenThe pullToken tests should include additional edge cases:
- Pulling 0 amount
- Pulling more than available balance
- Pulling with zero address receiver
Line range hint
196-238: Enhance reentrancy test coverageWhile the reentrancy test is good, consider adding tests for:
- Multiple reentrant calls
- Different reentrant paths
- Gas exhaustion scenarios
src/Facets/GenericSwapFacetV3.sol (1)
16-16: Improved native token handling with immutable storage.Good architectural decision to:
- Use an immutable variable for native token address
- Initialize via constructor
- Update version following semantic versioning
This change improves gas efficiency and maintainability by:
- Avoiding repeated storage reads
- Providing consistent native token handling across different chains
- Making the native token address configurable per deployment
Also applies to: 20-27
test/solidity/Facets/GenericSwapFacetV3_POL.t.sol (2)
100-100: Consider using chain-specific native address in tests.Currently using
address(0)in tests, but consider using the actual native token address for the Polygon network to better match production conditions.-genericSwapFacetV3 = new TestGenericSwapFacetV3(address(0)); +genericSwapFacetV3 = new TestGenericSwapFacetV3(WETH_ADDRESS);
Line range hint
1-1347: Comprehensive test coverage for swap scenarios.The test file thoroughly covers:
- Single/multiple token swaps
- ERC20/Native token combinations
- Fee collection scenarios
- Gas usage comparisons
However, some test functions contain commented-out code that should be cleaned up.
Consider removing or documenting the purpose of commented code blocks in test functions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
config/global.json(1 hunks)deployments/_deployments_log_file.json(1 hunks)deployments/bsc.diamond.staging.json(2 hunks)deployments/bsc.staging.json(1 hunks)docs/GenericSwapper.md(1 hunks)script/deploy/facets/DeployGenericSwapFacetV3.s.sol(1 hunks)script/deploy/facets/DeployGenericSwapper.s.sol(1 hunks)script/deploy/resources/deployRequirements.json(1 hunks)src/Facets/GenericSwapFacetV3.sol(7 hunks)src/Periphery/GenericSwapper.sol(1 hunks)src/Periphery/LiFiDEXAggregator.sol(1 hunks)test/solidity/Facets/AcrossFacetPacked.t.sol(0 hunks)test/solidity/Facets/AmarokFacetPacked.t.sol(0 hunks)test/solidity/Facets/GenericSwapFacetV3.t.sol(55 hunks)test/solidity/Facets/GenericSwapFacetV3_POL.t.sol(2 hunks)test/solidity/Facets/StargateFacet.t.sol(0 hunks)test/solidity/Facets/StargateFacetV2.t.sol(0 hunks)test/solidity/Periphery/GenericSwapper.t.sol(1 hunks)test/solidity/Periphery/ReceiverStargateV2.t.sol(2 hunks)test/solidity/utils/TestBase.sol(6 hunks)test/solidity/utils/TestHelpers.sol(3 hunks)
🔥 Files not summarized due to errors (1)
- deployments/_deployments_log_file.json: Error: Server error: no LLM provider could handle the message
💤 Files with no reviewable changes (4)
- test/solidity/Facets/StargateFacetV2.t.sol
- test/solidity/Facets/AmarokFacetPacked.t.sol
- test/solidity/Facets/StargateFacet.t.sol
- test/solidity/Facets/AcrossFacetPacked.t.sol
🧰 Additional context used
🪛 LanguageTool
docs/GenericSwapper.md
[uncategorized] ~9-~9: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...chain swaps. It will not emit any events and it can only use the LI.FI DEX Aggregato...
(COMMA_COMPOUND_SENTENCE)
[style] ~9-~9: Reusing ‘It’ could be redundant. Try omitting the pronoun.
Context: ... swaps. It will not emit any events and it can only use the LI.FI DEX Aggregator to ex...
(SUBJECT_DROP)
[style] ~11-~11: Consider a shorter alternative to avoid wordiness.
Context: ... to be set first (see function below). In order to still be able to trace transactions a `...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~12-~12: A comma might be missing here.
Context: ...w). In order to still be able to trace transactions a transactionId will be appended to t...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~58-~58: Possible missing preposition found.
Context: ...od is used to execute a swap from ERC20 any other token (ERC20 or native) incl. a p...
(AI_HYDRA_LEO_MISSING_TO)
🔇 Additional comments (24)
deployments/_deployments_log_file.json (2)
19867-19867:
Fix the future timestamp in deployment log
The deployment timestamp is set to "2024-07-10" which is a future date. This could cause issues with deployment verification and auditing.
Please update with the actual deployment timestamp.
⛔ Skipped due to learnings
Learnt from: ezynda3
PR: lifinance/contracts#823
File: deployments/_deployments_log_file.json:10542-10543
Timestamp: 2024-11-21T08:17:27.878Z
Learning: When reviewing deployment timestamps in `deployments/_deployments_log_file.json`, ensure that the timestamps are accurately compared with the current date to avoid incorrectly flagging valid timestamps as future deployment dates.
19863-19863: Verify version number discrepancy
The PR title mentions "GenericSwapper v1.0.0" but the deployment log shows version "1.0.1". Please confirm if this version bump is intentional.
src/Periphery/GenericSwapper.sol (3)
122-128: Validate native swap safety.
After a successful swap to native, ensure the contract does not inadvertently hold leftover native tokens from previous steps. The existing flow looks sound, but consider an additional flush if needed in complex flows.
351-377: Check for potential re-entrancy.
This wrapNative function relies on calling external code (e.g. WETH deposit/withdraw). While the contract uses a global lock mechanism, ensure no other internal function triggers state changes that open re-entrancy windows.
18-24: Validate constructor parameters.
While the constructor seems straightforward, ensure that zero addresses for the DEX aggregator or fee collector are never passed in from the deployment script. This could lead to misconfiguration.
Use this script to search for the constructor usage in the deployment scripts, verifying that nonzero addresses are passed:
✅ Verification successful
Zero address validation is properly enforced through deployment configuration
Based on the analysis of the deployment scripts and configuration:
- The
DeployGenericSwapper.s.solscript reads addresses from a deployment JSON file and passes them to the constructor. - The
deployRequirements.jsonexplicitly configures both constructor parameters with"allowToDeployWithZeroAddress": "false"under:
"GenericSwapper": {
"configData": {
"contractAddresses": {
"FeeCollector": {
"allowToDeployWithZeroAddress": "false"
},
"LiFiDEXAggregator": {
"allowToDeployWithZeroAddress": "false"
}
}
}
}This configuration ensures that zero addresses cannot be passed to the constructor during deployment, making the original review comment's concern already addressed by the deployment infrastructure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for constructor usage and parameters in deployment scripts
rg -A 10 'new GenericSwapper\s*\('
Length of output: 36
Script:
#!/bin/bash
# Let's first check the constructor implementation to understand the validation
ast-grep --pattern 'constructor($$$) {
$$$
}'
Length of output: 49
Script:
#!/bin/bash
# Let's also search for any test files that might show constructor usage
fd -e sol -e js -e ts "test|Test|spec" --exec rg -l "GenericSwapper"
Length of output: 68
Script:
#!/bin/bash
# Let's check for deploy scripts or deployment-related files
fd "deploy|Deploy|script" --exec rg -l "GenericSwapper"
Length of output: 355
Script:
#!/bin/bash
# Let's check the deployment script content
cat ./script/deploy/facets/DeployGenericSwapper.s.sol
Length of output: 1219
Script:
#!/bin/bash
# Let's also check the deployment requirements
cat ./script/deploy/resources/deployRequirements.json
Length of output: 12529
Script:
#!/bin/bash
# Let's try a different pattern for the constructor
ast-grep --pattern 'constructor(address $_, address $_) {
$$$
}'
Length of output: 68
Script:
#!/bin/bash
# Let's check the GenericSwapper contract implementation directly
cat src/Periphery/GenericSwapper.sol
Length of output: 15069
test/solidity/Periphery/GenericSwapper.t.sol (1)
120-129: Expand test for setApprovalForTokensAndSwap.
In test_CanSetApprovalsAndExecuteSingleSwapERC20ToNative_V4, ensure coverage of edge cases (empty token arrays, repeated tokens, or zero addresses).
src/Periphery/LiFiDEXAggregator.sol (3)
48-54: Evaluate paused/locked states.
Combining “locked” and “paused” flags in a single contract can get tricky. Confirm that the code paths do not prematurely unlock if paused, or vice versa.
122-139: Validate partial value sending.
In transferValueAndprocessRoute, confirm that partial failures with .call() do not leave the aggregator in a half-updated state. Currently you revert on failure, which is correct, but double-check the integration flow.
370-376: Check potential re-entrancy for unwrap.
When unwrapping, you rely on a direct .call. The contract is locked, so likely safe. Still, ensure no external calls are triggered that bypass lock.
test/solidity/Facets/GenericSwapFacetV3.t.sol (2)
22-23: Constructor clarity.
The new constructor in TestGenericSwapFacetV3 sets _nativeAddress. Confirm that referencing address(0) always denotes the actual chain’s native address. If a chain uses a different representation, this might need to be updated.
Line range hint 898-905: Double-check array usage in mock aggregator.
When minAmountOut is (minAmountOut - 1), ensure that negative or overflow conditions do not occur under certain big number scenarios.
deployments/bsc.staging.json (1)
Line range hint 1-30: Verify contract dependencies
The GenericSwapFacetV3 depends on other contracts like FeeCollector. Let's verify these dependencies are properly configured.
✅ Verification successful
Let me gather more information about the contract dependencies by analyzing the actual contract code.
Let me check the GenericSwapFacetV3 implementation and its interaction with FeeCollector.
Let me check how GenericSwapFacetV3 interacts with FeeCollector through the LibSwap library.
Let me check the GenericSwapper contract since it's the base contract that handles fee collection interactions.
Based on the code analysis, I can now generate the final response.
Dependencies are properly configured
The review comment's concern about FeeCollector dependencies can be marked as resolved because:
- The FeeCollector contract is deployed and available in the BSC staging environment at
0x7f98D45c7902f079fDb65811B633522e2d227BB6 - GenericSwapFacetV3 doesn't directly depend on FeeCollector - it's actually the other way around where fee collection is handled through the SwapData array in a standardized way
- The contract architecture shows that fee collection is handled through the LibSwap library and is properly integrated into the swapping process
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract dependencies
# Check if all required contracts are deployed
echo "Checking dependencies..."
required_contracts=("FeeCollector" "LiFiDiamond" "GenericSwapFacet")
for contract in "${required_contracts[@]}"; do
if ! grep -q "\"$contract\":" deployments/bsc.staging.json; then
echo "Missing required contract: $contract"
exit 1
fi
done
Length of output: 438
Script:
#!/bin/bash
# Search for GenericSwapFacetV3 contract implementation and its dependencies
ast-grep --pattern 'contract GenericSwapFacetV3 $$$'
# Search for FeeCollector references in GenericSwapFacetV3
rg "FeeCollector" -A 3 -B 3
Length of output: 67338
Script:
#!/bin/bash
# Search for GenericSwapFacetV3 implementation and its interaction with FeeCollector
ast-grep --pattern 'contract GenericSwapFacetV3 $$$'
# Search for FeeCollector usage in GenericSwapFacetV3
rg -l "FeeCollector" src/Facets/GenericSwapFacetV3.sol || true
cat src/Facets/GenericSwapFacetV3.sol
Length of output: 22303
Script:
#!/bin/bash
# Check LibSwap library for FeeCollector interactions
cat src/Libraries/LibSwap.sol
# Check for any other files that might show the connection between GenericSwapFacetV3 and FeeCollector
rg -l "FeeCollector.*GenericSwapFacetV3|GenericSwapFacetV3.*FeeCollector"
Length of output: 2620
Script:
#!/bin/bash
# Check GenericSwapper implementation
cat src/Periphery/GenericSwapper.sol
# Check if GenericSwapFacetV3 inherits from GenericSwapper
rg -A 5 "contract GenericSwapFacetV3" src/Facets/GenericSwapFacetV3.sol
Length of output: 15282
deployments/bsc.diamond.staging.json (2)
76-78: LGTM: New GenericSwapFacetV3 facet
The addition of GenericSwapFacetV3 v1.0.1 aligns with the PR objectives for introducing gas-optimized same-chain swaps.
88-88: Verify if ReceiverStargateV2 configuration is intentionally empty
The empty ReceiverStargateV2 entry might indicate incomplete configuration.
✅ Verification successful
Empty ReceiverStargateV2 configuration is expected in staging environment
Based on the evidence found:
- The empty ReceiverStargateV2 configuration is consistent with other staging deployment files (e.g., optimism.diamond.staging.json, polygon.diamond.staging.json)
- The ReceiverStargateV2 contract exists and is properly implemented in the codebase
- Production deployments have actual addresses configured (e.g., mainnet, arbitrum, bsc)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if ReceiverStargateV2 is configured in other deployment files
fd -e json -x jq -r 'if (.LiFiDiamond.Periphery.ReceiverStargateV2 != null and .LiFiDiamond.Periphery.ReceiverStargateV2 != "") then . else empty end' {}
Length of output: 4557
Script:
#!/bin/bash
# Let's check the structure and content of the deployment file directly
cat deployments/bsc.diamond.staging.json
Length of output: 2999
Script:
#!/bin/bash
# Let's check other staging deployment files to see if ReceiverStargateV2 is configured there
cat deployments/*.staging.json | grep -A 1 "ReceiverStargateV2"
Length of output: 1314
Script:
#!/bin/bash
# Let's check if there are any references to ReceiverStargateV2 in the source code
rg -A 2 "ReceiverStargateV2" --type ts --type sol
Length of output: 77
Script:
#!/bin/bash
# Let's check for any references to ReceiverStargateV2 in the source code without file type restrictions
rg -A 2 "ReceiverStargateV2"
Length of output: 8915
test/solidity/utils/TestHelpers.sol (1)
115-156: Review assembly code safety in mergeBytes
The mergeBytes function uses low-level assembly which is efficient but requires careful review:
- The function correctly handles memory allocation
- Proper bounds checking is implemented
- Memory pointer is properly updated
However, consider adding:
- Input validation for empty arrays
- Documentation explaining the assembly operations
Add NatSpec documentation explaining the purpose and safety considerations of this function.
✅ Verification successful
Let me analyze the usage of mergeBytes further by checking the test contexts where it's used.
Based on the analysis of the code, I can now provide a final response:
Assembly code in mergeBytes is safe and well-implemented
The mergeBytes function is used in test utilities for appending transaction IDs and is well-implemented:
- Memory allocation and pointer management is correct
- Bounds checking is properly handled through loop conditions
- Memory pointer updates are accurate
- Empty array cases are handled implicitly through length checks
The function is only used in test files for constructing test payloads and is not part of the production codebase, making its current implementation sufficient for its purpose.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if mergeBytes is used in critical paths
rg "mergeBytes" -A 3
Length of output: 1148
Script:
#!/bin/bash
# Check the test files that use mergeBytes to understand the context
cat test/solidity/Periphery/ReceiverStargateV2.t.sol test/solidity/Periphery/GenericSwapper.t.sol
Length of output: 42450
test/solidity/Periphery/ReceiverStargateV2.t.sol (1)
4-5: LGTM! Improved test organization
The switch to TestHelpers and explicit error imports improves test organization and error handling.
script/deploy/resources/deployRequirements.json (1)
416-426: LGTM! Well-structured configuration for GenericSwapper
The configuration correctly enforces non-zero addresses for critical dependencies (FeeCollector and LiFiDEXAggregator) and follows the established pattern.
test/solidity/utils/TestBase.sol (5)
92-92: New fee collection infrastructure added.
The addition of fee-related state variables establishes the foundation for fee collection functionality:
feeCollectorfor managing fee operations- Default fee amounts for different token types (USDC, Native)
- LiFi platform fee amount
Also applies to: 98-98, 102-105
141-141: New token and user addresses added.
The changes introduce:
- USDT token address for expanded token support
- USER_INTEGRATOR address for third-party integration support
Also applies to: 148-148
214-214: FeeCollector initialization in test setup.
The changes properly initialize the fee collection infrastructure:
- USDT token instance
- FeeCollector contract with diamond owner
Also applies to: 218-220
234-235: Fee collection amount initialization.
The default USDC fee collection amount is properly initialized.
8-9:
Duplicate import statement.
The ILiFi interface is imported twice.
Remove the duplicate import:
import { FeeCollector } from "lifi/Periphery/FeeCollector.sol";
-import { ILiFi } from "lifi/Interfaces/ILiFi.sol";Likely invalid or redundant comment.
src/Facets/GenericSwapFacetV3.sol (1)
125-125: Consistent usage of NATIVE_ADDRESS across the contract.
The changes systematically replace address(0) with NATIVE_ADDRESS in:
- Event emissions
- Token comparisons
- Return value checks
This improves consistency and makes the contract more maintainable.
Also applies to: 137-137, 194-194, 206-206, 537-537, 549-549
test/solidity/Facets/GenericSwapFacetV3_POL.t.sol (1)
20-21: Test contract properly inherits native address configuration.
The constructor correctly passes the native address to the parent contract.
| if (commandCode == 1) { | ||
| uint256 usedAmount = processMyERC20(stream); | ||
| if (step == 0) realAmountIn = usedAmount; | ||
| } else if (commandCode == 2) | ||
| processUserERC20(stream, amountIn); | ||
| else if (commandCode == 3) { | ||
| uint256 usedAmount = processNative(stream); | ||
| if (step == 0) realAmountIn = usedAmount; | ||
| } else if (commandCode == 4) processOnePool(stream); | ||
| else if (commandCode == 5) processInsideBento(stream); | ||
| else if (commandCode == 6) applyPermit(tokenIn, stream); | ||
| else revert("RouteProcessor: Unknown command code"); | ||
| ++step; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assess dynamic loops.
The while loop for reading commands from route ensures all commands are processed. Confirm that malicious routes with large data do not cause overly high gas usage. A maximum command count or chunking might be beneficial.
| fileSuffix, | ||
| "json" | ||
| ); | ||
| string memory json = vm.readFile(path); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for JSON file reading
The vm.readFile call should be wrapped in a try-catch block to handle potential file reading errors gracefully.
Apply this diff:
- string memory json = vm.readFile(path);
+ string memory json;
+ try vm.readFile(path) returns (string memory result) {
+ json = result;
+ } catch {
+ revert("DeployGenericSwapper: failed to read deployment config");
+ }📝 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.
| string memory json = vm.readFile(path); | |
| string memory json; | |
| try vm.readFile(path) returns (string memory result) { | |
| json = result; | |
| } catch { | |
| revert("DeployGenericSwapper: failed to read deployment config"); | |
| } |
| address dexAggregatorAddress = json.readAddress(".LiFiDEXAggregator"); | ||
| address feeCollectorAddress = json.readAddress(".FeeCollector"); | ||
|
|
There was a problem hiding this comment.
Add address validation checks
The constructor arguments should be validated to ensure they are non-zero addresses before deployment.
Apply this diff to add validation:
string memory json = vm.readFile(path);
address dexAggregatorAddress = json.readAddress(".LiFiDEXAggregator");
address feeCollectorAddress = json.readAddress(".FeeCollector");
+ require(dexAggregatorAddress != address(0), "DeployGenericSwapper: zero dexAggregator address");
+ require(feeCollectorAddress != address(0), "DeployGenericSwapper: zero feeCollector address");📝 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.
| address dexAggregatorAddress = json.readAddress(".LiFiDEXAggregator"); | |
| address feeCollectorAddress = json.readAddress(".FeeCollector"); | |
| address dexAggregatorAddress = json.readAddress(".LiFiDEXAggregator"); | |
| address feeCollectorAddress = json.readAddress(".FeeCollector"); | |
| require(dexAggregatorAddress != address(0), "DeployGenericSwapper: zero dexAggregator address"); | |
| require(feeCollectorAddress != address(0), "DeployGenericSwapper: zero feeCollector address"); |
| // extract network's native address | ||
| address nativeAddress = globalConfigJson.readAddress( | ||
| string.concat(".nativeAddress.", network) | ||
| ); |
There was a problem hiding this comment.
Add validation for native address
The native address is critical for gas-optimized swaps and should be validated before deployment.
Apply this diff:
address nativeAddress = globalConfigJson.readAddress(
string.concat(".nativeAddress.", network)
);
+ require(nativeAddress != address(0), "DeployGenericSwapFacetV3: invalid native address");📝 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.
| // extract network's native address | |
| address nativeAddress = globalConfigJson.readAddress( | |
| string.concat(".nativeAddress.", network) | |
| ); | |
| // extract network's native address | |
| address nativeAddress = globalConfigJson.readAddress( | |
| string.concat(".nativeAddress.", network) | |
| ); | |
| require(nativeAddress != address(0), "DeployGenericSwapFacetV3: invalid native address"); |
| @@ -26,5 +26,6 @@ | |||
| "ThorSwapFacet": "0xa6aAe470E7B8E8916e692882A5db25bB40C398A7", | |||
| "AmarokFacetPacked": "0x7ac3EB2D191EBAb9E925CAbFD4F8155be066b3aa", | |||
| "MayanBridgeFacet": "0x5Ba4FeD1DAd2fD057A9f687B399B8e4cF2368214", | |||
| "MayanFacet": "0xd596C903d78870786c5DB0E448ce7F87A65A0daD" | |||
| "MayanFacet": "0xd596C903d78870786c5DB0E448ce7F87A65A0daD", | |||
| "GenericSwapFacetV3": "0xE871874D8AC30E8aCD0eC67529b4a5dDD73Bf0d6" | |||
There was a problem hiding this comment.
💡 Codebase verification
Version mismatch detected in deployed contract
The deployed GenericSwapFacetV3 contract has version 1.0.1 (as shown in the source code comment @custom:version 1.0.1), which does not match the expected version v1.0.0 mentioned in the PR title.
🔗 Analysis chain
Verify contract deployment and version
Please verify that the deployed GenericSwapFacetV3 contract at 0xE871874D8AC30E8aCD0eC67529b4a5dDD73Bf0d6 matches the expected version (v1.0.0) mentioned in the PR title.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract deployment and version
# Check if contract is deployed and verify its bytecode
echo "Checking contract deployment..."
cast code 0xE871874D8AC30E8aCD0eC67529b4a5dDD73Bf0d6 --rpc-url https://bsc-testnet.public.blastapi.io
# Search for version string in contract source
echo "Checking contract version..."
rg -l "1\.0\.0" src/
Length of output: 1485
Script:
#!/bin/bash
# Let's check the GenericSwapFacetV3 contract source and its version
echo "Searching for GenericSwapFacetV3 contract..."
fd "GenericSwapFacetV3"
echo "Checking version in GenericSwapFacetV3..."
rg -A 5 "contract GenericSwapFacetV3"
# Check if there are any version-related constants or variables
echo "Checking for version constants..."
rg "VERSION|version" --type solidity
Length of output: 2015
Script:
#!/bin/bash
# Let's check the contract source and deployment scripts for version information
echo "Checking GenericSwapFacetV3 source and related files..."
rg -p "version|VERSION" src/Facets/GenericSwapFacetV3.sol script/deploy/facets/DeployGenericSwapFacetV3.s.sol script/deploy/facets/UpdateGenericSwapFacetV3.s.sol
# Let's also check the deployment script content to understand deployment configuration
echo "Checking deployment script content..."
cat script/deploy/facets/DeployGenericSwapFacetV3.s.sol
Length of output: 1732
| /// @notice Performs multiple swaps in one transaction, starting with ERC20 and ending with native | ||
| /// @param _receiver the address to receive the swapped tokens into (also excess tokens) | ||
| /// @param _minAmountOut the minimum amount of the final asset to receive | ||
| /// @param _swapData an object containing swap related data to perform swaps before bridging | ||
| function swapTokensMultipleV3ERC20ToAny( | ||
| address payable _receiver, | ||
| uint256 _minAmountOut, | ||
| LibSwap.SwapData[] calldata _swapData | ||
| ) | ||
|
|
There was a problem hiding this comment.
Incorrect documentation for swapTokensMultipleV3ERC20ToAny
The NatSpec comment states "ending with native" but the function name and description suggest it can end with any token type (ERC20 or native).
- /// @notice Performs multiple swaps in one transaction, starting with ERC20 and ending with native
+ /// @notice Performs multiple swaps in one transaction, starting with ERC20 and ending with any token (ERC20 or native)📝 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.
| /// @notice Performs multiple swaps in one transaction, starting with ERC20 and ending with native | |
| /// @param _receiver the address to receive the swapped tokens into (also excess tokens) | |
| /// @param _minAmountOut the minimum amount of the final asset to receive | |
| /// @param _swapData an object containing swap related data to perform swaps before bridging | |
| function swapTokensMultipleV3ERC20ToAny( | |
| address payable _receiver, | |
| uint256 _minAmountOut, | |
| LibSwap.SwapData[] calldata _swapData | |
| ) | |
| /// @notice Performs multiple swaps in one transaction, starting with ERC20 and ending with any token (ERC20 or native) | |
| /// @param _receiver the address to receive the swapped tokens into (also excess tokens) | |
| /// @param _minAmountOut the minimum amount of the final asset to receive | |
| /// @param _swapData an object containing swap related data to perform swaps before bridging | |
| function swapTokensMultipleV3ERC20ToAny( | |
| address payable _receiver, | |
| uint256 _minAmountOut, | |
| LibSwap.SwapData[] calldata _swapData | |
| ) |
| "blast": "0x0000000000000000000000000000000000000000", | ||
| "boba": "0x0000000000000000000000000000000000000000", | ||
| "bsc": "0x0000000000000000000000000000000000000000", | ||
| "celo": "0x471EcE3750Da237f93B8E339c536989b8978a438and", |
There was a problem hiding this comment.
Fix typo in Celo native address
The Celo native address has an erroneous "and" suffix which would make it an invalid address.
- "celo": "0x471EcE3750Da237f93B8E339c536989b8978a438and",
+ "celo": "0x471EcE3750Da237f93B8E339c536989b8978a438"📝 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.
| "celo": "0x471EcE3750Da237f93B8E339c536989b8978a438and", | |
| "celo": "0x471EcE3750Da237f93B8E339c536989b8978a438", |
No description provided.