LF-13168 GasZipPeriphery - LDA decoupling [WhitelistManagerFacet v1.0.3,IWhitelistManagerFacet v1.0.0,LibAllowList v1.0.1,GasZipPeriphery v1.0.2] #1137
Conversation
WalkthroughThis change set replaces the "DexManagerFacet" with a more generalized "WhitelistManagerFacet" throughout the codebase, configuration, deployment scripts, and documentation. All references to DEX-specific whitelisting are updated to generic address whitelisting. Associated function names, selectors, and event names are changed, and test contracts are refactored accordingly. The allowlist library is enhanced to track selectors explicitly, and new documentation and deployment scripts are introduced for the WhitelistManagerFacet. Changes
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 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 (2)
✨ Finishing Touches🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
test/solidity/Periphery/GasZipPeriphery.t.sol (1)
260-267:⚠️ Potential issue
vm.stopPrank()is executed without a correspondingvm.startPrankin this testInside
test_canCollectERC20FeesThenSwapToERC20ThenDepositThenBridge, no prank is started beforevm.stopPrank()is called (the earliervm.startPrankin a different test doesn’t carry over).
Foundry reverts whenvm.stopPrank()is invoked outside a prank context, so this statement will make the whole test fail.- vm.stopPrank();Simply delete the line or add a matching
vm.startPrank(...)beforehand if an impersonation is actually required.
🧹 Nitpick comments (7)
package.json (1)
108-115: Droppingsolhintfrom pre-commit removes an important safety net
solhintcatches issues (re-entrancy patterns, naming, visibility, etc.) that Prettier/Eslint cannot. Eliminating it from the*.solstage means new Solidity code can bypass static-analysis until CI, slowing feedback and increasing merge-time churn.Consider restoring a lightweight invocation instead of the previous complex bash wrapper, e.g.:
- "*.sol": [ - "prettier --write"] + "*.sol": [ + "prettier --write", + "solhint --max-warnings 0" + ]src/Facets/DexManagerFacet.sol (1)
153-161: Expose the view asexternalto save gasDeclaring
isContractApprovedasexternal viewis ~20-100 gas cheaper thanpublic viewfor external callers because the compiler avoids a redundant copy of calldata to memory.-function isContractApproved( - address _contract -) public view returns (bool approved) { +function isContractApproved( + address _contract +) external view returns (bool approved) {src/Periphery/GasZipPeriphery.sol (2)
70-75: Approving the exactfromAmountbreaks on subsequent callsIf a user calls twice with the same token & DEX, the second call reverts (
allowance < fromAmount). Approve max uint256 once to save gas on future swaps:-LibAsset.maxApproveERC20( - IERC20(_swapData.sendingAssetId), - _swapData.callTo, - _swapData.fromAmount -); +LibAsset.maxApproveERC20( + IERC20(_swapData.sendingAssetId), + _swapData.callTo, + type(uint256).max +);
137-140: Packing 8-bit IDs into 16-bit slots wastes 8 bits eachShifting by 16 means every second byte is zero, halving the theoretical max chain count to 16. If the extra byte is reserved for future flags, document it; otherwise continue shifting by 8 and keep the previous 32-ID capacity.
test/solidity/Periphery/GasZipPeriphery.t.sol (3)
56-66: Redundant deployment ofDexManagerFacetand shadowing of the variableYou deploy a fresh
DexManagerFacet, add it to the diamond, and then re-assigndexManagerFacettoDexManagerFacet(address(diamond)).
While functionally harmless, the shadowing:
- Makes the original standalone instance unreachable (wasting gas in CI / fork tests).
- Can confuse readers who expect
dexManagerFacetto point to the standalone contract.If you only need the facet through the diamond, deploy it to obtain the code address, call
addFacet, and keep a separate variable name for the diamond-cast instance, e.g.:- dexManagerFacet = new DexManagerFacet(); + DexManagerFacet _dexManager = new DexManagerFacet(); ... - addFacet(diamond, address(dexManagerFacet), functionSelectors); - dexManagerFacet = DexManagerFacet(address(diamond)); + addFacet(diamond, address(_dexManager), functionSelectors); + dexManagerFacet = DexManagerFacet(address(diamond));This avoids ambiguity and makes the intention explicit.
72-80: Left-overconsole.logdebugging statements clutter test output
console.logis great during development, but leaving dozens of prints (e.g.,"here", contract addresses, owner address):
- Spams CI logs, hiding meaningful revert traces.
- Slightly increases bytecode & deployment gas for the test contract.
Consider removing or guarding them behind a verbose flag:
- console.log("uniswap", address(uniswap)); - console.log("gasZipPeriphery", address(gasZipPeriphery)); - console.log("feeCollector", address(feeCollector)); ... - console.log("here"); - console.log("swap data callTo uniswap", address(uniswap)); ... - console.log("here2"); ... - console.log("here3"); - console.log("USER_DIAMOND_OWNER"); - console.log(USER_DIAMOND_OWNER);Also applies to: 242-249, 268-279
445-476: Commented-out whitelist code makes the test intention unclearThe revert-test intentionally does not whitelist
PROCESS_ROUTE_SELECTOR, but the commented block still sits here.
Future maintainers may uncomment it, accidentally turning the test green for the wrong reason.Either delete the dead code or move it into a dedicated helper with an explanatory comment such as:
// Intentionally NOT whitelisting PROCESS_ROUTE_SELECTOR to assert // that GasZipPeriphery reverts when an unapproved function is supplied.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.json(1 hunks)src/Facets/DexManagerFacet.sol(1 hunks)src/Periphery/GasZipPeriphery.sol(3 hunks)test/solidity/Periphery/GasZipPeriphery.t.sol(12 hunks)
🧰 Additional context used
🧠 Learnings (2)
src/Periphery/GasZipPeriphery.sol (4)
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:57-62
Timestamp: 2024-10-09T03:47:21.269Z
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.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:77-77
Timestamp: 2024-10-22T03:17:36.803Z
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:0-0
Timestamp: 2024-09-23T02:10:40.494Z
Learning: In the `depositToGasZipERC20` function, it's necessary to call `GasZipPeriphery(payable(address(this))).depositToGasZipNative{...}` to specify `msg.value` after the swap, as calling the internal function directly does not allow setting `msg.value`.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:0-0
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `depositToGasZipERC20` function, it's necessary to call `GasZipPeriphery(payable(address(this))).depositToGasZipNative{...}` to specify `msg.value` after the swap, as calling the internal function directly does not allow setting `msg.value`.
test/solidity/Periphery/GasZipPeriphery.t.sol (2)
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:4-14
Timestamp: 2024-10-09T03:47:21.269Z
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:57-62
Timestamp: 2024-10-09T03:47:21.269Z
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.
🪛 GitHub Check: Olympix Integrated Security
src/Periphery/GasZipPeriphery.sol
[notice] 40-40:
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor
[notice] 80-80:
Making an external call without a gas budget may consume all of the transaction's gas, causing it to revert. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/call-without-gas-budget
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
🔇 Additional comments (1)
src/Periphery/GasZipPeriphery.sol (1)
118-122: Gas-wastingconsole.logand implicit trust inremainingNativeBalanceSame debug issue; additionally, returning all leftover ETH to
msg.sendermay break multi-call flows where the following step expects that balance. Consider forwarding balance only after the entire LiFi pipeline finishes or expose a pull-pattern withdrawal.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
test/solidity/Periphery/GasZipPeriphery.t.sol (1)
489-496: 🛠️ Refactor suggestionUpdate test to reflect new MAX_CHAINID_LENGTH_ALLOWED value
The maximum allowed chain ID length was changed from 32 to 16 in the main contract, but this test is still testing with 33 chain IDs. The test should be updated to use 17 chain IDs instead.
- uint8[] memory chainIds = new uint8[](33); + uint8[] memory chainIds = new uint8[](17);
♻️ Duplicate comments (8)
src/Periphery/GasZipPeriphery.sol (7)
38-44: Consider adding zero-address validation in constructorThe constructor assigns critical address parameters without validation, which could lead to contract being bricked if zero addresses are passed.
Based on learnings from past reviews, I see this may be intentional due to your deployment flow ensuring valid parameters. If that's not the case, consider adding validation:
constructor( address _gasZipRouter, address _liFiDiamond, address _owner ) WithdrawablePeriphery(_owner) { + if (_gasZipRouter == address(0) || _liFiDiamond == address(0)) { + revert InvalidCallData(); + } GAS_ZIP_ROUTER = IGasZip(_gasZipRouter); LIFI_DIAMOND = _liFiDiamond; }🧰 Tools
🪛 GitHub Check: Olympix Integrated Security
[notice] 40-40:
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor
91-93: Consider adding a gas limit to the external callThe low-level call to
_swapData.callTodoes not specify a gas limit, which could consume all available gas and cause the transaction to revert unexpectedly.- (bool success, bytes memory res) = _swapData.callTo.call( + (bool success, bytes memory res) = _swapData.callTo.call{gas: gasBudget}( _swapData.callData );However, based on past learnings, I understand that in your codebase it may be intentional to let the call fail with all gas as a way to save gas on explicit error handling.
🧰 Tools
🪛 GitHub Check: Olympix Integrated Security
[notice] 91-91:
Making an external call without a gas budget may consume all of the transaction's gas, causing it to revert. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/call-without-gas-budget
11-13:⚠️ Potential issueRemove debug import and console.log statements before deployment
The
console.solimport should be removed from production code as it injectsrevertopcodes that will remain in the deployed bytecode, increasing contract size and gas costs.-import { console } from "forge-std/console.sol";
14-21: 🛠️ Refactor suggestionRelocate interface to dedicated file
The
IDexManagerinterface should be moved to a dedicated file in the Interfaces directory to maintain a single source of truth and prevent inconsistencies when multiple contracts need this interface.-interface IDexManager { - function isFunctionApproved( - bytes4 _signature - ) external view returns (bool); - function isContractApproved( - address _contract - ) external view returns (bool); -}Create a new file at
src/Interfaces/IDexManager.solwith the interface definition, then import it here.
88-102: 🛠️ Refactor suggestionRemove console logs and consider a safer approach for swap output detection
Remove all debug console logs. Additionally, the unchecked
abi.decode(res, (uint256))assumes a specific return format and could revert if a different DEX returns a different format.- console.log("1111"); // execute swap using the whitelisted DEX // solhint-disable-next-line avoid-low-level-calls + uint256 preBal = address(this).balance; (bool success, bytes memory res) = _swapData.callTo.call( _swapData.callData ); - console.log("2222"); if (!success) { LibUtil.revertWith(res); } - console.log("3333"); // extract the swap output amount from the call return value - uint256 swapOutputAmount = abi.decode(res, (uint256)); + uint256 swapOutputAmount = address(this).balance - preBal; - console.log("4444");This approach is more protocol-agnostic and doesn't depend on specific return data formats.
🧰 Tools
🪛 GitHub Check: Olympix Integrated Security
[notice] 91-91:
Making an external call without a gas budget may consume all of the transaction's gas, causing it to revert. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/call-without-gas-budget
71-76:⚠️ Potential issueUpdate selector extraction in approval check condition
The condition check needs to use the correct syntax for extracting the function selector.
if ( !dexManager.isContractApproved(_swapData.callTo) || - !dexManager.isFunctionApproved(bytes4(_swapData.callData[:4])) + !dexManager.isFunctionApproved(selector) ) { revert ContractCallNotAllowed(); }
60-70:⚠️ Potential issueRemove console logging and fix invalid bytes slicing syntax
The syntax
bytes4(_swapData.callData[:4])is not valid Solidity and will not compile. Also, remove all debugconsole.logstatements before deployment.- console.log("callTo", _swapData.callTo); - console.log("selector"); - console.logBytes4(bytes4(_swapData.callData[:4])); - console.log( - "contract approved?", - dexManager.isContractApproved(_swapData.callTo) - ); - console.log( - "function approved?", - dexManager.isFunctionApproved(bytes4(_swapData.callData[:4])) - ); + // Extract function selector using assembly + bytes4 selector; + assembly { + selector := shr(224, mload(add(_swapData.callData, 32))) + }test/solidity/Periphery/GasZipPeriphery.t.sol (1)
529-531:⚠️ Potential issueFix incorrect variable in setFacetAddressInTestBase
The function is using the state variable
gnosisBridgeFacetinstead of the local variable_gnosisBridgeFacetcreated in the function. This is likely to cause issues asgnosisBridgeFacetmight not be initialized yet.- setFacetAddressInTestBase(address(gnosisBridgeFacet), "GnosisFacet"); + setFacetAddressInTestBase(address(_gnosisBridgeFacet), "GnosisFacet");
🧹 Nitpick comments (3)
test/solidity/Periphery/GasZipPeriphery.t.sol (3)
60-113: Consider extracting whitelist setup to a helper functionThe whitelist setup for DEXs and function selectors is quite lengthy. Consider extracting this logic to a helper function to improve readability and maintainability.
+ function _setupWhitelists() internal { + // whitelist DEXs / Periphery contracts + dexManagerFacet.addDex(address(uniswap)); + dexManagerFacet.addDex(address(gasZipPeriphery)); + dexManagerFacet.addDex(address(feeCollector)); + + // add function selectors for GasZipPeriphery + dexManagerFacet.setFunctionApprovalBySignature( + gasZipPeriphery.depositToGasZipERC20.selector, + true + ); + dexManagerFacet.setFunctionApprovalBySignature( + gasZipPeriphery.depositToGasZipNative.selector, + true + ); + + // add function selectors for FeeCollector + dexManagerFacet.setFunctionApprovalBySignature( + feeCollector.collectTokenFees.selector, + true + ); + + // add function selectors for Uniswap + dexManagerFacet.setFunctionApprovalBySignature( + uniswap.swapExactTokensForTokens.selector, + true + ); + dexManagerFacet.setFunctionApprovalBySignature( + uniswap.swapExactTokensForETH.selector, + true + ); + dexManagerFacet.setFunctionApprovalBySignature( + uniswap.swapETHForExactTokens.selector, + true + ); + dexManagerFacet.setFunctionApprovalBySignature( + uniswap.swapExactETHForTokens.selector, + true + ); + }And then call it in setUp():
dexManagerFacet = DexManagerFacet(address(diamond)); - // whitelist DEXs / Periphery contracts - dexManagerFacet.addDex(address(uniswap)); - dexManagerFacet.addDex(address(gasZipPeriphery)); - dexManagerFacet.addDex(address(feeCollector)); - - console.log("uniswap", address(uniswap)); - console.log("gasZipPeriphery", address(gasZipPeriphery)); - console.log("feeCollector", address(feeCollector)); + _setupWhitelists(); + + // Debug logging + console.log("uniswap", address(uniswap)); + console.log("gasZipPeriphery", address(gasZipPeriphery)); + console.log("feeCollector", address(feeCollector));
243-249: Remove debug console logs in testsThe test file contains numerous debug console logs that should be removed or replaced with more meaningful assertions to improve test clarity and performance.
- console.log("here"); - console.log("swap data callTo uniswap", address(uniswap)); - console.log( - "swap data callTo gasZipPeriphery", - address(gasZipPeriphery) - );And similar patterns throughout the test file.
Also applies to: 268-279, 314-315
455-458: Remove or uncomment the commented codeThis section contains commented-out code that should either be removed entirely or uncommented if it's needed for the tests.
- // Add DEX and selector to whitelist - // vm.startPrank(USER_DIAMOND_OWNER); - // dexManagerFacet.addDex(LIFI_DEX_AGGREGATOR_MAINNET); - // dexManagerFacet.setFunctionApprovalBySignature(PROCESS_ROUTE_SELECTOR, true); - // vm.stopPrank();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Periphery/GasZipPeriphery.sol(3 hunks)test/solidity/Periphery/GasZipPeriphery.t.sol(13 hunks)
🧰 Additional context used
🧠 Learnings (2)
src/Periphery/GasZipPeriphery.sol (5)
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:29-32
Timestamp: 2024-09-23T01:48:39.325Z
Learning: In our codebase, input validation for zero addresses in constructor parameters is not required because our deployment flow ensures that contracts cannot be deployed with zero address parameters unless we are okay with it.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:57-62
Timestamp: 2024-10-09T03:47:21.269Z
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.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:77-77
Timestamp: 2024-10-22T03:17:36.803Z
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:0-0
Timestamp: 2024-09-23T02:10:40.494Z
Learning: In the `depositToGasZipERC20` function, it's necessary to call `GasZipPeriphery(payable(address(this))).depositToGasZipNative{...}` to specify `msg.value` after the swap, as calling the internal function directly does not allow setting `msg.value`.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:0-0
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `depositToGasZipERC20` function, it's necessary to call `GasZipPeriphery(payable(address(this))).depositToGasZipNative{...}` to specify `msg.value` after the swap, as calling the internal function directly does not allow setting `msg.value`.
test/solidity/Periphery/GasZipPeriphery.t.sol (4)
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:4-14
Timestamp: 2024-10-09T03:47:21.269Z
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:0-0
Timestamp: 2024-09-23T02:10:40.494Z
Learning: In the `depositToGasZipERC20` function, it's necessary to call `GasZipPeriphery(payable(address(this))).depositToGasZipNative{...}` to specify `msg.value` after the swap, as calling the internal function directly does not allow setting `msg.value`.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:0-0
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `depositToGasZipERC20` function, it's necessary to call `GasZipPeriphery(payable(address(this))).depositToGasZipNative{...}` to specify `msg.value` after the swap, as calling the internal function directly does not allow setting `msg.value`.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:57-62
Timestamp: 2024-10-09T03:47:21.269Z
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.
🪛 GitHub Check: Olympix Integrated Security
src/Periphery/GasZipPeriphery.sol
[notice] 40-40:
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor
[notice] 91-91:
Making an external call without a gas budget may consume all of the transaction's gas, causing it to revert. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/call-without-gas-budget
🔇 Additional comments (2)
src/Periphery/GasZipPeriphery.sol (1)
147-151:❓ Verification inconclusive
Verify impact of bit-shift change
The bit-shift operation for chain IDs was changed from 8 to 16 bits, which will result in different destination chain values and could break compatibility with existing calls or stored data.
Make sure that this change is synchronized with the Gas.zip protocol expectations and any dependent systems.
🏁 Script executed:
#!/bin/bash # Search for any usages of getDestinationChainsValue in the codebase rg "getDestinationChainsValue" --type solidityLength of output: 79
🏁 Script executed:
#!/bin/bash # Search for any references to getDestinationChainsValue in .sol files rg "getDestinationChainsValue" -g "*.sol" -n # Search for all occurrences of destinationChains to locate any decoding/unpacking logic rg "destinationChains" -g "*.sol" -nLength of output: 3175
🏁 Script executed:
#!/bin/bash # Locate the definition of defaultDestinationChains constant or variable rg "defaultDestinationChains" -g "*.sol" -nLength of output: 776
Ensure 16-bit shift change is synchronized with external systems
The internal implementations and tests for
getDestinationChainsValuehave been updated to use a 16-bit shift:
- src/Periphery/GasZipPeriphery.sol lines 142–150
- src/Facets/GasZipFacet.sol lines 134–142
- Tests updated in
- test/solidity/Periphery/GasZipPeriphery.t.sol
- test/solidity/Facets/GasZipFacet.t.sol
Please confirm that the Gas.zip protocol specification, deployed contracts, off-chain clients, and any stored data formats are all compatible with this new 16-bit spacing between chain IDs.
test/solidity/Periphery/GasZipPeriphery.t.sol (1)
534-568: Approve the updated implementation of _getUniswapERC20ToNativeSwapDataThe function has been refactored to directly use Uniswap instead of LiFiDEXAggregator, which aligns with the new architecture in GasZipPeriphery. This change is appropriate and helps decouple the periphery from LiFiDEXAggregator.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
src/Periphery/GasZipPeriphery.sol (6)
13-20: Consider moving the IDexManager interface to a dedicated file.The interface is defined locally but would be better placed in a dedicated file under the Interfaces directory for better reusability and maintainability.
Consider moving this interface to
src/Interfaces/IDexManager.sol:-interface IDexManager { - function isFunctionApproved( - bytes4 _signature - ) external view returns (bool); - function isContractApproved( - address _contract - ) external view returns (bool); -}
29-31: State variables renamed to uppercase and MAX_CHAINID_LENGTH_ALLOWED reduced.Variable naming is now more consistent with constants, and MAX_CHAINID_LENGTH_ALLOWED has been reduced from 32 to 16.
Ensure that reducing MAX_CHAINID_LENGTH_ALLOWED from 32 to 16 doesn't break existing functionality. This change should be consistent with other contracts using this constant.
#!/bin/bash # Check if other files use different values for MAX_CHAINID_LENGTH_ALLOWED rg -A 1 -B 1 "MAX_CHAINID_LENGTH_ALLOWED" --type solidity
38-44: Constructor parameters not validated for zero address.The constructor still doesn't validate if the input addresses are zero addresses.
Based on retrieved learnings, this is intentional - the deployment flow ensures that contracts cannot be deployed with zero address parameters unless explicitly intended.
🧰 Tools
🪛 GitHub Check: Olympix Integrated Security
[notice] 40-40:
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor
82-89: Direct DEX call without gas limit could potentially consume all gas.The low-level call to the DEX doesn't specify a gas limit.
Based on retrieved learnings, it's acceptable to let low-level calls fail without explicit gas limits, as failing the entire transaction is sufficient and saves gas.
🧰 Tools
🪛 GitHub Check: Olympix Integrated Security
[notice] 84-84:
Making an external call without a gas budget may consume all of the transaction's gas, causing it to revert. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/call-without-gas-budget
56-70: 🛠️ Refactor suggestionProper validation of swap parameters before execution.
The code now correctly verifies:
- The receiving asset must be native ETH (address(0))
- Both the contract and function signature must be approved through the DexManager
This verification follows security best practices by validating inputs early.
However, note that the syntax for extracting the function selector
bytes4(_swapData.callData[:4])may not be valid in all Solidity versions.Consider using assembly to extract the function selector:
-!dexManager.isFunctionApproved(bytes4(_swapData.callData[:4])) +bytes4 selector; +assembly { + selector := shr(224, mload(add(_swapData.callData, 32))) +} +!dexManager.isFunctionApproved(selector)
133-137: 💡 Verification agent🧩 Analysis chain
Bit-shifting logic updated in getDestinationChainsValue.
The shift operation was changed from 8 bits to 16 bits, which affects how chain IDs are encoded.
This change, along with reducing MAX_CHAINID_LENGTH_ALLOWED from 32 to 16, means:
- Each chain ID now occupies 16 bits instead of 8
- The maximum number of chain IDs is reduced from 32 to 16
- This allows for larger chain ID values (up to 2^16-1 instead of 2^8-1)
Verify that all dependent code has been updated to handle this change, particularly any code that decodes these values.
🏁 Script executed:
#!/bin/bash # Check for usages of getDestinationChainsValue to ensure they handle the new encoding rg "getDestinationChainsValue" --type solidityLength of output: 79
🏁 Script executed:
#!/bin/bash # Search for getDestinationChainsValue references in all Solidity files rg -n "getDestinationChainsValue" --glob '*.sol'Length of output: 1698
🏁 Script executed:
#!/bin/bash # Display getDestinationChainsValue implementations echo "=== src/Periphery/GasZipPeriphery.sol (lines 100-200) ===" sed -n '100,200p' src/Periphery/GasZipPeriphery.sol echo "" echo "=== src/Facets/GasZipFacet.sol (lines 100-200) ===" sed -n '100,200p' src/Facets/GasZipFacet.solLength of output: 4437
🏁 Script executed:
#!/bin/bash # Display test cases around getDestinationChainsValue in Periphery tests echo "=== test/solidity/Periphery/GasZipPeriphery.t.sol (lines 350-420) ===" sed -n '350,420p' test/solidity/Periphery/GasZipPeriphery.t.sol echo "" echo "=== test/solidity/Facets/GasZipFacet.t.sol (lines 370-440) ===" sed -n '370,440p' test/solidity/Facets/GasZipFacet.t.solLength of output: 5108
#!/bin/bash # Find definition of MAX_CHAINID_LENGTH_ALLOWED in Solidity files rg -n "MAX_CHAINID_LENGTH_ALLOWED" --glob '*.sol' # Show surrounding lines for context rg -n "MAX_CHAINID_LENGTH_ALLOWED" --glob 'src/**/*.sol' -C2
#!/bin/bash # Locate MAX_CHAINID_LENGTH_ALLOWED definitions in Solidity files rg -n "MAX_CHAINID_LENGTH_ALLOWED" -n src --glob '*.sol' -C2
🏁 Script executed:
#!/bin/bash # Locate MAX_CHAINID_LENGTH_ALLOWED to verify its value rg -n "MAX_CHAINID_LENGTH_ALLOWED" --glob '*.sol'Length of output: 461
Inconsistent MAX_CHAINID_LENGTH_ALLOWED in GasZipFacet.sol
The periphery contract limits to 16 chain IDs (
MAX_CHAINID_LENGTH_ALLOWED = 16), but the facet still allows 32. This mismatch can lead to unexpected behavior when encoding/decoding.Please update and align both implementations:
- src/Facets/GasZipFacet.sol (line 28): change
- uint256 internal constant MAX_CHAINID_LENGTH_ALLOWED = 32;
- uint256 internal constant MAX_CHAINID_LENGTH_ALLOWED = 16;
- Add a test in
test/solidity/Facets/GasZipFacet.t.solto assert that calls revert withTooManyChainIds()when_chainIds.length > 16.test/solidity/Periphery/GasZipPeriphery.t.sol (1)
583-586:⚠️ Potential issueIncorrect variable in setFacetAddressInTestBase.
The test is using
gnosisBridgeFacetinstead of_gnosisBridgeFacet. This could cause lookup failures.Use the local variable instead of the uninitialized state variable:
-setFacetAddressInTestBase(address(gnosisBridgeFacet), "GnosisFacet"); +setFacetAddressInTestBase(address(_gnosisBridgeFacet), "GnosisFacet");
🧹 Nitpick comments (1)
test/solidity/Periphery/GasZipPeriphery.t.sol (1)
438-442: Commented out legacy code.The commented code shows the transition from direct DEX whitelisting to the new DexManagerFacet approach. Consider removing these comments if they're no longer needed.
Remove commented-out code to improve readability:
- // Add DEX and selector to whitelist - // vm.startPrank(USER_DIAMOND_OWNER); - // dexManagerFacet.addDex(LIFI_DEX_AGGREGATOR_MAINNET); - // dexManagerFacet.setFunctionApprovalBySignature(PROCESS_ROUTE_SELECTOR, true); - // vm.stopPrank();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Periphery/GasZipPeriphery.sol(3 hunks)test/solidity/Periphery/GasZipPeriphery.t.sol(12 hunks)
🧰 Additional context used
🧠 Learnings (2)
src/Periphery/GasZipPeriphery.sol (3)
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:29-32
Timestamp: 2024-09-23T01:48:39.325Z
Learning: In our codebase, input validation for zero addresses in constructor parameters is not required because our deployment flow ensures that contracts cannot be deployed with zero address parameters unless we are okay with it.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:57-62
Timestamp: 2024-10-09T03:47:21.269Z
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.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:77-77
Timestamp: 2024-10-22T03:17:36.803Z
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.
test/solidity/Periphery/GasZipPeriphery.t.sol (2)
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:57-62
Timestamp: 2024-10-09T03:47:21.269Z
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.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:212-213
Timestamp: 2024-10-22T03:16:28.754Z
Learning: In the `GasZipFacetTest`, for the test case `testBase_Revert_SwapAndBridgeWithInvalidSwapData()`, a generic revert is expected, so `vm.expectRevert();` without specifying the expected error is appropriate.
🪛 GitHub Check: Olympix Integrated Security
src/Periphery/GasZipPeriphery.sol
[notice] 40-40:
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor
[notice] 84-84:
Making an external call without a gas budget may consume all of the transaction's gas, causing it to revert. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/call-without-gas-budget
🔇 Additional comments (21)
src/Periphery/GasZipPeriphery.sol (6)
11-12: Import statement update correctly includes ContractCallNotAllowed error.The import statement has been properly updated to include the needed errors for the new contract validation logic.
24-24: Version number correctly updated to 1.0.2.The custom version annotation has been properly updated, reflecting the changes made to the contract.
35-35: Added appropriate ERC20ReceivedInsteadOfNative error.The new error is used in the validation logic to ensure that only native ETH is received from swaps.
76-80: Approval target changed to direct contract call.The approval target is now correctly set to the DEX contract instead of an aggregator.
91-92: Improved output amount determination from contract balance.Using the contract's balance after the swap is a more robust approach than decoding the return data, as it works across different DEX implementations that might return data in different formats.
111-114: State variable reference updated to uppercase style.The code now uses the renamed GAS_ZIP_ROUTER constant, maintaining consistency with the naming changes.
test/solidity/Periphery/GasZipPeriphery.t.sol (15)
12-12: Added import for DexManagerFacet.The import is correctly added to support the new facet required for the refactored contract architecture.
23-24: Updated contract instance declaration.Now instantiating GasZipPeriphery directly and adding DexManagerFacet, which is consistent with the architectural changes.
31-31: Added selector constant for tests.The constant for the process route selector is useful for test validation.
37-37: Added SwapFailed error for test mocks.This error is used in the new test cases to validate proper error handling.
43-66: Setup refactored to use DexManagerFacet.The test setup has been properly updated to:
- Deploy a new DexManagerFacet
- Configure function selectors
- Add the facet to the diamond
- Create an instance pointing to the diamond
This matches the architectural changes in the contract under test.
67-109: Comprehensive whitelisting setup.The test now correctly configures:
- Whitelisted DEXs (Uniswap, GasZipPeriphery, FeeCollector)
- Multiple function selectors for each contract
- Labels for easier debugging
This provides good test coverage for the various approval flows.
127-137: Constructor parameter test updated.The test correctly validates that the constructor parameters are stored properly, including the diamond address instead of the previous aggregator.
262-262: Helper function name updated to match implementation.The function name has been updated to reflect that it now uses Uniswap directly instead of the aggregator.
391-404: Updated expected values for getDestinationChainsValue.The test assertions have been updated to match the new bit-shifting logic (16 bits instead of 8).
405-427: Added test case for maximum chain IDs.A new test case has been added to verify the behavior with exactly 16 chain IDs, which is the new maximum allowed.
448-460: Updated swap failure test.The test now correctly uses a valid selector with invalid parameters to pass the whitelist check but fail during execution.
495-563: Added new test cases for validation logic.Two important new test cases have been added:
testRevert_WillFailIfReceivingAssetIsNotNative- Validates that swaps must receive native ETHtestRevert_WillFailWhenSwapOperationIsUnsuccessful- Tests the error handling when a swap failsThese tests provide good coverage for the new validation logic in the contract.
573-573: Fixed function selectors array size.The array size is correctly set to 2, matching the actual number of selectors being added.
588-622: Updated swap data helper function.The function has been correctly refactored to use Uniswap directly instead of the aggregator, including:
- Setting the correct target contract
- Using the appropriate Uniswap function selector
- Setting the receiving asset to native ETH
- Directing the output to the periphery
This matches the changes in the contract under test.
625-643: Added mock contract for testing swap failures.The mock contract is well-designed for testing:
- It implements the same function signature as Uniswap's swapExactTokensForETH
- It always reverts with a custom error
- It includes a fallback function for robustness
- It can receive ETH
This provides a good test case for swap failure handling.
mirooon
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
test/solidity/Periphery/GasZipPeriphery.t.sol (1)
472-480: 🛠️ Refactor suggestionTest name doesn't match implementation.
The test name
testRevert_WillFailIfMoreThan32ChainIdsstates it will test for more than 32 chain IDs, but the implementation now limits to 16 chain IDs. The test name should be updated to match.- function testRevert_WillFailIfMoreThan32ChainIds() public { + function testRevert_WillFailIfMoreThan16ChainIds() public {
♻️ Duplicate comments (2)
src/Periphery/GasZipPeriphery.sol (1)
13-20: 🛠️ Refactor suggestionConsider importing the new
IDexManagerFacetinterface instead of defining a local one.You've created a dedicated interface file
IDexManagerFacet.solbut still defined a localIDexManagerinterface here. To maintain a single source of truth and prevent drift between implementations, import and use the official interface.-interface IDexManager { - function isFunctionApproved( - bytes4 _signature - ) external view returns (bool); - function isContractApproved( - address _contract - ) external view returns (bool); -} +import { IDexManagerFacet } from "../Interfaces/IDexManagerFacet.sol";test/solidity/Periphery/GasZipPeriphery.t.sol (1)
615-615:⚠️ Potential issuePotential reference to incorrect variable in
setFacetAddressInTestBase.The code uses
gnosisBridgeFacetwhich is set after the function returns (line 44) instead of the local variable_gnosisBridgeFacet.- setFacetAddressInTestBase(address(gnosisBridgeFacet), "GnosisFacet"); + setFacetAddressInTestBase(address(_gnosisBridgeFacet), "GnosisFacet");
🧹 Nitpick comments (2)
test/solidity/Periphery/GasZipPeriphery.t.sol (2)
122-123: Redundant labeling for GasZipPeriphery address.The address is already labeled at line 73, making this second label redundant.
- vm.label(address(gasZipPeriphery), "GasZipPeriphery");
439-442: Commented-out code should be removed.These commented-out lines are no longer relevant with the new DexManager integration and should be removed entirely.
- // vm.startPrank(USER_DIAMOND_OWNER); - // dexManagerFacet.addDex(LIFI_DEX_AGGREGATOR_MAINNET); - // dexManagerFacet.setFunctionApprovalBySignature(PROCESS_ROUTE_SELECTOR, true); - // vm.stopPrank();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Facets/DexManagerFacet.sol(7 hunks)src/Interfaces/IDexManagerFacet.sol(1 hunks)src/Periphery/GasZipPeriphery.sol(3 hunks)test/solidity/Periphery/GasZipPeriphery.t.sol(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Facets/DexManagerFacet.sol
🧰 Additional context used
🧠 Learnings (2)
src/Periphery/GasZipPeriphery.sol (3)
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:29-32
Timestamp: 2024-09-23T01:48:39.325Z
Learning: In our codebase, input validation for zero addresses in constructor parameters is not required because our deployment flow ensures that contracts cannot be deployed with zero address parameters unless we are okay with it.
Learnt from: mirooon
PR: lifinance/contracts#1137
File: src/Periphery/GasZipPeriphery.sol:62-71
Timestamp: 2025-05-14T15:19:24.944Z
Learning: The syntax `bytes4(calldata_bytes[:4])` is valid in Solidity 0.8.x for extracting a function selector from calldata bytes.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:57-62
Timestamp: 2024-10-09T03:47:21.269Z
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.
test/solidity/Periphery/GasZipPeriphery.t.sol (3)
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:57-62
Timestamp: 2024-10-09T03:47:21.269Z
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.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:212-213
Timestamp: 2024-10-22T03:16:28.754Z
Learning: In the `GasZipFacetTest`, for the test case `testBase_Revert_SwapAndBridgeWithInvalidSwapData()`, a generic revert is expected, so `vm.expectRevert();` without specifying the expected error is appropriate.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:337-339
Timestamp: 2024-10-22T03:24:24.705Z
Learning: In `GasZipFacet` tests, when expecting a failure due to insufficient balance for sending value, the test should expect a generic revert without specifying a specific error.
🪛 GitHub Check: Olympix Integrated Security
src/Periphery/GasZipPeriphery.sol
[notice] 91-91:
Making an external call without a gas budget may consume all of the transaction's gas, causing it to revert. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/call-without-gas-budget
🔇 Additional comments (20)
src/Interfaces/IDexManagerFacet.sol (1)
1-70: Well-structured new interface with comprehensive documentation.The added
IDexManagerFacetinterface provides a clean contract for managing DEX approvals and function signatures. The documentation is thorough and the function signatures are clearly defined.src/Periphery/GasZipPeriphery.sol (6)
29-31: Reduced maximum chain ID length.The
MAX_CHAINID_LENGTH_ALLOWEDconstant was reduced from 32 to 16. This decreases the maximum number of chains that can be processed at once. Ensure this limit is sufficient for all use cases.
43-52: Constructor validation is a security improvement.Adding zero address checks in the constructor is a good security practice and ensures that essential dependencies can't be inadvertently set to invalid values.
69-77: DexManager checks enhance security.Checking both contract address and function signature approval through the DexManager interface provides a more robust and flexible security mechanism than the previous approach.
I notice you're using
bytes4(_swapData.callData[:4])for function selector extraction. While this syntax is valid in Solidity 0.8.17 as confirmed in previous reviews, you might consider adding a comment explaining this syntax choice for future maintainers.
89-96: Low-level call pattern is consistent with project conventions.The external call without a specific gas limit is noted by static analysis, but as confirmed by project learnings, this is an acceptable pattern in this codebase since failing the entire transaction is the desired behavior if the call fails.
🧰 Tools
🪛 GitHub Check: Olympix Integrated Security
[notice] 91-91:
Making an external call without a gas budget may consume all of the transaction's gas, causing it to revert. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/call-without-gas-budget
98-99: Improved swap output calculation.Determining the output amount from the contract's balance instead of decoding return data is more robust, especially when interacting with various DEXs that might return different data structures.
141-145: Update bit shift in getDestinationChainsValue to match new chain ID length.The shift value in
(destinationChains << 16)has been updated to 16 bits to match the 16-bit chain IDs. This change is correct and aligns with the reducedMAX_CHAINID_LENGTH_ALLOWEDconstant.test/solidity/Periphery/GasZipPeriphery.t.sol (13)
9-11: Updated imports for DexManagerFacet integration.The imports have been appropriately updated to include the DexManagerFacet and error types needed for the new tests.
23-24: State variable type updates for direct instantiation.The removal of
TestGasZipPeripheryin favor of directly instantiatingGasZipPeripherysimplifies the test setup and better reflects the production usage.
46-84: DexManagerFacet setup and whitelisting configuration.The setup code properly deploys and configures the DexManagerFacet, which is essential for the new approval checking mechanism in GasZipPeriphery.
85-108: Comprehensive function selector approvals.The test thoroughly configures all necessary function selectors for the Uniswap router and periphery contracts, ensuring the tests have the required permissions to execute properly.
262-262: Updated swap data generation for direct DEX interaction.The function call has been updated to use the new Uniswap-based swap data generation instead of the LiFiDEXAggregator approach, aligning with the implementation changes.
391-391: Updated assertion value for 16-bit shift.The expected value has been updated to match the new 16-bit shift per chain ID instead of the previous 8-bit shift. This is correct and matches the implementation change.
401-426: Updated assertions and added test for maximum chain count.The test has been updated with correct expected values for the new bit-shift algorithm, and a new test case with exactly the maximum allowed 16 chain IDs has been added, which is a good boundary check.
452-460: Updated test approach for swap failures.The test now uses a valid function selector with invalid parameters instead of a random hex string, making it a more realistic failure scenario.
495-523: Good test coverage for non-native receiving asset checks.This new test verifies that the contract correctly rejects ERC20-to-ERC20 swaps, enforcing that only swaps to native ETH are allowed with
depositToGasZipERC20.
525-563: Good test coverage for swap failure handling.This new test uses a mock contract to ensure that failures during swap execution are properly handled and propagated.
565-593: Comprehensive constructor validation tests.These tests verify that the constructor properly rejects zero addresses for all critical parameters.
618-652: Well-implemented swap data generation function.The new function that generates Uniswap swap data directly is well-structured and correctly configured to swap ERC20 tokens to native ETH.
655-673: Good mock implementation for testing swap failures.The mock contract is simple but effective for testing how the main contract handles swap failures.
🤖 GitHub Action: Security Alerts Review 🔍🟢 Dismissed Security Alerts with Comments 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: ✅ No unresolved security alerts! 🎉 |
lifi-action-bot
changed the title
Test Coverage ReportLine Coverage: 82.65% (2268 / 2744 lines) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Periphery/GasZipPeriphery.sol (1)
78-88: 💡 Verification agent🧩 Analysis chain
Improved swap execution and output calculation.
The contract now:
- Executes swaps directly through whitelisted DEX contracts
- Calculates swap output based on the contract balance after execution
This approach is more robust than decoding return data, as different DEXs may return data in various formats. It also properly passes errors up to the caller.
🏁 Script executed:
#!/bin/bash # Check for external calls without gas limit in other periphery contracts rg -A 3 -B 3 "\.call(" --type sol src/Periphery/Length of output: 115
🏁 Script executed:
#!/bin/bash # Re-check for low-level .call usages without gas limits in periphery contracts rg -A 3 -B 3 '\.call\(' --type sol src/Periphery/Length of output: 79
🏁 Script executed:
#!/bin/bash # Re-check for low-level .call usage across all files in src/Periphery without filtering by file type rg -A 3 -B 3 '\.call\(' src/Periphery/Length of output: 2108
Review external low-level calls without gas limits
We found several instances of external calls made via
.call(...)that forward all remaining gas, which can lead to reentrancy or unpredictable execution if the callee consumes excessive gas. Please review and tighten these calls by specifying an explicit gas limit (e.g..call{gas: GAS_LIMIT}(…)) or by using a SafeCall utility, and ensure error handling remains intact. If forwarding full gas is intentional, add in-code comments explaining the rationale.Affected locations:
- src/Periphery/GasZipPeriphery.sol (L78–88)
- src/Periphery/RelayerCelerIM.sol (call at
(success, ) = _callTo.call(_callData))- src/Periphery/LiFiDEXAggregator.sol (approve via
address(token).call(…))- src/Periphery/Permit2Proxy.sol (diamond call via
LIFI_DIAMOND.call(…))test/solidity/Periphery/GasZipPeriphery.t.sol (1)
613-613:⚠️ Potential issueBug: Incorrect variable in setFacetAddressInTestBase.
The code uses
gnosisBridgeFacet(state variable) instead of_gnosisBridgeFacet(local variable) in the call tosetFacetAddressInTestBase. At this point, the state variable is likely still uninitialized.- setFacetAddressInTestBase(address(gnosisBridgeFacet), "GnosisFacet"); + setFacetAddressInTestBase(address(_gnosisBridgeFacet), "GnosisFacet");
🧹 Nitpick comments (1)
test/solidity/Periphery/GasZipPeriphery.t.sol (1)
437-442: Removed unnecessary DEX aggregator configuration.Commented out code that's no longer needed due to the architecture change. Consider removing these comments entirely to avoid confusion.
- // Add DEX and selector to whitelist - // vm.startPrank(USER_DIAMOND_OWNER); - // dexManagerFacet.addDex(LIFI_DEX_AGGREGATOR_MAINNET); - // dexManagerFacet.setFunctionApprovalBySignature(PROCESS_ROUTE_SELECTOR, true); - // vm.stopPrank();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Periphery/GasZipPeriphery.sol(3 hunks)test/solidity/Periphery/GasZipPeriphery.t.sol(12 hunks)
🧰 Additional context used
🧠 Learnings (2)
src/Periphery/GasZipPeriphery.sol (6)
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:29-32
Timestamp: 2024-09-23T01:48:39.325Z
Learning: In our codebase, input validation for zero addresses in constructor parameters is not required because our deployment flow ensures that contracts cannot be deployed with zero address parameters unless we are okay with it.
Learnt from: mirooon
PR: lifinance/contracts#1137
File: src/Periphery/GasZipPeriphery.sol:62-71
Timestamp: 2025-05-14T15:19:24.944Z
Learning: The syntax `bytes4(calldata_bytes[:4])` is valid in Solidity 0.8.x for extracting a function selector from calldata bytes.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:57-62
Timestamp: 2024-10-09T03:47:21.269Z
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.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:77-77
Timestamp: 2024-10-22T03:17:36.803Z
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:0-0
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `depositToGasZipERC20` function, it's necessary to call `GasZipPeriphery(payable(address(this))).depositToGasZipNative{...}` to specify `msg.value` after the swap, as calling the internal function directly does not allow setting `msg.value`.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:0-0
Timestamp: 2024-09-23T02:10:40.494Z
Learning: In the `depositToGasZipERC20` function, it's necessary to call `GasZipPeriphery(payable(address(this))).depositToGasZipNative{...}` to specify `msg.value` after the swap, as calling the internal function directly does not allow setting `msg.value`.
test/solidity/Periphery/GasZipPeriphery.t.sol (3)
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:57-62
Timestamp: 2024-10-09T03:47:21.269Z
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.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:212-213
Timestamp: 2024-10-22T03:16:28.754Z
Learning: In the `GasZipFacetTest`, for the test case `testBase_Revert_SwapAndBridgeWithInvalidSwapData()`, a generic revert is expected, so `vm.expectRevert();` without specifying the expected error is appropriate.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: test/solidity/Facets/GasZipFacet.t.sol:337-339
Timestamp: 2024-10-22T03:24:24.705Z
Learning: In `GasZipFacet` tests, when expecting a failure due to insufficient balance for sending value, the test should expect a generic revert without specifying a specific error.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
🔇 Additional comments (21)
src/Periphery/GasZipPeriphery.sol (8)
6-6: Good addition of IDexManagerFacet interface import.This import enables decoupling from a direct aggregator dependency to a more flexible interface-based approach, improving modularity.
12-12: Appropriate error imports.The imported errors (InvalidCallData, ContractCallNotAllowed, InvalidConfig) support the new validation checks added to the contract.
16-16: Version increment reflects changes correctly.Version bump from presumed 1.0.1 to 1.0.2 properly indicates this is a backward-compatible update with new functionality.
27-27: Appropriate error for asset validation.The new SwapOutputMustBeNative error provides clear feedback when attempting to use non-native tokens as receiving assets.
30-44: Zero address validation improves contract security.The constructor now properly validates all input addresses, preventing initialization with invalid parameters that could brick the contract. Good practice to use a custom error instead of a generic require statement.
55-66: Good security improvement with multi-layered validation.The function now:
- Ensures the receiving asset is native ETH
- Verifies the target contract is approved via DexManager
- Confirms the function selector is allowed by DexManager
This provides stronger security guarantees compared to the previous implementation.
72-75: Direct token approval to the DEX contract.This change correctly approves tokens to the actual DEX contract that will execute the swap, rather than to an aggregator, which aligns with the new architecture.
130-133: Updated bit shift for destination chains calculation.The bit shift was changed from 8 to 16 bits per chain ID, which allows for larger chain IDs but reduces the maximum number of chains. This change is consistent with the reduction in MAX_CHAINID_LENGTH_ALLOWED from 32 to 16.
test/solidity/Periphery/GasZipPeriphery.t.sol (13)
9-10: Good addition of required imports.Adding DexManagerFacet and error imports supports the changes in the implementation contract and ensures tests appropriately verify the new functionality.
23-24: Simplified test contract structure.Removing the TestGasZipPeriphery subclass and directly using GasZipPeriphery makes the tests clearer and more reflective of actual usage.
46-66: Comprehensive facet deployment and configuration.The test now properly deploys and configures the DexManagerFacet, which is key to the new contract approval mechanism. Good pattern for setting up the Diamond architecture for testing.
68-108: Thorough whitelist configuration for testing.Tests correctly whitelist all necessary contracts (Uniswap, GasZipPeriphery, FeeCollector) and function selectors, ensuring a realistic test environment that covers all swap scenarios.
127-138: Updated constructor test with correct assertions.The test now properly verifies storage of the constructor parameters, with assertions matching the renamed state variables.
261-261: Updated helper function name for swap data.The helper function name change from
_getLiFiDEXAggregatorCalldataForERC20ToNativeSwapto_getUniswapERC20ToNativeSwapDataaccurately reflects the new implementation's direct DEX interaction pattern.
450-459: Updated swap failure test with clearer calldata.The test now uses a more explicit approach to force a swap failure, using a valid function selector but invalid parameters. This better tests the error handling in the contract.
494-521: Good test for non-native receiving asset validation.This new test correctly verifies that the contract rejects swaps where the receiving asset is not native ETH, confirming the new validation logic works as expected.
523-561: Comprehensive test for swap execution failures.The new test with MockFailingDexWithCustomError properly verifies that the contract handles swap execution failures correctly, passing the error from the DEX up to the caller.
563-591: Good coverage of constructor validation.These three tests verify that the constructor correctly rejects zero addresses for all parameters, ensuring the contract cannot be deployed in an invalid state.
601-601: Simplification of gnosisBridgeFacet setup.The test no longer needs to manage DEX and function approvals for this facet, as that's now handled centrally via DexManagerFacet.
616-650: Updated swap data helper to use Uniswap directly.The helper now constructs calldata for direct interaction with Uniswap rather than through an aggregator, which aligns with the new architecture. It correctly sets the receiving asset to address(0) for native ETH.
653-671: Well-structured mock contract for testing.The MockFailingDexWithCustomError contract is a clean implementation for testing how the main contract handles DEX failures. Good practice to include both the specific function and a fallback that revert with the same error.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Periphery/GasZipPeriphery.sol(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/Periphery/GasZipPeriphery.sol (6)
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:29-32
Timestamp: 2024-09-23T01:48:39.325Z
Learning: In our codebase, input validation for zero addresses in constructor parameters is not required because our deployment flow ensures that contracts cannot be deployed with zero address parameters unless we are okay with it.
Learnt from: mirooon
PR: lifinance/contracts#1137
File: src/Periphery/GasZipPeriphery.sol:62-71
Timestamp: 2025-05-14T15:19:24.944Z
Learning: The syntax `bytes4(calldata_bytes[:4])` is valid in Solidity 0.8.x for extracting a function selector from calldata bytes.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:57-62
Timestamp: 2024-10-09T03:47:21.269Z
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.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:77-77
Timestamp: 2024-10-22T03:17:36.803Z
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:0-0
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `depositToGasZipERC20` function, it's necessary to call `GasZipPeriphery(payable(address(this))).depositToGasZipNative{...}` to specify `msg.value` after the swap, as calling the internal function directly does not allow setting `msg.value`.
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:0-0
Timestamp: 2024-09-23T02:10:40.494Z
Learning: In the `depositToGasZipERC20` function, it's necessary to call `GasZipPeriphery(payable(address(this))).depositToGasZipNative{...}` to specify `msg.value` after the swap, as calling the internal function directly does not allow setting `msg.value`.
⏰ 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
🔇 Additional comments (12)
src/Periphery/GasZipPeriphery.sol (12)
6-6: Good addition of the IDexManagerFacet import.This interface import enables the key architectural change of this PR: replacing the fixed DEX aggregator with a flexible contract/function approval system through the DexManagerFacet.
12-12: Appropriate error imports.Good addition of relevant error imports to standardize error handling with the rest of the codebase.
27-27: Good addition of the SwapOutputMustBeNative error.This error provides a clear message when the wrong token output is configured, improving the developer experience.
35-41: Good constructor parameter validation.Adding zero-address checks to the constructor parameters prevents the contract from being deployed in an unusable state. This addresses a previously identified security concern.
55-57: Good validation for native token output.Ensuring the receiving asset is the native token is essential as GasZip only works with native tokens for gas distribution.
59-66: Improved security through whitelist checks.Replacing the fixed DEX aggregator with a flexible whitelist mechanism improves security by:
- Ensuring only approved contracts can be called
- Ensuring only approved function selectors can be executed
This is a significant security improvement that provides greater flexibility without compromising security.
72-75: Correctly updated approval target.The approval target has been changed from the DEX aggregator to the direct swap contract (
_swapData.callTo), which aligns with the new architecture.
78-82: Good documentation of slippage protection.The added comments clearly explain the slippage protection mechanisms at both the individual swap level and the diamond contract level.
83-88: Appropriate low-level call implementation.The low-level call to execute the swap directly matches the new architecture. Error handling is maintained by forwarding any revert messages.
90-91: More robust output determination.Using the contract's balance after the swap to determine the output amount is more protocol-agnostic and robust than trying to decode potentially inconsistent return data formats.
110-113: Standardized state variable naming.Updated the call to use the renamed UPPER_CASE state variable, maintaining consistency with the state variable declarations.
133-136: Updated bit-shift for chain ID encoding.The bit-shift was correctly updated from 8 to 16 bits to align with the MAX_CHAINID_LENGTH_ALLOWED reduction from 32 to 16. This ensures consistent encoding/decoding of chain IDs.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/Libraries/LibAllowList.sol (1)
1-3:⚠️ Potential issueUpdate the contract version number.
The pipeline failure indicates that you should update the version tag since there are relevant changes to this contract.
// SPDX-License-Identifier: MIT -/// @custom:version 1.0.0 +/// @custom:version 1.0.1 pragma solidity ^0.8.17;🧰 Tools
🪛 GitHub Actions: VersionControlAndAuditVerification
[error] 1-1: The file has relevant changes but the file version was not updated. Please update the custom:version tag in this contract.
♻️ Duplicate comments (1)
test/solidity/Periphery/GasZipPeriphery.t.sol (1)
614-614:⚠️ Potential issueBug: State variable used in facet registration instead of local variable
In the
_getGnosisBridgeFacetfunction, line 614 uses the state variablegnosisBridgeFacetin the call tosetFacetAddressInTestBaseinstead of the local variable_gnosisBridgeFacet. At this point in the code execution,gnosisBridgeFacetis still uninitialized (it's only set after this function returns).This will cause
address(0)to be registered in the test base mapping, potentially leading to lookup failures later.-setFacetAddressInTestBase(address(gnosisBridgeFacet), "GnosisFacet"); +setFacetAddressInTestBase(address(_gnosisBridgeFacet), "GnosisFacet");
🧹 Nitpick comments (2)
docs/WhitelistManagerFacet.md (1)
1-11: Documentation accurately describes the WhitelistManagerFacet purpose.The documentation clearly explains the role of the WhitelistManagerFacet in approving/unapproving addresses for protocol interactions and the security considerations around admin key control.
Consider adding a comma after "admin key" in line 11:
-The Whitelist Manager manages which contracts and functions can be executed through the LI.FI main contract. This can be updated by a single admin key which if compromised could lead to malicious code being added to the allow list. +The Whitelist Manager manages which contracts and functions can be executed through the LI.FI main contract. This can be updated by a single admin key, which if compromised could lead to malicious code being added to the allow list.🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: Possible missing comma found.
Context: .... This can be updated by a single admin key which if compromised could lead to mali...(AI_HYDRA_LEO_MISSING_COMMA)
test/solidity/Periphery/GasZipPeriphery.t.sol (1)
163-163: Code cleanup - removed unnecessary blank lineRemoved an unnecessary blank line to improve code readability. This is a minor improvement but contributes to cleaner code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (14)
docs/DexManagerFacet.md(0 hunks)docs/WhitelistManagerFacet.md(1 hunks)script/deploy/facets/DeployDexManagerFacet.s.sol(0 hunks)script/deploy/facets/DeployWhitelistManagerFacet.s.sol(1 hunks)script/deploy/facets/UpdateWhitelistManagerFacet.s.sol(1 hunks)script/deploy/zksync/DeployDexManagerFacet.zksync.s.sol(0 hunks)script/deploy/zksync/DeployWhitelistManagerFacet.zksync.s.sol(1 hunks)src/Facets/WhitelistManagerFacet.sol(3 hunks)src/Interfaces/IWhitelistManagerFacet.sol(1 hunks)src/Libraries/LibAllowList.sol(3 hunks)src/Periphery/GasZipPeriphery.sol(3 hunks)test/solidity/Facets/DexManagerFacet.t.sol(0 hunks)test/solidity/Facets/WhitelistManagerFacet.t.sol(1 hunks)test/solidity/Periphery/GasZipPeriphery.t.sol(12 hunks)
💤 Files with no reviewable changes (4)
- script/deploy/facets/DeployDexManagerFacet.s.sol
- docs/DexManagerFacet.md
- script/deploy/zksync/DeployDexManagerFacet.zksync.s.sol
- test/solidity/Facets/DexManagerFacet.t.sol
✅ Files skipped from review due to trivial changes (3)
- script/deploy/zksync/DeployWhitelistManagerFacet.zksync.s.sol
- script/deploy/facets/DeployWhitelistManagerFacet.s.sol
- src/Interfaces/IWhitelistManagerFacet.sol
🧰 Additional context used
🪛 GitHub Actions: VersionControlAndAuditVerification
src/Libraries/LibAllowList.sol
[error] 1-1: The file has relevant changes but the file version was not updated. Please update the custom:version tag in this contract.
🪛 LanguageTool
docs/WhitelistManagerFacet.md
[uncategorized] ~11-~11: Possible missing comma found.
Context: .... This can be updated by a single admin key which if compromised could lead to mali...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (59)
script/deploy/facets/UpdateWhitelistManagerFacet.s.sol (1)
11-11: LGTM: Updated facet reference from DexManagerFacet to WhitelistManagerFacet.This change aligns with the PR objective of decoupling GasZipPeriphery from LDA (Lifi Diamond Architecture) by replacing the previous DexManagerFacet with a more generic WhitelistManagerFacet.
src/Libraries/LibAllowList.sol (4)
19-19: LGTM: Added storage for tracking selectors.This addition enables the library to keep track of all allowed function selectors, which will be useful for the WhitelistManagerFacet implementation.
75-82: Improved selector management logic.The updated function now properly adds selectors to the array only if not already approved, avoiding duplication while ensuring the mapping is updated in all cases.
88-103: Efficient selector removal implementation.The implementation uses the swap-and-pop pattern for efficient removal from the dynamic array, consistent with how contract addresses are removed.
112-115: LGTM: Added getter for allowed selectors.This new function complements the existing
getAllowedContractsfunction, providing a way to retrieve all approved function selectors.test/solidity/Facets/WhitelistManagerFacet.t.sol (12)
1-80: LGTM: Comprehensive test setup for WhitelistManagerFacet.The test contract properly sets up the testing environment with the diamond proxy pattern, deploys the WhitelistManagerFacet, and registers all its function selectors. It also correctly sets up the AccessManagerFacet for authorization testing.
82-109: LGTM: Basic add/remove address tests with event verification.The tests validate core functionality for adding and removing addresses to/from the whitelist, including proper event emission checks.
111-147: LGTM: Batch operations tests for addresses.The tests properly verify batch addition and removal of addresses, ensuring the whitelist state is correctly updated after operations.
149-180: LGTM: Function signature approval tests.The tests validate core functionality for approving function signatures individually and in batches.
182-232: LGTM: Input validation tests.The tests verify appropriate validation for zero addresses, non-contract addresses, and batch operations with invalid inputs.
234-321: LGTM: Authorization tests.The tests properly validate that unauthorized addresses cannot modify the whitelist or function approvals.
323-358: LGTM: Function approval state tests.The tests verify that function approvals can be both enabled and disabled correctly, individually and in batches.
360-416: LGTM: Access control delegation tests.The tests validate that addresses with delegated execution permission via AccessManagerFacet can perform whitelist operations, showing proper integration with the diamond's access control system.
418-451: LGTM: Whitelist state preservation test.The test confirms that batch additions preserve already whitelisted addresses and add new ones without duplication.
453-539: LGTM: Comprehensive function signature retrieval tests.The tests verify the correct behavior of retrieving approved function signatures in various scenarios, including empty lists and after removing signatures.
541-581: LGTM: Batch selector removal tests.The test validates that multiple function signatures can be removed in a batch operation and the state is updated correctly.
583-618: LGTM: Edge case handling tests.The tests verify that removing non-whitelisted addresses or non-approved function signatures does not disrupt the system state.
src/Facets/WhitelistManagerFacet.sol (12)
7-7: Appropriate interface import for the contract's implementationThe import of
IWhitelistManagerFacetmakes sense as the contract implements this interface. This is a good practice that ensures the contract adheres to a well-defined interface contract.
10-14: Contract renamed and repurposed from DexManagerFacet to WhitelistManagerFacetThe contract has been renamed from
DexManagerFacettoWhitelistManagerFacetwith updated documentation to reflect its broader purpose of managing a whitelist of addresses for various protocol interactions rather than just DEX addresses. The version update to 1.0.3 is also appropriate.
17-18: Method and event renamed for more generalized whitelist managementThe method has been renamed from
addDextoaddToWhitelistand the event fromDexAddedtoAddressWhitelistedto reflect the broader scope of the facet. The implementation correctly inherits documentation from the interface.Also applies to: 25-26
28-29: Batch whitelist addition with improved checksThe batch addition method has been updated with proper naming to reflect the generalized whitelist functionality. The implementation includes a key validation check to prevent self-authorization (line 37-38) which is an important security measure.
The code now also includes an optimization by checking if the contract is already whitelisted (line 40) before attempting to add it again, preventing redundant operations and events.
Also applies to: 33-48
54-61: Function renamed to reflect whitelist managementThe function has been appropriately renamed from
removeDextoremoveFromWhitelistand the event fromDexRemovedtoAddressRemovedwhile maintaining the same security checks and implementation logic.
63-76: Batch removal function renamed to match the new whitelist management approachSimilar to the individual removal function, the batch removal function has been renamed to match the new whitelist paradigm while maintaining identical functionality.
78-79: Inheritance documentation added for better code clarityAdding the
@inheritdoctag ensures that documentation from the interface is properly inherited, improving code readability and maintaining documentation consistency.
96-97: Inheritance documentation added for batch function approvalAs with the other functions, the
@inheritdoctag ensures proper documentation inheritance from the interface for better code clarity.
119-120: Inheritance documentation added for function approval checkConsistent use of
@inheritdocfor theisFunctionApprovedmethod maintains documentation quality across the contract.
126-133: Function renamed to reflect broader scopeThe function has been renamed from
getApprovedDexstogetWhitelistedAddressesto align with the contract's broader purpose of managing a general whitelist rather than just DEXes.
135-140: New method added to check if an address is whitelistedThis new method provides a convenient way to check if a specific address is whitelisted, improving the facet's utility. It follows the same pattern as
isFunctionApproved, providing a consistent API.
142-149: New method added to retrieve approved function signaturesThis method enhances the contract by providing access to the list of approved function signatures, which increases transparency and makes it easier to audit which functions are allowed. It leverages the
LibAllowList.getAllowedSelectors()to retrieve this information.src/Periphery/GasZipPeriphery.sol (14)
6-6: Updated import for WhitelistManagerFacetReplaced the DexManager interface import with the new WhitelistManagerFacet interface, which is needed for the updated contract interactions.
12-12: Added imports for new error typesAdded imports for the new error types used in the contract:
ContractCallNotAllowedandInvalidConfig. This ensures the errors are properly defined and accessible.
17-17: Version update to reflect changesThe contract version has been incremented to 1.0.2, reflecting the significant changes made to the contract's functionality.
22-24: State variables renamed and MAX_CHAINID_LENGTH_ALLOWED updatedThe state variables have been renamed to uppercase to follow Solidity naming conventions for constants and immutables. MAX_CHAINID_LENGTH_ALLOWED has been reduced from 32 to 16 which aligns with the 16-bit shift per chain ID.
28-28: Added new error for native output validationAdded a new error
SwapOutputMustBeNativeto ensure that the swap output is the native token. This is essential for the GasZip protocol which requires native tokens for deposits.
33-44: Added constructor parameter validationThe constructor now includes validation for zero addresses, which is a security best practice. If any of the constructor parameters (gasZipRouter, liFiDiamond, owner) is the zero address, the contract will revert with an
InvalidConfigerror, preventing misconfiguration.
48-48: Updated documentation commentThe documentation has been updated to accurately reflect that swaps are allowed via any whitelisted contract from the Diamond's WhitelistManagerFacet, which aligns with the new implementation.
56-71: Added validation for receiving asset and contract callThis new validation ensures that:
- The receiving asset is native (address(0)), which is required for GasZip
- The contract being called (
_swapData.callTo) is whitelisted- The function being called (
bytes4(_swapData.callData[:4])) is approvedThese checks enhance security by validating both the contract and function being called, ensuring only trusted operations are allowed.
79-80: Updated approval targetThe code now approves tokens to
_swapData.approveToinstead of a fixed DEX aggregator. This is more flexible and aligns with the new whitelisting approach.
83-86: Updated comments for swap executionThe comments now correctly note that the swap is executed using a whitelisted DEX rather than LiFiDEXAggregator. The comments about slippage protection are clear and informative.
88-88: Direct call to whitelisted contract for swap executionThe contract now makes a direct low-level call to the whitelisted contract (
_swapData.callTo) instead of going through a fixed aggregator. This is more flexible and aligns with the new architecture.Note: The Olympix security scanner flagged this line for making an external call without a gas budget. However, this pattern is acceptable here as noted in previous review comments, since failing the transaction is sufficient and saves gas. The whitelisted contract check provides sufficient protection.
95-96: Updated output amount calculationInstead of trying to decode the return value from the swap, the contract now uses its own ETH balance as the swap output amount. This is a more reliable approach that works across different DEX protocols which might return data in different formats.
115-115: Updated call to use GAS_ZIP_ROUTERUpdated the code to use the renamed state variable
GAS_ZIP_ROUTERin the deposit call, maintaining consistency with the naming convention changes.
139-141: Updated bit-shift operation for chain IDsThe bit-shift operation has been updated to shift by 16 bits per chain ID instead of 8, which aligns with the updated MAX_CHAINID_LENGTH_ALLOWED value of 16. This allows for larger chain IDs to be encoded but reduces the maximum number of chains that can be specified.
test/solidity/Periphery/GasZipPeriphery.t.sol (16)
9-11: Updated imports for new facet and errorsAdded imports for
WhitelistManagerFacetand the new error types (InvalidConfig) required for the updated tests. The test also now properly uses the TestBase import.
23-24: Updated contract variables and added constantAdded
whitelistManagerFacetvariable to interact with the new facet. ThePROCESS_ROUTE_SELECTORconstant is used in the tests to create a valid selector for testing swap failure scenarios.Also applies to: 31-31
37-37: Added new error type for swap failure testsAdded a
SwapFailederror to be used with the mock contract for testing swap failure scenarios.
43-114: Updated setUp function to use WhitelistManagerFacetThe setUp function has been extensively updated to:
- Deploy the WhitelistManagerFacet and add it to the diamond
- Deploy GasZipPeriphery with the diamond address
- Add function selectors for the WhitelistManagerFacet to the diamond
- Whitelist various contracts (Uniswap, GasZipPeriphery, FeeCollector)
- Approve specific function selectors for these contracts
- Add labels for key contracts for debugging
This setup enables testing of the new whitelist-based approach to contract and function validation.
131-134: Updated constructor test to reflect new parameter structureUpdated the constructor test to use the diamond address instead of the DEX aggregator, and to verify that the LIFI_DIAMOND state variable is correctly set.
Also applies to: 141-141
265-265: Updated function call to use new helper methodUpdated the test to use the renamed helper method
_getUniswapERC20ToNativeSwapDatainstead of the previous_getLiFiDEXAggregatorCalldataForERC20ToNativeSwap, reflecting the change in implementation approach.
394-394: Updated expected values for chain ID calculationsThe expected values for the
getDestinationChainsValuecalculations have been updated to match the new 16-bit shift per chain ID instead of the previous 8-bit shift.Also applies to: 406-407
409-430: Added test for maximum number of chain IDsAdded a comprehensive test case for exactly 16 chain IDs (the new maximum) with various chain ID values. This ensures that the contract can handle the full range of allowed chain IDs with the updated bit-shift logic.
445-445: Updated swap data helper method callSimilar to the change on line 265, updated the test to use the renamed helper method that works with direct Uniswap calls instead of through an aggregator.
448-457: Updated swap failure test approachThe test now uses a valid selector (PROCESS_ROUTE_SELECTOR) but with invalid parameters to ensure it passes the whitelist check but fails during execution. This matches the new validation flow in the GasZipPeriphery contract.
492-519: Added test for non-native receiving asset validationThis new test verifies that the
depositToGasZipERC20function correctly reverts withSwapOutputMustBeNativewhen the receiving asset is not the native token (address(0)). This ensures that the contract enforces the requirement that swaps must output native tokens for GasZip deposits.
521-562: Added test for swap operation failureThis new test uses a mock contract that always reverts with a custom error to verify that
depositToGasZipERC20properly handles swap failures. The test:
- Deploys a mock contract that always reverts with
SwapFailed- Whitelists the mock contract and approves its function selector
- Creates a swap data structure targeting this contract
- Verifies that the call to
depositToGasZipERC20reverts with the expected errorThis is an important test for ensuring error propagation works correctly.
564-592: Added tests for zero address validation in constructorThese three new tests verify that the GasZipPeriphery constructor correctly reverts with
InvalidConfigwhen provided with zero addresses for:
- gasZipRouter
- liFiDiamond
- owner
These tests ensure that the contract cannot be deployed with an invalid configuration.
602-602: Simplified facet setup for testThe function selector array size has been reduced from requiring extra selectors for DEX and function approval to just the core bridge functionality selectors, since whitelist management is now handled through the WhitelistManagerFacet.
617-651: Updated swap data helper functionThe helper function has been renamed from
_getLiFiDEXAggregatorCalldataForERC20ToNativeSwapto_getUniswapERC20ToNativeSwapDataand updated to:
- Use Uniswap directly instead of through an aggregator
- Specify the receiving asset as address(0) (native token)
- Use the
swapExactTokensForETHselector directly- Send the output to the GasZipPeriphery contract
This aligns with the updated implementation approach in GasZipPeriphery.
654-672: Added mock contract for testing swap failuresThis new contract
MockFailingDexWithCustomErrorprovides a simple way to test error handling in thedepositToGasZipERC20function. It:
- Defines a custom
SwapFailederror- Implements
swapExactTokensForETHto always revert with this error- Includes a fallback function that also reverts with the same error
- Provides a
receivefunction to accept ETHThis is a good testing pattern for verifying error propagation.
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
script/tasks/diamondSyncWhitelistedAddresses.sh (1)
59-65:⚠️ Potential issueDebug log references wrong function
The debug message still saysin function syncWhitelistedAddressesinstead ofdiamondSyncWhitelistedAddresses. This can confuse tracing in logs. Please correct it to reflect the actual function name.
♻️ Duplicate comments (1)
test/solidity/Periphery/GasZipPeriphery.t.sol (1)
614-614:⚠️ Potential issueBug in facet address registration.
The code is using the uninitialized state variable
gnosisBridgeFacetinsetFacetAddressInTestBaseinstead of the local variable_gnosisBridgeFacet.Apply this correction:
- setFacetAddressInTestBase(address(gnosisBridgeFacet), "GnosisFacet"); + setFacetAddressInTestBase(address(_gnosisBridgeFacet), "GnosisFacet");At this point in execution,
gnosisBridgeFacethasn't been assigned yet, resulting in registering address(0) instead of the actual facet address.
🧹 Nitpick comments (12)
script/tasks/diamondSyncWhitelistedAddresses_FAST.sh (3)
100-100: Consider using mapfile for array assignment.The array assignment could benefit from using mapfile or read -a to avoid potential word splitting issues.
- WHITELISTED_ADDRESSES=($(getWhitelistedAddresses)) + mapfile -t WHITELISTED_ADDRESSES < <(getWhitelistedAddresses)🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 100-100: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
131-131: Quote the private key parameter to prevent word splitting.The
getPrivateKeyoutput should be quoted to prevent potential issues with word splitting in bash.- cast send "$DIAMOND_ADDRESS" "batchAddToWhitelist(address[])" "${PARAMS[@]}" --rpc-url "$RPC_URL" --private-key $(getPrivateKey "$NETWORK" "$ENVIRONMENT") --legacy >/dev/null + cast send "$DIAMOND_ADDRESS" "batchAddToWhitelist(address[])" "${PARAMS[@]}" --rpc-url "$RPC_URL" --private-key "$(getPrivateKey "$NETWORK" "$ENVIRONMENT")" --legacy >/dev/null🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 131-131: Quote this to prevent word splitting.
(SC2046)
136-136: Consider using mapfile for array assignment.Similar to the previous instance, use mapfile to avoid potential word splitting issues when creating arrays.
- WHITELISTED_ADDRESSES_UPDATED=($(getWhitelistedAddresses)) + mapfile -t WHITELISTED_ADDRESSES_UPDATED < <(getWhitelistedAddresses)🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 136-136: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
script/deploy/healthCheck.ts (1)
348-352: Ensure console.log doesn't remain in production code.The debugging console.log statement should be replaced with a more appropriate logging mechanism like consola that's used elsewhere in the file.
- console.log('Function not approved:', sig) + consola.warn('Function not approved:', sig)script/helperFunctions.sh (4)
1343-1350: Refactor to validate config file and paths
The function hardcodes paths and assumesconfig/whitelistedAddresses.jsonandconfig/global.jsonalways exist. Consider extracting these paths into constants at the top of the script, checking for file existence before using them, and failing early with a clear error message if they’re missing.
1362-1363: Splitlocaldeclaration and assignment
ShellCheck SC2155 warns against combininglocalwith assignment as it masks the command’s return code. Rewrite as:local EXISTING_WHITELISTED_ADDRESSES EXISTING_WHITELISTED_ADDRESSES=$(jq --arg network "$NETWORK" '.[$network] | length' "$FILEPATH_WHITELISTED_ADDRESSES")🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 1363-1363: Declare and assign separately to avoid masking return values.
(SC2155)
1378-1383: Splitlocaldeclaration and assignment
Similarly, forEXISTS, avoid masking return values:local EXISTS EXISTS=$(jq --arg address "$CONTRACT_ADDRESS" --arg network "$NETWORK" \ '(.[$network] // []) | any(. == $address)' "$FILEPATH_WHITELISTED_ADDRESSES")🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 1379-1379: Declare and assign separately to avoid masking return values.
(SC2155)
1393-1394: Enhance success message clarity
Consider including the network in the success log for easier scanning in multi-network runs:success "[$NETWORK] Added $CONTRACT_ADDRESS to whitelistedAddresses.json"script/tasks/diamondSyncWhitelistedAddresses.sh (4)
76-79: Check and quote config file path and use robust splitting
You assume./config/whitelistedAddresses.jsonexists. Add a file-existence check withcheckIfFileExists. Also, when loadingCFG_WHITELISTED_ADDRESSES, prefer:mapfile -t CFG_WHITELISTED_ADDRESSES < <(jq -r --arg network "$NETWORK" '.[$network][]' "$CONFIG_WHITELISTED_ADDRESSES")to avoid word-splitting pitfalls.
88-97: Standardize error and warning output
Instead of rawecho "[warn]"/echo "[error]", use thewarninganderrorhelper functions for colored, consistent output. For example:warning "Failed to fetch whitelisted addresses ... retrying in $RETRY_DELAY seconds" error "Unable to fetch whitelisted addresses ... after $MAX_ATTEMPTS_PER_SCRIPT_EXECUTION attempts."Also applies to: 99-106
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 94-94: Quote this to prevent word splitting.
(SC2046)
115-120: Improve pluralization and clarity in debug output
When logging zero or multiple addresses, adapt the message for singular vs. plural:echoDebug "${#WHITELISTED_ADDRESSES[@]} whitelisted address(es) found..."
182-182: Unify completion message with function name
The final echo refers toscript syncWhitelistedAddresses completed. Update it to include the function name:echo "[info] <<<<<<<<<<<<<<<<<<<< diamondSyncWhitelistedAddresses completed"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (48)
config/global.json(2 hunks)conventions.md(2 hunks)docs/README.md(1 hunks)script/deploy/deployAllContracts.sh(2 hunks)script/deploy/facets/UpdateCoreFacets.s.sol(2 hunks)script/deploy/healthCheck.ts(4 hunks)script/deploy/resources/contractSpecificReminders.sh(1 hunks)script/deploy/zksync/UpdateCoreFacets.zksync.s.sol(2 hunks)script/helperFunctions.sh(5 hunks)script/tasks/diamondSyncSigs.ts(2 hunks)script/tasks/diamondSyncWhitelistedAddresses.sh(5 hunks)script/tasks/diamondSyncWhitelistedAddresses_FAST.sh(5 hunks)src/Facets/GenericSwapFacet.sol(1 hunks)src/Facets/GenericSwapFacetV3.sol(1 hunks)templates/facetTest.template.hbs(2 hunks)test/solidity/Facets/AcrossFacet.t.sol(2 hunks)test/solidity/Facets/AcrossFacetV3.t.sol(2 hunks)test/solidity/Facets/AllBridgeFacet.t.sol(2 hunks)test/solidity/Facets/ArbitrumBridgeFacet.t.sol(3 hunks)test/solidity/Facets/CBridge.t.sol(2 hunks)test/solidity/Facets/CBridgeAndFeeCollection.t.sol(2 hunks)test/solidity/Facets/CelerCircleBridgeFacet.t.sol(3 hunks)test/solidity/Facets/CelerIMFacet.t.sol(2 hunks)test/solidity/Facets/ChainflipFacet.t.sol(2 hunks)test/solidity/Facets/DeBridgeDlnFacet.t.sol(3 hunks)test/solidity/Facets/EmergencyPauseFacet.fork.t.sol(1 hunks)test/solidity/Facets/GasZipFacet.t.sol(3 hunks)test/solidity/Facets/GenericSwapFacet.t.sol(2 hunks)test/solidity/Facets/GenericSwapFacetV3.t.sol(10 hunks)test/solidity/Facets/GlacisFacet.t.sol(2 hunks)test/solidity/Facets/GnosisBridgeFacet.t.sol(3 hunks)test/solidity/Facets/HopFacet.t.sol(4 hunks)test/solidity/Facets/HopFacetOptimizedL1.t.sol(3 hunks)test/solidity/Facets/HopFacetOptimizedL2.t.sol(3 hunks)test/solidity/Facets/MayanFacet.t.sol(2 hunks)test/solidity/Facets/OmniBridgeFacet.t.sol(3 hunks)test/solidity/Facets/OmniBridgeL2Facet.t.sol(3 hunks)test/solidity/Facets/OptimismBridgeFacet.t.sol(3 hunks)test/solidity/Facets/PolygonBridgeFacet.t.sol(3 hunks)test/solidity/Facets/RelayFacet.t.sol(3 hunks)test/solidity/Facets/SquidFacet.t.sol(2 hunks)test/solidity/Facets/StargateFacetV2.t.sol(3 hunks)test/solidity/Facets/SymbiosisFacet.t.sol(2 hunks)test/solidity/Facets/ThorSwapFacet.t.sol(2 hunks)test/solidity/Helpers/SwapperV2.t.sol(2 hunks)test/solidity/Periphery/GasZipPeriphery.t.sol(12 hunks)test/solidity/utils/TestBase.sol(1 hunks)test/solidity/utils/TestHelpers.sol(2 hunks)
✅ Files skipped from review due to trivial changes (6)
- src/Facets/GenericSwapFacet.sol
- docs/README.md
- script/tasks/diamondSyncSigs.ts
- config/global.json
- conventions.md
- src/Facets/GenericSwapFacetV3.sol
🧰 Additional context used
🧬 Code Graph Analysis (2)
script/deploy/deployAllContracts.sh (1)
script/tasks/diamondSyncWhitelistedAddresses.sh (1)
diamondSyncWhitelistedAddresses(3-183)
script/tasks/diamondSyncWhitelistedAddresses.sh (2)
script/helperFunctions.sh (4)
echoDebug(2239-2247)error(2248-2250)doNotContinueUnlessGasIsBelowThreshold(2757-2785)getPrivateKey(2849-2880)script/tasks/diamondSyncWhitelistedAddresses_FAST.sh (1)
getWhitelistedAddresses(76-97)
🪛 Shellcheck (0.10.0)
script/tasks/diamondSyncWhitelistedAddresses_FAST.sh
[warning] 100-100: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 131-131: Quote this to prevent word splitting.
(SC2046)
[warning] 136-136: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
script/tasks/diamondSyncWhitelistedAddresses.sh
[warning] 109-109: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 146-146: Quote this to prevent word splitting.
(SC2046)
[warning] 151-151: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
script/helperFunctions.sh
[warning] 1363-1363: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1379-1379: Declare and assign separately to avoid masking return values.
(SC2155)
script/deploy/resources/contractSpecificReminders.sh
[warning] 8-8: FeeCollector appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 9-9: GasZipPeriphery appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 10-10: LiFiDEXAggregator appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 11-11: TokenWrapper appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
🔇 Additional comments (138)
script/deploy/resources/contractSpecificReminders.sh (1)
8-11:✅ Verification successful
Looks good - aligned with PR's DexManagerFacet to WhitelistManagerFacet refactoring.
The terminology change from "whitelisted DEXs" to "whitelisted addresses" in the reminder messages correctly reflects the architectural change from DEX-specific whitelisting to a more generic address whitelist mechanism.
🏁 Script executed:
#!/bin/bash # Check if these variables are exported or used elsewhere in the deployment process grep -r "FeeCollector=" --include="*.sh" . grep -r "GasZipPeriphery=" --include="*.sh" . grep -r "LiFiDEXAggregator=" --include="*.sh" . grep -r "TokenWrapper=" --include="*.sh" .Length of output: 843
Approved: Reminder messages update confirmed
The updated reminders in
script/deploy/resources/contractSpecificReminders.shnow correctly refer to “whitelisted addresses” and align with the DexManagerFacet → WhitelistManagerFacet refactoring. A search shows these variables are only defined here, so there’s no impact elsewhere in the deployment scripts.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 8-8: FeeCollector appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 9-9: GasZipPeriphery appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 10-10: LiFiDEXAggregator appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 11-11: TokenWrapper appears unused. Verify use (or export if used externally).
(SC2034)
test/solidity/Facets/SymbiosisFacet.t.sol (2)
47-47: Function selector update aligned with whitelist refactoring.Updated function selector from
addDex.selectortoaddToWhitelist.selectorto match the new WhitelistManagerFacet interface.
56-56: Function call update aligned with whitelist refactoring.Updated function call from
addDex(address(uniswap))toaddToWhitelist(address(uniswap))to match the new WhitelistManagerFacet interface.test/solidity/utils/TestBase.sol (1)
21-22: Function rename aligned with the whitelist refactoring.Renamed function from
addDex(address _dex)toaddToWhitelist(address _address)and updated parameter name to reflect the more general usage. The internal call toLibAllowList.addAllowedContract()remains unchanged as it already used a generic implementation.script/deploy/zksync/UpdateCoreFacets.zksync.s.sol (2)
21-24: Config address retrieval updated for WhitelistManagerFacet.Updated to retrieve
.WhitelistManagerFacetconfiguration address instead of the previous.DexManagerFacet, aligning with the architectural changes.
92-98: Facet references updated for WhitelistManagerFacet.Updated selectors retrieval, comment, and variable references from DexManagerFacet to WhitelistManagerFacet, maintaining consistency with the new whitelist management approach.
test/solidity/Facets/EmergencyPauseFacet.fork.t.sol (1)
17-18: Function name updated to reflect generalized whitelist approach.The function has been renamed from
addDextoaddToWhitelistand its parameter from_dexto_address, generalizing its purpose from DEX-specific to handling any whitelisted address. This change aligns with the PR's goal of refactoring the DEX management to a more generic whitelist system.test/solidity/Facets/OmniBridgeFacet.t.sol (3)
16-17: Function name updated to reflect generalized whitelist approach.The function has been renamed from
addDextoaddToWhitelistand its parameter from_dexto_address, making it consistent with the new whitelist management architecture.
51-52: Function selector reference updated for consistency.The reference to the function selector has been updated to use the new function name
addToWhitelist.
60-60: Function call updated to use new whitelist naming.The function call has been updated from
addDextoaddToWhitelist, maintaining consistent usage of the new whitelist approach.test/solidity/Facets/AllBridgeFacet.t.sol (3)
13-14: Function name updated to reflect generalized whitelist approach.The function has been renamed from
addDextoaddToWhitelistand its parameter from_dexto_address, aligning with the system-wide whitelist management refactoring.
43-43: Function selector reference updated for consistency.The reference to the function selector has been updated to use the new function name
addToWhitelist.
50-50: Function call updated to use new whitelist naming.The function call has been updated from
addDextoaddToWhitelist, maintaining consistent usage of the new whitelist approach.test/solidity/Facets/ThorSwapFacet.t.sol (3)
13-14: Function name updated to reflect generalized whitelist approach.The function has been renamed from
addDextoaddToWhitelistand its parameter from_dexto_address, making it consistent with the PR's objective to generalize the DEX management to a whitelist system.
41-41: Function selector reference updated for consistency.The reference to the function selector has been updated to use the new function name
addToWhitelist.
49-49: Function call updated to use new whitelist naming.The function call has been updated from
addDextoaddToWhitelist, maintaining consistent usage of the new whitelist approach.test/solidity/Facets/RelayFacet.t.sol (3)
25-26: Function renamed for generic whitelisting patternThe function has been renamed from
addDextoaddToWhitelistto align with the broader refactoring from DEX-specific approvals to a generic whitelist mechanism. The underlying implementation remains unchanged.
63-63: Function selector updated to match renamed functionUpdated selector to reference the renamed
addToWhitelistfunction - maintains consistency with the new naming pattern.
72-72: Function call updated to use renamed methodCall updated to use the new
addToWhitelistmethod to maintain consistency with the renamed function.script/deploy/facets/UpdateCoreFacets.s.sol (3)
21-24: Updated to use WhitelistManagerFacet instead of DexManagerFacetThe configuration key has been updated from
.DexManagerFacetto.WhitelistManagerFacetto reference the new generalized facet.
92-94: Updated facet name in comment and selector retrievalSection renamed from "DexManager Facet" to "Whitelist Manager Facet" and selector retrieval updated to match the new facet name, maintaining consistency with the refactored architecture.
95-98: Updated facet variable references in diamond cut operationsDiamond cut operations now reference the
whitelistMgrvariable instead of the previousdexMgrvariable, aligning with the renamed facet.test/solidity/Facets/AcrossFacet.t.sol (3)
18-19: Function renamed for generic whitelisting patternThe function has been renamed from
addDextoaddToWhitelistto align with the broader refactoring from DEX-specific approvals to a generic whitelist mechanism. The underlying implementation remains unchanged.
48-51: Function selector updated to match renamed functionUpdated selector to reference the renamed
addToWhitelistfunction - maintains consistency with the new naming pattern.
55-55: Function call updated to use renamed methodCall updated to use the new
addToWhitelistmethod to maintain consistency with the renamed function.test/solidity/Facets/PolygonBridgeFacet.t.sol (3)
16-17: Function renamed for generic whitelisting patternThe function has been renamed from
addDextoaddToWhitelistto align with the broader refactoring from DEX-specific approvals to a generic whitelist mechanism. The underlying implementation remains unchanged.
51-54: Function selector updated to match renamed functionUpdated selector to reference the renamed
addToWhitelistfunction - maintains consistency with the new naming pattern.
60-60: Function call updated to use renamed methodCall updated to use the new
addToWhitelistmethod to maintain consistency with the renamed function.test/solidity/Facets/CBridge.t.sol (3)
15-17: Function renamed for consistency with the whitelist approach.The function was renamed from
addDextoaddToWhitelistto align with the broader refactoring from DEX-specific management to a generic whitelist management system. This is consistent with the updated architecture that replacesDexManagerFacetwithWhitelistManagerFacet.
88-88: Updated function selector reference.The function selector reference was updated to match the renamed function
addToWhitelist.
95-95: Call updated to use the renamed function.The call to
addDexwas updated to use the renamedaddToWhitelistfunction. The functionality remains the same, just with updated naming.test/solidity/Facets/ChainflipFacet.t.sol (3)
18-20: Function renamed for consistency with the whitelist approach.The function was renamed from
addDextoaddToWhitelistto align with the broader refactoring from DEX-specific management to a generic whitelist management system.
57-57: Updated function selector reference.The function selector reference was updated to match the renamed function
addToWhitelist.
64-64: Call updated to use the renamed function.The call to
addDexwas updated to use the renamedaddToWhitelistfunction, maintaining consistent naming across the codebase.test/solidity/Helpers/SwapperV2.t.sol (3)
29-31: Function renamed for consistency with the whitelist approach.The function was renamed from
addDextoaddToWhitelistand the parameter name changed from_dexto_addressto reflect the more generic whitelist management approach.
49-49: Updated function selector reference.The function selector reference was updated to match the renamed function
addToWhitelist.
57-57: Call updated to use the renamed function.The call to
addDexwas updated to use the renamedaddToWhitelistfunction, maintaining consistent naming across the codebase.test/solidity/Facets/CelerCircleBridgeFacet.t.sol (4)
16-18: Function renamed for consistency with the whitelist approach.The function was renamed from
addDextoaddToWhitelistto align with the broader refactoring from DEX-specific management to a generic whitelist management system.
52-52: Updated function selector reference.The function selector reference was updated to match the renamed function
addToWhitelist.
61-61: Call updated to use the renamed function.The call to
addDexwas updated to use the renamedaddToWhitelistfunction, maintaining consistent naming across the codebase.
1-127:✅ Verification successful
Verify usage of the new WhitelistManagerFacet interface.
The changes in the test files are part of a broader architectural change from DEX-specific management (
DexManagerFacet) to a generic whitelist management (WhitelistManagerFacet). This should improve gas efficiency by allowing direct lookups in a mapping instead of fetching and searching through arrays.To verify the implementation and ensure consistency, run:
🏁 Script executed:
#!/bin/bash # Check the implementation of the WhitelistManagerFacet and isDexApproved function # Search for the WhitelistManagerFacet implementation echo "Searching for WhitelistManagerFacet implementation..." rg -l "contract WhitelistManagerFacet" --type sol # Check for the isDexApproved function echo "Checking for isDexApproved function implementation..." rg "function isDexApproved|function isWhitelisted" --type sol -A 10Length of output: 411
🏁 Script executed:
#!/bin/bash # Retry searching Solidity files without relying on rg's built-in types echo "Searching for WhitelistManagerFacet implementation..." rg -l "contract WhitelistManagerFacet" -g '*.sol' echo "Checking for isDexApproved function implementation..." rg "function isDexApproved" -g '*.sol' -A 10 echo "Checking for isWhitelisted function implementation..." rg "function isWhitelisted" -g '*.sol' -A 10Length of output: 582
🏁 Script executed:
#!/bin/bash # Display the implementation of WhitelistManagerFacet and its test echo "=== src/Facets/WhitelistManagerFacet.sol ===" sed -n '1,200p' src/Facets/WhitelistManagerFacet.sol || true echo "" echo "=== test/solidity/Facets/WhitelistManagerFacet.t.sol ===" sed -n '1,200p' test/solidity/Facets/WhitelistManagerFacet.t.sol || trueLength of output: 12011
WhitelistManagerFacet interface verified.
The generic
WhitelistManagerFacetis implemented atsrc/Facets/WhitelistManagerFacet.sol(conforming toIWhitelistManagerFacetwithaddToWhitelist,removeFromWhitelist,batch*,isAddressWhitelisted,isFunctionApproved, etc.), and its behavior is fully covered bytest/solidity/Facets/WhitelistManagerFacet.t.sol. The CelerCircleBridgeFacet tests stub whitelist operations viaLibAllowListintest/solidity/Facets/CelerCircleBridgeFacet.t.sol, aligning with the shift away fromDexManagerFacet. No further changes are needed.• src/Facets/WhitelistManagerFacet.sol – generic whitelist implementation
• test/solidity/Facets/WhitelistManagerFacet.t.sol – tests for batch operations, approvals, and queries
• test/solidity/Facets/CelerCircleBridgeFacet.t.sol – stubs whitelist calls viaLibAllowListconsistent with new interfacetest/solidity/utils/TestHelpers.sol (2)
8-9: Interface rename from DexManager to IWhitelistManagerThe change from a DEX-specific interface name to a more generic whitelist interface reflects the architectural shift toward a more flexible and reusable address management system. This is consistent with the PR objective of improving the efficiency of DEX approval checks.
56-76: Updated function calls to use the new whitelist interfaceAll calls have been consistently updated to use
IWhitelistManagerand itsaddToWhitelistmethod, maintaining the same functionality while adopting the more generic naming convention. This ensures test helpers work correctly with the updated facet implementation.test/solidity/Facets/DeBridgeDlnFacet.t.sol (3)
15-16: Function rename from addDex to addToWhitelistThe function rename preserves the same implementation while adopting a more generic naming convention. This aligns with the overall architectural change from a DEX-specific whitelist to a general-purpose whitelist mechanism.
61-61: Updated function selectorThe function selector has been appropriately updated to reference the renamed function, ensuring the diamond contract calls the correct function.
71-71: Updated function callThe function call to whitelist the Uniswap address has been correctly updated to use the new function name. This ensures consistency in the test setup.
test/solidity/Facets/OptimismBridgeFacet.t.sol (3)
11-12: Function rename from addDex to addToWhitelistThe function has been renamed while preserving its implementation, aligning with the broader refactoring initiative from DEX-specific to generic whitelist management.
61-61: Updated function selector referenceThe function selector in the array has been correctly updated to refer to the renamed method, maintaining proper diamond delegation.
78-78: Updated function callThe call to whitelist the Uniswap address has been properly updated to use the new function name, ensuring the test setup works correctly with the refactored implementation.
test/solidity/Facets/CBridgeAndFeeCollection.t.sol (3)
15-16: Function rename from addDex to addToWhitelistThe function has been renamed while maintaining the same implementation logic, consistent with the architectural shift to generic whitelist management.
43-43: Updated function selectorThe function selector has been properly updated to reference the renamed function, ensuring correct delegation in the diamond contract.
49-50: Updated function callsBoth calls to whitelist addresses (Uniswap and feeCollector) have been correctly updated to use the new function name, maintaining test functionality with the refactored implementation.
test/solidity/Facets/ArbitrumBridgeFacet.t.sol (3)
16-17: Renamed function to use more generic terminology.Function
addDexhas been renamed toaddToWhitelistwith parameter_dexchanged to_address, making the terminology more generic while maintaining the same functionality.
55-55: Updated function selector to match renamed function.Updated function selector reference from
addDex.selectortoaddToWhitelist.selectorto maintain consistency with the function rename.
64-64: Updated function call to use renamed method.Updated function call from
addDex(address(uniswap))toaddToWhitelist(address(uniswap))to match the renamed function.test/solidity/Facets/MayanFacet.t.sol (3)
15-16: Renamed function to use more generic terminology.Function
addDexhas been renamed toaddToWhitelistwith parameter_dexchanged to_address, making the terminology more generic while maintaining the same functionality.
113-113: Updated function selector to match renamed function.Updated function selector reference from
addDex.selectortoaddToWhitelist.selectorto maintain consistency with the function rename.
120-120: Updated function call to use renamed method.Updated function call from
addDex(ADDRESS_UNISWAP)toaddToWhitelist(ADDRESS_UNISWAP)to match the renamed function.templates/facetTest.template.hbs (3)
13-14: Renamed function to use more generic terminology in template.Function
addDexhas been renamed toaddToWhitelistwith parameter_dexchanged to_addressin the template file, ensuring that all new test files created from this template will use the more generic terminology.
43-43: Updated function selector in template to match renamed function.Updated function selector reference from
addDex.selectortoaddToWhitelist.selectorin the template to maintain consistency with the function rename.
50-50: Updated function call in template to use renamed method.Updated function call from
addDex(ADDRESS_UNISWAP)toaddToWhitelist(ADDRESS_UNISWAP)in the template to match the renamed function.test/solidity/Facets/AcrossFacetV3.t.sol (3)
20-21: Renamed function to use more generic terminology.Function
addDexhas been renamed toaddToWhitelistwith parameter_dexchanged to_address, making the terminology more generic while maintaining the same functionality.
54-54: Updated function selector to match renamed function.Updated function selector reference from
addDex.selectortoaddToWhitelist.selectorto maintain consistency with the function rename.
61-61: Updated function call to use renamed method.Updated function call from
addDex(ADDRESS_UNISWAP)toaddToWhitelist(ADDRESS_UNISWAP)to match the renamed function.test/solidity/Facets/OmniBridgeL2Facet.t.sol (3)
16-17: Function renaming: fromaddDextoaddToWhitelistThe function name has been updated to be more generic and better reflect its purpose. This is part of a broader refactoring from DEX-specific terminology to a more generic whitelist approach.
60-60: Updated function selector referenceThe function selector reference has been updated to match the renamed function.
69-69: Updated function call in test setupFunction call updated to use the renamed function.
test/solidity/Facets/HopFacetOptimizedL2.t.sol (3)
13-14: Function renaming: fromaddDextoaddToWhitelistThe function name has been updated to be more generic and better reflect its purpose, while maintaining the same internal implementation that calls
LibAllowList.addAllowedContract.
58-58: Updated function selector referenceThe function selector reference has been updated to match the renamed function.
67-67: Updated function call in test setupFunction call updated to use the renamed function.
test/solidity/Facets/HopFacet.t.sol (4)
14-15: Function renaming: fromaddDextoaddToWhitelistThe function name has been updated to be more generic and better reflect its purpose as part of the broader refactoring effort.
52-52: Updated function selector referenceThe function selector reference has been updated to match the renamed function.
67-67: Updated function call in test setupFunction call updated to use the renamed function.
245-245: Updated function selector reference in test caseThe function selector reference has been updated in the test case that creates a new diamond instance.
test/solidity/Facets/GenericSwapFacet.t.sol (3)
9-10: Function renaming: fromaddDextoaddToWhitelistThe function name has been updated to be more generic and better reflect its purpose as part of the broader refactoring from DEX-specific to whitelist management.
38-38: Updated function selector referenceThe function selector reference has been updated to match the renamed function.
46-46: Updated function call in test setupFunction call updated to use the renamed function.
test/solidity/Facets/SquidFacet.t.sol (3)
19-20: Renamed function to reflect generic whitelist pattern.The function has been renamed from
addDextoaddToWhitelist, which aligns with the broader refactoring of moving from a DEX-specific approach to a more generic whitelist management system.
56-57: Updated function selector reference.The function selector reference has been updated to reflect the renamed function, ensuring the selector array properly maps to the new function name.
63-64: Updated function call to use new whitelist pattern.The function call has been updated from
addDextoaddToWhitelist, maintaining consistent naming throughout the codebase.test/solidity/Facets/StargateFacetV2.t.sol (3)
19-20: Renamed function to reflect generic whitelist pattern.The function has been renamed from
addDextoaddToWhitelist, which aligns with the broader refactoring of moving from a DEX-specific approach to a more generic whitelist management system.
72-73: Updated function selector reference.The function selector reference has been updated to reflect the renamed function, ensuring the selector array properly maps to the new function name.
82-84: Updated function calls to use new whitelist pattern.The function calls have been updated from
addDextoaddToWhitelist, maintaining consistent naming throughout the codebase while preserving the same functionality.test/solidity/Facets/GlacisFacet.t.sol (3)
16-17: Renamed function to reflect generic whitelist pattern.The function has been renamed from
addDextoaddToWhitelist, which aligns with the broader refactoring of moving from a DEX-specific approach to a more generic whitelist management system.
57-58: Updated function selector reference.The function selector reference has been updated to reflect the renamed function, ensuring the selector array properly maps to the new function name.
64-65: Updated function call to use new whitelist pattern.The function call has been updated from
addDextoaddToWhitelist, maintaining consistent naming throughout the codebase.test/solidity/Facets/GnosisBridgeFacet.t.sol (3)
14-15: Renamed function to reflect generic whitelist pattern.The function has been renamed from
addDextoaddToWhitelist, which aligns with the broader refactoring of moving from a DEX-specific approach to a more generic whitelist management system.
47-48: Updated function selector reference.The function selector reference has been updated to reflect the renamed function, ensuring the selector array properly maps to the new function name.
56-57: Updated function call to use new whitelist pattern.The function call has been updated from
addDextoaddToWhitelist, maintaining consistent naming throughout the codebase.test/solidity/Facets/HopFacetOptimizedL1.t.sol (3)
13-14: RenamingaddDextoaddToWhitelistfor consistency with WhitelistManagerFacetThis function rename aligns with the broader refactoring that replaces the DexManagerFacet with a more generalized WhitelistManagerFacet. The function signature and underlying implementation remain unchanged, only using a more appropriate parameter name
_addressinstead of_dex.
54-54: Updated function selector to match the renamed methodThe function selector has been correctly updated to reference the new method name
addToWhitelistinstead ofaddDex.
63-63: Updated method call to use the renamed APIThe call to whitelist the Uniswap contract has been properly updated to use the new method name while maintaining the same functionality.
test/solidity/Facets/GasZipFacet.t.sol (4)
13-14: RenamedaddDextoaddToWhitelistfor consistency with the new WhitelistManagerFacetThe method has been renamed to use the more generic whitelist terminology while maintaining the same underlying functionality through LibAllowList.
17-18: RenamedremoveDextoremoveFromWhitelistfor consistency with the new WhitelistManagerFacetThe removal method has been updated to match the new whitelist terminology while preserving the core functionality.
65-66: Updated function selectors to match the renamed methodsFunction selectors in the array have been correctly updated to reference the new method names, ensuring proper diamond facet registration.
75-76: Updated method calls to use the renamed APIThe calls to whitelist the Uniswap router and the facet itself have been properly updated to use the new method names.
test/solidity/Facets/CelerIMFacet.t.sol (3)
27-28: RenamedaddDextoaddToWhitelistfor consistency with the WhitelistManagerFacetThe method has been renamed to use the more generic whitelist terminology while maintaining the same underlying functionality.
122-122: Updated function selector to match the renamed methodThe function selector has been correctly updated to reference the new method name in the configuration array.
130-130: Updated method call to use the renamed APIThe call to whitelist the Uniswap contract has been properly updated to use the new method name.
script/deploy/deployAllContracts.sh (4)
13-13: Updated import to use the renamed whitelist sync scriptThe script now sources
diamondSyncWhitelistedAddresses.shinstead ofdiamondSyncDEXs.sh, aligning with the terminology change from DEX-specific to generic whitelisted addresses.
122-122: Updated comment to reflect storage in whitelistedAddresses.jsonThe comment has been updated to indicate that periphery addresses are now added to whitelistedAddresses.json instead of dexs.json.
123-123: Renamed helper function to match the new file naming conventionThe function call has been updated from
addPeripheryToDexsJsontoaddPeripheryToWhitelistedAddressesJsonto maintain consistency with the new whitelist terminology.
125-126: Updated sync whitelist function callThe function call has been changed from
diamondSyncDEXstodiamondSyncWhitelistedAddressesto match the renamed function imported from the task script.script/tasks/diamondSyncWhitelistedAddresses_FAST.sh (3)
3-3: Function name properly updated to reflect new scope.The function name has been appropriately renamed from
diamondSyncDEXs_FASTtodiamondSyncWhitelistedAddresses_FASTto reflect the change from DEX-specific to generic whitelist management.
72-73: Config file path correctly updated.The script now correctly sources whitelisted addresses from the new configuration file path.
81-81: Contract method call updated correctly.The call has been properly updated from
approvedDexs()togetWhitelistedAddresses()to align with the new whitelist management interface.script/deploy/healthCheck.ts (3)
63-65: Import updated to match new whitelist mechanism.The import has been correctly updated from
dexs.jsontowhitelistedAddresses.jsonto align with the architectural change from DEX-specific to general whitelist management.
264-273: Variable name and contract interface appropriately updated.The variable name has been properly updated from
dexManagertowhitelistManagerand the method calls have been correctly adjusted to use the new whitelist management interface.
334-343: Improved efficiency with single contract call.The code now uses a single call to
getApprovedFunctionSignaturesinstead of batching individual calls toisFunctionApproved, which is more efficient. This is a good improvement.script/tasks/diamondSyncWhitelistedAddresses.sh (1)
3-6: Inconsistent function name in log
The function is nameddiamondSyncWhitelistedAddressesbut the startup message refers tosyncWhitelistedAddresses. Update the echo to match the function name or rename the function to maintain consistency.Likely an incorrect or invalid review comment.
test/solidity/Facets/GenericSwapFacetV3.t.sol (13)
15-17: Function renaming aligns with the new whitelist management architecture.The function has been appropriately renamed from
addDexto the more genericaddToWhitelist, reflecting the architectural shift from DEX-specific allowlisting to general address whitelisting.
19-21: Function renaming maintains consistent nomenclature.The function has been properly renamed from
removeDextoremoveFromWhitelist, maintaining consistency with the whitelist architectural changes.
29-31: Internal test helper updated to match new contract interface.The test stub for
TestGenericSwapFacetcorrectly implements the renamed function to maintain compatibility with the whitelist-based architecture.
33-35: Consistent function renaming in test helper.The
removeFromWhitelistimplementation maintains the same internal logic while updating the function name, ensuring compatibility with the new architecture.
71-73: Updated function selectors to match renamed methods.The function selectors in the diamond setup have been properly updated to reference the new function names, ensuring the diamond facade retains the same functionality.
106-106: Updated function call to match renamed method.The test setup correctly uses the new
addToWhitelistmethod to prepare the test environment.
120-120: Consistent method call updates across all test setup.The function call for whitelisting Uniswap in the GenericSwapFacetV3 context has been properly updated.
136-136: Fee collector whitelisting updated to new function name.Fee collector whitelisting consistently uses the new naming convention.
144-144: Consistent function name updates for fee collector whitelisting.The fee collector is properly whitelisted using the renamed function for V3 facet.
671-671: Updated DEX whitelist removal function call.Test case properly uses the renamed
removeFromWhitelistfunction to test functionality when a DEX is not whitelisted.
827-827: Consistent usage of updated function name in native token tests.The native token test cases correctly use the renamed function.
1135-1135: Updated whitelist removal in multi-swap tests.The multi-swap test case maintains consistency by using the renamed function.
1968-1969: Updated whitelist addition for mock DEX.The test for ERC20 dust handling correctly uses the new function name when setting up the mock DEX.
test/solidity/Periphery/GasZipPeriphery.t.sol (15)
9-11: Added imports for new whitelist architecture and error codes.The added imports appropriately support the architectural changes, importing the
WhitelistManagerFacetfor whitelist management and error types for proper error handling.
23-24: Contract state variables updated to match architectural changes.The test now uses the direct
GasZipPeripheryimplementation and adds aWhitelistManagerFacetvariable to support the new whitelist-based architecture.
31-31: Added selector constant for function call testing.The added constant provides a clear and maintainable reference for the process route selector used in testing.
37-37: Added error definition for swap failures.The
SwapFailederror definition supports new error handling in the tests.
43-53: Updated test setup for new architecture.The setup process now properly instantiates the necessary components for the new whitelist-based architecture, including the GnosisBridgeFacet, WhitelistManagerFacet, and GasZipPeriphery with the diamond address.
56-112: Comprehensive whitelist setup for test environment.The test now properly sets up the whitelist and function approvals for all components (Uniswap, GasZipPeriphery, FeeCollector) through the WhitelistManagerFacet, ensuring that test cases can execute against the new architecture.
131-141: Updated constructor parameters and assertions.The test now correctly checks for the LIFI_DIAMOND address instead of DEX aggregator, reflecting the new architecture where the contract interacts directly with the diamond.
265-265: Updated swap data generation method.The test now uses Uniswap directly instead of the LiFiDEXAggregator, matching the architectural changes.
445-445: Updated method for swap data generation in revert test.The revert test consistently uses the new swap data generation approach.
449-457: Updated function call simulation for failure testing.The test now sets up a properly formatted function call that will pass whitelist checks but fail during execution, providing a comprehensive test for error handling.
492-562: New tests for input validation and error handling.Added tests verify that:
- The function correctly rejects non-native receiving assets
- Swap failures are properly handled with the expected error
These tests strengthen the test suite by verifying the contract's error handling capabilities.
564-592: New constructor validation tests.Added tests verify that the constructor properly rejects invalid configurations with zero addresses for critical parameters, enhancing security and ensuring proper initialization.
602-603: Simplified facet setup for GnosisBridgeFacet.The facet setup is now correctly simplified to only include the necessary function selectors without DEX approvals, aligning with the new whitelist-based architecture.
617-651: Updated swap data generation to use Uniswap directly.The function has been properly renamed and reimplemented to use Uniswap directly instead of going through the LiFiDEXAggregator, matching the architectural changes. The function now:
- Properly constructs the path for token swapping
- Calculates expected output amounts
- Generates swap calldata targeting Uniswap directly
- Sets the receiving contract to GasZipPeriphery
This change aligns with the updated architecture where the contract interacts directly with DEXs through whitelist controls.
654-672: Added mock contract for failure testing.The
MockFailingDexWithCustomErrorprovides a controlled way to test failure handling in the GasZipPeriphery contract, enhancing the test coverage and ensuring proper error propagation.
lifi-action-bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/solidity/Facets/WhitelistManagerFacet.t.sol (3)
172-177: Consider consistent use of unchecked increments in loops.This loop uses unchecked increment for gas optimization, but not all loops in the file follow this pattern. Consider standardizing this approach across the test file for consistency.
- for (uint256 i = 0; i < 5; ) { + for (uint256 i = 0; i < 5; i++) { assertTrue(whitelistMgr.isFunctionApproved(signatures[i])); - unchecked { - ++i; - } }Or consistently use the unchecked pattern in all loops:
for (uint256 i = 0; i < 3; i++) { assertTrue(whitelistMgr.isFunctionApproved(signatures[i])); }To:
for (uint256 i = 0; i < 3; ) { assertTrue(whitelistMgr.isFunctionApproved(signatures[i])); unchecked { ++i; } }
492-496: Consider adding unchecked increment for gas optimization.Similar to the earlier comment, this loop could use unchecked increment for gas optimization.
- for (uint256 i = 0; i < signatures.length; i++) { + for (uint256 i = 0; i < signatures.length; ) { if (signatures[i] == testSignatures[0]) foundSig0 = true; if (signatures[i] == testSignatures[1]) foundSig1 = true; if (signatures[i] == testSignatures[2]) foundSig2 = true; + unchecked { + ++i; + } }
1-714: Comprehensive test suite with excellent coverage.The test suite provides thorough coverage of the WhitelistManagerFacet functionality, including basic operations, batch operations, access control, error handling, edge cases, and migration logic. The tests validate both positive and negative scenarios, ensuring the contract behaves as expected under all conditions.
However, consider adding tests for:
- Attempting to whitelist an already whitelisted address (idempotency test)
- More explicit verification of event emissions in batch operations
- Complex permission scenarios (e.g., adding an address, removing permissions, then trying to remove the address)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (8)
deployments/_deployments_log_file.json(1 hunks)deployments/arbitrum.diamond.staging.json(2 hunks)deployments/arbitrum.staging.json(1 hunks)script/deploy/healthCheck.ts(6 hunks)src/Libraries/LibAllowList.sol(4 hunks)src/Periphery/GasZipPeriphery.sol(1 hunks)test/solidity/Facets/WhitelistManagerFacet.t.sol(1 hunks)test/solidity/Periphery/GasZipPeriphery.t.sol(5 hunks)
✅ Files skipped from review due to trivial changes (5)
- src/Periphery/GasZipPeriphery.sol
- deployments/arbitrum.staging.json
- deployments/arbitrum.diamond.staging.json
- deployments/_deployments_log_file.json
- test/solidity/Periphery/GasZipPeriphery.t.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Libraries/LibAllowList.sol
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
🔇 Additional comments (15)
script/deploy/healthCheck.ts (8)
63-65: LGTM: The import has been properly updated to use whitelistedAddresses.jsonThe change from
dexs.jsontowhitelistedAddresses.jsonis consistent with the broader refactoring from DEX-specific to a generic whitelist approach.
259-262: LGTM: UI text properly updated for whitelist terminologyThe console output headings have been updated from DEX-specific language to the more general whitelist terminology, maintaining consistency with the refactoring throughout the codebase.
267-272: LGTM: ABI definitions properly include both old and new function signaturesThe ABI now includes both the legacy
approvedDexs()function and the newgetWhitelistedAddresses()function, allowing for backward compatibility during migration.
276-292: LGTM: Well-handled backward compatibility for contract functionsThe code appropriately tries the new
getWhitelistedAddresses()function first and falls back to the legacyapprovedDexs()function if needed, with a clear error message indicating the need for an upgrade.
294-337: LGTM: Variable and message references updated consistentlyAll references to DEXs have been replaced with appropriate whitelist terminology throughout the validation logic while maintaining the same functionality.
347-380: LGTM: More efficient approach for signature verificationThe code now retrieves all approved function signatures in a single call rather than making multiple individual calls to check each signature. This is a better approach that reduces the number of contract calls and improves readability.
655-666: LGTM: Well-structured helper function for signature retrievalThe new
getApprovedFunctionSignaturesfunction properly encapsulates the logic for retrieving approved function signatures and includes appropriate error handling.
585-585: LGTM: Error message updated for consistencyThe error message has been updated from "No dexs configured" to "No whitelisted addresses configured", maintaining consistency with the new terminology.
test/solidity/Facets/WhitelistManagerFacet.t.sol (7)
1-80: Test setup and imports look good.The test setup properly initializes all the necessary contracts for testing the WhitelistManagerFacet, including accessMgr for access control testing. Event definitions are well structured to verify emitted events during testing.
82-109: Well-structured tests for basic whitelist operations.The tests correctly verify owner privileges for adding and removing addresses from the whitelist. The event emission verification is properly implemented.
182-321: Comprehensive error case testing.The test suite properly verifies all error conditions including adding zero addresses, non-contract addresses, and unauthorized operations. The use of
vm.expectRevertcorrectly validates that the expected errors are thrown.
416-448: Good test for whitelist preservation during batch operations.This test correctly verifies that batch operations preserve existing whitelist entries and don't create duplicates when adding already whitelisted addresses again.
580-596: Good edge case testing for removing non-whitelisted addresses.This test correctly verifies that attempting to remove an address that was never whitelisted does not disrupt the state of the contract or cause issues.
597-615: Good edge case testing for removing non-approved signatures.The test verifies that removing a signature that was never approved doesn't affect the contract state, which is an important edge case to cover.
657-712: Excellent test for legacy selector migration.This test thoroughly validates the migration path from the legacy mapping-based selector storage to the new array-based implementation, including direct storage manipulation to simulate legacy state. This ensures backward compatibility with existing contracts.
lifi-action-bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test/solidity/Periphery/GasZipPeriphery.t.sol (1)
614-615: Still registeringaddress(0)for facet – duplicate of earlier feedback
setFacetAddressInTestBase(address(gnosisBridgeFacet), "GnosisFacet");
is executed before the state variable is assigned, resulting inaddress(0)being stored. Use the local_gnosisBridgeFacetinstead.- setFacetAddressInTestBase(address(gnosisBridgeFacet), "GnosisFacet"); + setFacetAddressInTestBase(address(_gnosisBridgeFacet), "GnosisFacet");
🧹 Nitpick comments (3)
script/deploy/zksync/DeployGasZipPeriphery.zksync.s.sol (1)
31-33: Out-of-date comment – update to reflectLiFiDiamondlookupThe code below now fetches the
LiFiDiamondaddress, but the comment still says “LiFiDEXAggregator”. This will confuse future readers and grep-based tooling.- // get LiFiDEXAggregator address + // get LiFiDiamond addresssrc/Periphery/GasZipPeriphery.sol (1)
64-71: Function approval not bound to address – consider tightening
isFunctionApprovedis global. A malicious contract that mimics a trusted selector could be whitelisted later and misuse existing approvals.
Binding(address, selector)pairs in the allow-list gives finer-grained control and reduces blast radius.test/solidity/Periphery/GasZipPeriphery.t.sol (1)
394-430: Magic numbers – add comment for expected 16-bit packingThe hard-coded expected values (e.g.,
3342388,276716361166703427643, …) rely on the new 16-bit-per-chain encoding.
Consider adding a helper that computes the packed value on-the-fly to avoid brittle tests when the packing logic changes again.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
script/deploy/facets/DeployGasZipPeriphery.s.sol(3 hunks)script/deploy/zksync/DeployGasZipPeriphery.zksync.s.sol(2 hunks)src/Periphery/GasZipPeriphery.sol(3 hunks)test/solidity/Periphery/GasZipPeriphery.t.sol(12 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
🔇 Additional comments (1)
script/deploy/facets/DeployGasZipPeriphery.s.sol (1)
31-41: LGTM – comment & variable names are consistentThe switch from
LiFiDEXAggregator→LiFiDiamondis correctly reflected in both the comment and variable names. No further action required.
Which Jira task belongs to this PR?
LF-13168
Why did I implement it this way?
The
DexManagerFacetwas updated to include a newisDexApprovedfunction, which performs a direct lookup in the mapping. Previously, contracts likeGasZipPeripheryhad to fetch the full array using theapprovedDexs()function and manually search for the DEX address, which was inefficient. Since more contracts will likely interact with the diamond architecture via DexManagerFacet, introducingisDexApprovedimproves gas efficiencyChecklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)