feat: NFT and FT upgradeable using OpenZeppelin's UUPS #6
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
fadeev
changed the title
|
@andresaiello @skosito please, review. |
![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 comments (2)
contracts/nft/contracts/evm/UniversalNFT.sol (1)
Line range hint
77-91: Enhance tokenId generation security.The current tokenId generation method using
block.numbercould be predictable. Consider using a more secure random number generation method.function safeMint(address to, string memory uri) public onlyOwner { uint256 hash = uint256( keccak256( - abi.encodePacked(address(this), block.number, _nextTokenId++) + abi.encodePacked( + address(this), + block.prevrandao, + block.timestamp, + _nextTokenId++ + ) ) );contracts/nft/contracts/zetachain/UniversalNFT.sol (1)
Line range hint
169-209: Add input validation for amount parameter.The
onCallfunction should validate that theamountparameter is non-zero before proceeding with the swap.function onCall( MessageContext calldata context, address zrc20, uint256 amount, bytes calldata message ) external override onlyGateway { + if (amount == 0) revert("Zero amount"); if (context.sender != connected[zrc20]) revert Unauthorized();
♻️ Duplicate comments (2)
contracts/token/contracts/zetachain/UniversalToken.sol (1)
29-32:⚠️ Potential issueRemove unused state variable
_nextTokenId.The
_nextTokenIdvariable appears to be unused in this contract and should be removed to optimize gas costs.contracts/token/contracts/evm/UniversalToken.sol (1)
139-141: 🛠️ Refactor suggestionEnhance upgrade security with timelock mechanism.
The current upgrade mechanism could be enhanced with a timelock to provide users time to review and react to pending upgrades.
🧹 Nitpick comments (5)
contracts/token/contracts/zetachain/UniversalToken.sol (3)
73-76: Add event emission for gas limit changes.Consider emitting an event when the gas limit is updated to maintain transparency and enable off-chain tracking of important state changes.
function setGasLimit(uint256 gas) external onlyOwner { if (gas == 0) revert InvalidGasLimit(); gasLimitAmount = gas; + emit GasLimitUpdated(gas); }
90-119: Consider extracting gas fee handling logic.The transferCrossChain function handles multiple concerns. Consider extracting the gas fee handling logic into a separate internal function for better maintainability and readability.
+ function _handleGasFeePayment(address destination) internal returns (uint256) { + (address gasZRC20, uint256 gasFee) = IZRC20(destination) + .withdrawGasFeeWithGasLimit(gasLimitAmount); + if (destination != gasZRC20) revert InvalidAddress(); + + address WZETA = gateway.zetaToken(); + IWETH9(WZETA).deposit{value: msg.value}(); + IWETH9(WZETA).approve(uniswapRouter, msg.value); + + uint256 out = SwapHelperLib.swapTokensForExactTokens( + uniswapRouter, + WZETA, + gasFee, + gasZRC20, + msg.value + ); + + uint256 remaining = msg.value - out; + if (remaining > 0) { + IWETH9(WZETA).withdraw(remaining); + (bool success, ) = msg.sender.call{value: remaining}(""); + if (!success) revert TransferFailed(); + } + return gasFee; + }
203-207: Add NatSpec documentation for upgrade authorization.The empty
_authorizeUpgradefunction should include NatSpec documentation explaining that only the owner can upgrade the contract and why the function body is intentionally empty.+ /// @notice Function that should revert when `msg.sender` is not authorized to upgrade the contract. + /// @dev This function is called by {upgradeTo} and {upgradeToAndCall}. Empty implementation as access + /// control is handled by the `onlyOwner` modifier. + /// @param newImplementation Address of the new implementation contract function _authorizeUpgrade( address newImplementation ) internal override onlyOwner {}contracts/nft/contracts/evm/UniversalNFT.sol (1)
47-64: Consider emitting an initialization event.For better transparency and tracking, consider emitting an event after successful initialization.
+ event NFTInitialized(address indexed owner, string name, string symbol); function initialize( address initialOwner, string memory name, string memory symbol, address payable gatewayAddress, uint256 gas ) public initializer { __ERC721_init(name, symbol); __ERC721Enumerable_init(); __ERC721URIStorage_init(); __Ownable_init(initialOwner); __ERC721Burnable_init(); __UUPSUpgradeable_init(); if (gatewayAddress == address(0)) revert InvalidAddress(); if (gas == 0) revert InvalidGasLimit(); gasLimitAmount = gas; gateway = GatewayEVM(gatewayAddress); + emit NFTInitialized(initialOwner, name, symbol); }contracts/nft/contracts/zetachain/UniversalNFT.sol (1)
94-126: Refactor complex transfer logic into smaller functions.The
transferCrossChainfunction handles multiple responsibilities. Consider splitting it into smaller, more focused functions for better maintainability and testing.+ function _handleGasToken(uint256 msgValue, address gasZRC20, uint256 gasFee) private returns (uint256) { + address WZETA = gateway.zetaToken(); + IWETH9(WZETA).deposit{value: msgValue}(); + IWETH9(WZETA).approve(uniswapRouter, msgValue); + + return SwapHelperLib.swapTokensForExactTokens( + uniswapRouter, + WZETA, + gasFee, + gasZRC20, + msgValue + ); + } function transferCrossChain( uint256 tokenId, address receiver, address destination ) public payable { if (msg.value == 0) revert ZeroMsgValue(); if (receiver == address(0)) revert InvalidAddress(); string memory uri = tokenURI(tokenId); _burn(tokenId); (address gasZRC20, uint256 gasFee) = IZRC20(destination) .withdrawGasFeeWithGasLimit(gasLimitAmount); if (destination != gasZRC20) revert InvalidAddress(); - address WZETA = gateway.zetaToken(); - IWETH9(WZETA).deposit{value: msg.value}(); - IWETH9(WZETA).approve(uniswapRouter, msg.value); - - uint256 out = SwapHelperLib.swapTokensForExactTokens( - uniswapRouter, - WZETA, - gasFee, - gasZRC20, - msg.value - ); + uint256 out = _handleGasToken(msg.value, gasZRC20, gasFee);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
contracts/nft/contracts/evm/UniversalNFT.sol(3 hunks)contracts/nft/contracts/shared/UniversalNFTEvents.sol(1 hunks)contracts/nft/contracts/zetachain/UniversalNFT.sol(7 hunks)contracts/nft/tasks/mint.ts(3 hunks)contracts/token/contracts/evm/UniversalToken.sol(3 hunks)contracts/token/contracts/shared/UniversalTokenEvents.sol(1 hunks)contracts/token/contracts/zetachain/UniversalToken.sol(6 hunks)contracts/token/tasks/mint.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- contracts/token/contracts/shared/UniversalTokenEvents.sol
- contracts/nft/contracts/shared/UniversalNFTEvents.sol
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/nft/tasks/mint.ts
- contracts/token/tasks/mint.ts
🔇 Additional comments (2)
contracts/token/contracts/zetachain/UniversalToken.sol (2)
Line range hint 53-71: LGTM: Initialization pattern follows UUPS best practices.
The initialize function correctly:
- Initializes all inherited contracts in the proper order
- Validates critical parameters
- Sets up the initial state
10-25: LGTM: Inheritance chain follows OpenZeppelin's recommendations.
The contract correctly inherits from upgradeable base contracts in the recommended order: Initializable first, followed by the upgradeable variants of ERC20, ERC20Burnable, Ownable, and UUPS.
✅ Verification successful
Inheritance chain follows OpenZeppelin's best practices
The codebase consistently follows OpenZeppelin's recommended inheritance patterns for upgradeable contracts. Both UniversalToken and UniversalNFT contracts:
- Correctly list Initializable as the first inherited contract
- Properly order the OpenZeppelin upgradeable contracts
- Maintain consistent UUPS upgrade pattern implementation
The inheritance structure is uniform across all contract variants (ZetaChain, EVM) and follows production-grade practices for proxy-based upgradeability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify no other inheritance patterns exist in the codebase
rg "contract.*is.*Initializable" --type sol
Length of output: 73
Script:
#!/bin/bash
# Search for contract declarations with Initializable inheritance
rg "contract.*is.*Initializable"
# Also check for any other UUPS upgrade patterns
rg "contract.*is.*UUPSUpgradeable"
Length of output: 71
Script:
#!/bin/bash
# First, find all Solidity files
fd -e sol -t f -x cat {} \; | grep -A 5 "contract.*is"
# Also check for specific imports of upgradeable contracts
fd -e sol -t f -x cat {} \; | grep -B 2 -A 2 "import.*Upgradeable"
Length of output: 5002
| contract UniversalToken is | ||
| Initializable, | ||
| ERC20Upgradeable, | ||
| ERC20BurnableUpgradeable, | ||
| OwnableUpgradeable, | ||
| UUPSUpgradeable, | ||
| UniversalTokenEvents | ||
| { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding ReentrancyGuard for enhanced security.
The contract handles value transfers and should implement ReentrancyGuardUpgradeable to prevent potential reentrancy attacks.
+import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
contract UniversalToken is
Initializable,
ERC20Upgradeable,
ERC20BurnableUpgradeable,
OwnableUpgradeable,
UUPSUpgradeable,
+ ReentrancyGuardUpgradeable,
UniversalTokenEvents📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| contract UniversalToken is | |
| Initializable, | |
| ERC20Upgradeable, | |
| ERC20BurnableUpgradeable, | |
| OwnableUpgradeable, | |
| UUPSUpgradeable, | |
| UniversalTokenEvents | |
| { | |
| import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol"; | |
| contract UniversalToken is | |
| Initializable, | |
| ERC20Upgradeable, | |
| ERC20BurnableUpgradeable, | |
| OwnableUpgradeable, | |
| UUPSUpgradeable, | |
| ReentrancyGuardUpgradeable, | |
| UniversalTokenEvents | |
| { |
| function initialize( | ||
| address initialOwner, | ||
| string memory name, | ||
| string memory symbol, | ||
| address payable gatewayAddress, | ||
| uint256 gas | ||
| ) public initializer { | ||
| __ERC20_init(name, symbol); | ||
| __ERC20Burnable_init(); | ||
| __Ownable_init(initialOwner); | ||
| __UUPSUpgradeable_init(); | ||
| if (gatewayAddress == address(0)) revert InvalidAddress(); | ||
| gasLimitAmount = gas; | ||
| gateway = GatewayEVM(gatewayAddress); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add gas validation in initialize function.
The gas parameter should be validated to prevent setting an invalid gas limit during initialization.
function initialize(
address initialOwner,
string memory name,
string memory symbol,
address payable gatewayAddress,
uint256 gas
) public initializer {
__ERC20_init(name, symbol);
__ERC20Burnable_init();
__Ownable_init(initialOwner);
__UUPSUpgradeable_init();
if (gatewayAddress == address(0)) revert InvalidAddress();
+ if (gas == 0) revert InvalidGasLimit();
gasLimitAmount = gas;
gateway = GatewayEVM(gatewayAddress);
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/token/tasks/mint.ts (1)
45-49: Update parameter documentation to reflect specific contract types.The name parameter description should clearly indicate the supported contract types as per the PR changes.
Consider updating the parameter documentation:
.addOptionalParam( "name", - "The contract name to interact with", + "The contract name to interact with (ZetaChainUniversalToken or EVMUniversalToken)", "ZetaChainUniversalToken" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
contracts/nft/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
contracts/nft/package.json(2 hunks)contracts/nft/tasks/mint.ts(4 hunks)contracts/token/tasks/mint.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/nft/tasks/mint.ts
- contracts/nft/package.json
| (event: any) => event.event === "Transfer" | ||
| ); | ||
| const tokenId = transferEvent?.args?.tokenId; | ||
| await tx.wait(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance transaction receipt handling for production reliability.
The current implementation lacks proper error handling and event verification for the mint operation.
Consider implementing this more robust approach:
- await tx.wait();
+ const receipt = await tx.wait();
+ const mintEvent = receipt.events?.find(e => e.event === 'Transfer' && e.args?.from === ethers.constants.AddressZero);
+ if (!mintEvent) {
+ throw new Error('Mint transaction completed but no Transfer event found');
+ }
+ // Verify mint amount matches requested amount
+ if (mintEvent.args?.value.toString() !== args.amount.toString()) {
+ throw new Error(`Minted amount ${mintEvent.args?.value} differs from requested amount ${args.amount}`);
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (3)
contracts/token/tasks/connectedSetUniversal.ts (1)
Line range hint
24-25: Add gas limit for upgradeable contract interactionUpgradeable contracts typically require more gas due to proxy delegation. Consider adding a gas limit to prevent transaction failures.
-const tx = await contract.setUniversal(args.universal); +const tx = await contract.setUniversal(args.universal, { + gasLimit: 1000000 +});contracts/nft/tasks/connectedSetUniversal.ts (1)
Line range hint
35-39: Fix inconsistent variable usage in log messageThe log message uses
args.universalContractwhich doesn't exist in the args object.console.log(`🚀 Successfully set the universal contract. 📜 Contract address: ${args.contract} -🔗 Universal contract address: ${args.universalContract} +🔗 Universal contract address: ${args.universal} 🔗 Transaction hash: ${tx.hash}`);contracts/nft/tasks/universalSetConnected.ts (1)
Line range hint
39-43: Fix inconsistent variable usage in log messageThe log message uses
args.contractAddresswhich should beargs.connected.console.log(`🚀 Successfully set the connected contract. 📜 Contract address: ${args.contract} 🔗 ZRC20 address: ${args.zrc20} -🔗 Connected contract address: ${args.contractAddress} +🔗 Connected contract address: ${args.connected} 🔗 Transaction hash: ${tx.hash}`);
♻️ Duplicate comments (1)
contracts/nft/tasks/connectedSetUniversal.ts (1)
18-21: 🛠️ Refactor suggestionAdd proxy contract validation
When working with upgradeable contracts, it's important to verify we're interacting with the proxy contract and not the implementation directly.
const contract: EVMUniversalNFT = await hre.ethers.getContractAt( "EVMUniversalNFT", args.contract ); +const implementationAddress = await hre.upgrades.erc1967.getImplementationAddress(args.contract); +if (args.contract === implementationAddress) { + throw new Error('Provided address is implementation contract, not proxy'); +}
🧹 Nitpick comments (6)
contracts/token/tasks/universalSetConnected.ts (2)
15-21: Enhance error message specificity for address validationWhile the address validation is a good security measure, the error message could be more specific to help users identify which address is invalid.
Consider implementing more granular validation:
- if ( - !isAddress(args.contract) || - !isAddress(args.zrc20) || - !isAddress(args.connected) - ) { - throw new Error("Invalid Ethereum address provided."); - } + if (!isAddress(args.contract)) { + throw new Error("Invalid contract address provided."); + } + if (!isAddress(args.zrc20)) { + throw new Error("Invalid ZRC20 address provided."); + } + if (!isAddress(args.connected)) { + throw new Error("Invalid connected contract address provided."); + }
Line range hint
28-45: Consider adding debug logging for contract interactionsWhile the success logging is comprehensive, consider adding debug logs before the contract interaction to aid in troubleshooting.
Add debug logging before the transaction:
+ console.log(`Attempting to set connected contract... + Contract: ${args.contract} + ZRC20: ${args.zrc20} + Connected: ${args.connected}`); const tx = await contract.setConnected(args.zrc20, args.connected);contracts/token/tasks/transfer.ts (1)
8-12: Enhance error message specificity for address validationWhile the address validation is a good addition, the error message could be more specific to help users identify which address is invalid.
Consider this improvement:
- if (!isAddress(args.to) || !isAddress(args.revertAddress)) { - throw new Error("Invalid Ethereum address provided."); + if (!isAddress(args.to)) { + throw new Error(`Invalid 'to' address provided: ${args.to}`); + } + if (!isAddress(args.revertAddress)) { + throw new Error(`Invalid 'revertAddress' provided: ${args.revertAddress}`); + }contracts/nft/tasks/connectedSetUniversal.ts (1)
23-25: Consider making gas limit configurableWhile adding a gas limit is good, hardcoding it might not be optimal for different networks or contract states.
+const DEFAULT_GAS_LIMIT = 1000000; + const tx = await contract.setUniversal(args.universal, { - gasLimit: 1000000, + gasLimit: args.gasLimit || DEFAULT_GAS_LIMIT, });Update task parameters:
task("connected-set-universal", "Sets the universal contract address", main) .addParam("contract", "The address of the deployed contract") .addParam("universal", "The address of the universal contract to set") + .addOptionalParam("gasLimit", "Custom gas limit for the transaction") .addFlag("json", "Output the result in JSON format");contracts/token/tasks/deploy.ts (1)
5-6: Enhance error message specificity for address validation.The address validation is well-implemented, but the error message could be more informative by indicating which address is invalid.
- throw new Error("Invalid Ethereum address provided."); + throw new Error( + `Invalid address provided: ${ + !isAddress(args.gateway) + ? `gateway=${args.gateway}` + : `uniswapRouter=${args.uniswapRouter}` + }` + );Also applies to: 16-22
contracts/nft/tasks/transfer.ts (1)
8-12: Maintain consistent error message format across files.For consistency with the token deployment task, enhance the error message specificity.
- throw new Error("Invalid Ethereum address provided."); + throw new Error( + `Invalid address provided: ${ + !isAddress(args.to) + ? `to=${args.to}` + : `revertAddress=${args.revertAddress}` + }` + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
contracts/nft/tasks/connectedSetUniversal.ts(1 hunks)contracts/nft/tasks/deploy.ts(3 hunks)contracts/nft/tasks/mint.ts(3 hunks)contracts/nft/tasks/transfer.ts(3 hunks)contracts/nft/tasks/universalSetConnected.ts(1 hunks)contracts/token/tasks/connectedSetUniversal.ts(1 hunks)contracts/token/tasks/deploy.ts(3 hunks)contracts/token/tasks/mint.ts(3 hunks)contracts/token/tasks/transfer.ts(2 hunks)contracts/token/tasks/universalSetConnected.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- contracts/token/tasks/mint.ts
- contracts/nft/tasks/deploy.ts
- contracts/nft/tasks/mint.ts
🔇 Additional comments (10)
contracts/token/tasks/universalSetConnected.ts (2)
3-3: LGTM: Import statement aligns with contract renaming strategy
The updated import reflects the new contract naming convention, maintaining consistency with the broader refactoring effort.
23-25: Verify UUPS implementation at the specified address
The contract retrieval has been updated for the new UUPS pattern. Ensure that the contract at the specified address is indeed a UUPS implementation.
✅ Verification successful
UUPS implementation verified successfully
The contract implementation follows the UUPS pattern correctly:
ZetaChainUniversalTokeninherits fromUniversalTokenwhich implementsUUPSUpgradeable- Proper initialization pattern with
initializefunction and disabled initializers in constructor - Required
_authorizeUpgradefunction implementation with owner-only modifier - All necessary UUPS-related imports from OpenZeppelin
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if the contract at the specified address is a UUPS implementation
# Search for UUPS implementation in the contract
ast-grep --pattern 'contract ZetaChainUniversalToken is UUPSUpgradeable {
$$$
}'
# Search for initialize function required by UUPS
ast-grep --pattern 'function initialize($$$) {
$$$
}'
Length of output: 142
Script:
#!/bin/bash
# Let's search for the contract implementation with a broader pattern
rg -l "contract ZetaChainUniversalToken" --type solidity
# Then examine the contract inheritance and implementation
rg "contract ZetaChainUniversalToken" -A 5 --type solidity
# Also search for initialize function in Solidity files
rg "function initialize" -A 5 --type solidity
# Check for UUPS related imports
rg "import.*UUPSUpgradeable" --type solidity
Length of output: 346
Script:
#!/bin/bash
# Search for contract files and implementation
fd -e sol . | xargs cat
# Alternative search for UUPS implementation
rg "contract ZetaChainUniversalToken"
# Search for initialize function
rg "function initialize"
# Search for UUPS imports
rg "import.*UUPSUpgradeable"
Length of output: 30775
contracts/token/tasks/connectedSetUniversal.ts (1)
15-17: LGTM: Address validation implementation
The addition of address validation using isAddress is a good practice for preventing invalid inputs.
contracts/nft/tasks/universalSetConnected.ts (2)
14-21: LGTM: Comprehensive address validation
The implementation properly validates all required addresses: contract, zrc20, and connected addresses.
22-25: 🛠️ Refactor suggestion
Add proxy validation and gas limit
Similar to other tasks, this needs proxy validation and gas limit configuration for upgradeable contracts.
const contract: ZetaChainUniversalNFT = await hre.ethers.getContractAt(
"ZetaChainUniversalNFT",
args.contract
);
+const implementationAddress = await hre.upgrades.erc1967.getImplementationAddress(args.contract);
+if (args.contract === implementationAddress) {
+ throw new Error('Provided address is implementation contract, not proxy');
+}
-const tx = await contract.setConnected(args.zrc20, args.connected);
+const tx = await contract.setConnected(args.zrc20, args.connected, {
+ gasLimit: args.gasLimit || 1000000
+});Likely invalid or redundant comment.
contracts/token/tasks/deploy.ts (2)
25-32: Add UUPS-specific proxy deployment options.
The proxy deployment should specify the UUPS implementation type and initializer function.
const contract = await hre.upgrades.deployProxy(factory, [
signer.address,
args.tokenName,
args.tokenSymbol,
args.gateway,
args.gasLimit,
...(args.uniswapRouter ? [args.uniswapRouter] : []),
-]);
+], {
+ initializer: 'initialize',
+ kind: 'uups'
+});54-58: LGTM: Contract name change aligns with PR objectives.
The rename to "ZetaChainUniversalToken" provides a unique identifier, which is beneficial for ThirdWeb integration as mentioned in the PR objectives.
contracts/nft/tasks/transfer.ts (3)
25-28: LGTM: Contract name change maintains consistency.
The rename to "ZetaChainUniversalNFT" maintains consistency with the token contract changes and supports unique identification.
73-73: LGTM: Gas limit aligned with proxy deployment requirements.
The increased gas limit of 10000000 is appropriate for proxy contract interactions.
34-39: Verify gas handling in cross-chain transfer.
The implementation allows ZETA for gas payment as intended, but we should verify the contract handles this correctly.
✅ Verification successful
The previous pattern didn't yield results, so I'm broadening the search to find the contract implementation and any gas-related handling. This will help us verify how the gas payment is processed in the cross-chain transfer.
Gas handling in cross-chain transfer is properly implemented
The contract correctly handles the gas payment mechanism:
- Validates non-zero msg.value with
if (msg.value == 0) revert ZeroMsgValue() - Converts ZETA to wrapped ZETA (WZETA) for gas fee processing
- Performs gas token swap through Uniswap router
- Handles remaining gas refunds to the sender
- Includes safety checks with proper revert conditions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for gas handling in the NFT contract
ast-grep --pattern 'function transferCrossChain($_, $_, $_) {
$$$
value
$$$
}'
Length of output: 86
Script:
#!/bin/bash
# Search for transferCrossChain function implementation
rg -U "function transferCrossChain.*?\{.*?\}" -A 20
# Also search for any gas or value handling patterns
rg -U "(msg\.value|gasAmount|value:)" -A 2 -B 2
Length of output: 8475
| const contract = await ethers.getContractAt( | ||
| "ZetaChainUniversalToken", | ||
| args.from | ||
| ); |
There was a problem hiding this comment.
Add contract existence validation
The code assumes the contract exists at the given address. Given the issues mentioned in PR comments about contract not existing at specified addresses, we should add validation.
Add this validation before proceeding with the transfer:
const contract = await ethers.getContractAt(
"ZetaChainUniversalToken",
args.from
);
+ const code = await ethers.provider.getCode(args.from);
+ if (code === '0x') {
+ throw new Error(`No contract found at address ${args.from}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const contract = await ethers.getContractAt( | |
| "ZetaChainUniversalToken", | |
| args.from | |
| ); | |
| const contract = await ethers.getContractAt( | |
| "ZetaChainUniversalToken", | |
| args.from | |
| ); | |
| const code = await ethers.provider.getCode(args.from); | |
| if (code === '0x') { | |
| throw new Error(`No contract found at address ${args.from}`); | |
| } |
| .addParam("gasAmount", "The amount of gas to transfer", "0") | ||
| .addParam("amount", "The amount of gas to transfer", "0"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update gas limit and improve parameter descriptions
- There's an inconsistency in the gas limit value (7000000 in definition vs 10000000 mentioned in PR)
- The parameter descriptions for gasAmount and amount are identical and not descriptive enough
Consider these improvements:
.addOptionalParam(
"txOptionsGasLimit",
"The gas limit for the transaction",
- 7000000,
+ 10000000,
types.int
)
// ... other params ...
.addParam("gasAmount", "The amount of gas to transfer", "0")
- .addParam("amount", "The amount of gas to transfer", "0");
+ .addParam("amount", "The amount of tokens to transfer", "0");Committable suggestion skipped: line range outside the PR's diff.
| const value = ethers.utils.parseUnits(args.amount, 18); | ||
| const tokenApprove = await contract.approve(args.from, value); | ||
| await tokenApprove.wait(); | ||
|
|
||
| const gasAmount = ethers.utils.parseUnits(args.gasAmount, 18); | ||
|
|
||
| const receiver = args.receiver || signer.address; | ||
|
|
||
| tx = await (contract as any).transferCrossChain( | ||
| const tx = await (contract as any).transferCrossChain( | ||
| args.to, | ||
| receiver, |
There was a problem hiding this comment.
Enhance error handling and add decimal validation
The current implementation has several potential issues:
- The approval transaction could fail silently
- The gas amount parsing assumes 18 decimals
- No error handling for the transfer operation
Consider these improvements:
+ const decimals = await contract.decimals().catch(() => 18);
const value = ethers.utils.parseUnits(args.amount, decimals);
- const tokenApprove = await contract.approve(args.from, value);
- await tokenApprove.wait();
+ try {
+ const tokenApprove = await contract.approve(args.from, value);
+ await tokenApprove.wait();
+ } catch (error) {
+ throw new Error(`Token approval failed: ${error.message}`);
+ }
const gasAmount = ethers.utils.parseUnits(args.gasAmount, 18);
const receiver = args.receiver || signer.address;
- const tx = await (contract as any).transferCrossChain(
- args.to,
- receiver,
- args.amount,
- { ...txOptions, value: gasAmount }
- );
+ try {
+ const tx = await (contract as any).transferCrossChain(
+ args.to,
+ receiver,
+ args.amount,
+ { ...txOptions, value: gasAmount }
+ );
+ await tx.wait();
+ } catch (error) {
+ throw new Error(`Cross-chain transfer failed: ${error.message}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const value = ethers.utils.parseUnits(args.amount, 18); | |
| const tokenApprove = await contract.approve(args.from, value); | |
| await tokenApprove.wait(); | |
| const gasAmount = ethers.utils.parseUnits(args.gasAmount, 18); | |
| const receiver = args.receiver || signer.address; | |
| tx = await (contract as any).transferCrossChain( | |
| const tx = await (contract as any).transferCrossChain( | |
| args.to, | |
| receiver, | |
| const decimals = await contract.decimals().catch(() => 18); | |
| const value = ethers.utils.parseUnits(args.amount, decimals); | |
| try { | |
| const tokenApprove = await contract.approve(args.from, value); | |
| await tokenApprove.wait(); | |
| } catch (error) { | |
| throw new Error(`Token approval failed: ${error.message}`); | |
| } | |
| const gasAmount = ethers.utils.parseUnits(args.gasAmount, 18); | |
| const receiver = args.receiver || signer.address; | |
| try { | |
| const tx = await (contract as any).transferCrossChain( | |
| args.to, | |
| receiver, | |
| args.amount, | |
| { ...txOptions, value: gasAmount } | |
| ); | |
| await tx.wait(); | |
| } catch (error) { | |
| throw new Error(`Cross-chain transfer failed: ${error.message}`); | |
| } |
| const contract: EVMUniversalToken = await hre.ethers.getContractAt( | ||
| "EVMUniversalToken", | ||
| args.contract | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proxy contract validation
When working with upgradeable contracts, it's important to verify we're interacting with the proxy contract and not the implementation directly.
const contract: EVMUniversalToken = await hre.ethers.getContractAt(
"EVMUniversalToken",
args.contract
);
+const implementationAddress = await hre.upgrades.erc1967.getImplementationAddress(args.contract);
+if (args.contract === implementationAddress) {
+ throw new Error('Provided address is implementation contract, not proxy');
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const contract: EVMUniversalToken = await hre.ethers.getContractAt( | |
| "EVMUniversalToken", | |
| args.contract | |
| ); | |
| const contract: EVMUniversalToken = await hre.ethers.getContractAt( | |
| "EVMUniversalToken", | |
| args.contract | |
| ); | |
| const implementationAddress = await hre.upgrades.erc1967.getImplementationAddress(args.contract); | |
| if (args.contract === implementationAddress) { | |
| throw new Error('Provided address is implementation contract, not proxy'); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
contracts/nft/tasks/transfer.ts (1)
8-12: Enhance error message specificity for address validation.While the address validation is a good addition, the error message could be more specific to help users identify which address is invalid.
Consider this improvement:
- if (!isAddress(args.to) || !isAddress(args.revertAddress)) { - throw new Error("Invalid Ethereum address provided."); + if (!isAddress(args.to)) { + throw new Error(`Invalid destination address provided: ${args.to}`); + } + if (!isAddress(args.revertAddress)) { + throw new Error(`Invalid revert address provided: ${args.revertAddress}`); + }contracts/nft/tasks/setConnected.ts (1)
14-20: Enhance error messages with specific validation failuresWhile the address validation is good, the error message could be more specific about which address is invalid.
Consider implementing more detailed error messages:
- if ( - !isAddress(args.contract) || - !isAddress(args.zrc20) || - !isAddress(args.connected) - ) { - throw new Error("Invalid Ethereum address provided."); - } + for (const [key, value] of Object.entries({ + contract: args.contract, + zrc20: args.zrc20, + connected: args.connected + })) { + if (!isAddress(value)) { + throw new Error(`Invalid Ethereum address provided for ${key}: ${value}`); + } + }contracts/nft/tasks/mint.ts (1)
15-17: Enhance error message for address validationWhile the validation is correct, the error message could be more helpful by including the invalid address.
- throw new Error("Invalid Ethereum address provided."); + throw new Error(`Invalid Ethereum address provided: ${args.contract}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
contracts/nft/hardhat.config.ts(1 hunks)contracts/nft/scripts/localnet.sh(2 hunks)contracts/nft/tasks/deploy.ts(2 hunks)contracts/nft/tasks/index.ts(1 hunks)contracts/nft/tasks/mint.ts(2 hunks)contracts/nft/tasks/setConnected.ts(2 hunks)contracts/nft/tasks/setUniversal.ts(2 hunks)contracts/nft/tasks/transfer.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- contracts/nft/tasks/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/nft/scripts/localnet.sh
- contracts/nft/tasks/deploy.ts
🔇 Additional comments (11)
contracts/nft/tasks/transfer.ts (1)
25-28: Verify error handling for contract interaction.
The direct contract interaction without try-catch could potentially lead to unhandled errors. Consider how failed contract interactions should be handled.
✅ Verification successful
Error handling follows task-level pattern
Based on the codebase analysis, the absence of try-catch blocks in contract interactions is consistent across all task files (setUniversal.ts, transfer.ts, setConnected.ts, mint.ts). This appears to be an intentional design pattern where error handling is managed at the task execution level rather than individual contract calls.
- The pattern is uniform across all contract interactions in the tasks directory
- Similar contract calls in
setConnected.tsandmint.tsalso use direct interactions - Task runners typically handle errors at a higher level, which is a valid architectural choice
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in similar contract interactions
rg -A 5 "getContractAt.*ZetaChainUniversalNFT" --type ts
Length of output: 58
Script:
#!/bin/bash
# Let's first check for any contract interactions in the tasks directory
rg -A 5 "getContractAt" contracts/nft/tasks/ --type ts
# Also check for error handling patterns in tasks
rg -A 5 "try.*catch" contracts/nft/tasks/ --type ts
Length of output: 2017
contracts/nft/hardhat.config.ts (1)
4-5: LGTM: Import changes align with upgradeability requirements
The imports have been correctly updated to support OpenZeppelin's UUPS upgradeability pattern and the new task structure.
Also applies to: 13-14
contracts/nft/tasks/setUniversal.ts (2)
14-16: LGTM: Enhanced input validation
The addition of address validation using isAddress improves the robustness of the task.
44-48: LGTM: Task naming follows new convention
The task naming convention nft:set-universal is consistent with the new structure.
contracts/nft/tasks/setConnected.ts (2)
22-24: LGTM: Contract type correctly updated
The contract type has been properly updated to ZetaChainUniversalNFT.
47-51: LGTM: Task naming follows new convention
The task naming convention nft:set-connected is consistent with the new structure.
contracts/nft/tasks/mint.ts (5)
3-7: LGTM: Enhanced input validation setup
The addition of URL validation and ethers utilities strengthens the input validation, which is crucial for smart contract interactions.
19-32: LGTM: Robust token URI validation
The implementation provides thorough validation with clear error messaging and proper protocol restrictions.
70-79: LGTM: Consistent token ID formatting
The addition of toString() ensures consistent string representation of BigNumber values in both JSON and text output formats.
83-94: LGTM: Improved task organization
The task definition changes enhance clarity through:
- Namespaced task name (
nft:mint) - Updated default contract name to align with the new contract structure
34-36: Verify contract type consistency
The contract types have been updated to use specific NFT contract names. Let's verify the consistency across the codebase.
✅ Verification successful
Contract type usage is consistent across the codebase
The contract types ZetaChainUniversalNFT and EVMUniversalNFT are used consistently throughout the codebase:
- Contract definitions exist in their respective
.solfiles - Type imports and contract interactions in tasks are properly aligned
- Deployment scripts correctly reference the contract names
- The type union in
mint.tsaccurately reflects the available contract types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for contract type references
rg -t typescript '"(ZetaChainUniversalNFT|EVMUniversalNFT)"' contracts/
Length of output: 108
Script:
#!/bin/bash
# Search for contract type references without file type restriction
rg "\"(ZetaChainUniversalNFT|EVMUniversalNFT)\"" contracts/
# Also search for contract names without quotes
rg "(ZetaChainUniversalNFT|EVMUniversalNFT)" contracts/
# Search for contract artifacts
fd "ZetaChainUniversalNFT|EVMUniversalNFT" contracts/
Length of output: 2880
| const tx = await contract.transferCrossChain( | ||
| args.tokenId, | ||
| receiver, | ||
| args.to, | ||
| { ...txOptions, value: gasAmount } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for gas amount parameter.
The gas amount is parsed but not validated. Consider adding checks for negative values or amounts exceeding reasonable limits.
Add this validation before the transaction:
const gasAmount = ethers.utils.parseUnits(args.gasAmount, 18);
+ if (gasAmount.lte(0)) {
+ throw new Error("Gas amount must be greater than 0");
+ }
+ const maxGasAmount = ethers.utils.parseUnits("1000", 18); // Adjust limit as needed
+ if (gasAmount.gt(maxGasAmount)) {
+ throw new Error(`Gas amount exceeds maximum limit of 1000 ZETA`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const tx = await contract.transferCrossChain( | |
| args.tokenId, | |
| receiver, | |
| args.to, | |
| { ...txOptions, value: gasAmount } | |
| ); | |
| const gasAmount = ethers.utils.parseUnits(args.gasAmount, 18); | |
| if (gasAmount.lte(0)) { | |
| throw new Error("Gas amount must be greater than 0"); | |
| } | |
| const maxGasAmount = ethers.utils.parseUnits("1000", 18); // Adjust limit as needed | |
| if (gasAmount.gt(maxGasAmount)) { | |
| throw new Error(`Gas amount exceeds maximum limit of 1000 ZETA`); | |
| } | |
| const tx = await contract.transferCrossChain( | |
| args.tokenId, | |
| receiver, | |
| args.to, | |
| { ...txOptions, value: gasAmount } | |
| ); |
| @@ -70,7 +74,7 @@ | |||
| .addOptionalParam( | |||
| "txOptionsGasLimit", | |||
| "The gas limit for the transaction", | |||
| 7000000, | |||
| 10000000, | |||
There was a problem hiding this comment.
💡 Codebase verification
Gas limit value appears to be inconsistent with codebase patterns
The hardcoded gas limit of 10000000 is significantly higher than other gas limits found in the codebase:
- Other tasks use configurable gas limits via arguments (e.g.,
args.txOptionsGasLimit) - Similar task
setUniversal.tsuses a much lower gas limit of 1000000 - Previous version of this file used a configurable gas limit through
args.txOptionsGasLimit
Consider:
- Keeping the gas limit configurable via
args.txOptionsGasLimitas it was before - If a fixed value is required, align it with other similar operations (~1000000)
- Document the reason for such a high gas limit if it's absolutely necessary
🔗 Analysis chain
Verify the increased gas limit requirement.
The gas limit has been increased from 7000000 to 10000000. Please ensure this increase is necessary and doesn't lead to excessive gas costs.
Let me search for gas limit patterns and their context to verify if this increase is justified based on the codebase patterns and history.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar gas limit patterns
rg "gasLimit.*10000000" --type ts
# Search for deployment transaction receipts
rg "receipt.*gasUsed" --type ts
Length of output: 69
Script:
#!/bin/bash
# Search for gas limit configurations in the codebase
rg "gasLimit.*\d+" --type ts -A 2
# Search for any gas-related configurations or constants
rg "GAS_LIMIT|gas.*limit" --type ts -i
# Look for the previous version of this file in git history
git show HEAD~1:contracts/nft/tasks/transfer.ts | grep -A 5 -B 5 "gasLimit"
Length of output: 1655
| solidity: { | ||
| compilers: [ | ||
| { | ||
| settings: { | ||
| optimizer: { | ||
| enabled: true, | ||
| runs: 1000, | ||
| }, | ||
| }, | ||
| version: "0.8.26", | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent compiler settings detected between contracts
The verification revealed inconsistent Solidity compiler configurations across the project:
contracts/nft/hardhat.config.ts: Uses detailed configuration with optimization enabled (1000 runs)contracts/token/hardhat.config.ts: Uses simplified configuration without optimization settings
This inconsistency could lead to:
- Different bytecode optimization levels between contracts
- Potential verification issues on block explorers
- Varying gas costs across deployments
Recommendation: Align the compiler settings by applying the same optimization configuration to both contracts, preferably using the more detailed configuration from the NFT contract.
🔗 Analysis chain
Verify compiler optimization settings across all contracts
The compiler settings look appropriate for production deployment. However, ensure these settings are consistent across all contract deployments to prevent any potential verification issues on block explorers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent compiler settings across all Hardhat configs
fd -e ts -e js 'hardhat.config' | xargs rg -A 10 'solidity.*{'
Length of output: 148
Script:
#!/bin/bash
# Check for consistent compiler settings across all Hardhat configs
fd -e ts -e js 'hardhat.config' | xargs rg -l 'solidity'
# If files are found, examine their compiler settings
fd -e ts -e js 'hardhat.config' | while read -r file; do
echo "=== $file ==="
rg -A 15 'solidity:' "$file" || true
done
Length of output: 768
| const tx = await contract.setUniversal(args.universal, { | ||
| gasLimit: 1000000, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider dynamic gas limit calculation
Using a fixed gas limit of 1,000,000 might not be optimal across different networks and could lead to failed transactions.
Consider implementing dynamic gas estimation:
- const tx = await contract.setUniversal(args.universal, {
- gasLimit: 1000000,
- });
+ const gasEstimate = await contract.estimateGas.setUniversal(args.universal);
+ const tx = await contract.setUniversal(args.universal, {
+ gasLimit: Math.ceil(gasEstimate.mul(120).div(100)), // Add 20% buffer
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const tx = await contract.setUniversal(args.universal, { | |
| gasLimit: 1000000, | |
| }); | |
| const gasEstimate = await contract.estimateGas.setUniversal(args.universal); | |
| const tx = await contract.setUniversal(args.universal, { | |
| gasLimit: Math.ceil(gasEstimate.mul(120).div(100)), // Add 20% buffer | |
| }); |
| const transferEvent = receipt.logs | ||
| .map((log: any) => { | ||
| try { | ||
| return contract.interface.parseLog(log); | ||
| } catch { | ||
| return null; | ||
| } | ||
| }) | ||
| .find((parsedLog: any) => parsedLog?.name === "Transfer"); | ||
|
|
||
| if (!transferEvent) { | ||
| throw new Error("Transfer event not found in transaction logs."); | ||
| } | ||
|
|
||
| const tokenId = transferEvent?.args?.tokenId; | ||
|
|
||
| if (!tokenId) { | ||
| throw new Error("Transfer event not found in transaction logs."); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Differentiate error messages and enhance error handling
The event parsing is more robust, but there are two issues:
- The same error message is used for different error conditions
- The error handling could be more informative
- throw new Error("Transfer event not found in transaction logs.");
+ throw new Error("Failed to find Transfer event in transaction logs.");
}
const tokenId = transferEvent?.args?.tokenId;
if (!tokenId) {
- throw new Error("Transfer event not found in transaction logs.");
+ throw new Error("Token ID not found in Transfer event.");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const transferEvent = receipt.logs | |
| .map((log: any) => { | |
| try { | |
| return contract.interface.parseLog(log); | |
| } catch { | |
| return null; | |
| } | |
| }) | |
| .find((parsedLog: any) => parsedLog?.name === "Transfer"); | |
| if (!transferEvent) { | |
| throw new Error("Transfer event not found in transaction logs."); | |
| } | |
| const tokenId = transferEvent?.args?.tokenId; | |
| if (!tokenId) { | |
| throw new Error("Transfer event not found in transaction logs."); | |
| } | |
| const transferEvent = receipt.logs | |
| .map((log: any) => { | |
| try { | |
| return contract.interface.parseLog(log); | |
| } catch { | |
| return null; | |
| } | |
| }) | |
| .find((parsedLog: any) => parsedLog?.name === "Transfer"); | |
| if (!transferEvent) { | |
| throw new Error("Failed to find Transfer event in transaction logs."); | |
| } | |
| const tokenId = transferEvent?.args?.tokenId; | |
| if (!tokenId) { | |
| throw new Error("Token ID not found in Transfer event."); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
contracts/token/tasks/mint.ts (1)
26-26: 🛠️ Refactor suggestionEnhance transaction receipt handling
The current implementation lacks proper error handling and event verification for the mint operation.
- await tx.wait(); + const receipt = await tx.wait(); + const mintEvent = receipt.events?.find(e => e.event === 'Transfer' && e.args?.from === ethers.constants.AddressZero); + if (!mintEvent) { + throw new Error('Mint transaction completed but no Transfer event found'); + } + // Verify mint amount matches requested amount + if (mintEvent.args?.value.toString() !== args.amount.toString()) { + throw new Error(`Minted amount ${mintEvent.args?.value} differs from requested amount ${args.amount}`); + }contracts/token/tasks/deploy.ts (1)
25-32:⚠️ Potential issueConfigure UUPS proxy deployment
The proxy deployment should be configured for UUPS upgradeability pattern.
const contract = await hre.upgrades.deployProxy(factory, [ signer.address, args.tokenName, args.tokenSymbol, args.gateway, args.gasLimit, ...(args.uniswapRouter ? [args.uniswapRouter] : []), -]); +], { + initializer: 'initialize', + kind: 'uups' +});contracts/token/tasks/transfer.ts (1)
19-23:⚠️ Potential issueAdd contract existence validation
Validate contract existence at the specified address.
const contract = await ethers.getContractAt( "ZetaChainUniversalToken", args.from ); + const code = await ethers.provider.getCode(args.from); + if (code === '0x') { + throw new Error(`No contract found at address ${args.from}`); + }
🧹 Nitpick comments (7)
contracts/token/tasks/setUniversal.ts (2)
6-7: Enhance error message specificityWhile the address validation is a good security measure, the error message could be more informative by indicating which address is invalid.
Consider this improvement:
- throw new Error("Invalid Ethereum address provided."); + throw new Error( + `Invalid Ethereum address provided. Contract: ${args.contract}, Universal: ${args.universal}` + );Also applies to: 15-17
19-22: Consider adding gas limit configurationWhile the contract interaction is correct, consider adding a gas limit configuration to prevent potential transaction failures during complex operations.
Consider adding gas configuration:
- const tx = await contract.setUniversal(args.universal); + const tx = await contract.setUniversal(args.universal, { + gasLimit: 100000 // Adjust based on your needs + });contracts/token/tasks/mint.ts (1)
14-16: Enhance address validation error messageThe error message should specify which address is invalid for better debugging.
- throw new Error("Invalid Ethereum address provided."); + throw new Error(`Invalid contract address: ${args.contract}`);contracts/token/tasks/deploy.ts (1)
16-21: Enhance address validation error messagesSpecify which address is invalid for better debugging.
if ( !isAddress(args.gateway) || (args.uniswapRouter && !isAddress(args.uniswapRouter)) ) { - throw new Error("Invalid Ethereum address provided."); + throw new Error( + !isAddress(args.gateway) + ? `Invalid gateway address: ${args.gateway}` + : `Invalid uniswap router address: ${args.uniswapRouter}` + ); }contracts/token/tasks/transfer.ts (1)
10-12: Enhance address validation error messagesSpecify which address is invalid for better debugging.
- if (!isAddress(args.to) || !isAddress(args.revertAddress)) { - throw new Error("Invalid Ethereum address provided."); + if (!isAddress(args.to)) { + throw new Error(`Invalid destination address: ${args.to}`); + } + if (!isAddress(args.revertAddress)) { + throw new Error(`Invalid revert address: ${args.revertAddress}`); + }contracts/token/tasks/setConnected.ts (2)
15-21: Enhance error message specificityWhile the address validation is good, the error message could be more helpful by indicating which specific address is invalid.
Consider this improvement:
- throw new Error("Invalid Ethereum address provided."); + throw new Error( + `Invalid Ethereum address provided. Please verify:\n` + + `Contract: ${args.contract}\n` + + `ZRC20: ${args.zrc20}\n` + + `Connected: ${args.connected}` + );
23-24: Consider using type-safe contract factoryInstead of using a string literal for the contract name, consider using the contract factory for better type safety.
- const contract: ZetaChainUniversalToken = await hre.ethers.getContractAt( - "ZetaChainUniversalToken", + const contract = await hre.ethers.getContractAt( + (await hre.ethers.getContractFactory("ZetaChainUniversalToken")).interface.format(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
contracts/nft/hardhat.config.ts(1 hunks)contracts/token/contracts/example/EVMUniversalToken.sol(1 hunks)contracts/token/contracts/example/ZetaChainUniversalToken.sol(1 hunks)contracts/token/hardhat.config.ts(1 hunks)contracts/token/scripts/localnet.sh(2 hunks)contracts/token/tasks/deploy.ts(3 hunks)contracts/token/tasks/index.ts(1 hunks)contracts/token/tasks/mint.ts(2 hunks)contracts/token/tasks/setConnected.ts(2 hunks)contracts/token/tasks/setUniversal.ts(2 hunks)contracts/token/tasks/transfer.ts(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- contracts/token/contracts/example/EVMUniversalToken.sol
- contracts/token/contracts/example/ZetaChainUniversalToken.sol
- contracts/token/tasks/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- contracts/token/hardhat.config.ts
- contracts/nft/hardhat.config.ts
- contracts/token/scripts/localnet.sh
🔇 Additional comments (8)
contracts/token/tasks/setUniversal.ts (2)
3-3: LGTM: Import change aligns with contract upgrades
The import change from Connected to EVMUniversalToken correctly reflects the architectural shift to upgradeable contracts.
41-45: LGTM: Task registration follows improved naming convention
The task registration changes improve code organization through:
- Clear namespace separation with "token:" prefix
- Better modularity with explicit export
contracts/token/tasks/deploy.ts (1)
Line range hint 63-67: Align gas limit with NFT deployment
The gas limit should be consistent with the NFT deployment since both use proxy deployment.
.addOptionalParam(
"gasLimit",
"Gas limit for the transaction",
- 1000000,
+ 10000000,
types.int
)contracts/token/tasks/transfer.ts (2)
Line range hint 24-36: Enhance error handling and add decimal validation
The current implementation lacks proper error handling for approval and transfer operations.
+ const decimals = await contract.decimals().catch(() => 18);
const value = ethers.utils.parseUnits(args.amount, decimals);
- const tokenApprove = await contract.approve(args.from, value);
- await tokenApprove.wait();
+ try {
+ const tokenApprove = await contract.approve(args.from, value);
+ await tokenApprove.wait();
+ } catch (error) {
+ throw new Error(`Token approval failed: ${error.message}`);
+ }
const gasAmount = ethers.utils.parseUnits(args.gasAmount, 18);
const receiver = args.receiver || signer.address;
- const tx = await (contract as any).transferCrossChain(
- args.to,
- receiver,
- args.amount,
- { ...txOptions, value: gasAmount }
- );
+ try {
+ const tx = await (contract as any).transferCrossChain(
+ args.to,
+ receiver,
+ args.amount,
+ { ...txOptions, value: gasAmount }
+ );
+ await tx.wait();
+ } catch (error) {
+ throw new Error(`Cross-chain transfer failed: ${error.message}`);
+ }97-98: Update parameter descriptions
The parameter descriptions for gasAmount and amount are identical and not descriptive enough.
.addParam("gasAmount", "The amount of gas to transfer", "0")
- .addParam("amount", "The amount of gas to transfer", "0");
+ .addParam("amount", "The amount of tokens to transfer", "0");contracts/token/tasks/setConnected.ts (3)
3-3: LGTM: Type import aligns with contract renaming
The import statement correctly references the renamed contract type, maintaining type safety.
48-52: LGTM: Improved task naming and modularity
The task registration changes improve the codebase in two ways:
- The new naming convention "token:set-connected" is more consistent and descriptive
- Exporting the task enhances modularity and reusability
Line range hint 1-57: Verify integration with upgrade system
Since this PR implements upgradeable contracts, we should verify that this task properly integrates with the upgrade system.
✅ Verification successful
Let me analyze the deployment and contract implementation to understand how setConnected integrates with the upgrade system before making a final determination.
Integration with upgrade system verified successfully
The task properly integrates with the upgrade system:
- The contract is deployed as a proxy using
hre.upgrades.deployProxyin the deployment task - The
setConnectedfunction is part of the upgradeable contract implementation and includes proper access control (onlyOwner) - Address validation in the task provides an additional security layer for proxy interactions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related upgrade tasks and proxy implementations
echo "Searching for related upgrade tasks..."
rg -l "UUPSUpgradeable|ERC1967Proxy" contracts/
echo "Checking for other tasks that might need similar address validation..."
rg -l "task\(" contracts/token/tasks/
Length of output: 699
Script:
#!/bin/bash
# Check if other tasks handle proxy interactions
echo "Checking deploy task for proxy setup..."
cat contracts/token/tasks/deploy.ts
echo "Checking if setConnected is used in proxy context..."
rg -C 5 "setConnected" contracts/token/contracts/zetachain/UniversalToken.sol contracts/token/contracts/evm/UniversalToken.sol
Length of output: 3410
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (3)
contracts/nft/contracts/evm/UniversalNFT.sol (1)
Line range hint
79-93: Improve token ID generation mechanism.The current token ID generation using block number and keccak256 could lead to collisions in high-frequency minting scenarios. Consider using OpenZeppelin's Counters:
+ using Counters for Counters.Counter; + Counters.Counter private _tokenIdCounter; + function safeMint( address to, string memory uri ) public whenNotPaused onlyOwner { - uint256 hash = uint256( - keccak256( - abi.encodePacked(address(this), block.number, _nextTokenId++) - ) - ); - uint256 tokenId = hash & 0x00FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF; + uint256 tokenId = _tokenIdCounter.current(); + _tokenIdCounter.increment(); _safeMint(to, tokenId); _setTokenURI(tokenId, uri);contracts/nft/contracts/zetachain/UniversalNFT.sol (2)
Line range hint
59-79: Enhance initializer securityThe initialize function should include additional safeguards:
- Add nonReentrant modifier
- Validate gas limit against reasonable bounds
+ uint256 public constant MIN_GAS_LIMIT = 21000; + uint256 public constant MAX_GAS_LIMIT = 1000000; function initialize( address initialOwner, string memory name, string memory symbol, address payable gatewayAddress, uint256 gas, address uniswapRouterAddress - ) public initializer { + ) public initializer nonReentrant { __ERC721_init(name, symbol); __ERC721Enumerable_init(); __ERC721URIStorage_init(); __Ownable_init(initialOwner); __ERC721Burnable_init(); __UUPSUpgradeable_init(); if (gatewayAddress == address(0) || uniswapRouterAddress == address(0)) revert InvalidAddress(); - if (gas == 0) revert InvalidGasLimit(); + if (gas < MIN_GAS_LIMIT || gas > MAX_GAS_LIMIT) revert InvalidGasLimit(); gateway = GatewayZEVM(gatewayAddress); uniswapRouter = uniswapRouterAddress; gasLimitAmount = gas; }
Line range hint
94-127: Refactor complex transfer logic and enhance error handlingThe
transferCrossChainfunction handles multiple concerns and could benefit from:
- Extracting swap logic to a separate internal function
- Adding more specific error messages
- Implementing slippage protection
+ error SlippageExceeded(uint256 expected, uint256 received); + error InsufficientGasFee(uint256 required, uint256 provided); + function _handleGasSwap( + address WZETA, + uint256 gasFee, + address gasZRC20 + ) internal returns (uint256 remaining) { + IWETH9(WZETA).deposit{value: msg.value}(); + IWETH9(WZETA).approve(uniswapRouter, msg.value); + + uint256 minOut = gasFee * 98 / 100; // 2% max slippage + uint256 out = SwapHelperLib.swapTokensForExactTokens( + uniswapRouter, + WZETA, + gasFee, + gasZRC20, + msg.value + ); + if (out < minOut) revert SlippageExceeded(minOut, out); + + return msg.value - out; + } function transferCrossChain( uint256 tokenId, address receiver, address destination ) public payable whenNotPaused { if (msg.value == 0) revert ZeroMsgValue(); + if (msg.value < gasFee) revert InsufficientGasFee(gasFee, msg.value); // ... rest of the function }
🧹 Nitpick comments (2)
contracts/token/contracts/evm/UniversalToken.sol (1)
Line range hint
82-100: Optimize gas fee handling in cross-chain transfers.The
transferCrossChainfunction could benefit from:
- Minimum value validation
- Gas fee estimation helper
- Clearer error messages
+ error InsufficientValue(uint256 required, uint256 provided); + + function estimateGasFee(address destination) public view returns (uint256) { + return gateway.estimateGasFee(destination, gasLimitAmount); + } + function transferCrossChain( address destination, address receiver, uint256 amount ) external payable whenNotPaused { + uint256 requiredGas = estimateGasFee(destination); + if (msg.value < requiredGas) { + revert InsufficientValue(requiredGas, msg.value); + } if (receiver == address(0)) revert InvalidAddress();contracts/nft/contracts/evm/UniversalNFT.sol (1)
56-61: Optimize initialization sequence.The initialization sequence is correct but could be more gas-efficient by combining related initializations:
- __ERC721_init(name, symbol); - __ERC721Enumerable_init(); - __ERC721URIStorage_init(); - __Ownable_init(initialOwner); - __ERC721Burnable_init(); - __UUPSUpgradeable_init(); + __ERC721_init_unchained(name, symbol); + __ERC721Enumerable_init_unchained(); + __ERC721URIStorage_init_unchained(); + __Ownable_init_unchained(initialOwner); + __ERC721Burnable_init_unchained(); + __UUPSUpgradeable_init_unchained();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
contracts/nft/contracts/evm/UniversalNFT.sol(4 hunks)contracts/nft/contracts/zetachain/UniversalNFT.sol(8 hunks)contracts/token/contracts/evm/UniversalToken.sol(3 hunks)contracts/token/contracts/zetachain/UniversalToken.sol(7 hunks)contracts/token/hardhat.config.ts(1 hunks)
🔇 Additional comments (8)
contracts/token/hardhat.config.ts (3)
9-10: LGTM! Required plugins for UUPS implementation
The addition of hardhat-ethers and hardhat-upgrades plugins is necessary for implementing upgradeable contracts using the UUPS pattern.
✅ Verification successful
Required plugins are properly configured in package.json
Both @nomiclabs/hardhat-ethers (v2.0.0) and @openzeppelin/hardhat-upgrades (v1.28.0) are correctly declared as devDependencies in the contracts/token/package.json file. The imports in hardhat.config.ts are properly supported by the project's dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify package dependencies
# Check if dependencies are properly declared in package.json
rg -A 1 '"@nomiclabs/hardhat-ethers"' package.json
rg -A 1 '"@openzeppelin/hardhat-upgrades"' package.json
Length of output: 110
Script:
#!/bin/bash
# Let's check the package.json content directly to see all dependencies
cat package.json
# Also check if there are any package.json files in subdirectories
fd package.json
Length of output: 369
Script:
#!/bin/bash
# Check the package.json in the token directory since the file under review is in contracts/token/
cat contracts/token/package.json
Length of output: 2129
1-1: Verify tasks directory structure after import consolidation
The consolidation of task imports improves maintainability. However, we should verify that all tasks are properly exported from the tasks directory.
✅ Verification successful
Import consolidation is properly implemented
The tasks directory structure is well-organized with all task files properly exported through the index.ts. The consolidation includes:
- All 5 task files (deploy.ts, mint.ts, setConnected.ts, setUniversal.ts, transfer.ts)
- Proper named exports in index.ts matching each task file
- Correct import statement in hardhat.config.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify tasks directory structure and exports
# Check if tasks/index.ts exists and exports all tasks
fd -t f "index.ts" tasks/
# List all task files to ensure they're properly organized
fd -t f . tasks/
Length of output: 232
Script:
#!/bin/bash
# Let's first check if the tasks directory exists relative to the token directory
fd -t d "tasks" contracts/token/
# Check for any .ts files in the token directory that might be task-related
fd -e ts . contracts/token/
# Use ripgrep to find any task-related imports or exports
rg -g "*.ts" "import.*task" contracts/token/
Length of output: 941
Script:
#!/bin/bash
# Let's verify the index.ts exports to ensure all tasks are properly consolidated
cat contracts/token/tasks/index.ts
# Check if all task files are properly imported in index.ts
rg -g "index.ts" "import.*" contracts/token/tasks/
Length of output: 311
16-28: LGTM! Optimized compiler settings for production
The compiler optimization settings are well-configured for production use:
- Optimizer enabled for better gas efficiency
- 1000 runs is a good balance between deployment cost and runtime efficiency
✅ Verification successful
Compiler settings are consistent and well-configured across contracts
The verification confirms that both NFT and token contracts use identical, production-grade compiler settings:
- Both use Solidity 0.8.26
- Both have optimizer enabled with 1000 runs
- Both use OpenZeppelin upgrades plugin
This configuration is appropriate for the contract complexity, considering the universal token and NFT implementations across EVM and ZetaChain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify compiler settings consistency across related contracts
# Check for consistent compiler settings across contract configurations
rg -A 10 'solidity.*version.*0\.8\.26' .
Length of output: 42
Script:
#!/bin/bash
# Let's try a broader search for compiler settings
rg -A 10 'solidity.*compilers' .
# Also search for any hardhat config files
fd 'hardhat.config' .
# Check for any other solidity version specifications
rg -A 5 'version.*0\.' .
Length of output: 67195
Script:
#!/bin/bash
# Let's check the content of both hardhat config files to compare compiler settings
cat ./contracts/nft/hardhat.config.ts
cat ./contracts/token/hardhat.config.ts
# Also check for any solidity files to understand the contract complexity
fd -e sol .
Length of output: 1935
contracts/token/contracts/evm/UniversalToken.sol (2)
54-61:
Initialize all inherited contracts.
The initialize function is missing initialization calls for ERC20PausableUpgradeable and ERC20PermitUpgradeable. Add the following initializations:
function initialize(
address initialOwner,
string memory name,
string memory symbol,
address payable gatewayAddress,
uint256 gas
) public initializer {
__ERC20_init(name, symbol);
__ERC20Burnable_init();
+ __ERC20Pausable_init();
+ __ERC20Permit_init(name);
__Ownable_init(initialOwner);
__UUPSUpgradeable_init();Likely invalid or redundant comment.
145-147: 🛠️ Refactor suggestion
Enhance upgrade security mechanism.
The current _authorizeUpgrade implementation lacks additional security measures. Consider implementing:
- Timelock mechanism
- Upgrade validation checks
- Events for upgrade transparency
+ uint256 private constant UPGRADE_TIMELOCK = 2 days;
+ uint256 private upgradeProposalTime;
+ address private proposedImplementation;
+
+ event UpgradeProposed(address indexed implementation);
+ event UpgradeExecuted(address indexed implementation);
+
+ function proposeUpgrade(address newImplementation) external onlyOwner {
+ proposedImplementation = newImplementation;
+ upgradeProposalTime = block.timestamp;
+ emit UpgradeProposed(newImplementation);
+ }
+
function _authorizeUpgrade(
address newImplementation
- ) internal override onlyOwner {}
+ ) internal override onlyOwner {
+ require(newImplementation == proposedImplementation, "Invalid implementation");
+ require(block.timestamp >= upgradeProposalTime + UPGRADE_TIMELOCK, "Timelock active");
+ emit UpgradeExecuted(newImplementation);
+ }Likely invalid or redundant comment.
contracts/token/contracts/zetachain/UniversalToken.sol (2)
36-36:
Remove unused state variable.
The _nextTokenId variable appears to be accidentally copied from the NFT contract and is never used.
- uint256 private _nextTokenId;Likely invalid or redundant comment.
104-122: 🛠️ Refactor suggestion
Enhance security and efficiency of token swaps.
The current implementation of token swaps could be improved:
- Add slippage protection
- Implement checks-effects-interactions pattern
- Add reentrancy protection
+ uint256 private constant MAX_SLIPPAGE = 100; // 1%
+
address WZETA = gateway.zetaToken();
+ uint256 minOut = (gasFee * (10000 - MAX_SLIPPAGE)) / 10000;
IWETH9(WZETA).deposit{value: msg.value}();
IWETH9(WZETA).approve(uniswapRouter, msg.value);
uint256 out = SwapHelperLib.swapTokensForExactTokens(
uniswapRouter,
WZETA,
gasFee,
gasZRC20,
- msg.value
+ msg.value,
+ minOut
);Likely invalid or redundant comment.
contracts/nft/contracts/zetachain/UniversalNFT.sol (1)
22-32: Review inheritance order for diamond problem mitigation
The contract inherits from multiple base contracts that share common ancestors. While the current order works, it's recommended to document the inheritance hierarchy to prevent future maintenance issues.
Run this script to analyze the inheritance hierarchy:
✅ Verification successful
Let me analyze the inheritance hierarchy by first getting the content of the relevant files. The previous command might have failed due to pattern matching issues with Solidity's syntax, so let's examine the raw files first.
The inheritance order is correctly implemented with no diamond inheritance issues
The inheritance hierarchy follows OpenZeppelin's recommended order for upgradeable contracts, with proper initialization sequence:
Initializableis first, as required- Base ERC721 implementation (
ERC721Upgradeable) - Storage extensions (
ERC721URIStorageUpgradeable,ERC721EnumerableUpgradeable) - Behavioral extensions (
ERC721PausableUpgradeable) - Access control (
OwnableUpgradeable) - Additional features (
ERC721BurnableUpgradeable,UniversalContract,UUPSUpgradeable) - Events interface (
UniversalNFTEvents)
All override implementations are properly handled with correct super calls, particularly in _update, _increaseBalance, tokenURI, and supportsInterface functions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify inheritance hierarchy using ast-grep
ast-grep --pattern 'contract $_ is $_'
Length of output: 40
Script:
#!/bin/bash
# Search for the contract file and its content
fd "UniversalNFT.sol" --type f --exec cat {}
# Also search for any related interface or base contract files
fd "Universal.*\.sol" --type f --exec cat {}
Length of output: 49485
| import {ERC20BurnableUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20BurnableUpgradeable.sol"; | ||
| import {ERC20PausableUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20PausableUpgradeable.sol"; | ||
| import {ERC20PermitUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20PermitUpgradeable.sol"; | ||
| import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate imports.
The following imports appear multiple times:
Initializable(lines 10 and 16)ERC20BurnableUpgradeable(lines 7 and 17)
Also applies to: 13-13, 16-16, 17-17
| GatewayZEVM public gateway; | ||
| address public uniswapRouter; | ||
| uint256 private _nextTokenId; | ||
| bool public constant isUniversal = true; | ||
| uint256 public immutable gasLimitAmount; | ||
| uint256 public gasLimitAmount; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing state variable protection mechanisms
The removal of immutable modifiers is necessary for upgradeability, but it introduces potential security risks. Consider:
- Adding a setter function with timelock for critical state variables
- Implementing events to track state changes
+ event GatewayUpdated(address indexed oldGateway, address indexed newGateway);
+ event UniswapRouterUpdated(address indexed oldRouter, address indexed newRouter);
+ function setGateway(address newGateway) external onlyOwner {
+ if (newGateway == address(0)) revert InvalidAddress();
+ address oldGateway = address(gateway);
+ gateway = GatewayZEVM(newGateway);
+ emit GatewayUpdated(oldGateway, newGateway);
+ }
+ function setUniswapRouter(address newRouter) external onlyOwner {
+ if (newRouter == address(0)) revert InvalidAddress();
+ address oldRouter = uniswapRouter;
+ uniswapRouter = newRouter;
+ emit UniswapRouterUpdated(oldRouter, newRouter);
+ }Committable suggestion skipped: line range outside the PR's diff.
| function _authorizeUpgrade( | ||
| address newImplementation | ||
| ) internal override onlyOwner {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance upgrade authorization security
The _authorizeUpgrade function should implement additional security measures:
- Add a timelock mechanism
- Require multi-signature approval
- Emit an event for transparency
+ event UpgradeAuthorized(address indexed implementation);
+ uint256 private constant UPGRADE_TIMELOCK = 2 days;
+ mapping(address => uint256) private _pendingUpgrades;
function _authorizeUpgrade(
address newImplementation
- ) internal override onlyOwner {}
+ ) internal override onlyOwner {
+ require(_pendingUpgrades[newImplementation] > 0, "Upgrade not initiated");
+ require(block.timestamp >= _pendingUpgrades[newImplementation], "Timelock active");
+ emit UpgradeAuthorized(newImplementation);
+ }
+
+ function initiateUpgrade(address newImplementation) external onlyOwner {
+ _pendingUpgrades[newImplementation] = block.timestamp + UPGRADE_TIMELOCK;
+ }Committable suggestion skipped: line range outside the PR's diff.
EVMUniversalNFTandZetaChainUniversalNFTso that they have unique names (useful for ThirdWeb)Summary by CodeRabbit
New Features
EVMUniversalNFTandZetaChainUniversalNFT.EVMUniversalTokenandZetaChainUniversalToken.Bug Fixes
Documentation
Chores