add isContract check when reading contract addresses from config files #931
Conversation
WalkthroughThis pull request introduces a systematic refactoring of configuration address retrieval across multiple deployment scripts. The primary change involves replacing direct JSON file reading with a new method Changes
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Test Coverage ReportLine Coverage: 78.66% (2238 / 2845 lines) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (19)
script/deploy/facets/DeployMayanFacet.s.sol (1)
26-28: Consider adding error context to improve debugging.To improve error handling, consider including the network name in the error message when the bridge address is invalid.
address bridge = _getConfigContractAddress( path, - string.concat(".bridges.", network, ".bridge") + string.concat(".bridges.", network, ".bridge"), + string.concat("Invalid bridge address for network: ", network) );script/deploy/facets/DeployDeBridgeDlnFacet.s.sol (1)
27-29: Consider adding error messages for contract validation failures.To improve debugging and maintainability, consider adding descriptive error messages when the contract validation fails.
address dlnSource = _getConfigContractAddress( path, - string.concat(".", network, ".dlnSource") + string.concat(".", network, ".dlnSource"), + "DLN source must be a valid contract" );script/deploy/facets/DeployStargateFacetV2.s.sol (2)
25-27: Document the expected JSON configuration format.Consider adding documentation about:
- The expected structure of the JSON configuration file
- The format of the network parameter
- Error scenarios and how they're handled
Add a comment block above the function:
+ /// @notice Retrieves a validated contract address from the config file + /// @param path Path to the JSON config file + /// @param networkKey Concatenated key path for network-specific address + /// @return The validated contract address + /// @dev Throws if the address is not a contract function getConstructorArgs() internal override returns (bytes memory) {
25-27: Consider adding error handling for network configuration.The current implementation might throw unclear errors if:
- The network configuration is missing
- The tokenMessaging address is invalid
Consider adding explicit error messages for these cases:
address tokenMessaging = _getConfigContractAddress( path, - string.concat(".tokenMessaging.", network) + string.concat(".tokenMessaging.", network), + "Stargate TokenMessaging" // Add a descriptive name for error messages );script/deploy/facets/DeployThorSwapFacet.s.sol (2)
25-27: Add documentation for the configuration file format.Please add comments explaining:
- The expected format of the thorswap.json configuration file
- The contract validation logic performed by
_getConfigContractAddress- Error handling behavior for invalid addresses or non-existent config files
function getConstructorArgs() internal override returns (bytes memory) { string memory path = string.concat(root, "/config/thorswap.json"); + // thorswap.json format: + // { + // "<network>": { + // "thorchainRouter": "<address>" + // } + // } + // Validates that the retrieved address belongs to a deployed contract address thorchainRouter = _getConfigContractAddress( path, string.concat(".", network, ".thorchainRouter") );
25-27: Consider adding error messages for better debugging.The error message in case of invalid addresses or configuration would help with debugging deployment issues.
address thorchainRouter = _getConfigContractAddress( path, - string.concat(".", network, ".thorchainRouter") + string.concat(".", network, ".thorchainRouter"), + "Invalid or non-contract address for ThorChain Router" );script/deploy/facets/DeployGasZipFacet.s.sol (1)
25-27: Another JSON path structure variation found.The contract validation is correctly implemented, but this introduces a third JSON path structure:
".gasZipRouters.<network>"Consider standardizing path structures across all facets.
script/deploy/facets/DeployAmarokFacet.s.sol (1)
25-27: LGTM! Consider this JSON path structure as the standard.This implementation follows what appears to be the most common pattern (
".<network>.<key>") across facets. Consider standardizing other facets to follow this structure.Standardize JSON path structure across all facets to:
.<network>.<key>This would improve maintainability and reduce cognitive load when working across different facets.
script/deploy/facets/DeployExecutor.s.sol (1)
32-32: Standardize configuration key format across deployment scripts.The key format here (".ERC20Proxy") differs from other deployment scripts that use "..key" pattern. Consider standardizing the approach across all deployment scripts for better maintainability.
script/deploy/facets/DeployLiFiDiamond.s.sol (1)
31-34: Standardize configuration path structure.The path construction pattern differs from other deployment scripts:
- Uses
/deployments/instead of/config/- Includes
fileSuffixin path construction
Consider standardizing the configuration file locations and path construction patterns across all deployment scripts.script/deploy/facets/DeployAcrossFacet.s.sol (2)
29-32: Enhance WETH address validation.Consider maintaining a list of known WETH addresses per network and validating against it in addition to the contract check.
25-32: Consider caching validated addresses.Multiple contract validations in a single function could impact deployment performance. Consider caching the validated addresses if they're used frequently during deployment.
script/deploy/facets/utils/ScriptBase.sol (2)
12-12: Enhance error type with more context.Consider adding the address to the error for better debugging:
-error NotAContract(string key); +error NotAContract(string key, address addr);
28-44: Improve error handling and message construction.The implementation looks good but could be enhanced:
- Use the custom error type instead of string concatenation
- Add address validation before contract check
function _getConfigContractAddress( string memory path, string memory key ) internal returns (address contractAddress) { // load json file string memory json = vm.readFile(path); // read address contractAddress = json.readAddress(key); + // validate address is not zero + if (contractAddress == address(0)) + revert("Zero address not allowed"); // check if address contains code if (!LibAsset.isContract(contractAddress)) - revert( - string.concat(key, " in file ", path, " is not a contract") - ); + revert NotAContract(key, contractAddress); }templates/facetDeployScript.template.hbs (1)
27-33: Enhance documentation clarity.The comments are helpful but could be more precise:
-// If you need to read an address from your config file or from a network deploy log that is supposed to be a contract, use the -// following helper function which makes sure that the address contains code: -// -// address example = _getConfigContractAddress(json,string.concat(".", network, ".weth")); -// -// in the address is not a supposed to be an EOA, you can use the following standard approach: -address example = json.readAddress(".RefundWallet"); +// For contract addresses (e.g., tokens, bridges), use _getConfigContractAddress to ensure the address contains code: +// address weth = _getConfigContractAddress(path, string.concat(".", network, ".weth")); +// +// For Externally Owned Accounts (EOAs) like wallets, use the standard json.readAddress: +address refundWallet = json.readAddress(".RefundWallet");script/deploy/facets/DeployPermit2Proxy.s.sol (1)
24-31: Consider extracting repeated path construction logic.The pattern of constructing config file paths is repeated across multiple locations. Consider extracting this into a helper function in the base class.
+ function getConfigPath(string memory fileName) internal view returns (string memory) { + return string.concat(root, "/config/", fileName); + } + + function getDeploymentPath() internal view returns (string memory) { + return string.concat(root, "/deployments/", network, ".", fileSuffix, "json"); + }Also applies to: 36-36, 44-44
script/deploy/facets/DeployReceiverAcrossV3.s.sol (1)
Line range hint
25-35: Consider using _getConfigContractAddress for refundWallet.For consistency and security, consider using
_getConfigContractAddressfor reading the refundWallet address as well, ensuring it's a valid contract address if intended to be one.script/deploy/facets/DeployReceiverStargateV2.s.sol (2)
Line range hint
25-35: Consider standardizing config reading approach.The file mixes two approaches for reading configuration:
- Direct JSON reading for refundWallet
_getConfigContractAddressfor contract addressesConsider standardizing the approach across all address readings for consistency and better maintainability.
Also applies to: 42-48, 60-60
Line range hint
1-1: Consider implementing a ConfigurationManager contract.Given the repeated patterns across deployment scripts, consider implementing a dedicated ConfigurationManager contract/class that would:
- Centralize all configuration reading logic
- Implement secure path construction
- Standardize contract address validation
- Manage network-specific parameters (gas values, etc.)
- Cache validated addresses to optimize deployment scripts
This would improve maintainability, security, and consistency across all deployment scripts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
config/across.json(2 hunks)deployments/optimism.json(1 hunks)script/deploy/facets/DeployAcrossFacet.s.sol(1 hunks)script/deploy/facets/DeployAcrossFacetPacked.s.sol(1 hunks)script/deploy/facets/DeployAcrossFacetPackedV3.s.sol(1 hunks)script/deploy/facets/DeployAcrossFacetV3.s.sol(1 hunks)script/deploy/facets/DeployAllBridgeFacet.s.sol(1 hunks)script/deploy/facets/DeployAmarokFacet.s.sol(1 hunks)script/deploy/facets/DeployAmarokFacetPacked.s.sol(1 hunks)script/deploy/facets/DeployArbitrumBridgeFacet.s.sol(1 hunks)script/deploy/facets/DeployCBridgeFacet.s.sol(1 hunks)script/deploy/facets/DeployCBridgeFacetPacked.s.sol(1 hunks)script/deploy/facets/DeployCelerCircleBridgeFacet.s.sol(1 hunks)script/deploy/facets/DeployCelerIMFacet.s.sol(2 hunks)script/deploy/facets/DeployDeBridgeDlnFacet.s.sol(1 hunks)script/deploy/facets/DeployExecutor.s.sol(1 hunks)script/deploy/facets/DeployGasZipFacet.s.sol(1 hunks)script/deploy/facets/DeployGasZipPeriphery.s.sol(1 hunks)script/deploy/facets/DeployGenericSwapFacetV3.s.sol(1 hunks)script/deploy/facets/DeployGnosisBridgeFacet.s.sol(1 hunks)script/deploy/facets/DeployLiFiDiamond.s.sol(1 hunks)script/deploy/facets/DeployMayanFacet.s.sol(1 hunks)script/deploy/facets/DeployOmniBridgeFacet.s.sol(1 hunks)script/deploy/facets/DeployPermit2Proxy.s.sol(1 hunks)script/deploy/facets/DeployPolygonBridgeFacet.s.sol(1 hunks)script/deploy/facets/DeployReceiver.s.sol(2 hunks)script/deploy/facets/DeployReceiverAcrossV3.s.sol(2 hunks)script/deploy/facets/DeployReceiverStargateV2.s.sol(2 hunks)script/deploy/facets/DeployRelayFacet.s.sol(1 hunks)script/deploy/facets/DeployRelayerCelerIM.s.sol(2 hunks)script/deploy/facets/DeploySquidFacet.s.sol(1 hunks)script/deploy/facets/DeployStargateFacet.s.sol(1 hunks)script/deploy/facets/DeployStargateFacetV2.s.sol(1 hunks)script/deploy/facets/DeploySymbiosisFacet.s.sol(1 hunks)script/deploy/facets/DeployThorSwapFacet.s.sol(1 hunks)script/deploy/facets/DeployTokenWrapper.s.sol(1 hunks)script/deploy/facets/UpdateCoreFacets.s.sol(1 hunks)script/deploy/facets/UpdateOptimismBridgeFacet.s.sol(1 hunks)script/deploy/facets/utils/ScriptBase.sol(2 hunks)templates/facetDeployScript.template.hbs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- deployments/optimism.json
- config/across.json
🧰 Additional context used
📓 Learnings (3)
script/deploy/facets/DeployGasZipFacet.s.sol (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: script/deploy/facets/DeployGasZipFacet.s.sol:22-35
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In deployment scripts like `DeployGasZipFacet.s.sol`, do not report issues suggesting adding error handling for missing configuration files or unexpected JSON structures, as the script will fail if the file is missing.
script/deploy/facets/DeployGasZipPeriphery.s.sol (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: script/deploy/facets/DeployGasZipPeriphery.s.sol:1-50
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In scripts with *.s.sol extensions executed locally, such as deployment scripts, authentication and specific error handling are not necessary, as they will be executed locally only.
script/deploy/facets/DeployPermit2Proxy.s.sol (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/facets/DeployPermit2Proxy.s.sol:17-20
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In `DeployScriptBase`, `getConstructorArgs()` is called and constructor arguments are handled, so child deploy scripts do not need to pass constructor arguments explicitly to the `deploy` function.
🔇 Additional comments (42)
script/deploy/facets/DeployMayanFacet.s.sol (1)
26-28: Verify the contract validation in_getConfigContractAddress.The change aligns with the PR objective of validating contract addresses. However, we should verify that
_getConfigContractAddressproperly implements the contract validation check.Let's examine the implementation of
_getConfigContractAddressin the base class:✅ Verification successful
Contract validation is properly implemented in
_getConfigContractAddressThe function includes contract validation using
LibAsset.isContractand reverts with a descriptive error message if the address does not contain code. This aligns with the PR objective of adding contract validation checks when reading addresses.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the implementation of _getConfigContractAddress ast-grep --pattern 'function _getConfigContractAddress($$$) $$$' # Verify that isContract check is implemented rg -A 10 'isContract|extcodesize'Length of output: 70509
Script:
#!/bin/bash # Search for DeployScriptBase implementation and config-related functions rg -A 10 "contract DeployScriptBase" # Search for _getConfigContractAddress implementation rg -A 10 "_getConfigContractAddress" # Search for config-related functions in deploy scripts fd -e sol -e s.sol . | xargs rg -l "getConfig|readConfig|_getConfig"Length of output: 42601
Script:
#!/bin/bash # Get the implementation of _getConfigContractAddress from ScriptBase.sol rg -A 15 "function _getConfigContractAddress" "script/deploy/facets/utils/ScriptBase.sol"Length of output: 593
script/deploy/facets/DeployOmniBridgeFacet.s.sol (3)
25-31: Ensure path validation for config file access.The code constructs paths by concatenating strings with the
networkvariable. While this is a common pattern, it's important to ensure that:
- The
networkvariable is properly sanitized to prevent path traversal attacks- The config file paths are validated to be within the expected directory structure
Let's verify the path validation:
25-31: Verify contract validation in_getConfigContractAddress.The refactoring to use
_getConfigContractAddressaligns with the PR objective of validating contract addresses. However, since the implementation of this function is not visible in the current file, we need to verify that it properly implements the contract validation check.Let's verify the implementation:
✅ Verification successful
Contract validation is properly implemented in
_getConfigContractAddressThe function correctly validates contract addresses using
LibAsset.isContract()and provides clear error messages that include both the key and file path for debugging.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find and analyze the implementation of _getConfigContractAddress # to ensure it includes contract validation checks # Search for the function implementation ast-grep --pattern 'function _getConfigContractAddress($_, $_) { $$$ }' # Look for contract validation patterns rg -A 5 'isContract|extcodesize'Length of output: 67639
Script:
#!/bin/bash # Search for the implementation of _getConfigContractAddress in ScriptBase files rg -A 10 "_getConfigContractAddress" "script/deploy/facets/utils/ScriptBase.sol" "script/deploy/facets/utils/DeployScriptBase.sol"Length of output: 924
Script:
#!/bin/bash # Get more context around _getConfigContractAddress implementation rg -A 20 "_getConfigContractAddress" "script/deploy/facets/utils/ScriptBase.sol"Length of output: 584
25-31: Add comprehensive error handling for config operations.The code should handle various failure scenarios when reading from config files:
- Missing config files
- Malformed JSON
- Missing or invalid addresses in the config
Let's check the existing error handling:
script/deploy/facets/DeployDeBridgeDlnFacet.s.sol (1)
27-29: Verify the contract validation implementation in_getConfigContractAddress.The change introduces a more structured way to read contract addresses, but we need to ensure that the contract validation is properly implemented.
Let's verify the implementation of
_getConfigContractAddressand its contract validation:Additionally, verify that the DLN source contract exists at the specified path:
✅ Verification successful
Contract validation is properly implemented and used correctly.
The
_getConfigContractAddressmethod includes proper contract validation usingLibAsset.isContract, which checks the contract size usingextcodesize. The implementation will revert with a clear error message if the address from the config doesn't contain code.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of _getConfigContractAddress in the base class # and verify if it includes the isContract check. # Find the base class implementation rg -A 10 "_getConfigContractAddress" "script/deploy/facets/utils/" # Check if isContract is used in the implementation ast-grep --pattern 'function _getConfigContractAddress($$$) { $$$ isContract($$$) $$$ }'Length of output: 959
Script:
#!/bin/bash # Check if the DLN config file exists and contains dlnSource fd -t f "dln.json" config/ --exec cat {} | jq -r ".[].dlnSource"Length of output: 141
Script:
#!/bin/bash # Check the actual structure of config files fd -t f ".json" config/ --exec cat {} | head -n 20 # Look for any contract validation mechanisms rg -A 5 "isContract|readAddress" "script/" # Check if there are any other files related to DLN configuration fd -t f . config/ | grep -i "dln"Length of output: 29378
Script:
#!/bin/bash # Check DLN config content and structure cat config/dln.json # Find LibAsset.isContract implementation rg -A 5 "LibAsset.isContract|function isContract" "contracts/"Length of output: 1878
Script:
#!/bin/bash # Search for LibAsset.isContract implementation in the entire repository rg -A 5 "LibAsset.isContract|library LibAsset" # Search for any .sol files containing LibAsset fd -e sol . | xargs rg "LibAsset"Length of output: 123990
script/deploy/facets/DeployStargateFacetV2.s.sol (1)
25-27: Verify the contract validation logic in_getConfigContractAddress.The change aligns with the PR objective of validating contract addresses. However, we should verify the implementation of the validation logic.
✅ Verification successful
Contract validation is properly implemented in
_getConfigContractAddressThe method validates contract addresses using LibAsset.isContract (extcodesize check) and provides clear error messages. The implementation aligns with the PR objectives of adding contract address validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the implementation of _getConfigContractAddress ast-grep --pattern 'function _getConfigContractAddress($_, $_) { $$$ }' # Look for isContract checks in the codebase rg 'isContract|extcodesize'Length of output: 79696
Script:
#!/bin/bash # Search for _getConfigContractAddress implementation ast-grep --pattern 'function _getConfigContractAddress($_, $_) { $$$ }' # Search for files that might contain the implementation rg "_getConfigContractAddress" --type solLength of output: 145
Script:
#!/bin/bash # Search for the method name and inheritance rg -t sol "_getConfigContractAddress" # Find base classes in deploy scripts rg -t sol "contract.*DeployScriptBase|abstract contract.*Deploy" -A 5 # Look for config-related functions in deploy scripts rg -t sol "function.*[Cc]onfig.*ddress" -A 3Length of output: 240
Script:
#!/bin/bash # Search for the method name rg "_getConfigContractAddress" # Find base classes in deploy scripts rg "contract.*DeployScriptBase|abstract contract.*Deploy" script/ # Look for config-related functions rg "getConfig|_getConfig" script/ # Find files that might contain the implementation fd . script/ -e solLength of output: 29790
Script:
#!/bin/bash # Get the implementation of _getConfigContractAddress cat script/deploy/facets/utils/ScriptBase.solLength of output: 1462
script/deploy/facets/DeployThorSwapFacet.s.sol (2)
25-27: Verify the contract validation logic in_getConfigContractAddress.The method name suggests it performs contract validation, but we need to verify its implementation to ensure proper address validation.
Let's check the implementation of
_getConfigContractAddress:✅ Verification successful
Contract validation is properly implemented in
_getConfigContractAddressThe function includes proper contract validation through
LibAsset.isContract()and throws a descriptive error if the address doesn't contain code. This ensures that the thorchainRouter address will be validated during deployment.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the implementation of _getConfigContractAddress ast-grep --pattern 'function _getConfigContractAddress($$$) { $$$ }'Length of output: 72
Script:
#!/bin/bash # First, find the DeployScriptBase file fd "DeployScriptBase" # Then, let's try a more flexible search pattern for the function rg "_getConfigContractAddress" -A 10Length of output: 38949
Script:
#!/bin/bash # Get the implementation from ScriptBase.sol cat script/deploy/facets/utils/ScriptBase.solLength of output: 1462
25-27: Verify the thorchainRouter address in the config file.Since this is a critical contract address that will be used for swaps, we should verify that the configured address in the JSON file matches the official ThorChain Router for each network.
Let's check the configured addresses:
script/deploy/facets/DeployRelayerCelerIM.s.sol (3)
Line range hint
24-35: Consider validating refundWallet address consistency.The
refundWalletaddress is still being read using the oldreadAddressmethod without contract validation. While it might not be a contract address, consider:
- Using a consistent method for all address retrievals
- Adding validation to ensure it's not a contract address (as it should be an EOA)
- Verifying its consistency across different deployment configurations
Run this script to check refundWallet configurations:
52-52: Verify LiFiDiamond proxy implementation.Since this is accessing the Diamond proxy address, ensure that:
- The address points to a valid contract
- The Diamond has the correct facets installed
Run this script to verify the Diamond setup:
✅ Verification successful
Diamond proxy implementation verified successfully
The Diamond proxy implementation follows EIP-2535 specification with:
- Secure initialization and ownership controls
- Proper facet management (add/replace/remove)
- Protected function delegation
- Comprehensive deployment configuration across chains
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Diamond proxy setup in deployment artifacts # Search for Diamond-related patterns in the codebase ast-grep --pattern 'contract LiFiDiamond { $$$ }' # Check deployment artifacts for Diamond implementation for file in $(fd -e json . deployments/); do if jq -e 'select(.contractName == "LiFiDiamond")' "$file" > /dev/null 2>&1; then echo "Diamond deployment found in: $file" jq '{address: .address, facets: (.abi | map(select(.type == "function")) | length)}' "$file" fi doneLength of output: 29478
Script:
#!/bin/bash # Search for Diamond contract implementation and facet setup echo "=== Diamond Contract Implementation ===" ast-grep --pattern 'contract LiFiDiamond { $$$ }' -f .sol echo -e "\n=== Facet Registration Patterns ===" rg -A 5 "function registerFacet|registerFacets" . echo -e "\n=== Diamond Cut Function Calls ===" rg -A 3 "diamondCut\(" . echo -e "\n=== Diamond Configuration in Config Files ===" fd -e json -e yaml -e toml . config/ -x cat {} 2>/dev/nullLength of output: 68990
Script:
#!/bin/bash # Search for Diamond deployment and configuration echo "=== Diamond Deployment Configuration ===" rg -A 5 "LiFiDiamond\(" . echo -e "\n=== Diamond Configuration Files ===" fd -e json -e yaml -e toml . config/ -x cat {} 2>/dev/null echo -e "\n=== Facet Registration in Deploy Scripts ===" rg -A 5 "new FacetCut\[\]|registerFacet|addFacet" . echo -e "\n=== Diamond Storage Access ===" rg -A 3 "_getConfigContractAddress.*LiFiDiamond" .Length of output: 83397
39-41: Verify that messageBus is a valid contract address.The
_getConfigContractAddressmethod should validate that the messageBus address points to a deployed contract. This is crucial for cross-chain messaging functionality.Run this script to verify the contract:
✅ Verification successful
Contract validation is properly implemented for messageBus address
The _getConfigContractAddress method in ScriptBase.sol enforces contract validation using LibAsset.isContract, which checks the contract existence using extcodesize. The implementation will revert deployment if the messageBus address does not contain code, making it impossible to deploy with an invalid contract address.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the messageBus configuration contains valid contract addresses # across different networks # Search for messageBus addresses in config files rg -l "messageBus" config/ # Check if the addresses are mentioned in any deployment artifacts for file in $(fd -e json . deployments/); do if jq -e '.address' "$file" > /dev/null 2>&1; then echo "Deployment found in: $file" jq '{address: .address, abi: (.abi | length)}' "$file" fi doneLength of output: 34080
Script:
#!/bin/bash # Find and show _getConfigContractAddress implementation ast-grep --pattern 'function _getConfigContractAddress($$$) { $$$ }' # Show messageBus addresses from config echo "MessageBus addresses from config:" jq -r '.. | .messageBus? | select(. != null)' config/cbridge.jsonLength of output: 776
Script:
#!/bin/bash # Find script files that might contain the base implementation echo "=== Potential Base Scripts ===" fd -e sol . script/ # Find files containing getConfigContractAddress echo -e "\n=== Files with getConfigContractAddress ===" rg -l "getConfigContractAddress" . # Show the implementation if found echo -e "\n=== Implementation Details ===" rg -A 10 "getConfigContractAddress" .Length of output: 48732
Script:
#!/bin/bash # Show the complete implementation of _getConfigContractAddress echo "=== Complete Implementation ===" rg -U "_getConfigContractAddress.*?\}" script/deploy/facets/utils/ScriptBase.sol # Also check for any isContract or similar validation functions echo -e "\n=== Contract Validation Functions ===" rg "isContract|ExtendedType.isContract|.code.length" script/Length of output: 1144
Script:
#!/bin/bash # Find LibAsset.isContract implementation echo "=== LibAsset Implementation ===" rg -l "LibAsset" . rg -U "library LibAsset.*?isContract.*?}" . # Show complete ScriptBase.sol implementation echo -e "\n=== ScriptBase Implementation ===" cat script/deploy/facets/utils/ScriptBase.solLength of output: 77494
Script:
#!/bin/bash # Show ScriptBase.sol content using rg with context echo "=== ScriptBase Implementation ===" rg -U "abstract contract ScriptBase.*?}$" script/deploy/facets/utils/ScriptBase.sol # Specifically show _getConfigContractAddress echo -e "\n=== Config Contract Address Implementation ===" rg -U "_getConfigContractAddress.*?}$" script/deploy/facets/utils/ScriptBase.solLength of output: 354
Script:
#!/bin/bash # Try multiple approaches to get the implementation echo "=== Attempt 1: Direct file read ===" cat script/deploy/facets/utils/ScriptBase.sol echo -e "\n=== Attempt 2: Simple grep ===" grep -A 20 "_getConfigContractAddress" script/deploy/facets/utils/ScriptBase.sol echo -e "\n=== Attempt 3: Find imports and usages ===" grep -l "LibAsset" script/deploy/facets/utils/ScriptBase.sol grep "isContract" script/deploy/facets/utils/ScriptBase.solLength of output: 2522
script/deploy/facets/DeployCBridgeFacet.s.sol (1)
25-27: LGTM! Verify JSON path construction for cBridge address.The implementation correctly uses
_getConfigContractAddressto validate the contract address, following the new pattern.✅ Verification successful
✓ JSON path construction is correct and matches config structure
The path construction
.{network}.cBridgecorrectly maps to contract addresses in config/cbridge.json for all supported networks.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify JSON structure matches the expected path jq ".[\"$NETWORK\"].cBridge" config/cbridge.json || echo "Invalid JSON path"Length of output: 45
Script:
#!/bin/bash # Check if file exists and show its content echo "=== Checking config/cbridge.json ===" if [ -f config/cbridge.json ]; then cat config/cbridge.json else echo "File not found" fi echo -e "\n=== Searching for other cBridge config files ===" fd -t f -e json cbridgeLength of output: 10977
script/deploy/facets/DeployStargateFacet.s.sol (1)
25-27: Consider standardizing JSON path structure across facets.While the implementation correctly validates the contract address, the JSON path structure differs from other facets:
- Other facets:
".<network>.<key>"- This facet:
".composers.<network>"Consider standardizing to reduce maintenance overhead.
script/deploy/facets/DeployAllBridgeFacet.s.sol (1)
25-27: Verify network variable sanitization for path construction.The path construction uses string concatenation with the
networkvariable. Ensure thatnetworkis properly sanitized to prevent path traversal attacks.✅ Verification successful
Network variable usage is secure in path construction.
The network value is:
- Sourced securely from environment variables using forge-std's vm.envString
- Used only as a JSON key in string concatenation
- Protected by _getConfigContractAddress which validates the returned contract address
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for network variable sanitization in base classes rg -A 5 "network\s*=|network.*=" --type solLength of output: 73
Script:
#!/bin/bash # Find Solidity file extensions fd -e sol . -t f # Search for network variable in Solidity files rg -t sol "network\s*=|network.*=" || true # Search for base script implementation rg -t sol "contract.*Script" -A 5 # Search for _getConfigContractAddress implementation ast-grep --pattern '_getConfigContractAddress($$$)'Length of output: 15926
Script:
#!/bin/bash # Search for network variable and _getConfigContractAddress in base scripts cat script/deploy/facets/utils/ScriptBase.sol cat script/deploy/facets/utils/DeployScriptBase.sol # Check the specific deployment script cat script/deploy/facets/DeployAllBridgeFacet.s.solLength of output: 4676
script/deploy/facets/DeployGnosisBridgeFacet.s.sol (1)
27-29: Verify contract validation for xDaiBridge address.The PR aims to add contract validation. Please provide the implementation of
_getConfigContractAddressto ensure proper validation of the xDaiBridge contract address.✅ Verification successful
Contract validation for xDaiBridge address is properly implemented.
The
_getConfigContractAddressfunction inScriptBase.solprovides standardized contract address validation through JSON config files, consistent with the pattern used across all bridge facets in the project.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for _getConfigContractAddress implementation rg -l "_getConfigContractAddress" --type solLength of output: 72
Script:
#!/bin/bash # Search for _getConfigContractAddress implementation without file type restriction rg "_getConfigContractAddress" -A 5Length of output: 30095
script/deploy/facets/DeployCBridgeFacetPacked.s.sol (1)
Line range hint
25-32: Request implementation details of contract validation.The changes consistently replace direct JSON reading with
_getConfigContractAddressacross all deployment scripts, which aligns with the PR objective. However, the following items need clarification:
- Implementation of
_getConfigContractAddressand its contract validation logic- Handling of validation failures
- Impact on deployment process if validation fails
- Test coverage for the new validation logic
✅ Verification successful
Contract validation is properly implemented in
_getConfigContractAddressThe function includes comprehensive validation:
- Verifies contract code presence using
LibAsset.isContract- Fails deployment with clear error message if validation fails
- Consistently used across all deployment scripts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files covering the new validation logic rg -l "test.*_getConfigContractAddress|_getConfigContractAddress.*test" --type solLength of output: 112
Script:
#!/bin/bash # Find implementation of _getConfigContractAddress ast-grep --pattern 'function _getConfigContractAddress($$$) { $$$ }' # Search for test files without file type filter rg -l "test.*_getConfigContractAddress|_getConfigContractAddress.*test" # Search for any validation-related code near _getConfigContractAddress rg -B 2 -A 5 "_getConfigContractAddress"Length of output: 37930
Script:
#!/bin/bash # Find the implementation of _getConfigContractAddress in ScriptBase.sol rg -A 15 "function _getConfigContractAddress" script/deploy/facets/utils/ScriptBase.solLength of output: 593
script/deploy/facets/DeployAmarokFacetPacked.s.sol (2)
27-29: Verify path construction and existence.The path construction using string concatenation could be vulnerable to path manipulation. Consider:
- Validating the network variable
- Adding path existence checks before usage
27-29: Verify contract validation implementation.Please ensure that
_getConfigContractAddressimplements proper contract validation usingextcodesizeor similar checks.✅ Verification successful
Contract validation is properly implemented using extcodesize checks
The
_getConfigContractAddressfunction usesLibAsset.isContract()which implements proper contract validation usingextcodesizeassembly checks. The implementation includes appropriate error handling and follows security best practices.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for contract validation implementation ast-grep --pattern 'function _getConfigContractAddress($_, $_) { $$$ }'Length of output: 75
Script:
#!/bin/bash # First, let's look at the deployment script content cat script/deploy/facets/DeployAmarokFacetPacked.s.sol # Also search for files containing _getConfigContractAddress rg "_getConfigContractAddress" -lLength of output: 2936
Script:
#!/bin/bash # Check the base contract implementations cat script/deploy/facets/utils/DeployScriptBase.sol cat script/deploy/facets/utils/ScriptBase.sol # Also search for any extcodesize checks in the codebase rg "extcodesize" -A 3Length of output: 70540
script/deploy/facets/DeployRelayFacet.s.sol (1)
25-31: Enhance error handling for contract validation failures.Ensure that
_getConfigContractAddressprovides clear error messages when:
- The address is not a contract
- The contract is not deployed
- The configuration file is missing or malformed
script/deploy/facets/DeployGenericSwapFacetV3.s.sol (1)
26-32: Verify contract validation for native address.The change to use
_getConfigContractAddressaligns with the PR objective of adding contract validation. However, we should verify if the native address should actually be a contract address, as it typically represents the zero address or a native token.script/deploy/facets/DeploySymbiosisFacet.s.sol (1)
24-30: LGTM! Contract validation is appropriate for Symbiosis integration.The change to validate that both
metaRouterandgatewayare contract addresses is correct, as these are critical protocol contracts that must exist for the integration to work.script/deploy/facets/DeployArbitrumBridgeFacet.s.sol (1)
27-33: LGTM! Contract validation is appropriate for Arbitrum bridge contracts.The change to validate that both
gatewayRouterandinboxare contract addresses is correct, as these are core Arbitrum protocol contracts that must exist for the bridge to function.script/deploy/facets/DeployAcrossFacetPacked.s.sol (2)
27-33: LGTM! Contract validation and variable naming improvements.The changes improve code clarity through better variable naming (
spokePool→acrossSpokePool) and add appropriate contract validation for both the Across protocol contract and WETH token contract.
36-36: Verify deployerAddress usage.The
deployerAddressis included in the constructor args but isn't validated as a contract. This is correct if it's meant to be an EOA, but we should verify its intended usage.✅ Verification successful
deployerAddressusage is correctThe
deployerAddressis correctly used as an EOA (Externally Owned Account) derived from the deployment private key. This is the standard pattern used across all facet deployments in the codebase, where the deployer address is used for administrative purposes during deployment.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check deployerAddress usage in other files rg "deployerAddress" . --type solLength of output: 61
Script:
#!/bin/bash # Search for deployerAddress usage without file type restriction rg "deployerAddress" -A 3 -B 3 # Search for constructor patterns with deployerAddress ast-grep --pattern 'constructor($$$) { $$$ }' # Search specifically in the deployment script cat script/deploy/facets/DeployAcrossFacetPacked.s.solLength of output: 10506
script/deploy/facets/DeployAcrossFacetPackedV3.s.sol (1)
27-36: LGTM! Improved variable naming and contract validation.The changes properly implement contract validation via
_getConfigContractAddresswhile improving clarity through better variable naming (acrossSpokePool, weth).script/deploy/facets/DeployPolygonBridgeFacet.s.sol (1)
27-32: LGTM! Consistent implementation of contract validation.The changes properly implement contract validation for both critical dependencies (rootChainManager and erc20Predicate).
script/deploy/facets/DeployCelerCircleBridgeFacet.s.sol (1)
31-34: Verify USDC contract validation behavior.While the changes are consistent, note that USDC might be a token address which may not be a contract on all networks. Please verify that
_getConfigContractAddresshandles token addresses appropriately.✅ Verification successful
USDC contract validation behavior is correct and safe.
The
_getConfigContractAddressfunction explicitly validates that the address contains code usingLibAsset.isContract(), which is appropriate for USDC tokens as they must be smart contracts implementing ERC20 functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if USDC addresses in config are valid contracts # Get the USDC address from config and verify its contract status rg -l "usdc.*0x[a-fA-F0-9]{40}" config/Length of output: 84
Script:
#!/bin/bash # Check USDC entries in config files and _getConfigContractAddress implementation echo "=== USDC addresses in configs ===" rg "usdc.*0x[a-fA-F0-9]{40}" config/ echo -e "\n=== _getConfigContractAddress implementation ===" ast-grep --pattern 'function _getConfigContractAddress($$$) { $$$ }'Length of output: 1176
Script:
#!/bin/bash # Search for _getConfigContractAddress implementation echo "=== Searching for _getConfigContractAddress ===" rg "_getConfigContractAddress" -A 5Length of output: 30200
Script:
#!/bin/bash # Get implementation of _getConfigContractAddress from ScriptBase.sol echo "=== _getConfigContractAddress implementation ===" cat script/deploy/facets/utils/ScriptBase.solLength of output: 1569
script/deploy/facets/DeployAcrossFacetV3.s.sol (1)
25-32: LGTM! Consistent implementation of contract validation.The changes properly implement contract validation for acrossSpokePool and weth addresses.
script/deploy/facets/UpdateOptimismBridgeFacet.s.sol (1)
33-38: LGTM! Good use of the new contract validation.The implementation correctly uses
_getConfigContractAddressto validate that the standardBridge address points to an actual contract.script/deploy/facets/utils/ScriptBase.sol (1)
6-7: LGTM! Good choice of dependencies.The imports and using directive are appropriate for the implementation.
Also applies to: 10-10
script/deploy/facets/DeployTokenWrapper.s.sol (1)
24-30: LGTM! Correct usage of contract validation.Good implementation:
- Uses contract validation for wrappedNativeAddress which should be a contract
- Correctly uses direct reading for refundWalletAddress which is expected to be an EOA
script/deploy/facets/DeployPermit2Proxy.s.sol (2)
33-33: LGTM: Proper contract validation implemented.The use of
_getConfigContractAddressensures that all addresses read from config files are validated as actual contract addresses, which aligns with the PR objectives.Also applies to: 38-40, 46-48
24-31: Validate fileSuffix before using in path construction.The
fileSuffixvariable is used directly in path construction without validation, which could potentially lead to path traversal if not properly sanitized.✅ Verification successful
Path construction with fileSuffix is secure
The fileSuffix is safely sourced from environment variables using Foundry's vm.envString, which is controlled by the deployment infrastructure, not user input. The implementation provides adequate security for deployment file path construction.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if fileSuffix is properly validated in ScriptBase rg -A 5 "fileSuffix\s*=|fileSuffix.*=" .Length of output: 1875
script/deploy/facets/DeployGasZipPeriphery.s.sol (1)
26-28: Verify GasZip router configuration across networks.The GasZip router address is network-specific. Ensure all supported networks have valid contract addresses configured.
✅ Verification successful
GasZip router configurations verified across all networks
All supported networks (22 in total) have valid contract addresses configured in
config/gaszip.json. Network-specific router deployments are properly maintained.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check GasZip router configuration across networks jq -r '.gasZipRouters | keys[]' config/gaszip.json | while read -r network; do echo "Checking network: $network" jq -r ".gasZipRouters.$network" config/gaszip.json doneLength of output: 4018
script/deploy/facets/DeployReceiverAcrossV3.s.sol (2)
42-44: LGTM: Proper validation for critical contract addresses.The implementation correctly validates both the Across spoke pool and executor addresses using
_getConfigContractAddress.Also applies to: 56-56
61-66: Consider validating recoverGas value.The hardcoded
recoverGasvalue of 100000 should be validated against network-specific gas limits.script/deploy/facets/DeployReceiverStargateV2.s.sol (1)
42-48: LGTM: Proper validation for LayerZero contracts.The implementation correctly validates both the EndpointV2 and TokenMessaging contract addresses.
script/deploy/facets/DeployReceiver.s.sol (1)
40-43: Verify router addresses in the configuration files.The router addresses (stargateRouter, amarokRouter) and executor are critical infrastructure contracts. While the new
_getConfigContractAddressadds contract validation, we should verify the configured addresses match the official deployments.Run this script to verify the addresses against known deployments:
Also applies to: 48-50, 61-61
script/deploy/facets/DeployCelerIMFacet.s.sol (2)
77-78: Verify diamond contract selection logic.The diamond contract selection depends on the
DIAMOND_TYPEenvironment variable. Ensure this is consistently set across all deployment environments.✅ Verification successful
Diamond contract selection logic is properly implemented and consistent.
The DIAMOND_TYPE environment variable is consistently handled across all deployment scripts with proper validation, error checking, and type-specific deployment paths.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check diamond type configuration consistency # Search for DIAMOND_TYPE usage echo "Checking DIAMOND_TYPE usage..." rg "DIAMOND_TYPE" -A 3Length of output: 11599
42-45: Ensure mainnet-specific token validation.The cfUSDC token validation is correctly mainnet-specific, but we should verify the token contract's interface compatibility.
Run this script to verify the token interface:
Also applies to: 53-56
✅ Verification successful
Token validation is properly implemented
The cfUSDC token validation is correctly implemented with:
- Mainnet-specific address validation
- Immutable storage
- Special case handling using canonical() interface
- Configuration-based address loading
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify cfUSDC token interface compatibility # Search for token interface requirements echo "Checking token interface requirements..." ast-grep --pattern 'interface I$_Token { $$$ function $_($$$) external returns (bool); $$$ }' # Check for any existing token validation tests echo "Checking for token validation tests..." fd -e sol -e js -e ts --full-path "test.*token.*validation"Length of output: 341
Script:
#!/bin/bash # Search for token-related interfaces and imports echo "Checking for token-related interfaces and imports..." rg -l "IERC20|ERC20|Token" --type sol echo -e "\nChecking for token usage patterns..." ast-grep --pattern 'IERC20($$$)' echo -e "\nChecking deployment script token handling..." rg -A 5 "cfUSDC" --type solLength of output: 467
Script:
#!/bin/bash # Find Solidity files first echo "Finding Solidity files..." fd -e sol echo -e "\nChecking token interfaces and validation..." fd -e sol -x rg -l "IERC20|ERC20|Token" {} echo -e "\nChecking cfUSDC usage..." fd -e sol -x rg -A 5 "cfUSDC" {} echo -e "\nChecking token validation patterns..." fd -e sol -x rg -A 3 "require.*address.*" {}Length of output: 25853
script/deploy/facets/UpdateCoreFacets.s.sol (1)
23-56: Verify deployment order and facet dependencies.The sequential validation of core facets is security-critical. We should verify:
- Deployment order dependencies
- Interface compatibility between facets
- No duplicate selectors
✅ Verification successful
Deployment order and selector handling are properly implemented
The script follows best practices:
- Diamond Loupe and Ownership facets are correctly prioritized
- Selector conflicts are prevented through proper extraction and filtering
- Handles both fresh deployments and updates safely
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify facet deployment dependencies and interfaces # Search for interface definitions and selector conflicts echo "Checking facet interfaces..." ast-grep --pattern 'interface $_Facet { $$$ function $_($$$) external; $$$ }' # Check for selector calculations echo "Checking selector handling..." rg "getSelectors|buildDiamondCut" -A 3Length of output: 7358
script/deploy/facets/DeploySquidFacet.s.sol (1)
25-27: Verify Squid router compatibility.The Squid router is critical for cross-chain operations. Ensure the router contract implements the expected interface.
✅ Verification successful
Router interface compatibility confirmed
The codebase includes the ISquidRouter interface with the expected
bridgeCallfunction. The router address validation in the deployment script is compatible with the interface requirements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Squid router interface compatibility # Search for router interface requirements echo "Checking router interface..." ast-grep --pattern 'interface ISquidRouter { $$$ function $_($$$) external; $$$ }' # Check for existing router validation echo "Checking for router validation..." fd -e sol -e js -e ts --full-path "test.*squid.*router"Length of output: 292
Script:
#!/bin/bash # Search for router-related code and imports echo "Checking for router-related imports and usage..." rg -l "router" --type sol echo "Checking specific router usage patterns..." rg -A 3 "router\." --type sol echo "Looking for router-related interfaces..." fd -e sol | xargs cat | grep -A 5 "interface.*Router"Length of output: 2419
| address cBridge = _getConfigContractAddress( | ||
| path, | ||
| string.concat(".", network, ".cBridge") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider validating deployerAddress used in constructor.
While the PR adds contract validation for cBridge, the deployerAddress used in the constructor arguments is not validated. Consider adding similar validation for the deployer address to ensure it exists and has the expected properties.
| address relayReceiver = _getConfigContractAddress( | ||
| path, | ||
| string.concat(".", network, ".relayReceiver") | ||
| ); | ||
|
|
||
| address relaySolver = json.readAddress( | ||
| address relaySolver = _getConfigContractAddress( | ||
| path, | ||
| string.concat(".", network, ".relaySolver") |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for ScriptBase address validation.
While address validation is implemented in ScriptBase._getConfigContractAddress(), there's no test coverage for these validation cases. Add test cases to verify:
- Revert on non-contract addresses
- Revert on zero addresses
- Revert on missing config entries
🔗 Analysis chain
Add test coverage for validation failures.
Add test cases to verify proper handling of:
- Invalid contract addresses
- Non-contract addresses
- Zero addresses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for contract validation
rg -p "test.*ContractValidation|test.*InvalidAddress" .
Length of output: 167
Script:
#!/bin/bash
# Search for DeployRelayFacet test files and relay-related tests
rg -p "DeployRelayFacet" test/
fd "Deploy.*Relay.*\.t\.sol" test/
# Check for address validation patterns in deployment scripts
rg -p "require.*address|revert.*address" script/deploy/
Length of output: 390
Script:
#!/bin/bash
# Search for base deployment script and config contract address implementation
rg -p "_getConfigContractAddress" script/
rg -p "contract.*Deploy.*Base" script/
# Look for common validation utilities
rg -p "function.*validate.*address|function.*check.*address" script/
Length of output: 19306
Script:
#!/bin/bash
# Check ScriptBase implementation
cat script/deploy/facets/utils/ScriptBase.sol
# Look for any revert conditions in base classes
rg -p "revert" script/deploy/facets/utils/
Length of output: 1641
| //TODO: REMOVE | ||
| address erc20Proxy = _getConfigContractAddress(path, ".ERC20Proxy"); |
There was a problem hiding this comment.
Remove TODO comment and unused code.
The erc20Proxy address is retrieved but not used in the return statement. The TODO comment suggests this should be removed.
- //TODO: REMOVE
- address erc20Proxy = _getConfigContractAddress(path, ".ERC20Proxy");
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (13)
script/deploy/facets/DeployStargateFacet.s.sol (1)
25-27: Consider adding a comment about address validation.To make the validation behavior more explicit, consider adding a comment that documents how
_getConfigContractAddressvalidates the address:+ // Retrieves composer address from config and validates it's a contract address address composer = _getConfigContractAddress( path, string.concat(".composers.", network) );script/deploy/facets/DeploySymbiosisFacet.s.sol (1)
24-27: Consider adding descriptive error messagesWhile the implementation is correct, consider passing custom error messages to
_getConfigContractAddressto provide more context when validation fails.address metaRouter = _getConfigContractAddress( path, - string.concat(".", network, ".metaRouter") + string.concat(".", network, ".metaRouter"), + "Symbiosis metaRouter must be a contract address" ); address gateway = _getConfigContractAddress( path, - string.concat(".", network, ".gateway") + string.concat(".", network, ".gateway"), + "Symbiosis gateway must be a contract address" );Also applies to: 28-31
script/deploy/facets/DeployCelerCircleBridgeFacet.s.sol (1)
27-34: Consider future enhancements to address validation.While the current implementation successfully validates that the addresses point to contracts, consider these future enhancements:
- Validate that the contracts implement the expected interfaces (e.g., USDC should implement IERC20).
- Add configuration validation (e.g., ensure USDC decimals match expectations).
script/deploy/facets/DeployGasZipPeriphery.s.sol (1)
32-44: Consider extracting deployment log path construction.The implementation correctly uses the contract validation helper, but the path construction logic could be simplified.
Consider extracting the deployment log path construction to a helper function for reusability:
+ function _getDeploymentLogPath(string memory fileName) internal view returns (string memory) { + return string.concat(root, "/deployments/", network, ".", fileSuffix, "json"); + } function getConstructorArgs() internal override returns (bytes memory) { // ... - path = string.concat( - root, - "/deployments/", - network, - ".", - fileSuffix, - "json" - ); + path = _getDeploymentLogPath("LiFiDEXAggregator"); // ... }script/deploy/facets/DeployRelayFacet.s.sol (1)
25-28: Consider improving error messages for address validation.While the implementation is correct, consider enhancing the key paths passed to
_getConfigContractAddressto include more context in error messages. This would help quickly identify which specific component (RelayFacet) and role (receiver/solver) failed validation.- string.concat(".", network, ".relayReceiver") + string.concat(".", network, ".RelayFacet.relayReceiver") - string.concat(".", network, ".relaySolver") + string.concat(".", network, ".RelayFacet.relaySolver")Also applies to: 29-32
script/deploy/facets/DeployMayanFacet.s.sol (1)
26-28: Consider adding error messages for better debugging.While the implementation is correct, consider making the error handling more specific by passing a descriptive error message to help identify which bridge address failed validation.
address bridge = _getConfigContractAddress( path, - string.concat(".bridges.", network, ".bridge") + string.concat(".bridges.", network, ".bridge"), + string.concat("Mayan bridge address for ", network, " is not a contract") );script/deploy/facets/DeployStargateFacetV2.s.sol (1)
25-27: Consider standardizing the config key format.The key format
.tokenMessaging.${network}differs from the pattern used in other files (e.g.,.${network}.key). Consider standardizing to.${network}.tokenMessagingfor consistency across all deployment scripts.- string.concat(".tokenMessaging.", network) + string.concat(".", network, ".tokenMessaging")script/deploy/facets/DeployLiFiDiamond.s.sol (2)
31-34: Consider standardizing config file locations and key formats.This file differs from others in two ways:
- Uses
deployments/directory instead ofconfig/- Key format
.DiamondCutFacetdoesn't follow the.<network>.keypatternConsider:
- Moving the config to the standard
config/directory- Using the standard key format:
.${network}.DiamondCutFacet- string memory path = string.concat( - root, - "/deployments/", - network, - ".", - fileSuffix, - "json" - ); + string memory path = string.concat(root, "/config/diamond.json"); address diamondCut = _getConfigContractAddress( path, - ".DiamondCutFacet" + string.concat(".", network, ".DiamondCutFacet") );
Line range hint
1-1: Consider documenting the config file structure and key format standards.The implementation successfully adds contract validation across deployment scripts, but there are inconsistencies in:
- Config file locations (some use
config/, others usedeployments/)- Key formats (some use
.key.${network}, others use.${network}.key)Consider:
- Documenting the expected config file structure
- Adding validation for config paths and key formats in
_getConfigContractAddress- Creating a helper function to standardize key construction
Example helper function:
function _constructConfigKey(string memory key) internal view returns (string memory) { return string.concat(".", network, ".", key); }script/deploy/facets/DeployAcrossFacetPacked.s.sol (1)
Line range hint
1-1: Excellent architectural improvement!The systematic refactoring to use
_getConfigContractAddressacross all deployment scripts brings several benefits:
- Prevents deployment with incorrect addresses through contract validation
- Centralizes address retrieval logic, improving maintainability
- Provides consistent error handling across all facets
- Reduces the risk of copy/paste errors mentioned in the PR objectives
Consider adding this validation pattern to the deployment documentation to ensure future facets follow the same practice.
script/deploy/facets/utils/ScriptBase.sol (1)
28-44: Consider enhancing error message with actual address value.The function implementation looks solid with proper validation. However, including the actual address value in the error message would make debugging easier.
- revert( - string.concat(key, " in file ", path, " is not a contract") - ); + revert( + string.concat( + key, + " in file ", + path, + " is not a contract. Address: ", + LibAsset.addressToString(contractAddress) + ) + );script/deploy/facets/DeployReceiverStargateV2.s.sol (1)
Line range hint
29-33: Consider validating refundWallet address.While the PR adds contract validation for config addresses, the
refundWalletaddress is still read directly without validation. Although it's likely an EOA (not a contract), consider adding a validation check to ensure it's a valid address.script/deploy/facets/DeployReceiver.s.sol (1)
Line range hint
66-72: Document the magic number in constructor args.The value
100000is hardcoded in the constructor arguments without explanation. Consider adding a comment explaining its purpose or extracting it to a named constant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
config/across.json(2 hunks)deployments/optimism.json(1 hunks)script/deploy/facets/DeployAcrossFacet.s.sol(1 hunks)script/deploy/facets/DeployAcrossFacetPacked.s.sol(1 hunks)script/deploy/facets/DeployAcrossFacetPackedV3.s.sol(1 hunks)script/deploy/facets/DeployAcrossFacetV3.s.sol(1 hunks)script/deploy/facets/DeployAllBridgeFacet.s.sol(1 hunks)script/deploy/facets/DeployAmarokFacet.s.sol(1 hunks)script/deploy/facets/DeployAmarokFacetPacked.s.sol(1 hunks)script/deploy/facets/DeployArbitrumBridgeFacet.s.sol(1 hunks)script/deploy/facets/DeployCBridgeFacet.s.sol(1 hunks)script/deploy/facets/DeployCBridgeFacetPacked.s.sol(1 hunks)script/deploy/facets/DeployCelerCircleBridgeFacet.s.sol(1 hunks)script/deploy/facets/DeployCelerIMFacet.s.sol(2 hunks)script/deploy/facets/DeployDeBridgeDlnFacet.s.sol(1 hunks)script/deploy/facets/DeployExecutor.s.sol(1 hunks)script/deploy/facets/DeployGasZipFacet.s.sol(1 hunks)script/deploy/facets/DeployGasZipPeriphery.s.sol(1 hunks)script/deploy/facets/DeployGenericSwapFacetV3.s.sol(1 hunks)script/deploy/facets/DeployGnosisBridgeFacet.s.sol(1 hunks)script/deploy/facets/DeployLiFiDiamond.s.sol(1 hunks)script/deploy/facets/DeployMayanFacet.s.sol(1 hunks)script/deploy/facets/DeployOmniBridgeFacet.s.sol(1 hunks)script/deploy/facets/DeployPermit2Proxy.s.sol(1 hunks)script/deploy/facets/DeployPolygonBridgeFacet.s.sol(1 hunks)script/deploy/facets/DeployReceiver.s.sol(2 hunks)script/deploy/facets/DeployReceiverAcrossV3.s.sol(2 hunks)script/deploy/facets/DeployReceiverStargateV2.s.sol(2 hunks)script/deploy/facets/DeployRelayFacet.s.sol(1 hunks)script/deploy/facets/DeployRelayerCelerIM.s.sol(2 hunks)script/deploy/facets/DeploySquidFacet.s.sol(1 hunks)script/deploy/facets/DeployStargateFacet.s.sol(1 hunks)script/deploy/facets/DeployStargateFacetV2.s.sol(1 hunks)script/deploy/facets/DeploySymbiosisFacet.s.sol(1 hunks)script/deploy/facets/DeployThorSwapFacet.s.sol(1 hunks)script/deploy/facets/DeployTokenWrapper.s.sol(1 hunks)script/deploy/facets/UpdateCoreFacets.s.sol(1 hunks)script/deploy/facets/UpdateOptimismBridgeFacet.s.sol(1 hunks)script/deploy/facets/utils/ScriptBase.sol(2 hunks)templates/facetDeployScript.template.hbs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- deployments/optimism.json
- config/across.json
🧰 Additional context used
📓 Learnings (3)
script/deploy/facets/DeployGasZipFacet.s.sol (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: script/deploy/facets/DeployGasZipFacet.s.sol:22-35
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In deployment scripts like `DeployGasZipFacet.s.sol`, do not report issues suggesting adding error handling for missing configuration files or unexpected JSON structures, as the script will fail if the file is missing.
script/deploy/facets/DeployPermit2Proxy.s.sol (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/facets/DeployPermit2Proxy.s.sol:17-20
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In `DeployScriptBase`, `getConstructorArgs()` is called and constructor arguments are handled, so child deploy scripts do not need to pass constructor arguments explicitly to the `deploy` function.
script/deploy/facets/DeployGasZipPeriphery.s.sol (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: script/deploy/facets/DeployGasZipPeriphery.s.sol:1-50
Timestamp: 2024-11-12T09:43:10.543Z
Learning: In scripts with *.s.sol extensions executed locally, such as deployment scripts, authentication and specific error handling are not necessary, as they will be executed locally only.
🔇 Additional comments (49)
script/deploy/facets/DeployGasZipFacet.s.sol (2)
23-23: LGTM! Clean path construction.The path construction using string concatenation is clear and follows the established pattern.
25-27: LGTM! Good use of the new contract address validation.The change to use
_getConfigContractAddressaligns with the PR's objective of validating contract addresses during deployment. This will help catch cases where the config accidentally contains an EOA address instead of a contract address.script/deploy/facets/DeployStargateFacet.s.sol (1)
25-27: Great refactoring to include address validation!The switch to
_getConfigContractAddressadds important runtime validation to ensure the composer address points to an actual contract rather than an EOA. This aligns well with preventing deployment issues from copy/paste errors.script/deploy/facets/DeployGnosisBridgeFacet.s.sol (1)
27-29: LGTM! The changes align with the PR objectives.The implementation correctly uses
_getConfigContractAddressto validate that the xDaiBridge address points to a contract and not an EOA, which helps prevent deployment with incorrect addresses.Let's verify the contract validation functionality:
✅ Verification successful
Contract validation is properly implemented and working as intended.
The
_getConfigContractAddressfunction includes proper contract validation that:
- Verifies addresses contain code using
extcodesize- Reverts with clear error messages if validation fails
- Prevents deployment with incorrect addresses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that _getConfigContractAddress includes isContract check # Expected: Find implementation of _getConfigContractAddress with isContract check # Search for the implementation ast-grep --pattern 'function _getConfigContractAddress($_, $_) { $$$ }' # Search for isContract usage within the implementation rg -A 5 'isContract|extcodesize'Length of output: 67639
script/deploy/facets/DeployCBridgeFacet.s.sol (1)
25-27: LGTM! The change aligns with the PR's objective of validating contract addresses.The switch to
_getConfigContractAddressadds an important safety check to ensure the cBridge address points to an actual contract.Let's verify the implementation of the contract validation:
✅ Verification successful
Contract validation is properly implemented and used correctly.
The
_getConfigContractAddressfunction includes proper contract validation throughLibAsset.isContract, which checksextcodesize > 0and reverts with a clear error message if the address does not point to a contract. This ensures that only valid contract addresses can be configured.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that _getConfigContractAddress includes isContract check # Expected: The base contract should contain isContract validation # Search for the _getConfigContractAddress implementation ast-grep --pattern 'function _getConfigContractAddress($$$) { $$$ }' # Look for isContract usage within the implementation rg -A 5 'isContract|extcodesize'Length of output: 71753
script/deploy/facets/DeploySquidFacet.s.sol (1)
25-27: LGTM! Good security improvement.The change to use
_getConfigContractAddressadds an important validation layer that ensures the router address points to an actual contract and not an EOA, helping prevent deployment issues from copy/paste errors.Let's verify that the base contract implements the expected validation:
✅ Verification successful
Verification successful - Base contract implements proper validation
The
_getConfigContractAddressfunction inScriptBase.solcorrectly implements contract address validation usingLibAsset.isContract, which ensures the router address points to actual contract code. The implementation matches the security objectives mentioned in the original review.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that DeployScriptBase implements isContract check # Expected: Find implementation of _getConfigContractAddress with isContract check # Search for the implementation ast-grep --pattern 'function _getConfigContractAddress($$$) { $$$ extcodesize($$$) $$$ }' # Alternatively, search for the function definition rg "_getConfigContractAddress.*\{" -A 10Length of output: 140
Script:
#!/bin/bash # First find the DeployScriptBase.sol file fd "DeployScriptBase.sol" # Then search for the function implementation rg -l "_getConfigContractAddress" # Get the full implementation context rg "function _getConfigContractAddress" -B 2 -A 15Length of output: 3373
script/deploy/facets/DeployGenericSwapFacetV3.s.sol (2)
26-26: Verify the config file path exists.The hardcoded path assumes a specific directory structure. Let's verify the existence of this configuration file.
✅ Verification successful
Config file path verified successfully.
The networks.json configuration file exists at the expected location in the repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the networks.json config file # Expected: File should exist at the specified path # Check if the config file exists fd -t f "networks.json" -d 2Length of output: 49
29-31: LGTM! Good use of the new contract validation.The change implements the PR's objective by using
_getConfigContractAddresswhich includes contract validation, helping prevent deployment issues from copy/paste errors of EOA addresses.Let's verify the implementation of this validation:
✅ Verification successful
Contract validation confirmed - Implementation is secure
The
_getConfigContractAddressfunction includes mandatory contract validation usingLibAsset.isContract, which will revert the deployment if an EOA address is provided instead of a contract address. This prevents deployment issues from copy/paste errors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of _getConfigContractAddress # Expected: Should find the base contract with isContract validation # Search for the contract validation implementation ast-grep --pattern 'function _getConfigContractAddress($_, $_) { $$$ isContract($$$) $$$ }'Length of output: 99
Script:
#!/bin/bash # First, find files containing the function name rg "_getConfigContractAddress" -l # Then show the implementation with context rg "_getConfigContractAddress" -B 5 -A 15Length of output: 59866
script/deploy/facets/DeploySymbiosisFacet.s.sol (1)
23-31: Excellent improvement in address validation!The switch to
_getConfigContractAddressadds an important safety check to ensure we're deploying with valid contract addresses. This helps prevent common deployment issues where EOA addresses might be accidentally used.Let's verify the contract addresses in the config file:
✅ Verification successful
Config file structure and addresses verified
The config file contains valid contract addresses for both metaRouter and gateway across all supported networks. The
_getConfigContractAddressmethod will correctly read these addresses from the well-structured JSON config.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the addresses in symbiosis.json are actually contracts # and match expected patterns for metaRouter and gateway # Find and read the symbiosis config file fd -t f "symbiosis.json" | xargs cat | jq -r '.[][] | select(.metaRouter != null or .gateway != null) | [.metaRouter, .gateway] | @csv' | \ while IFS=, read -r metaRouter gateway; do # Clean up the quotes metaRouter=$(echo $metaRouter | tr -d '"') gateway=$(echo $gateway | tr -d '"') echo "Found addresses:" echo "MetaRouter: $metaRouter" echo "Gateway: $gateway" # Look for contract-like patterns in the codebase referencing these addresses echo "Checking references to these addresses in the codebase:" rg -l "$metaRouter|$gateway" doneLength of output: 244
Script:
#!/bin/bash # First, let's find and examine the config file echo "Looking for symbiosis config files:" fd -t f "symbiosis.json" echo -e "\nExamining config file structure:" fd -t f "symbiosis.json" | xargs catLength of output: 3398
script/deploy/facets/DeployRelayerCelerIM.s.sol (3)
39-41: LGTM! Using the new contract validation helper.The change aligns with the PR objective by using
_getConfigContractAddressto validate that the messageBus address points to an actual contract.
52-52: LGTM! Using the new contract validation helper.The change aligns with the PR objective by using
_getConfigContractAddressto validate that the diamond address points to an actual contract.
Line range hint
28-31: Consider using contract validation for refundWallet.While refundWallet might intentionally be an EOA (Externally Owned Account), it would be safer to explicitly validate this assumption using a helper method that checks for EOA addresses. This would prevent accidental use of contract addresses for refund operations.
Let's verify if refundWallet is consistently used as an EOA across the codebase:
script/deploy/facets/DeployCelerCircleBridgeFacet.s.sol (2)
31-34: LGTM! Consistent validation approach.The USDC address validation follows the same pattern, ensuring both external contract dependencies are verified during deployment.
Let's verify the contract status of the configured USDC address:
✅ Verification successful
USDC Contract Addresses Verified ✓
All configured USDC addresses are valid and correspond to the official USDC deployments across different networks (Ethereum, Arbitrum, Avalanche, BNB Chain, Base, and Optimism).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that USDC is a contract address # Get the current address and check its bytecode length # Find and read the config file config_file=$(fd -t f "celerCircle.json" config) if [[ -n "$config_file" ]]; then # Extract the USDC address using jq jq -r ".[].usdc" "$config_file" fiLength of output: 408
27-30: LGTM! Good use of the new address validation.The change to use
_getConfigContractAddressforcircleBridgeProxyaligns with the PR's objective of preventing deployments with incorrect addresses. This will help catch cases where the config contains an EOA instead of a contract address.Let's verify the contract status of the current circleBridgeProxy address in the config:
✅ Verification successful
Verification successful - Implementation correctly uses network-specific config
The
_getConfigContractAddressusage matches the JSON structure perfectly, and the contract address validation will happen at deployment time for the network-specific CircleBridge proxy address.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that circleBridgeProxy is a contract address # Get the current address and check its bytecode length # Find and read the config file config_file=$(fd -t f "celerCircle.json" config) if [[ -n "$config_file" ]]; then # Extract the circleBridgeProxy address using jq jq -r ".[].circleBridgeProxy" "$config_file" fiLength of output: 421
Script:
#!/bin/bash # Get the full JSON structure to understand network mapping cat config/celerCircle.jsonLength of output: 1023
script/deploy/facets/DeployGasZipPeriphery.s.sol (2)
24-28: LGTM! Verify config file path.The implementation correctly uses the new
_getConfigContractAddresshelper to validate the GasZip router address.✅ Verification successful
Config file path and structure verified successfully
The
config/gaszip.jsonfile exists and contains the requiredgasZipRoutersconfiguration key, confirming that the path construction and configuration access in the code are correct.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of the gaszip.json config file fd -t f "gaszip.json" config/Length of output: 48
Script:
#!/bin/bash # Inspect the contents of gaszip.json to verify the structure cat config/gaszip.json | jq 'has("gasZipRouters")'Length of output: 57
47-51: LGTM! Verify safe address configuration.The implementation correctly validates the safe address, which is crucial for contract ownership and fund withdrawals.
✅ Verification successful
Safe addresses properly configured for all production networks
The implementation correctly retrieves safe addresses from networks.json, which has valid configurations for all production networks. The only empty safe address entry is for the local test network (localanvil), which is expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of the networks config file and safe address configuration fd -t f "networks.json" config/ # Check if safe address is properly configured for all networks rg -A 1 "safeAddress" config/networks.jsonLength of output: 6632
Script:
#!/bin/bash # Find the network with empty safe address rg -B 2 '"safeAddress": "",' config/networks.json # Check if there are any test networks configured rg -i "test|local|development" config/networks.jsonLength of output: 284
script/deploy/facets/DeployRelayFacet.s.sol (1)
25-32: Improved address validation through _getConfigContractAddress.The refactoring aligns well with the PR's objective of preventing deployments with incorrect addresses. The use of
_getConfigContractAddressensures that bothrelayReceiverandrelaySolverare validated as contract addresses rather than EOAs.Let's verify the contract validation for these addresses:
✅ Verification successful
Contract validation confirmed for relay addresses.
The
_getConfigContractAddressimplementation in ScriptBase.sol properly validates that bothrelayReceiverandrelaySolverare contract addresses usingLibAsset.isContract(). The method will revert if either address is an EOA, effectively preventing deployments with incorrect addresses.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that relayReceiver and relaySolver in config are contract addresses # Find and check the relay.json config file config_file=$(fd -t f "relay.json" -E "node_modules") if [ -n "$config_file" ]; then echo "Found config file: $config_file" # Extract and display addresses for manual verification jq -r '.. | .relayReceiver?, .relaySolver? | select(. != null)' "$config_file" fiLength of output: 1987
Script:
#!/bin/bash # Find and examine the implementation of _getConfigContractAddress ast-grep --pattern 'function _getConfigContractAddress($$$) $$$' # Also check the base contract fd -t f "DeployScriptBase" -E "node_modules" --exec cat {}Length of output: 4808
Script:
#!/bin/bash # Find and examine ScriptBase.sol fd -t f "ScriptBase.sol" -E "node_modules" --exec cat {}Length of output: 15569
script/deploy/facets/DeployMayanFacet.s.sol (1)
26-28: LGTM! The change aligns with the PR objectives.The replacement of direct JSON file reading with
_getConfigContractAddressadds contract validation, helping prevent deployment with incorrect addresses.Let's verify the contract validation by checking the implementation of
_getConfigContractAddressand examining the Mayan bridge addresses in the config:✅ Verification successful
Contract validation is properly implemented ✅
The
_getConfigContractAddressfunction in ScriptBase ensures that bridge addresses contain code before deployment, preventing the use of invalid addresses. The implementation includes:
- Reading addresses from config files
- Validating that addresses contain code using
LibAsset.isContract- Clear error reporting with file and key information
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the contract validation implementation and bridge addresses # Check the implementation of _getConfigContractAddress ast-grep --pattern 'function _getConfigContractAddress($_, $_) $_ { $$$ }' # Check Mayan bridge addresses in config cat config/mayan.json | jq '.bridges'Length of output: 669
Script:
#!/bin/bash # Find DeployScriptBase contract and related test files fd -e sol -e t.sol . | xargs rg -l "DeployScriptBase|_getConfigContractAddress" # Search for bridge address validation tests rg -A 5 "0x0654874eb7F59C6f5b39931FC45dC45337c967c3" --type-add 'test:*.t.sol' -t testLength of output: 5442
Script:
#!/bin/bash # Check the implementation of contract validation in DeployScriptBase cat script/deploy/facets/utils/DeployScriptBase.sol # Check the test implementation for validation patterns cat test/solidity/Facets/MayanFacet.t.solLength of output: 20786
Script:
#!/bin/bash # Check the ScriptBase implementation cat script/deploy/facets/utils/ScriptBase.sol # Verify if the bridge address is used in other deployment scripts rg -l "0x0654874eb7F59C6f5b39931FC45dC45337c967c3" script/deploy/Length of output: 1528
script/deploy/facets/DeployPermit2Proxy.s.sol (5)
24-32: LGTM! Clear and consistent path construction.The path construction for the deployments file follows a clear pattern and properly includes network-specific information.
36-36: LGTM! Clear path construction for Permit2 config.The path construction correctly points to the dedicated Permit2Proxy configuration file.
44-44: LGTM! Clear path construction for networks config.The path construction correctly points to the networks configuration file.
33-33: Verify that the LiFiDiamond address is validated.Since LiFiDiamond is a critical contract, ensure that
_getConfigContractAddressperforms the isContract check as intended.✅ Verification successful
✓ LiFiDiamond address validation confirmed
The
_getConfigContractAddresshelper properly validates contract existence usingLibAsset.isContract()and reverts if no code is found at the address.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that _getConfigContractAddress includes isContract check # Expected: Function implementation should include address.code.length check ast-grep --pattern 'function _getConfigContractAddress($_, $_) { $$$ require($_.code.length > 0, $_); $$$ }'Length of output: 116
Script:
#!/bin/bash # Find the file containing _getConfigContractAddress function rg -l "_getConfigContractAddress" # Once found, examine its implementationLength of output: 1848
Script:
#!/bin/bash # Examine the ScriptBase implementation cat script/deploy/facets/utils/ScriptBase.solLength of output: 1462
38-41: Verify that the Permit2 address is validated.Since Permit2 is a critical protocol contract, ensure that
_getConfigContractAddressperforms the isContract check as intended.✅ Verification successful
Permit2 address validation is properly implemented
The
_getConfigContractAddresshelper performs a thorough validation by:
- Reading the address from config
- Verifying the address contains code using
LibAsset.isContract()- Reverting if validation fails
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the Permit2 address exists in the config # Expected: Config file should contain network-specific Permit2 address rg -l "permit2Proxy.json" | xargs cat 2>/dev/null || echo "Config file not found"Length of output: 18956
Script:
#!/bin/bash # Check the implementation of _getConfigContractAddress in DeployScriptBase ast-grep --pattern 'function _getConfigContractAddress($$$) $$$' || rg -A 10 "_getConfigContractAddress"Length of output: 66
Script:
#!/bin/bash # First find DeployScriptBase.sol fd -t f "DeployScriptBase.sol" | xargs catLength of output: 4725
Script:
#!/bin/bash # Find and examine ScriptBase.sol fd -t f "ScriptBase.sol" | xargs catLength of output: 15549
script/deploy/facets/UpdateCoreFacets.s.sol (1)
23-56: LGTM! Consistent usage of the new address validation helper.The systematic replacement of direct JSON reads with
_getConfigContractAddressaligns well with the PR's objective of validating contract addresses.Consider refactoring the repeated pattern for better maintainability.
The repeated pattern of address retrieval could be made more maintainable. Consider creating a mapping or array of facet configurations.
Example approach:
struct FacetConfig { string name; address addr; } FacetConfig[] facets = [ FacetConfig(".DiamondLoupeFacet", address(0)), FacetConfig(".OwnershipFacet", address(0)), // ... other facets ]; for (uint i = 0; i < facets.length; i++) { facets[i].addr = _getConfigContractAddress(path, facets[i].name); }Verify the contract validation implementation.
Since this PR aims to prevent deployment with incorrect addresses, we should verify that
_getConfigContractAddressproperly implements the contract validation.✅ Verification successful
Contract validation is properly implemented and enforced.
The
_getConfigContractAddresshelper ensures all retrieved addresses contain code by:
- Using
LibAsset.isContractto verify contract deployment- Reverting with descriptive error messages if validation fails
- No paths exist to bypass this validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of _getConfigContractAddress in the base script # Search for the implementation ast-grep --pattern 'function _getConfigContractAddress(string $_path, string $_facetName) $_return_type { $$$ }' # Look for isContract check usage rg -A 5 'isContract|extcodesize'Length of output: 71797
Script:
#!/bin/bash # Description: Verify the implementation and usage of _getConfigContractAddress # Find the implementation of _getConfigContractAddress ast-grep --pattern 'function _getConfigContractAddress(string $_path, string $_facetName) $_return_type { $$$ }' # Find any direct uses of the function rg -A 2 '_getConfigContractAddress\('Length of output: 19172
Script:
#!/bin/bash # Description: Find the complete implementation of _getConfigContractAddress # Search for the file content rg -A 15 'function _getConfigContractAddress.*\{' script/deploy/facets/utils/ScriptBase.sol # Also check if there are any other implementations rg -l '_getConfigContractAddress.*\{'Length of output: 133
Script:
#!/bin/bash # Description: Find the implementation using more resilient patterns # View the entire ScriptBase.sol file cat script/deploy/facets/utils/ScriptBase.sol # Backup search with simpler pattern rg -A 20 'ScriptBase' script/deploy/facets/utils/ScriptBase.solLength of output: 2206
script/deploy/facets/DeployCBridgeFacetPacked.s.sol (1)
27-29: LGTM! The change aligns with the PR objectives.The replacement of direct JSON reading with
_getConfigContractAddresshelps prevent deployment with incorrect addresses by adding contract validation.Let's verify the base class implementation to ensure proper contract validation:
✅ Verification successful
Verification successful! The base implementation properly validates contract addresses.
The
_getConfigContractAddressfunction in ScriptBase.sol ensures all addresses from config files point to actual contracts by using LibAsset's isContract check, which validates contract bytecode presence using extcodesize. This aligns with the PR objectives of preventing deployment with incorrect addresses.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of _getConfigContractAddress in the base class # Find and display the base class implementation ast-grep --pattern 'function _getConfigContractAddress($_, $_) $_ { $$$ }' # Check for isContract validation in the base class rg -A 5 'isContract|extcodesize'Length of output: 117691
Script:
#!/bin/bash # Find the base class implementation fd -e sol -e s.sol | xargs rg -l "DeployScriptBase" # Search for _getConfigContractAddress implementation ast-grep --pattern 'function _getConfigContractAddress($_, $_) $_ { $$$ }' # Backup search using ripgrep rg -A 10 "_getConfigContractAddress"Length of output: 43512
Script:
#!/bin/bash # Find and display the complete ScriptBase.sol implementation rg -l "ScriptBase" | xargs cat # Backup search using direct file read cat script/deploy/facets/utils/ScriptBase.solLength of output: 83220
Script:
#!/bin/bash # Try direct file read cat script/deploy/facets/utils/ScriptBase.sol # Backup search with ripgrep for the specific function rg -A 15 "_getConfigContractAddress" script/deploy/facets/utils/ScriptBase.sol # Search for contract validation related code rg "enforceHasContractCode|isContract" script/deploy/facets/utils/ScriptBase.solLength of output: 2179
script/deploy/facets/DeployAmarokFacet.s.sol (1)
25-27: LGTM! Verify contract validation behavior.The implementation correctly uses
_getConfigContractAddressto fetch and validate the connextHandler address from the config file.Let's verify the contract validation behavior:
✅ Verification successful
Implementation correctly handles network-specific connextHandler addresses
The config file structure perfectly matches the code's expectations, with each network having a properly formatted connextHandler address. Active networks (mainnet, optimism, bsc, polygon, etc.) have valid contract addresses, while inactive networks are correctly marked with zero addresses.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if connextHandler address in config is a valid contract # Expected: Address should exist in config and be a contract address # Find and check the config file fd -t f amarok.json | while read -r config; do echo "Checking config: $config" # Extract and validate connextHandler address for each network jq -r 'to_entries[] | .key as $network | .value.connextHandler | select(. != null) | "\($network):\(.)"' "$config" doneLength of output: 350
Script:
#!/bin/bash # First, let's examine the structure of the config file echo "=== Config file structure ===" cat config/amarok.json | head -n 20 echo -e "\n=== Searching for connextHandler mentions ===" # Search for lines containing connextHandler to understand the context grep -A 2 -B 2 "connextHandler" config/amarok.jsonLength of output: 7118
script/deploy/facets/DeployAllBridgeFacet.s.sol (1)
25-27: LGTM! Verify contract validation behavior.The implementation correctly uses
_getConfigContractAddressto fetch and validate the allBridge address from the config file.Let's verify the contract validation behavior:
✅ Verification successful
✅ Contract validation behavior verified
The implementation correctly validates the allBridge contract address by:
- Reading from the standard config file location
- Using the established
_getConfigContractAddressvalidation pattern- Maintaining consistent address validation across all supported networks (mainnet: 0x609c690e8F7D68a59885c9132e812eEbDaAf0c9e, arbitrum: 0x9Ce3447B58D58e8602B7306316A5fF011B92d189, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if allBridge address in config is a valid contract # Expected: Address should exist in config and be a contract address # Find and check the config file fd -t f allbridge.json | while read -r config; do echo "Checking config: $config" # Extract and validate allBridge address for each network jq -r 'to_entries[] | .key as $network | .value.allBridge | select(. != null) | "\($network):\(.)"' "$config" doneLength of output: 671
Script:
#!/bin/bash # Check deployment script validation and contract usage echo "Checking deployment script validation:" ast-grep --pattern 'function _getConfigContractAddress($$$)' -f '*.sol' echo -e "\nChecking AllBridge contract interface/implementation:" rg -A 5 "interface.*AllBridge|contract.*AllBridge" -t solLength of output: 549
Script:
#!/bin/bash # Check deployment script validation and contract usage echo "Checking deployment script validation:" ast-grep --pattern 'function _getConfigContractAddress($_) { $$$ }' echo -e "\nChecking AllBridge contract interface/implementation:" rg -A 5 "interface.*AllBridge|contract.*AllBridge" --type-add 'sol:*.sol' -tsolLength of output: 2311
Script:
#!/bin/bash # Check deployment base script for validation logic echo "Checking base deployment script:" rg -A 10 "_getConfigContractAddress" --type-add 'sol:*.sol' -tsol echo -e "\nChecking config validation patterns:" rg -B 2 -A 5 "require.*address" --type-add 'sol:*.sol' -tsolLength of output: 39909
script/deploy/facets/DeployThorSwapFacet.s.sol (1)
25-27: LGTM! Verify contract validation behavior.The implementation correctly uses
_getConfigContractAddressto fetch and validate the thorchainRouter address from the config file.Let's verify the contract validation behavior:
✅ Verification successful
✅ Contract validation behavior verified and secure
The implementation in
ScriptBase.solproperly validates the thorchainRouter address by:
- Reading the address from the config file
- Verifying it contains deployed code using
LibAsset.isContract- Reverting with a clear error message if validation fails
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if thorchainRouter address in config is a valid contract # Expected: Address should exist in config and be a contract address # Find and check the config file fd -t f thorswap.json | while read -r config; do echo "Checking config: $config" # Extract and validate thorchainRouter address for each network jq -r 'to_entries[] | .key as $network | .value.thorchainRouter | select(. != null) | "\($network):\(.)"' "$config" doneLength of output: 422
Script:
#!/bin/bash # Find implementation of _getConfigContractAddress ast-grep --pattern 'function _getConfigContractAddress($$$) $$$' # Also search for any validation related code rg -A 5 "function _getConfigContractAddress"Length of output: 584
Script:
#!/bin/bash # Get the complete implementation of _getConfigContractAddress cat script/deploy/facets/utils/ScriptBase.solLength of output: 1462
script/deploy/facets/DeployExecutor.s.sol (1)
32-32: Consider consistent key construction across deployment scripts.Unlike other deployment scripts that use network-prefixed keys (e.g.,
string.concat(".", network, ".thorchainRouter")), this script uses a direct key.ERC20Proxy. Consider maintaining consistency in key construction across all deployment scripts.- address erc20Proxy = _getConfigContractAddress(path, ".ERC20Proxy"); + address erc20Proxy = _getConfigContractAddress( + path, + string.concat(".", network, ".ERC20Proxy") + );Also, let's verify the contract validation behavior:
script/deploy/facets/DeployDeBridgeDlnFacet.s.sol (1)
27-29: LGTM! Implementation follows the standard pattern.The key format and path construction are consistent with the expected pattern, and the implementation aligns with the PR's objective of validating contract addresses.
script/deploy/facets/DeployAmarokFacetPacked.s.sol (1)
27-28: LGTM! Verify deployerAddress is intentionally not validated.The implementation correctly validates the connextHandler address. Note that deployerAddress is not validated, which is expected since it's likely an EOA (deployer wallet) rather than a contract.
✅ Verification successful
Confirmed: deployerAddress validation is intentionally omitted
The deployerAddress is derived directly from the deployer's private key using Foundry's vm.addr, ensuring it's a valid EOA. This is consistent across all deployment scripts and follows secure deployment patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if deployerAddress is used consistently across deployment scripts rg -l "deployerAddress" --type solLength of output: 62
Script:
#!/bin/bash # Find all Solidity files and search for deployerAddress with context fd -e sol | xargs rg "deployerAddress" -A 2 -B 2Length of output: 6757
script/deploy/facets/DeployAcrossFacet.s.sol (1)
25-32: LGTM! Verify contract addresses in config.The refactoring to use
_getConfigContractAddressadds important validation to ensure the configured addresses are actual contracts.Let's verify the addresses in the config file:
✅ Verification successful
Config structure and addresses verified ✓
The config file structure matches the path construction in the code, and contains valid Ethereum addresses for both acrossSpokePool and WETH across all networks. The Mainnet WETH address is the canonical address, providing additional confidence in the configuration's accuracy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the configured addresses exist and are contracts # Get the addresses from the config file CONFIG_PATH="config/across.json" if [ -f "$CONFIG_PATH" ]; then echo "Checking addresses in $CONFIG_PATH" rg -o '"acrossSpokePool":\s*"0x[a-fA-F0-9]{40}"' "$CONFIG_PATH" rg -o '"weth":\s*"0x[a-fA-F0-9]{40}"' "$CONFIG_PATH" fiLength of output: 1688
Script:
#!/bin/bash # Check the structure of the config file to ensure proper network mapping echo "Config file structure:" cat config/across.json | jq 'keys' # Show a sample network config to verify path construction echo -e "\nSample network configuration:" cat config/across.json | jq '.[].acrossSpokePool, .[].weth' | head -n 4Length of output: 478
Script:
#!/bin/bash # Examine structure of a specific network section echo "Sample network configuration (mainnet):" cat config/across.json | jq '.mainnet' echo -e "\nVerifying path construction for mainnet:" cat config/across.json | jq '.mainnet.acrossSpokePool, .mainnet.weth'Length of output: 1055
script/deploy/facets/DeployOmniBridgeFacet.s.sol (1)
25-31: LGTM! Verify Omni Bridge contract addresses.The implementation consistently uses
_getConfigContractAddressfor both bridge addresses, ensuring they are valid contracts.Let's verify the addresses in the config file:
✅ Verification successful
Bridge configurations verified across all networks
The configuration file contains valid bridge addresses for all supported networks (mainnet, BSC, Gnosis, and Kovan), with proper test configuration in hardhat environment. The zero address for wethOmniBridge in Gnosis chain appears to be an intentional configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the configured bridge addresses exist and are contracts # Get the addresses from the config file CONFIG_PATH="config/omni.json" if [ -f "$CONFIG_PATH" ]; then echo "Checking addresses in $CONFIG_PATH" rg -o '"foreignOmniBridge":\s*"0x[a-fA-F0-9]{40}"' "$CONFIG_PATH" rg -o '"wethOmniBridge":\s*"0x[a-fA-F0-9]{40}"' "$CONFIG_PATH" fiLength of output: 929
Script:
#!/bin/bash # Get the full context of bridge configurations including network information echo "Full bridge configurations:" rg -A 2 -B 2 '"(foreignOmniBridge|wethOmniBridge)":\s*"0x[a-fA-F0-9]{40}"' config/omni.jsonLength of output: 1282
script/deploy/facets/DeployArbitrumBridgeFacet.s.sol (1)
27-33: LGTM! Verify Arbitrum infrastructure addresses.The implementation correctly validates both the gateway router and inbox addresses.
Let's verify the Arbitrum infrastructure addresses:
✅ Verification successful
✅ Arbitrum infrastructure addresses verified
The configuration correctly maps network-specific addresses:
- Production addresses for mainnet/hardhat environments
- Separate test addresses for Goerli and Rinkeby networks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the configured Arbitrum addresses exist and are contracts # Get the addresses from the config file CONFIG_PATH="config/arbitrum.json" if [ -f "$CONFIG_PATH" ]; then echo "Checking addresses in $CONFIG_PATH" rg -o '"gatewayRouter":\s*"0x[a-fA-F0-9]{40}"' "$CONFIG_PATH" rg -o '"inbox":\s*"0x[a-fA-F0-9]{40}"' "$CONFIG_PATH" fiLength of output: 759
Script:
#!/bin/bash # Get the full context of the config file to understand network mappings echo "Full config context:" cat config/arbitrum.json | jq -r '.' # Search for network-specific patterns in the deployment script echo -e "\nNetwork references in deployment script:" rg -A 2 "network\s*=" script/deploy/facets/DeployArbitrumBridgeFacet.s.solLength of output: 843
script/deploy/facets/DeployAcrossFacetPacked.s.sol (1)
27-36: LGTM! Improved naming consistency.The implementation follows the pattern and improves clarity by renaming variables to match other facets (e.g.,
spokePool→acrossSpokePool).Let's verify the addresses and their usage:
✅ Verification successful
✓ Naming and address configurations verified
The implementation maintains clean internal naming while using explicit names in deployment scripts. Both
acrossSpokePoolandwethaddresses are properly configured across networks.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify addresses and check for consistent naming across files # Check for both old and new variable names to ensure complete migration rg -g '*.sol' 'spokePool|acrossSpokePool' rg -g '*.sol' 'wrappedNative|weth' # Verify addresses in config CONFIG_PATH="config/across.json" if [ -f "$CONFIG_PATH" ]; then echo "Checking addresses in $CONFIG_PATH" rg -o '"acrossSpokePool":\s*"0x[a-fA-F0-9]{40}"' "$CONFIG_PATH" rg -o '"weth":\s*"0x[a-fA-F0-9]{40}"' "$CONFIG_PATH" fiLength of output: 14975
script/deploy/facets/utils/ScriptBase.sol (1)
6-12: LGTM! Well-structured imports and error definition.The imports and error definition are clear and purposeful. The
stdJsonlibrary provides JSON parsing capabilities whileLibAssetprovides contract validation functionality.script/deploy/facets/DeployAcrossFacetPackedV3.s.sol (1)
27-36: LGTM! Clean implementation with improved variable naming.The changes properly utilize the new validation function and improve clarity with better variable names (acrossSpokePool, weth).
script/deploy/facets/UpdateOptimismBridgeFacet.s.sol (1)
33-38:⚠️ Potential issueFix undefined variable usage.
The
pathvariable is used in_getConfigContractAddressbut appears to be undefined. This could lead to runtime errors.Define the variable before use:
function getCallData() internal override returns (bytes memory) { - path = string.concat(root, "/config/optimism.json"); - json = vm.readFile(path); + string memory path = string.concat(root, "/config/optimism.json"); + string memory json = vm.readFile(path); address standardBridge = _getConfigContractAddress( path, string.concat(".", network, ".standardBridge") );Likely invalid or redundant comment.
script/deploy/facets/DeployTokenWrapper.s.sol (1)
24-29: LGTM! Verify wrapped native token contract.The implementation correctly uses
_getConfigContractAddressto validate that the wrapped native token address points to an actual contract.Run this script to verify the wrapped native token contract:
✅ Verification successful
Implementation correctly validates wrapped native token contract
The
_getConfigContractAddressfunction in ScriptBase.sol ensures that the wrapped native token address points to an actual contract by:
- Reading the address from networks.json
- Validating that the address contains code using
LibAsset.isContract()- Reverting if the validation fails
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the wrapped native token address exists in the config and contains code rg -l "wrappedNativeAddress" config/networks.json && \ ast-grep --pattern 'function _getConfigContractAddress(string memory $_, string memory $_) internal returns (address)'Length of output: 191
Script:
#!/bin/bash # Check networks.json content and _getConfigContractAddress implementation cat config/networks.json | jq -r '.[][] | select(.wrappedNativeAddress != null) | {network: input_filename, wrappedNativeAddress}' && \ echo "=== Function Implementation ===" && \ ast-grep --pattern 'function _getConfigContractAddress($$$) { $$$ }'Length of output: 215
Script:
#!/bin/bash # First, check the content of networks.json echo "=== Networks Config ===" && \ cat config/networks.json | jq '.' && \ echo "=== Function Implementation ===" && \ ast-grep --pattern 'function _getConfigContractAddress(string memory $_, string memory $_) internal returns (address) { $$$ }'Length of output: 29211
Script:
#!/bin/bash # Find _getConfigContractAddress implementation with a more flexible pattern ast-grep --pattern 'function _getConfigContractAddress($_) { $$$ }'Length of output: 71
Script:
#!/bin/bash # Search for _getConfigContractAddress function with context rg -A 10 "_getConfigContractAddress"Length of output: 38831
Script:
#!/bin/bash # Get the implementation of _getConfigContractAddress from ScriptBase.sol cat script/deploy/facets/utils/ScriptBase.solLength of output: 1462
templates/facetDeployScript.template.hbs (1)
27-33: Great documentation on address validation!The comments effectively explain when to use:
_getConfigContractAddressfor contract addressesjson.readAddressfor EOA addresses (like RefundWallet)script/deploy/facets/DeployReceiverAcrossV3.s.sol (2)
42-44: LGTM! Verify Across SpokePool contract.The implementation correctly validates that the acrossSpokePool address points to a contract.
Run this script to verify the Across SpokePool contract:
✅ Verification successful
Verification successful: acrossSpokePool address is validated as a contract
The
_getConfigContractAddressfunction in ScriptBase.sol properly validates that the acrossSpokePool address contains code usingLibAsset.isContract. The function will revert the deployment if the address is not a contract, ensuring the safety of the integration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the Across SpokePool address exists in the config and contains code rg -l "acrossSpokePool" config/across.json && \ ast-grep --pattern 'function _getConfigContractAddress(string memory $_, string memory $_) internal returns (address)'Length of output: 182
Script:
#!/bin/bash # Get acrossSpokePool config and _getConfigContractAddress implementation echo "=== Across SpokePool Configuration ===" rg "acrossSpokePool" config/across.json -A 2 -B 2 echo -e "\n=== Contract Address Validation Implementation ===" ast-grep --pattern 'function _getConfigContractAddress($$$) { $$$ }'Length of output: 2693
Script:
#!/bin/bash # Search for _getConfigContractAddress implementation with broader patterns echo "=== Searching for _getConfigContractAddress ===" rg "_getConfigContractAddress" -A 10 -B 2 echo -e "\n=== Searching in potential parent contracts ===" ast-grep --pattern 'contract $_ is $_ { $$$ function _getConfigContractAddress $$$ }'Length of output: 46221
Script:
#!/bin/bash # Get the complete implementation of _getConfigContractAddress echo "=== ScriptBase._getConfigContractAddress Implementation ===" rg -A 15 "function _getConfigContractAddress" script/deploy/facets/utils/ScriptBase.solLength of output: 722
56-56: LGTM! Verify Executor contract.The implementation correctly validates that the executor address points to a contract.
Run this script to verify the Executor contract:
✅ Verification successful
Verified: The implementation correctly validates that the executor address points to a contract
The
_getConfigContractAddressfunction in ScriptBase.sol performs explicit contract validation usingLibAsset.isContract(), which checks the contract code size usingextcodesize. The function reverts with a clear error message if the address doesn't contain code, ensuring the Executor address is valid.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the Executor address exists in deployments and contains code fd -g "*.json" deployments/ -x rg -l "Executor" {} \; && \ ast-grep --pattern 'function _getConfigContractAddress(string memory $_, string memory $_) internal returns (address)'Length of output: 7521
Script:
#!/bin/bash # Find implementation of _getConfigContractAddress and any contract validation ast-grep --pattern 'function _getConfigContractAddress' -l && \ ast-grep --pattern 'function _getConfigContractAddress' -A 20 && \ rg -l "require.*code.*length" && \ rg "require.*code.*length" -A 2Length of output: 166
Script:
#!/bin/bash # Search for _getConfigContractAddress implementation and contract validation echo "=== Function Implementation ===" ast-grep --lang solidity --pattern 'function _getConfigContractAddress' -l ast-grep --lang solidity --pattern 'function _getConfigContractAddress(string memory $_, string memory $_) internal returns (address) { $$$ }' -A 30 echo -e "\n=== Contract Validation ===" rg -t sol "require.*code.*length|require.*extcodesize" -A 2Length of output: 697
Script:
#!/bin/bash echo "=== Finding Solidity Files with Implementation ===" fd -e sol -x grep -l "_getConfigContractAddress" {} echo -e "\n=== Function Implementation ===" fd -e sol -x grep -A 10 "function _getConfigContractAddress" {} echo -e "\n=== Contract Validation Checks ===" rg "require.*code\.length|require.*extcodesize|require.*\.code\." -A 2Length of output: 81004
Script:
#!/bin/bash # Search for _getConfigContractAddress implementation in deployment scripts echo "=== Function Implementation in ScriptBase ===" rg -A 10 "_getConfigContractAddress" "script/deploy/facets/utils/ScriptBase.sol" echo -e "\n=== Usage in DeployReceiverAcrossV3 ===" rg -A 10 "_getConfigContractAddress" "script/deploy/facets/DeployReceiverAcrossV3.s.sol"Length of output: 1303
Script:
#!/bin/bash # Get complete implementation of _getConfigContractAddress echo "=== Complete Function Implementation ===" rg -A 15 "_getConfigContractAddress" "script/deploy/facets/utils/ScriptBase.sol"Length of output: 673
script/deploy/facets/DeployCelerIMFacet.s.sol (3)
77-78: LGTM! Verify Diamond contract.The implementation correctly validates that the diamond address points to a contract, handling both mutable and immutable variants.
Run this script to verify both Diamond contract variants:
✅ Verification successful
Diamond contract verification successful ✅
The implementation correctly handles both Diamond variants (mutable and immutable) and the deployment files confirm their existence across multiple networks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that both Diamond addresses exist in deployments and contain code fd -g "*.json" deployments/ -x rg -l "LiFiDiamond|LiFiDiamondImmutable" {} \; && \ ast-grep --pattern 'function _getConfigContractAddress(string memory $_, string memory $_) internal returns (address)'Length of output: 7428
Script:
#!/bin/bash # Find _getConfigContractAddress implementation and Diamond contracts ast-grep --pattern 'function _getConfigContractAddress' && \ echo "=== Diamond Contracts ===" && \ ast-grep --pattern 'contract LiFiDiamond' && \ ast-grep --pattern 'contract LiFiDiamondImmutable' && \ echo "=== Contract Validation ===" && \ rg -A 5 "_getConfigContractAddress"Length of output: 30376
42-44: LGTM! Verify MessageBus contract.The implementation correctly validates that the messageBus address points to a contract.
Run this script to verify the MessageBus contract:
47-56: LGTM! Verify cfUSDC contract for mainnet.The implementation correctly validates that the cfUSDC address points to a contract, but only for mainnet.
Run this script to verify the cfUSDC contract:
script/deploy/facets/DeployPolygonBridgeFacet.s.sol (1)
27-32: LGTM! Verify contract validation for Polygon bridge addresses.The implementation correctly uses
_getConfigContractAddressto retrieve and validate therootChainManageranderc20Predicateaddresses, aligning with the PR's objective to prevent deployment with incorrect addresses.Let's verify that these addresses are indeed contract addresses in the configuration:
✅ Verification successful
✓ Contract address validation is properly implemented and used
The
_getConfigContractAddressfunction ensures bothrootChainManageranderc20Predicateare valid contract addresses by:
- Verifying the addresses exist in the config
- Checking that the addresses contain code (are actual contracts)
- Throwing descriptive errors if validation fails
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that rootChainManager and erc20Predicate are contract addresses # Expected: Both addresses should be contracts, not EOAs # Extract and verify addresses from polygon.json jq -r --arg network "$NETWORK" \ '.[$network] | "\(.rootChainManager)\n\(.erc20Predicate)"' \ config/polygon.json | \ while read -r address; do # Use cast to check if address is a contract cast code $address | wc -c doneLength of output: 403
Script:
#!/bin/bash # Check if config exists and examine structure if [ -f "config/polygon.json" ]; then echo "Config file exists. Examining structure:" cat config/polygon.json else echo "Config file not found at config/polygon.json" # Search for potential polygon config files fd -t f -e json polygon fi # Look for address validation logic echo -e "\nChecking validation logic:" rg -A 5 "_getConfigContractAddress" --type solLength of output: 610
Script:
#!/bin/bash # Search for validation logic in Solidity files echo "Searching for validation logic:" rg -A 5 "_getConfigContractAddress" -g "*.sol" # Also search for the base script to understand validation echo -e "\nSearching for base script implementation:" fd -e sol "Script" --exec cat {}Length of output: 45266
script/deploy/facets/DeployReceiverStargateV2.s.sol (1)
42-47: LGTM! Verify contract validation for StarGate addresses.The implementation correctly uses
_getConfigContractAddressto retrieve and validate theendpointV2,tokenMessaging, andexecutoraddresses.Let's verify that these addresses are indeed contract addresses in the configurations:
Also applies to: 60-60
✅ Verification successful
Contract validation for StarGate addresses is properly implemented and secure.
The implementation uses
_getConfigContractAddresswhich validates that all addresses contain code usingLibAsset.isContract, ensuring that only deployed contracts can be used. The config file contains official StarGate contract addresses with proper documentation and clear placeholders for unsupported networks.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that StarGate and executor addresses are contract addresses # Expected: All addresses should be contracts, not EOAs # Extract and verify addresses from stargate.json jq -r --arg network "$NETWORK" \ '.endpointV2[$network], .tokenMessaging[$network]' \ config/stargate.json | \ while read -r address; do # Use cast to check if address is a contract cast code $address | wc -c done # Extract and verify executor address from deployment json jq -r '.Executor' \ deployments/$NETWORK.$FILE_SUFFIX.json | \ while read -r address; do cast code $address | wc -c doneLength of output: 528
Script:
#!/bin/bash # Find config files and deployment files echo "=== Config files ===" fd -e json -e yaml -e yml --search-path ./config echo -e "\n=== Deployment files ===" fd -e json --search-path ./deployments # Check stargate config content echo -e "\n=== Stargate config content ===" find ./config -type f -name "*stargate*.json" -exec cat {} \; # Look for address validation echo -e "\n=== Address validation logic ===" rg -A 5 "_getConfigContractAddress|validateAddress" --type solLength of output: 22022
Script:
#!/bin/bash # Search for validation logic in Solidity files echo "=== Deployment script content ===" cat script/deploy/facets/DeployReceiverStargateV2.s.sol echo -e "\n=== Address validation logic ===" rg -t sol -A 5 "_getConfigContractAddress|validateAddress"Length of output: 2428
Script:
#!/bin/bash # Find and examine the base deployment script echo "=== Base deployment script ===" fd -t f "DeployScriptBase.sol" --exec cat {} echo -e "\n=== Config contract address validation ===" ast-grep -p 'function _getConfigContractAddress($$$) { $$$ }'Length of output: 4967
Script:
#!/bin/bash # Find and examine the script base echo "=== Script base content ===" fd -t f "ScriptBase.sol" --exec cat {} # Look for address validation in all sol files echo -e "\n=== Address validation in sol files ===" fd -e sol -x grep -l "address" {} | xargs catLength of output: 78518
script/deploy/facets/DeployReceiver.s.sol (2)
Line range hint
29-33: Consider validating refundWallet address.While the PR adds contract validation for config addresses, the
refundWalletaddress is still read directly without validation. Although it's likely an EOA (not a contract), consider adding a validation check to ensure it's a valid address.
40-41: LGTM! Verify contract validation for router addresses.The implementation correctly uses
_getConfigContractAddressto retrieve and validate thestargateRouter,amarokRouter, andexecutoraddresses.Let's verify that these addresses are indeed contract addresses in the configurations:
Also applies to: 48-49, 61-61
✅ Verification successful
Validation confirmed: Router addresses are properly validated as contracts
The
_getConfigContractAddressfunction in ScriptBase.sol explicitly verifies that all loaded addresses contain code (are contracts, not EOAs) usingLibAsset.isContract(). The deployment will fail if any router or executor address is not a valid contract address.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that router and executor addresses are contract addresses # Expected: All addresses should be contracts, not EOAs # Extract and verify addresses from stargate.json jq -r --arg network "$NETWORK" \ '.composers[$network]' \ config/stargate.json | \ while read -r address; do cast code $address | wc -c done # Extract and verify address from amarok.json jq -r --arg network "$NETWORK" \ '.[$network].connextHandler' \ config/amarok.json | \ while read -r address; do cast code $address | wc -c done # Extract and verify executor address from deployment json jq -r '.Executor' \ deployments/$NETWORK.$FILE_SUFFIX.json | \ while read -r address; do cast code $address | wc -c doneLength of output: 591
Script:
#!/bin/bash # Find config files echo "=== Config files ===" fd -e json -e yaml -e yml stargate config/ fd -e json -e yaml -e yml amarok config/ echo -e "\n=== Config content ===" for f in $(fd -e json -e yaml -e yml 'stargate|amarok' config/); do echo "=== $f ===" cat "$f" | head -n 20 done echo -e "\n=== Address validation logic ===" rg -A 5 "_getConfigContractAddress"Length of output: 32692
Script:
#!/bin/bash # Find and examine the implementation of _getConfigContractAddress echo "=== ScriptBase.sol implementation ===" rg -A 15 "function _getConfigContractAddress" script/deploy/facets/utils/ScriptBase.solLength of output: 678
| address safeAddress = _getConfigContractAddress( | ||
| path, | ||
| string.concat(".", network, ".safeAddress") | ||
| ); |
There was a problem hiding this comment.
Warning: SAFE address validation may fail.
The _getConfigContractAddress helper includes an isContract check, but SAFE addresses are typically Externally Owned Accounts (EOAs) or proxy contracts. This validation might fail for non-proxy SAFE implementations.
Consider using a separate helper function without the isContract check for retrieving SAFE addresses, or modify the existing helper to make the contract check optional.
- address safeAddress = _getConfigContractAddress(
+ address safeAddress = _getConfigAddress( // New helper without contract check
path,
string.concat(".", network, ".safeAddress")
);Committable suggestion skipped: line range outside the PR's diff.
| //TODO: REMOVE | ||
| address erc20Proxy = _getConfigContractAddress(path, ".ERC20Proxy"); |
There was a problem hiding this comment.
Remove or implement the TODO comment.
There's a TODO comment about removing the ERC20Proxy address retrieval. Additionally, this address is retrieved but not used in the return value.
Either:
- Remove the ERC20Proxy address retrieval if it's not needed, or
- Include it in the return value if it's required
Which Jira task belongs to this PR?
Why did I implement it this way?
We do frequently run into the issue to deploy contracts with wrong addresses due to copy/paste errors or the like.
This PR introduces a helper function that adds a isContract-check when reading an address from a config or deploy log file. This way we get notified if we use an address that is an EOA.
It is important that we use the helper function for all future deploy/update scripts.
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)