NFT: improvements to make production-ready #217
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
examples/nft/scripts/test.sh (3)
5-5: Consider adding timeout and health check for localnet startupThe current implementation uses a fixed sleep duration which might be insufficient on slower systems. Consider implementing a health check loop.
-if [ "$1" = "localnet" ]; then npx hardhat localnet --exit-on-error & sleep 10; fi +if [ "$1" = "localnet" ]; then + npx hardhat localnet --exit-on-error & + ATTEMPTS=0 + until npx hardhat localnet-check &>/dev/null || [ $ATTEMPTS -eq 30 ]; do + sleep 1 + ((ATTEMPTS++)) + done + if [ $ATTEMPTS -eq 30 ]; then + echo "Error: Localnet failed to start within 30 seconds" + exit 1 + fi +fi
Line range hint
21-35: Add error handling for contract deployment operationsThe script should validate the contract addresses after deployment and handle potential failures gracefully.
+function check_address() { + local addr=$1 + local name=$2 + if [[ ! $addr =~ ^0x[a-fA-F0-9]{40}$ ]]; then + echo "Error: Invalid contract address for $name: $addr" + exit 1 + fi +} CONTRACT_ZETACHAIN=$(npx hardhat deploy --network localhost --json | jq -r '.contractAddress') +check_address "$CONTRACT_ZETACHAIN" "ZetaChain NFT" echo -e "\n🚀 Deployed NFT contract on ZetaChain: $CONTRACT_ZETACHAIN" CONTRACT_ETHEREUM=$(npx hardhat deploy --name Connected --json --network localhost --gateway "$GATEWAY_ETHEREUM" | jq -r '.contractAddress') +check_address "$CONTRACT_ETHEREUM" "Ethereum NFT" echo -e "🚀 Deployed NFT contract on Ethereum: $CONTRACT_ETHEREUM" CONTRACT_BNB=$(npx hardhat deploy --name Connected --json --network localhost --gateway "$GATEWAY_BNB" | jq -r '.contractAddress') +check_address "$CONTRACT_BNB" "BNB NFT" echo -e "🚀 Deployed NFT contract on BNB chain: $CONTRACT_BNB"
72-72: Enhance cleanup processConsider adding a trap to ensure localnet is stopped even if the script fails.
+if [ "$1" = "localnet" ]; then + trap 'npx hardhat localnet-stop' EXIT +fi -if [ "$1" = "localnet" ]; then npx hardhat localnet-stop; fiexamples/nft/contracts/Connected.sol (2)
28-31: Verify inheritance initialization orderThe constructor properly initializes both ERC721 and Ownable base contracts. However, consider documenting the inheritance order in a comment for better maintainability, as Solidity's C3 linearization can be complex with multiple inheritance.
Add a comment above the contract:
+/// @dev Inheritance order: Connected -> ERC721URIStorage -> ERC721Enumerable -> ERC721 -> Ownable contract Connected is ERC721, ERC721Enumerable, ERC721URIStorage, Ownable {
Line range hint
1-156: Review payable functions securityThe contract includes payable
receive()andfallback()functions without access controls or accounting logic. This could lead to trapped ETH if not handled properly.Consider implementing one of these solutions:
- receive() external payable {} - fallback() external payable {} + receive() external payable { + require(msg.sender == address(gateway), "Only gateway can send ETH"); + } + + fallback() external payable { + require(msg.sender == address(gateway), "Only gateway can send ETH"); + }Or if ETH acceptance is not needed:
- receive() external payable {} - fallback() external payable {} + receive() external payable { + revert("ETH not accepted"); + } + + fallback() external payable { + revert("ETH not accepted"); + }examples/nft/contracts/Universal.sol (3)
43-46: LGTM! Consider adding input validation.The constructor changes improve contract flexibility by allowing custom NFT names and symbols. The parameter naming is clear and follows best practices.
Consider adding input validation for the name and symbol parameters:
constructor( address payable gatewayAddress, address owner, string memory name, string memory symbol - ) ERC721(name, symbol) Ownable(owner) { + ) ERC721(name, symbol) Ownable(owner) { + require(bytes(name).length > 0, "Name cannot be empty"); + require(bytes(symbol).length > 0, "Symbol cannot be empty"); + require(gatewayAddress != address(0), "Invalid gateway address"); gateway = GatewayZEVM(gatewayAddress); }
Line range hint
69-95: Security: Enhance cross-chain transfer safety.The
transferCrossChainfunction follows good practices but could benefit from additional safety measures:
- Consider implementing a reentrancy guard
- Add events for better transaction tracking
- Validate the destination chain's counterparty contract before transfer
Consider implementing these improvements:
+ event CrossChainTransferInitiated(uint256 tokenId, address receiver, address destination); + + modifier nonReentrant() { + require(!_locked, "ReentrancyGuard: reentrant call"); + _locked = true; + _; + _locked = false; + } + - function transferCrossChain( + function transferCrossChain( uint256 tokenId, address receiver, address destination - ) public { + ) public nonReentrant { + require(counterparty[destination].length > 0, "Invalid destination chain"); string memory uri = tokenURI(tokenId); _burn(tokenId); + emit CrossChainTransferInitiated(tokenId, receiver, destination);
Line range hint
96-146: Optimize gas usage and improve configuration flexibility.The
onCallfunction uses hardcoded gas values and could benefit from gas optimizations:
- Use the contract's gasLimit state variable instead of hardcoding 700000
- Cache frequently accessed state variables
- Consider implementing a configurable gas limit
Implement these optimizations:
- (, uint256 gasFee) = IZRC20(destination).withdrawGasFeeWithGasLimit( - 700000 - ); + (, uint256 gasFee) = IZRC20(destination).withdrawGasFeeWithGasLimit( + gasLimit + ); SwapHelperLib.swapExactTokensForTokens( systemContract, zrc20, amount, destination, 0 ); IZRC20(destination).approve(address(gateway), gasFee); gateway.call( counterparty[destination], destination, abi.encode(tokenId, sender, uri), - CallOptions(700000, false), + CallOptions(gasLimit, false), RevertOptions(address(0), false, address(0), "", 0) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
examples/nft/contracts/Connected.sol(1 hunks)examples/nft/contracts/Universal.sol(1 hunks)examples/nft/scripts/test.sh(3 hunks)examples/nft/tasks/deploy.ts(2 hunks)
🧰 Additional context used
🪛 Shellcheck
examples/nft/scripts/test.sh
[warning] 8-8: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 9-9: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 10-10: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (4)
examples/nft/tasks/deploy.ts (2)
41-42: LGTM: Well-structured task parameter definitions.
The optional parameters are appropriately defined with meaningful default values that align with standard NFT naming conventions.
15-20: LGTM: Constructor parameter updates properly implemented.
The deployment call correctly includes the new NFT name and symbol parameters, maintaining consistency with the updated contract constructors.
Let's verify the contract constructors match these parameters:
✅ Verification successful
This script will:
- Find all Solidity files
- For each file, search for constructor definitions
- Show surrounding context to verify parameter alignment
The previous command might have failed due to syntax issues with tree-sitter patterns. This approach using fd and rg should provide better visibility into the constructor implementations.
The previous search revealed that the NFT contract files exist but didn't show their constructors, possibly due to regex limitations. Let's examine their complete content to verify the constructor parameters.
Constructor parameters correctly aligned with deployment call
The deployment parameters in deploy.ts perfectly match the constructor signatures in both NFT contracts:
- Connected.sol:
constructor(
address payable gatewayAddress,
address owner,
string memory name,
string memory symbol
)- Universal.sol:
constructor(
address payable gatewayAddress,
address owner,
string memory name,
string memory symbol
)The deployment call:
const contract = await factory.deploy(
args.gateway,
signer.address,
args.nftName,
args.nftSymbol
);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify constructor signatures in NFT contracts
# Expect: Constructors accepting gateway, signer, name, and symbol parameters
# Search for constructor definitions in Connected and Universal contracts
ast-grep --pattern 'constructor($$$) {
$$$
}'
Length of output: 49
Script:
#!/bin/bash
# Let's try a different approach to find the constructor definitions
# First, let's locate the NFT contract files
fd -e sol | while read -r file; do
echo "=== $file ==="
# Search for constructor definitions with surrounding context
rg -U "constructor.*?\{" -A 5 "$file"
done
Length of output: 3320
Script:
#!/bin/bash
# Let's specifically examine the NFT contract files content
echo "=== Connected.sol ==="
cat examples/nft/contracts/Connected.sol
echo -e "\n=== Universal.sol ==="
cat examples/nft/contracts/Universal.sol
Length of output: 10321
examples/nft/contracts/Connected.sol (1)
28-31: Constructor signature change improves contract flexibility
The updated constructor parameters allow for dynamic NFT naming and symbol assignment, which is a good practice for reusable contracts. The change from initialOwner to owner better aligns with OpenZeppelin's Ownable pattern.
Let's verify if there are any direct contract deployments that need updating:
✅ Verification successful
Let me search for any references to the Connected contract to ensure we don't miss any deployments or tests that might need updating.
Let me examine the deployment script and test files in the NFT example directory to verify how the Connected contract is being deployed.
Constructor changes are properly supported in deployment scripts
The deployment script in examples/nft/tasks/deploy.ts already handles the updated constructor signature correctly, passing all required parameters including gateway, owner (as signer.address), nftName, and nftSymbol. The script provides default values and optional parameters for all constructor arguments, maintaining backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct contract deployments that might need updates
rg -A 5 "new Connected\("
Length of output: 27
Script:
#!/bin/bash
# Search for contract deployments and tests using different patterns
rg -l "Connected" --type ts --type js --type sol
# Search for deploy scripts or test files
fd -e ts -e js -e sol -E "node_modules" -E "dist" -E "build" | rg -i "deploy|test"
# Look for contract factory patterns
rg -A 3 "ContractFactory.*Connected"
Length of output: 497
Script:
#!/bin/bash
# Check the deploy script content for NFT example
cat examples/nft/tasks/deploy.ts
# Look for any test files specific to NFT example
fd -e ts -e js "test" examples/nft/
Length of output: 1608
examples/nft/contracts/Universal.sol (1)
Line range hint 1-238: Verify cross-chain compatibility.
Please ensure that the contract has been tested with all supported destination chains and their respective gas requirements.
|
@andresaiello moved gas limit into the constructor, but something tells me for production you'd want different gas limits for each connected chain 🤔 so having a mapping and a way to update these values. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (15)
examples/nft/tsconfig.json (1)
3-6: Consider enhancing type checking configuration.While the path configuration is solid, you might want to add
"exactOptionalPropertyTypes": trueto strengthen type checking for optional properties in your NFT-related interfaces.{ "compilerOptions": { "baseUrl": ".", "paths": { "@/*": ["./*"] }, + "exactOptionalPropertyTypes": true, "module": "nodenext",examples/nft/contracts/shared/Events.sol (1)
1-2: Consider using a more flexible Solidity version range.The current pragma
^0.8.26is very specific. For better compatibility and maintenance, consider using a more flexible version range while maintaining security.-pragma solidity ^0.8.26; +pragma solidity ^0.8.0;examples/nft/tasks/connectedSetCounterparty.ts (2)
13-16: Enhance contract instantiation with constants and error handlingWhile the type safety improvement is good, consider these production-ready enhancements:
+const CONNECTED_CONTRACT_NAME = "Connected" as const; + const main = async (args: any, hre: HardhatRuntimeEnvironment) => { // ... existing code ... - const contract: Connected = await hre.ethers.getContractAt( - "Connected", - args.contract - ); + const contract = await hre.ethers.getContractAt( + CONNECTED_CONTRACT_NAME, + args.contract + ).then((contract): Connected => { + if (!contract) throw new Error(`Failed to get contract at ${args.contract}`); + return contract as Connected; + });This refactor:
- Eliminates magic strings
- Adds explicit error handling for contract instantiation
- Maintains type safety while being more explicit
5-5: Improve type safety and input validationConsider these production-critical improvements:
- Define proper types for task arguments:
interface ConnectedSetCounterpartyArgs { contract: string; counterparty: string; json?: boolean; }
- Add address validation:
const main = async (args: ConnectedSetCounterpartyArgs, hre: HardhatRuntimeEnvironment) => { if (!hre.ethers.utils.isAddress(args.contract)) { throw new Error(`Invalid contract address: ${args.contract}`); } if (!hre.ethers.utils.isAddress(args.counterparty)) { throw new Error(`Invalid counterparty address: ${args.counterparty}`); } // ... rest of the code };
- Consider adding gas estimation and management:
const tx = await contract.setCounterparty(args.counterparty, { gasLimit: await contract.estimateGas.setCounterparty(args.counterparty).then(limit => limit.mul(120).div(100) // Add 20% buffer ) });examples/nft/tasks/universalSetCounterparty.ts (1)
Line range hint
33-33: Fix inconsistent parameter reference in console outputThe console output references
args.contractAddressbut the parameter is namedargs.counterpartyin the task definition. This inconsistency could confuse users.Apply this fix:
-🔗 Connected contract address: ${args.contractAddress} +🔗 Connected contract address: ${args.counterparty}examples/nft/tasks/mint.ts (1)
19-21: Enhance transaction error handling and add gas estimation.For a production NFT system, consider adding proper error handling and gas estimation to prevent failed transactions and improve user experience.
- const tx = await contract.safeMint(recipient, args.tokenUri); - const receipt = await tx.wait(); + try { + const gasEstimate = await contract.safeMint.estimateGas(recipient, args.tokenUri); + const tx = await contract.safeMint(recipient, args.tokenUri, { + gasLimit: Math.ceil(gasEstimate * 1.2), // Add 20% buffer + }); + const receipt = await tx.wait(); + if (!receipt.status) { + throw new Error('Transaction failed'); + } + } catch (error) { + throw new Error(`Failed to mint NFT: ${error.message}`); + }examples/nft/tasks/transfer.ts (3)
64-64: Consider adding TypeScript type information in the parameter description.The receiver parameter is well-documented, but adding type information would improve developer experience.
-.addOptionalParam("receiver", "The address to receive the NFT") +.addOptionalParam("receiver", "The address to receive the NFT (type: string, format: hex address)")
Line range hint
41-41: Add address validation for the receiver parameter.The receiver address should be validated before making contract calls to prevent failed transactions.
-const receiver = args.receiver || signer.address; +const receiver = args.receiver || signer.address; +if (receiver && !ethers.utils.isAddress(receiver)) { + throw new Error(`Invalid receiver address format: ${receiver}`); +}
Line range hint
15-38: Refactor for better separation of concerns and error handling.The current implementation mixes contract detection, gas calculations, and approvals in a single try-catch block. Consider restructuring for better maintainability and error handling.
Consider splitting into separate functions:
async function detectContractType(address: string, ethers: any) { try { const contract = await ethers.getContractAt("Universal", address); await contract.isUniversal(); return { type: "Universal", contract }; } catch { const contract = await ethers.getContractAt("Connected", address); return { type: "Connected", contract }; } } async function handleGasApproval( contract: any, zrc20Address: string, gasLimit: any, signer: any, txOptions: any ) { const zrc20 = new ethers.Contract(zrc20Address, ZRC20ABI.abi, signer); const [, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit); const zrc20TransferTx = await zrc20.approve(contract.address, gasFee, txOptions); await zrc20TransferTx.wait(); }examples/nft/scripts/test.sh (1)
28-32: Improve contract deployment configuration and verificationThe hardcoded gas limit of 1000000 should be configurable, and deployments should be verified.
Consider:
- Moving the gas limit to a configuration file
- Adding deployment verification
- Adding retry logic for failed deployments
Example configuration approach:
+# Add to config.sh +declare -A CHAIN_GAS_LIMITS=( + ["zetachain"]=1000000 + ["ethereum"]=2000000 + ["bnb"]=2000000 +) + +function verify_deployment() { + local contract=$1 + local chain=$2 + + if ! cast code "$contract" &>/dev/null; then + echo "Error: Failed to verify contract deployment on $chain" + return 1 + fi +}examples/nft/contracts/Connected.sol (3)
24-25: LGTM: Gas-efficient custom errors.Using custom errors instead of string messages is gas-efficient. Consider adding error parameters to provide more context about which address or operation was invalid.
-error InvalidAddress(); +error InvalidAddress(address invalidAddress, string reason); -error Unauthorized(); +error Unauthorized(address sender, address expected);
50-56: Consider using a more robust token ID generation mechanism.While the current implementation is simple, it could be vulnerable to front-running. Consider using a deterministic approach based on sender address and nonce, or implementing a queue-based system.
-uint256 tokenId = _nextTokenId++; +uint256 tokenId = uint256(keccak256(abi.encodePacked(block.timestamp, msg.sender, _nextTokenId++)));
92-104: Enhance error handling in callback functions.Consider providing more context in error messages and validating the decoded parameters.
function onCall( MessageContext calldata context, bytes calldata message ) external payable onlyGateway returns (bytes4) { - if (context.sender != counterparty) revert Unauthorized(); + if (context.sender != counterparty) revert Unauthorized(context.sender, counterparty); (uint256 tokenId, address receiver, string memory uri) = abi.decode( message, (uint256, address, string) ); + if (receiver == address(0)) revert InvalidAddress(receiver, "Invalid receiver in cross-chain callback");Also applies to: 108-116
examples/nft/contracts/Universal.sol (2)
69-69: Consider adding gas limit validation in transferCrossChain.While the function includes proper address validation and event emission, it should validate that the current gasLimit is sufficient for the target chain.
Consider implementing a chain-specific gas limit mapping that can be updated by the owner:
mapping(address => uint256) public chainGasLimits; function setChainGasLimit(address destination, uint256 limit) external onlyOwner { if (limit == 0) revert("Invalid gas limit"); chainGasLimits[destination] = limit; }Also applies to: 80-80, 100-100
135-135: Add message format validation in onCall and onRevert.While the functions properly emit events and use dynamic gas limits, they should validate the format of incoming messages to prevent potential issues with malformed data.
Consider adding try-catch blocks around message decoding:
function onCall(...) external override onlyGateway { try abi.decode(message, (uint256, address, string, address)) returns ( uint256 tokenId, address sender, string memory uri, address destination ) { // existing logic } catch { revert("Invalid message format"); } }Also applies to: 138-138, 154-154, 169-169
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
examples/nft/contracts/Connected.sol(2 hunks)examples/nft/contracts/Universal.sol(6 hunks)examples/nft/contracts/shared/Events.sol(1 hunks)examples/nft/scripts/test.sh(3 hunks)examples/nft/tasks/connectedSetCounterparty.ts(2 hunks)examples/nft/tasks/deploy.ts(3 hunks)examples/nft/tasks/mint.ts(1 hunks)examples/nft/tasks/transfer.ts(1 hunks)examples/nft/tasks/universalSetCounterparty.ts(2 hunks)examples/nft/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/nft/tasks/deploy.ts
🧰 Additional context used
🪛 Shellcheck
examples/nft/scripts/test.sh
[warning] 8-8: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 9-9: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 10-10: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (9)
examples/nft/tsconfig.json (1)
3-6: Path aliasing configuration looks good.
The addition of path aliasing with @/* will improve import readability and maintainability. However, let's verify that all build tools support this configuration.
✅ Verification successful
Path alias configuration is correctly integrated and in use
The path alias configuration is properly set up and already being utilized in the codebase:
- Found active usage of the
@/*alias in NFT tasks (e.g.,import { Universal } from "@/typechain-types") - The configuration is compatible with the project's TypeScript and build setup
- No conflicting path configurations found in other build tools
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if any existing imports need to be updated to use the new alias
# and verify build tool configurations support the path alias
# Check for relative imports that could be simplified
rg -g '*.ts' -g '*.tsx' '\.\./\.\.'
# Check build tool configs for path alias support
fd -e json -e js -e yaml -e yml -x grep -l "paths\|alias"
Length of output: 148
Script:
#!/bin/bash
# Let's check the actual content of the build configuration files
# and look for any existing imports in TypeScript files
# Check content of build configs
echo "=== tsconfig.json ==="
cat examples/nft/tsconfig.json
echo -e "\n=== slither.config.json ==="
cat slither.config.json
# Look for any imports in TypeScript files
echo -e "\n=== TypeScript imports ==="
rg -g '*.ts' -g '*.tsx' '^import .+ from' -A 1
Length of output: 8802
examples/nft/contracts/shared/Events.sol (1)
7-12: Verify the necessity of duplicate transfer events.
The TokenTransfer and TokenTransferToDestination events appear to capture very similar information. This might lead to confusion in event handling and unnecessary gas costs.
Let's check how these events are used in the codebase:
Also applies to: 24-29
examples/nft/tasks/connectedSetCounterparty.ts (1)
3-3: LGTM: Type-safe import addition
The addition of the Connected type import enhances type safety and compiler checks.
examples/nft/tasks/universalSetCounterparty.ts (1)
3-3: Excellent type safety improvements!
The addition of explicit typing through the Universal import and contract type annotation enhances code reliability and IDE support.
Also applies to: 13-16
examples/nft/contracts/Connected.sol (3)
11-12: LGTM: Clean separation of events into a shared contract.
The separation of events into a shared contract promotes code reusability and maintains a single source of truth for event definitions across related contracts.
Also applies to: 13-19
28-30: LGTM: Proper validation and event emission.
The function correctly validates the input and emits an event for off-chain tracking.
64-88: 🛠️ Refactor suggestion
Enhance cross-chain transfer validation and gas handling.
The function should validate msg.value against minimum required gas and handle chain-specific gas limits.
function transferCrossChain(
uint256 tokenId,
address receiver,
address destination
) external payable {
if (receiver == address(0)) revert InvalidAddress();
+ uint256 requiredGas = destination == address(0) ?
+ _defaultGasLimit :
+ _chainGasLimits[block.chainid] != 0 ? _chainGasLimits[block.chainid] : _defaultGasLimit;
+ if (msg.value < requiredGas) revert("Insufficient gas");examples/nft/contracts/Universal.sol (2)
15-15: LGTM: Clean integration of Events contract.
The Events contract integration follows proper inheritance patterns and import organization.
Also applies to: 22-23
30-30: LGTM: Well-structured state variable and error definitions.
The gasLimit variable and custom errors enhance contract maintainability and gas efficiency. Custom errors are preferred over require statements for gas optimization.
Also applies to: 33-34
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
examples/nft/contracts/Connected.sol (3)
24-25: Consider enhancing error debugging capabilities.While the custom errors are gas-efficient, consider adding parameters to provide more context during failures.
-error InvalidAddress(); +error InvalidAddress(address invalidAddress); -error Unauthorized(); +error Unauthorized(address sender, address expected);
39-46: Enhance address validation specificity.While the address validation is good, consider separating the checks to provide more specific error context.
-if (gatewayAddress == address(0) || owner == address(0)) - revert InvalidAddress(); +if (gatewayAddress == address(0)) revert InvalidAddress(gatewayAddress); +if (owner == address(0)) revert InvalidAddress(owner);
27-30: Improve setCounterparty validation and event emission.Consider adding the old value to the event for better traceability.
function setCounterparty(address contractAddress) external onlyOwner { if (contractAddress == address(0)) revert InvalidAddress(); + address oldCounterparty = counterparty; counterparty = contractAddress; - emit SetCounterparty(contractAddress); + emit SetCounterparty(oldCounterparty, contractAddress); }examples/nft/contracts/Universal.sol (2)
69-69: Cross-chain transfer security improvements noted.The addition of zero-address validation for the receiver enhances security. Consider adding validation for the destination address as well.
function transferCrossChain( uint256 tokenId, address receiver, address destination ) public { if (receiver == address(0)) revert InvalidAddress(); + if (destination == address(0)) revert InvalidAddress(); string memory uri = tokenURI(tokenId);Also applies to: 80-80, 88-88, 95-95
138-138: Verify gas limit usage across chains.The dynamic gas limit is now used consistently across the contract, but consider implementing chain-specific gas limits as suggested in the PR comments.
Consider implementing a mapping for chain-specific gas limits:
mapping(address => uint256) public chainGasLimits; function setChainGasLimit(address chain, uint256 limit) external onlyOwner { if (limit < MIN_GAS_LIMIT || limit > MAX_GAS_LIMIT) revert InvalidGasLimit(); chainGasLimits[chain] = limit; }Also applies to: 154-154
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
examples/nft/contracts/Connected.sol(2 hunks)examples/nft/contracts/Universal.sol(6 hunks)
🔇 Additional comments (5)
examples/nft/contracts/Connected.sol (1)
8-8: Excellent security improvement with Ownable2Step!
The switch from Ownable to Ownable2Step enhances security by implementing a two-step ownership transfer process. The addition of the Events contract and proper inheritance structure provides better event handling capabilities.
Also applies to: 11-11, 13-19
examples/nft/contracts/Universal.sol (4)
8-8: Inheritance structure improvements noted.
The switch to Ownable2Step from Ownable enhances security by requiring a two-step process for ownership transfers. The addition of the Events contract promotes better event organization.
Also applies to: 15-15, 21-23
39-39: Improved gateway authorization check.
The change from require to a custom error improves gas efficiency and provides better error reporting.
44-53: Constructor validation can be enhanced.
While the constructor properly validates addresses and initializes parameters, it should also validate the gas limit parameter.
100-100: Event emissions enhance transparency.
The addition of events for token transfers, receipts, and reverts improves the contract's observability and enables better tracking of cross-chain operations.
Also applies to: 135-135, 157-157, 169-169
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
examples/nft/contracts/Universal.sol (2)
45-55: Consider additional parameter validation.While address and gas validation are implemented, consider adding validation for name and symbol parameters to prevent empty strings.
constructor( address payable gatewayAddress, address owner, string memory name, string memory symbol, uint256 gas ) ERC721(name, symbol) Ownable(owner) { if (gatewayAddress == address(0) || owner == address(0)) revert InvalidAddress(); if (gas == 0) revert InvalidGasLimit(); + if (bytes(name).length == 0 || bytes(symbol).length == 0) + revert("Invalid name or symbol"); gateway = GatewayZEVM(gatewayAddress); gasLimit = gas; }
71-71: Good improvements to cross-chain transfer logic!The address validation and event emission enhance security and traceability. Consider adding validation for the destination address as well.
function transferCrossChain( uint256 tokenId, address receiver, address destination ) public { if (receiver == address(0)) revert InvalidAddress(); + if (destination == address(0)) revert InvalidAddress(); string memory uri = tokenURI(tokenId);Also applies to: 82-82, 90-90, 97-97, 102-102
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
examples/nft/contracts/Universal.sol(6 hunks)
🔇 Additional comments (4)
examples/nft/contracts/Universal.sol (4)
8-8: Excellent security and architectural improvements!
The switch to Ownable2Step enhances security by requiring a two-step process for ownership transfers, and the separation of events into a dedicated contract improves modularity.
Also applies to: 15-15, 21-23
30-35: Well-structured error handling implementation!
The custom errors improve gas efficiency and provide clear error messages compared to require statements with strings.
40-40: Improved modifier implementation!
The use of custom errors in the modifier is more gas-efficient than string-based require statements.
171-171: Good addition of event emission!
The event emission for reverted transfers improves transaction traceability.
| @@ -121,9 +134,10 @@ contract Universal is | |||
| if (destination == address(0)) { | |||
| _safeMint(sender, tokenId); | |||
| _setTokenURI(tokenId, uri); | |||
| emit TokenTransferReceived(tokenId, sender, uri); | |||
There was a problem hiding this comment.
💡 Codebase verification
Reentrancy protection is recommended for external calls
The onCall function in Universal.sol contains external calls through gateway.call after state modifications (_safeMint, _setTokenURI). While the function is protected by onlyGateway modifier, implementing the following safeguards is recommended:
- Add OpenZeppelin's
ReentrancyGuardand usenonReentrantmodifier - Alternatively, strictly follow checks-effects-interactions pattern by:
- Moving the
TokenTransferReceivedevent emission after all state changes - Ensuring
gateway.callis the last operation in all execution paths
- Moving the
Example implementation:
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
contract Universal is ..., ReentrancyGuard {
function onCall(...) external override onlyGateway nonReentrant {
// existing implementation
}
}🔗 Analysis chain
Consider reentrancy protection for external calls.
The function makes external calls to swapExactTokensForTokens and gateway.call. Consider implementing a reentrancy guard or ensuring proper checks-effects-interactions pattern.
Also applies to: 140-140, 156-156, 159-159
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for ReentrancyGuard usage in the codebase
ast-grep --pattern 'contract $_ is $$$, ReentrancyGuard, $$$ {'
Length of output: 65
Script:
#!/bin/bash
# Let's check for ReentrancyGuard imports and usage with different patterns
rg -i "reentrancy|ReentrancyGuard" -A 3
# Check the specific onCall function implementation
ast-grep --pattern 'function onCall($$$) {
$$$
}'
# Check for nonReentrant modifier usage
ast-grep --pattern 'nonReentrant'
# Look for the gateway.call pattern to understand the external call context
rg "gateway.*call" -A 3
Length of output: 1514
Script:
#!/bin/bash
# Let's examine the full context of the onCall function to understand the control flow
rg "function onCall" -A 20
# Check for existing modifiers on the contract and function
ast-grep --pattern 'function onCall($$$) $_ {
$$$
}'
# Look for existing checks-effects-interactions pattern
rg "require\(.*\)" -A 2 examples/nft/contracts/Universal.sol
Length of output: 7522
Ownable2StepSummary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor