refactor: extracted core logic into an abstract UniversalNFTCore contract
#36
Conversation
|
Warning Rate limit exceeded@fadeev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 46 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe pull request introduces substantial refactoring of the NFT and token contracts across EVM and ZetaChain platforms. Key changes include updating the Solidity pragma, reorganizing imports, and removing cross-chain transfer functionalities. New abstract contracts, Changes
Sequence DiagramsequenceDiagram
participant Owner
participant UniversalNFT
participant Gateway
Owner->>UniversalNFT: initialize(owner, name, symbol)
Owner->>UniversalNFT: safeMint(recipient, tokenId)
alt Pause/Unpause
Owner->>UniversalNFT: pause()
Owner->>UniversalNFT: unpause()
end
The sequence diagram illustrates the key interactions within the 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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function onCall( | ||
| MessageContext calldata context, | ||
| bytes calldata message | ||
| ) external payable onlyGateway returns (bytes4) { | ||
| if (context.sender != universal) revert Unauthorized(); | ||
|
|
||
| ( | ||
| address receiver, | ||
| uint256 tokenId, | ||
| string memory uri, | ||
| uint256 gasAmount, | ||
| address sender | ||
| ) = abi.decode(message, (address, uint256, string, uint256, address)); | ||
|
|
||
| _safeMint(receiver, tokenId); | ||
| _setTokenURI(tokenId, uri); | ||
| if (gasAmount > 0) { | ||
| if (sender == address(0)) revert InvalidAddress(); | ||
| (bool success, ) = payable(sender).call{value: gasAmount}(""); | ||
| if (!success) revert GasTokenTransferFailed(); | ||
| } | ||
| emit TokenTransferReceived(receiver, tokenId, uri); | ||
| return ""; | ||
| } |
Check warning
Code scanning / Slither
Low-level calls Warning
| 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 remaining = msg.value - out; | ||
|
|
||
| if (remaining > 0) { | ||
| IWETH9(WZETA).withdraw(remaining); | ||
| (bool success, ) = msg.sender.call{value: remaining}(""); | ||
| if (!success) revert TransferFailed(); | ||
| } | ||
|
|
||
| bytes memory message = abi.encode( | ||
| receiver, | ||
| tokenId, | ||
| uri, | ||
| 0, | ||
| msg.sender | ||
| ); | ||
| CallOptions memory callOptions = CallOptions(gasLimitAmount, false); | ||
|
|
||
| RevertOptions memory revertOptions = RevertOptions( | ||
| address(this), | ||
| true, | ||
| address(0), | ||
| abi.encode(tokenId, uri, msg.sender), | ||
| gasLimitAmount | ||
| ); | ||
|
|
||
| IZRC20(gasZRC20).approve(address(gateway), gasFee); | ||
| gateway.call( | ||
| abi.encodePacked(connected[destination]), | ||
| destination, | ||
| message, | ||
| callOptions, | ||
| revertOptions | ||
| ); | ||
|
|
||
| emit TokenTransfer(receiver, destination, tokenId, uri); | ||
| } |
Check warning
Code scanning / Slither
Low-level calls Warning
fadeev
changed the title
UniversalNFTCore contract
| function onCall( | ||
| MessageContext calldata context, | ||
| bytes calldata message | ||
| ) external payable onlyGateway returns (bytes4) { | ||
| if (context.sender != universal) revert Unauthorized(); | ||
| ( | ||
| address receiver, | ||
| uint256 amount, | ||
| uint256 gasAmount, | ||
| address sender | ||
| ) = abi.decode(message, (address, uint256, uint256, address)); | ||
| _mint(receiver, amount); | ||
| if (gasAmount > 0) { | ||
| if (sender == address(0)) revert InvalidAddress(); | ||
| (bool success, ) = payable(sender).call{value: amount}(""); | ||
| if (!success) revert GasTokenTransferFailed(); | ||
| } | ||
| emit TokenTransferReceived(receiver, amount); | ||
| return ""; | ||
| } |
Check notice
Code scanning / Slither
Reentrancy vulnerabilities Low
| function onCall( | ||
| MessageContext calldata context, | ||
| bytes calldata message | ||
| ) external payable onlyGateway returns (bytes4) { | ||
| if (context.sender != universal) revert Unauthorized(); | ||
| ( | ||
| address receiver, | ||
| uint256 amount, | ||
| uint256 gasAmount, | ||
| address sender | ||
| ) = abi.decode(message, (address, uint256, uint256, address)); | ||
| _mint(receiver, amount); | ||
| if (gasAmount > 0) { | ||
| if (sender == address(0)) revert InvalidAddress(); | ||
| (bool success, ) = payable(sender).call{value: amount}(""); | ||
| if (!success) revert GasTokenTransferFailed(); | ||
| } | ||
| emit TokenTransferReceived(receiver, amount); | ||
| return ""; | ||
| } |
Check warning
Code scanning / Slither
Low-level calls Warning
| function transferCrossChain( | ||
| address destination, | ||
| address receiver, | ||
| uint256 amount | ||
| ) public payable { | ||
| if (msg.value == 0) revert ZeroMsgValue(); | ||
| if (receiver == address(0)) revert InvalidAddress(); | ||
| _burn(msg.sender, amount); | ||
|
|
||
| (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(); | ||
| } | ||
|
|
||
| bytes memory message = abi.encode(receiver, amount, 0, msg.sender); | ||
|
|
||
| CallOptions memory callOptions = CallOptions(gasLimitAmount, false); | ||
|
|
||
| RevertOptions memory revertOptions = RevertOptions( | ||
| address(this), | ||
| true, | ||
| address(0), | ||
| abi.encode(amount, msg.sender), | ||
| gasLimitAmount | ||
| ); | ||
|
|
||
| IZRC20(gasZRC20).approve(address(gateway), gasFee); | ||
| gateway.call( | ||
| abi.encodePacked(connected[destination]), | ||
| destination, | ||
| message, | ||
| callOptions, | ||
| revertOptions | ||
| ); | ||
| emit TokenTransfer(destination, receiver, amount); | ||
| } |
Check warning
Code scanning / Slither
Low-level calls Warning
| function onCall( | ||
| MessageContext calldata context, | ||
| address zrc20, | ||
| uint256 amount, | ||
| bytes calldata message | ||
| ) external override onlyGateway { | ||
| if (context.sender != connected[zrc20]) revert Unauthorized(); | ||
| ( | ||
| address destination, | ||
| address receiver, | ||
| uint256 tokenAmount, | ||
| address sender | ||
| ) = abi.decode(message, (address, address, uint256, address)); | ||
| if (destination == address(0)) { | ||
| _mint(receiver, tokenAmount); | ||
| } else { | ||
| (address gasZRC20, uint256 gasFee) = IZRC20(destination) | ||
| .withdrawGasFeeWithGasLimit(gasLimitAmount); | ||
| if (destination != gasZRC20) revert InvalidAddress(); | ||
| uint256 out = SwapHelperLib.swapExactTokensForTokens( | ||
| uniswapRouter, | ||
| zrc20, | ||
| amount, | ||
| destination, | ||
| 0 | ||
| ); | ||
| if (!IZRC20(destination).approve(address(gateway), out)) { | ||
| revert ApproveFailed(); | ||
| } | ||
| gateway.withdrawAndCall( | ||
| abi.encodePacked(connected[destination]), | ||
| out - gasFee, | ||
| destination, | ||
| abi.encode(receiver, tokenAmount, out - gasFee, sender), | ||
| CallOptions(gasLimitAmount, false), | ||
| RevertOptions( | ||
| address(this), | ||
| true, | ||
| address(0), | ||
| abi.encode(tokenAmount, sender), | ||
| 0 | ||
| ) | ||
| ); | ||
| } | ||
| emit TokenTransferToDestination(destination, receiver, amount); | ||
| } |
Check notice
Code scanning / Slither
Reentrancy vulnerabilities Low
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
contracts/token/contracts/evm/UniversalToken.sol (1)
50-50: Consider access controls for minting.
Currently, only the owner can mint. This is fine for certain scenarios but consider if there's a need for role-based access controls (e.g.MINTER_ROLE) for flexible future usage.contracts/token/contracts/zetachain/UniversalToken.sol (1)
9-9: Remove duplicate import.
ERC20BurnableUpgradeableappears to be imported twice (line 9 and line 9 repeated). Ensure each import is intentional, or remove duplicates to declutter.- import {ERC20BurnableUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20BurnableUpgradeable.sol"; - import {ERC20BurnableUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20BurnableUpgradeable.sol";contracts/nft/contracts/evm/UniversalNFT.sol (2)
2-2: Include rationale for version references in a comment.
Mentioning compatibility with OpenZeppelin Contracts ^5.0.0 is helpful. Consider adding which features from v5.0.0 are being leveraged.
44-44: Initialize pausable last for clarity.
__ERC721Pausable_init()is placed with other inits. Consider grouping related initializers clearly for maintainability.contracts/token/contracts/evm/UniversalTokenCore.sol (2)
4-10: Imports are well-organized.
Separating protocol-contract imports, OpenZeppelin imports, and local references (UniversalTokenEvents) is good for readability.
91-110:onCallcallback
Logic properly checkscontext.sender == universal, then mints tokens and attempts to forward gas fees. Watch for re-entrancy on external call topayable(sender).call{value:amount}("").// Potentially use the OpenZeppelin ReentrancyGuard: +import "@openzeppelin/contracts/security/ReentrancyGuard.sol" ... function onCall(...) external payable onlyGateway nonReentrant returns (bytes4) { ... }contracts/nft/contracts/evm/UniversalNFTCore.sol (2)
36-40:setUniversalFunction
This function updates theuniversaladdress and emits an event. Consider adding more checks (e.g., code size of the address) if you expect only contract addresses.
63-103: Potential Re-Entrancy Risks When Transferring Cross-Chain
The code performs multiple external calls (e.g.,gateway.call/gateway.depositAndCall) and sends ETH. Although you are using short flows and burning the token prior to cross-chain messaging, carefully review the call order and guard your contract if you add more state changes later.contracts/token/contracts/zetachain/UniversalTokenCore.sol (3)
35-38:onlyGatewayModifier
Similar to the NFT core, this gating is consistent. Document or reference in the code comments that cross-chain messages must come from the official gateway to avoid confusion for new integrators.
53-56:setGasLimit
Matches the pattern from the NFT core. Confirm that gas usage in the cross-chain calls is consistent with this user-defined field. Also consider event emission for clarity.
66-121:transferCrossChain
You handle zero-value checks (ZeroMsgValue), burn tokens, handle gas fee, deposit WZETA, and swap to pay cross-chain costs. This approach is thorough. Carefully track leftover WZETA logic in future expansions to avoid dust amounts that remain locked.contracts/nft/contracts/zetachain/UniversalNFTCore.sol (1)
66-128:transferCrossChainand Re-Entrancy
Calls likeIWETH9(WZETA).approve(),IZRC20(gasZRC20).approve()and subsequentgateway.call()can be re-entrancy vectors if new storage writes are introduced in the future. An extranonReentrantguard or pattern might be advisable for safety. Also, ignore return values from the approvals with caution—if any approval fails, it should revert.contracts/nft/tasks/transfer.ts (1)
45-45: JSON Output
The structured JSON includes the relevant data. Consider also including gas costs or cross-chain fees in this output for debugging or cost analysis.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
contracts/nft/contracts/evm/UniversalNFT.sol(5 hunks)contracts/nft/contracts/evm/UniversalNFTCore.sol(1 hunks)contracts/nft/contracts/zetachain/UniversalNFT.sol(5 hunks)contracts/nft/contracts/zetachain/UniversalNFTCore.sol(1 hunks)contracts/nft/scripts/localnet.sh(1 hunks)contracts/nft/tasks/transfer.ts(5 hunks)contracts/token/contracts/evm/UniversalToken.sol(2 hunks)contracts/token/contracts/evm/UniversalTokenCore.sol(1 hunks)contracts/token/contracts/zetachain/UniversalToken.sol(4 hunks)contracts/token/contracts/zetachain/UniversalTokenCore.sol(1 hunks)
🔇 Additional comments (72)
contracts/token/contracts/evm/UniversalToken.sol (6)
11-11: Use explicit interface references for clarity.
Importing UniversalTokenCore.sol is necessary for the new inheritance model. Ensure code references to its functions or events are explicitly documented to maintain clarity around cross-chain functionalities.
20-20: Inherited abstract contract usage is appropriate.
The shift to inheriting UniversalTokenCore consolidates cross-chain logic in a dedicated contract and simplifies this file. The approach aligns with DRY principles.
42-43: Protecting pause functionality.
The pause() function is restricted to onlyOwner, which is generally acceptable. Ensure any operational team using multi-sig or governance structure also has access, if needed.
46-47: Unpausing logic is correct.
Similarly, unpausing is restricted to onlyOwner. This is consistent with how pausable patterns are typically implemented.
58-58: Override _update usage is consistent.
Overriding _update to combine ERC20Upgradeable and ERC20PausableUpgradeable logic is straightforward. No issues spotted.
36-39: Check upgrade implications of combined initializers.
Calling multiple initializers (__ERC20Pausable_init(), __UniversalTokenCore_init()) is valid, but ensure they don’t conflict or double-initialize state. Consider verifying each newly inherited member’s storage alignment.
✅ Verification successful
Storage layout appears safe for upgrades
Based on the analysis of the inheritance chain and storage variables:
-
The contract follows a proper initialization pattern with distinct initializers:
UniversalTokeninherits fromInitializable,ERC20Upgradeable,ERC20BurnableUpgradeable,ERC20PausableUpgradeable, andOwnableUpgradeable- Each base contract uses the OpenZeppelin upgradeable pattern
- The initialization calls are ordered correctly from most basic to most specific
-
Storage layout examination shows:
- Base contracts use isolated storage patterns from OpenZeppelin's upgradeable contracts
UniversalTokenCoredefines non-conflicting storage variables (gateway,universal,gasLimitAmount)- No storage collisions are detected between the inherited contracts
The initialization sequence is safe and follows upgradeable contract best practices. The storage layout maintains proper isolation between inherited contracts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Examine the storage layout for collisions in upgraded contracts.
ast-grep --pattern $'contract $_ {\n $$$\n}' | rg "storage|slot"
Length of output: 64
Script:
#!/bin/bash
# Let's examine the inheritance chain and initializers
ast-grep --pattern 'contract $_ is $_' contracts/token/contracts/evm/UniversalToken.sol
# Check the implementation of UniversalTokenCore initialization
ast-grep --pattern 'function __UniversalTokenCore_init($$$) {
$$$
}'
# Check ERC20Pausable initialization
ast-grep --pattern 'function __ERC20Pausable_init() {
$$$
}'
# Look for storage variable declarations in the inheritance chain
rg "^\s*(uint|int|bool|address|mapping|struct|bytes|string)" contracts/token/contracts/evm/UniversalToken.sol -A 1
# Check for storage gaps in the contracts
rg "uint256\[\d+\]\s+private\s+__gap" -A 1
Length of output: 775
Script:
#!/bin/bash
# Let's check the base contracts and their inheritance
rg "contract UniversalToken" -A 5 contracts/token/contracts/evm/UniversalToken.sol
# Check UniversalTokenCore contract
rg "contract UniversalTokenCore" -A 5 -r .
# Look for storage variables in related contracts
rg "^\s*(uint|int|bool|address|mapping|struct|bytes|string)" -g "**/*Token*.sol" -g "**/*Pausable*.sol" -g "**/*Ownable*.sol" -A 1
# Check for initialize functions in the inheritance chain
ast-grep --pattern 'function initialize($$$) {
$$$
}'
Length of output: 10292
contracts/token/contracts/zetachain/UniversalToken.sol (5)
17-17: Use consistent import formatting.
import "./UniversalTokenCore.sol"; clarifies the local path. If code is part of the same package, keep imports consistent with the rest of the codebase (relative or absolute).
26-26: Good approach to unify cross-chain logic via UniversalTokenCore.
Inheriting UniversalTokenCore ensures that cross-chain features are consistently maintained in a separate module.
45-45: Confirm compatibility of addresses in _init().
Ensure gatewayAddress and uniswapRouterAddress are valid. Although checks happen in __UniversalTokenCore_init(), confirm no additional constraints are needed (e.g., whitelists or pre-set routers).
56-58: mint is well-protected.
Restricting mint rights to the owner is acceptable. For production scenarios, consider role-based or multi-sig governance.
74-74: Optional fallback function usage.
Allowing receive() external payable {} can be beneficial, but confirm if the contract truly needs to accept native tokens.
contracts/nft/contracts/zetachain/UniversalNFT.sol (4)
13-13: Consistent reference to UniversalNFTCore.
Inheriting a core module mirrors the approach used in tokens. This reduces duplication and centralizes cross-chain logic.
24-24: Inheritance alignment is sound.
UniversalNFTCore integration ensures feature parity and code reuse. No apparent conflicts with other inherited classes.
44-44: Pause functionality properly initialized.
__ERC721Pausable_init() sets up the pausable capacity. No issues seen.
48-48: Initialize cross-chain logic in a single call.
__UniversalNFTCore_init(gatewayAddress, gas, uniswapRouterAddress) clarifies cross-chain setup. That said, ensure thorough testing is done to avoid uninitialized storage collisions.
contracts/nft/contracts/evm/UniversalNFT.sol (14)
3-3: Newer pragma version is acceptable.
^0.8.26 is stable unless there's a known bug. Confirm any known vulnerabilities are addressed if the contract is widely used.
6-6: Burnable NFT extension is properly included.
This extension is a standard approach to allow NFT burning. Ensure any additional burning restrictions are in place if user minted tokens shouldn’t be burned arbitrarily.
8-8: Pausable NFT extension is consistent.
Adding pausing offers an operational safeguard. Check if ephemeral token states are impacted by pause/unpause transitions.
10-10: Initializable usage is standard.
Initializable is necessary for upgradeable proxies. This is correct.
14-14: Core functionality factorization.
UniversalNFTCore centralizes cross-chain NFT logic. This fosters reusability across chain-specific contracts.
20-20: Override ordering for clarity.
ERC721URIStorageUpgradeable is placed before ERC721PausableUpgradeable. The order is relevant for function overrides. Confirm that no overshadow of _update() or _setTokenURI() occurs.
25-25: Abstract inheritance with UniversalNFTCore.
Centralizing cross-chain operations in a single base is a good design move.
48-48: Chain-specific parameter passing.
Passing address(this) to _init ensures the forward references are correct. Confirm extra trust assumptions around calling the same address.
54-54: safeMint gating with both onlyOwner and whenNotPaused
Combining the two modifiers is typically best practice for controlled minting. Good approach.
67-68: Pause function.
Only the owner can invoke pause(). This is consistent with typical pattern usage.
71-72: Unpause function.
Similarly, unpause() is restricted to the owner. This is standard and correct.
75-77: Ensure upgrade logic is tested.
_authorizeUpgrade is limited to onlyOwner. Confirm thorough testing or usage of test upgrade proxies before production.
109-113: tokenURI override order is valid.
Overriding across multiple inheritors, including UniversalNFTCore, is consistent. Confirm that any custom token URI logic in UniversalNFTCore is accounted for here.
127-128: supportsInterface override is thorough.
Aggregating interfaces from each inherited class helps ensure broad compatibility. No issues found.
contracts/token/contracts/evm/UniversalTokenCore.sol (10)
1-3: File header and pragma usage.
Using // SPDX-License-Identifier: MIT with ^0.8.26 is standard. This approach is recommended for clarity on licensing.
11-15: Abstract contract with multi-inheritance.
UniversalTokenCore extends multiple classes. This is acceptable, but confirm that each base class’s storage layouts align.
16-18: Gateway, universal, and gas limit are clearly named.
Straightforward naming and public visibility for essential cross-chain variables. No issues found.
20-23: Custom errors enhance revert clarity.
Defining typed errors clarifies revert reasons cost-effectively. Good practice.
25-28: onlyGateway guard ensures call origin.
This helps secure onCall and onRevert. Verify the gateway contract is properly authenticated off-chain.
30-33: Gas limit setter with zero check.
Reverts on zero gas limit, preventing misconfiguration. This is correct.
35-39: Universal address setter.
Ensuring non-zero address with an event is a robust approach for cross-chain usage. Good practice.
41-52: Core initializer validations.
Ensuring non-zero addresses and non-zero gas is crucial. Contract properly references gateway = GatewayEVM(gatewayAddress).
112-119: onRevert callback
Re-mints tokens to revert a failed cross-chain transfer. This ensures user funds aren’t lost. Consider event logs for debugging cross-chain reverts. The emit TokenTransferReverted(...) is sufficient.
120-120: Contract end
No issues.
contracts/nft/contracts/evm/UniversalNFTCore.sol (9)
1-10: Ensure Up-to-Date Imports and Licenses
The imports look correct and the license is set to MIT. Confirm that all imported contracts (e.g., GatewayEVM) are at the desired versions for production.
11-20: Inheritance Order and Access Modifiers
Your inheritance hierarchy clearly places ERC721Upgradeable and related contracts before OwnableUpgradeable. This is acceptable. Review whether any constructor-based logic remains in these inherited classes and ensure they do not conflict with your upgradeable initialization approach.
21-25: Centralized State Variables
Storing gateway, universal address, and gas limit in one contract centralizes cross-chain logic. Ensure no naming conflicts with derived classes or sibling contracts that might also define similar variables.
26-29: onlyGateway Modifier
This implementation correctly restricts callable functions from the gateway only. Make sure to document external usage if integrators need to identify gateway addresses programmatically.
31-34: setGasLimit Validation
The check if (gas == 0) revert InvalidGasLimit(); helps prevent invalid input. Verify that a zero gas limit is indeed never a valid scenario.
42-53: Initializer Naming: Not in MixedCase
This function name __UniversalNFTCore_init triggers a prior naming-convention warning.
105-128: onCall Implementation
Good approach to decode the message and safely mint tokens. Confirm that all reverts or errors are handled consistently across all cross-chain callbacks to avoid inconsistent state on partial failures.
130-140: Revert Logic
When reverting a cross-chain transfer, the contract remints the token to the original sender. Explicitly ensure that any partial state updates made before the revert scenario are also accounted for in derived contracts.
141-163: tokenURI and supportsInterface Overrides
Your overrides conform to OpenZeppelin’s approach, which is good. Keep documentation updated to inform integrators about differences between local URIs and cross-chain minted URIs if that arises.
contracts/token/contracts/zetachain/UniversalTokenCore.sol (7)
1-11: Imports & Modularity
You are combining multiple interfaces (e.g., IGatewayZEVM, IWZETA) and libraries (SwapHelperLib). Continue ensuring that contract size remains manageable; otherwise, consider splitting some functionality into separate libraries.
12-19: Contract Declaration & State Variables
Centralizing gateway, uniswapRouter, and gasLimitAmount is consistent with the cross-chain approach. Ensure these do not conflict with possible inherited variables if the derived contracts expand functionality further.
26-34: Comprehensive Error Handling
Your custom errors (e.g., TransferFailed, Unauthorized, etc.) provide clarity. Confirm all revert messages are consistent across the codebase so integrators can reliably detect error states.
40-51: __UniversalTokenCore_init
Checks for gatewayAddress != address(0) and gas != 0 are robust. If you anticipate future expansions, consider a pattern for re-initialization or versioning if your contract is upgradeable.
58-64: setConnected
The function sets up a mapping from ZRC20 to connected contract addresses. Consider validating code size for contractAddress if needed.
123-168: onCall Logic & Another Nested Swap
The contract does a nested swap for gas fees or token bridging, then calls gateway.withdrawAndCall. Confirm that the usage of approvals is validated or at least logged for transparency, since it's granting permission to external contracts.
170-178: onRevert
Similar to the NFT approach, you mint tokens back to the sender. This is a sensible fallback. Maintain consistent revert messages (abi.encode) to unify the debugging process across all cross-chain calls.
contracts/nft/contracts/zetachain/UniversalNFTCore.sol (7)
1-13: Imports and Base Structure
The contract extends UniversalContract, ERC721Upgradeable, ERC721URIStorageUpgradeable, and OwnableUpgradeable. This multi-inheritance approach is logical for cross-chain NFT use cases. Validate final bytecode size if usage grows.
14-20: State Variables & connected Mapping
You track gateway, uniswapRouter, and a connected mapping. This parallels the approach in UniversalTokenCore. Keep consistent naming across token and NFT cores to ease comprehension for integrators.
28-34: Error Descriptions
The custom errors are consistent with your approach in the EVM version. Ensure mismatch scenarios (e.g., user provides a 0 address or invalid ZRC20) always revert, since partial usage could lead to unpredictability.
40-51: Initializer: Non-MixedCase
Like the EVM version, __UniversalNFTCore_init triggers a naming convention warning.
130-182: onCall Handling
Correctly decodes cross-chain messages and mints or swaps tokens as needed. Keep a watch for potential gas griefing scenarios if the contract runs out of gas partway. Document recommended gas usage for clients.
184-193: onRevert
Recreate the NFT for the original sender. Matches earlier patterns. Make sure all reverts are consistently documented.
195-218: ERC721 Standard Overrides
tokenURI and supportsInterface overrides align with standard practices. Provide clarity in documentation if any cross-chain minted URI differs from local minted URI assumptions.
contracts/nft/tasks/transfer.ts (7)
10-10: Validation of Addresses
Nice to see you validate both args.destination and args.revertAddress. This proactive check is valuable for scripts.
14-17: Approving the Contract
You approve the same contract as your args.contract. Verify you actually need to approve the contract to transfer from itself or if another contract address is more appropriate (e.g., the universal core).
27-27: Contract Retrieval
The script retrieves ZetaChainUniversalNFT by args.contract. Ensure that the correct ABI (IERC721 or other advanced interface) is used for subsequent calls if the contract extends beyond basic ERC721.
37-37: transferCrossChain Usage
You pass args.destination to the contract with an optional receiver. Confirm that your contract can handle a scenario where receiver is not the same as signer.address.
53-53: Success Log
Clear message for the user. Ensure that this log remains correct if the user changes the addresses or the flow.
66-66: Renamed Parameter
--contract replaced the old --from. This is a better name but ensure existing script references are updated to avoid confusion.
96-96: destination Parameter
Renaming --to to --destination clarifies usage. Ensure the help text is equally clear that this expects a ZRC20 address for cross-chain bridging.
contracts/nft/scripts/localnet.sh (3)
59-59: Parameter Updates: ZetaChain → Ethereum
You’ve replaced --to with --destination and --from with --contract. This aligns with your updated Hardhat task. Confirm other references in your documentation are also updated.
65-65: Parameter Updates: Ethereum → BNB
Same parameter rename. Good consistency across chain calls.
71-71: No --destination Provided
Here you omit --destination. If the contract’s default handling is to treat a missing destination as a local chain transfer or a defined fallback, confirm that’s intended.
| function transferCrossChain( | ||
| address destination, | ||
| address receiver, | ||
| uint256 amount | ||
| ) external payable { | ||
| if (receiver == address(0)) revert InvalidAddress(); | ||
| _burn(msg.sender, amount); | ||
|
|
||
| bytes memory message = abi.encode( | ||
| destination, | ||
| receiver, | ||
| amount, | ||
| msg.sender | ||
| ); | ||
| if (destination == address(0)) { | ||
| gateway.call( | ||
| universal, | ||
| message, | ||
| RevertOptions(address(this), false, address(0), message, 0) | ||
| ); | ||
| } else { | ||
| gateway.depositAndCall{value: msg.value}( | ||
| universal, | ||
| message, | ||
| RevertOptions( | ||
| address(this), | ||
| true, | ||
| address(0), | ||
| abi.encode(amount, msg.sender), | ||
| gasLimitAmount | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| emit TokenTransfer(destination, receiver, amount); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cross-chain transfer logic.
Burning tokens locally, building the message, and dispatching to gateway is a typical cross-chain design.
Consider potential bridging re-entrancy or concurrency with repeated messages.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
contracts/token/contracts/evm/UniversalTokenCore.sol (1)
30-33: Consider emitting an event for gas limit updates
Emitting an event whenever the gas limit changes would enhance transparency and traceability, enabling off-chain services or UIs to react efficiently.function setGasLimit(uint256 gas) external onlyOwner { if (gas == 0) revert InvalidGasLimit(); gasLimitAmount = gas; + emit GasLimitUpdated(gas); }contracts/nft/contracts/evm/UniversalNFTCore.sol (1)
31-34: Consider emitting an event for gas limit updates
Similar to token contracts, emitting an event upon gas limit changes will help downstream services track this variable.function setGasLimit(uint256 gas) external onlyOwner { if (gas == 0) revert InvalidGasLimit(); gasLimitAmount = gas; + emit GasLimitUpdated(gas); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/nft/contracts/evm/UniversalNFTCore.sol(1 hunks)contracts/nft/contracts/zetachain/UniversalNFTCore.sol(1 hunks)contracts/token/contracts/evm/UniversalTokenCore.sol(1 hunks)contracts/token/contracts/zetachain/UniversalTokenCore.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/token/contracts/zetachain/UniversalTokenCore.sol
🔇 Additional comments (15)
contracts/nft/contracts/zetachain/UniversalNFTCore.sol (8)
1-3: License and Solidity version look good.
No issues found with the SPDX license identifier or the specified Solidity compiler version.
8-13: External dependencies are standard.
Imports appear consistent with project conventions and address cross-chain requirements.
14-20: Abstract contract structure is appropriate.
Declaring the contract as abstract is a good approach for shared functionality.
40-51: Function naming style deviates from mixedCase.
This is a known pattern for internal initializer functions in upgradeable contracts, but note that the naming here has been flagged previously.
130-182: Cross-chain message handling is well-structured.
The onCall function decodes the message payload and verifies the sender. The consistent usage of _safeMint and URI setting is correct and ensures the NFT is properly minted.
184-193: Revert handling logic is clear.
The onRevert function suitably restores the NFT on failure, minimizing disruption to the end user.
195-205: Overriding tokenURI is correct.
Method signature aligns with ERC721URIStorageUpgradeable. No issues found.
207-217: Interface support override is correct.
This override is in line with multiple inheritance from ERC721Upgradeable and ERC721URIStorageUpgradeable.
contracts/token/contracts/evm/UniversalTokenCore.sol (3)
9-15: Modular architecture is well-structured
The abstract contract's inheritance of multiple modules (ERC20, Ownable, and UniversalTokenEvents) provides a clear separation of concerns. No immediate issues are detected in the base contract definition.
54-90: Consider adding protection against potential re-entrancy
This function performs an external call to the gateway. If the gateway or its callback mechanism is compromised, re-entrant invocations could be possible. Adding a reentrancy guard or carefully structuring state changes after external calls can improve security.
114-121: Reversion logic is straightforward
The onRevert function correctly decodes the revert message, re-mints the tokens, and emits a suitable event to indicate the operation. No further issues detected.
contracts/nft/contracts/evm/UniversalNFTCore.sol (4)
55-103: Consider adding re-entrancy mitigations
Like in the token core contract, this function interacts with an external gateway. Even though state changes appear in a safe order, implementing a reentrancy guard or equivalent pattern would further reduce potential attack vectors.
105-128: Cross-chain NFT minting logic
The function decodes the message, validates the sender, and mints the NFT for the receiver. The conditional Ether transfer for gas also uses the correct gasAmount. This section demonstrates secure coding practices.
130-139: Clear fall-back flow for reverting NFT transfers
The onRevert function appropriately re-mints the NFT to the sender if the cross-chain transfer fails. This ensures no assets are irrecoverably lost.
141-163: Overrides of tokenURI and supportsInterface
The overrides correctly delegate to the parent implementations, preserving expected ERC721 behavior. No issues to report.
| 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); | ||
|
|
||
| emit TokenTransfer(receiver, destination, tokenId, uri); | ||
|
|
||
| (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(); | ||
| } | ||
|
|
||
| bytes memory message = abi.encode( | ||
| receiver, | ||
| tokenId, | ||
| uri, | ||
| 0, | ||
| msg.sender | ||
| ); | ||
| CallOptions memory callOptions = CallOptions(gasLimitAmount, false); | ||
|
|
||
| RevertOptions memory revertOptions = RevertOptions( | ||
| address(this), | ||
| true, | ||
| address(0), | ||
| abi.encode(tokenId, uri, msg.sender), | ||
| gasLimitAmount | ||
| ); | ||
|
|
||
| IZRC20(gasZRC20).approve(address(gateway), gasFee); | ||
|
|
||
| gateway.call( | ||
| abi.encodePacked(connected[destination]), | ||
| destination, | ||
| message, | ||
| callOptions, | ||
| revertOptions | ||
| ); | ||
| } |
There was a problem hiding this comment.
Potential reentrancy risk and multiple external calls.
The transferCrossChain function performs a series of external calls (token deposits, approvals, withdrawals, low-level calls, etc.). Consider using a reentrancy guard or reorganizing state changes before external calls to mitigate reentrancy.
| IWETH9(WZETA).approve(uniswapRouter, msg.value); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ignoring return value from ERC20 approve calls.
Since some ERC20 tokens return a boolean on approve, ensure you check the result to confirm successful approval. For example, wrap it in a require statement or a condition check, as already done later in the contract for a different token.
IWETH9(WZETA).approve(uniswapRouter, msg.value);
+// Verify the result to ensure approval is successful, e.g.:
+require(IWETH9(WZETA).approve(uniswapRouter, msg.value), "Approve failed");
IZRC20(gasZRC20).approve(address(gateway), gasFee);
+// Similarly, check the return value:
+require(IZRC20(gasZRC20).approve(address(gateway), gasFee), "Approve failed");Also applies to: 119-119
|
@zeta-chain/smart-contracts please, review. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
contracts/nft/contracts/evm/UniversalNFT.sol (1)
Line range hint
49-63: Consider optimizing tokenId generationWhile the current implementation is secure, it could be more gas efficient. Consider using a simple incremental counter if sequential tokenIds are acceptable for your use case.
- uint256 hash = uint256( - keccak256( - abi.encodePacked(address(this), block.number, _nextTokenId++) - ) - ); - uint256 tokenId = hash & 0x00FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF; + uint256 tokenId = _nextTokenId++;contracts/token/contracts/evm/UniversalTokenCore.sol (1)
114-121: LGTM with a minor suggestionThe revert handling logic is sound. Consider adding an event parameter to indicate the reason for reversion for better monitoring.
- event TokenTransferReverted(address indexed receiver, uint256 amount); + event TokenTransferReverted( + address indexed receiver, + uint256 amount, + string reason + ); function onRevert(RevertContext calldata context) external onlyGateway { (uint256 amount, address receiver) = abi.decode( context.revertMessage, (uint256, address) ); _mint(receiver, amount); - emit TokenTransferReverted(receiver, amount); + emit TokenTransferReverted(receiver, amount, "cross_chain_transfer_failed"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/nft/contracts/evm/UniversalNFT.sol(5 hunks)contracts/nft/contracts/zetachain/UniversalNFTCore.sol(1 hunks)contracts/token/contracts/evm/UniversalTokenCore.sol(1 hunks)contracts/token/contracts/zetachain/UniversalTokenCore.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/token/contracts/zetachain/UniversalTokenCore.sol
- contracts/nft/contracts/zetachain/UniversalNFTCore.sol
🔇 Additional comments (2)
contracts/nft/contracts/evm/UniversalNFT.sol (2)
15-24: LGTM: Well-structured inheritance chain
The inheritance order is correct and follows OpenZeppelin's best practices for upgradeable contracts.
2-2: Consider using a more stable Solidity version
While ^0.8.26 is recent, it might be prudent to use a more battle-tested version like 0.8.19 or 0.8.20 for production contracts, as newer versions may have undiscovered issues.
| function onCall( | ||
| MessageContext calldata context, | ||
| bytes calldata message | ||
| ) external payable onlyGateway returns (bytes4) { | ||
| if (context.sender != universal) revert Unauthorized(); | ||
| ( | ||
| address receiver, | ||
| uint256 amount, | ||
| uint256 gasAmount, | ||
| address sender | ||
| ) = abi.decode(message, (address, uint256, uint256, address)); | ||
| _mint(receiver, amount); | ||
| if (gasAmount > 0) { | ||
| if (sender == address(0)) revert InvalidAddress(); | ||
| (bool success, ) = payable(sender).call{value: gasAmount}(""); | ||
| if (!success) revert GasTokenTransferFailed(); | ||
| } | ||
| emit TokenTransferReceived(receiver, amount); | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance low-level call safety
The low-level call for gas token transfer should implement additional safety measures:
- Consider using OpenZeppelin's
Address.sendValue - Add a gas limit to prevent potential DoS attacks
+ import {Address} from "@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol";
if (gasAmount > 0) {
if (sender == address(0)) revert InvalidAddress();
- (bool success, ) = payable(sender).call{value: gasAmount}("");
- if (!success) revert GasTokenTransferFailed();
+ try Address.sendValue(payable(sender), gasAmount) {
+ } catch {
+ revert GasTokenTransferFailed();
+ }
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
contracts/nft/contracts/evm/UniversalNFT.sol (2)
Line range hint
34-49: Add input parameter validation in initializeThe initialize function accepts critical parameters without validation. Add checks for zero addresses and reasonable gas limits.
function initialize( address initialOwner, string memory name, string memory symbol, address payable gatewayAddress, uint256 gas ) public initializer { + require(initialOwner != address(0), "Invalid owner address"); + require(gatewayAddress != address(0), "Invalid gateway address"); + require(gas > 0 && gas <= 3000000, "Invalid gas limit"); + require(bytes(name).length > 0, "Empty name"); + require(bytes(symbol).length > 0, "Empty symbol"); __ERC721_init(name, symbol); // ... rest of the initialization }
Line range hint
51-65: Improve token ID generation to prevent collisionsThe current token ID generation method using block number could lead to collisions. Consider using a more robust approach.
function safeMint( address to, string memory uri ) public onlyOwner whenNotPaused { - uint256 hash = uint256( - keccak256( - abi.encodePacked(address(this), block.number, _nextTokenId++) - ) - ); - uint256 tokenId = hash & 0x00FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF; + uint256 tokenId = _nextTokenId++; + require(tokenId > 0, "Token ID overflow"); + require(!_exists(tokenId), "Token ID already exists"); _safeMint(to, tokenId); _setTokenURI(tokenId, uri); }
♻️ Duplicate comments (1)
contracts/nft/contracts/evm/UniversalNFT.sol (1)
2-3:⚠️ Potential issueFix Solidity version to avoid known issues
The caret (^) in the pragma allows using versions with known issues. Lock the version to a specific secure release.
-pragma solidity ^0.8.26; +pragma solidity 0.8.26;
🧹 Nitpick comments (3)
contracts/nft/contracts/evm/UniversalNFT.sol (1)
16-26: Document inheritance chain and potential function shadowingThe contract inherits from multiple base contracts. Consider adding NatSpec documentation to clarify the inheritance hierarchy and potential function shadowing.
Add documentation above the contract declaration:
/// @title UniversalNFT /// @notice ERC721 implementation with universal cross-chain capabilities /// @dev Inherits from multiple OpenZeppelin contracts and UniversalNFTCore /// @dev Inheritance order: ERC721 base -> Extensions -> Access Control -> Universal Corecontracts/nft/contracts/zetachain/UniversalNFT.sol (2)
Line range hint
51-65: Consider extracting common minting logic to UniversalNFTCoreThe safeMint implementation is identical in both EVM and ZetaChain versions. Consider moving it to the core contract.
This would reduce code duplication and ensure consistent minting behavior across chains.
17-25: Optimize inheritance chain for gas efficiencyThe current inheritance structure, while clean, increases deployment and runtime gas costs. Consider:
- Combining compatible interfaces
- Reducing inheritance depth where possible
This would require careful refactoring and thorough testing to ensure all functionality is preserved.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/nft/contracts/evm/UniversalNFT.sol(5 hunks)contracts/nft/contracts/zetachain/UniversalNFT.sol(5 hunks)
🔇 Additional comments (1)
contracts/nft/contracts/evm/UniversalNFT.sol (1)
Line range hint 76-134: Override implementations look correct
The contract properly implements all required overrides from the inherited contracts, including the new UniversalNFTCore contract.
| @@ -299,5 +134,5 @@ contract UniversalNFT is | |||
| _unpause(); | |||
| } | |||
|
|
|||
| receive() external payable {} | |||
| receive() external payable {} // Receive ZETA to pay for gas | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation to receive function
The receive function accepts ZETA tokens without any validation. Consider adding checks for minimum/maximum amounts and proper accounting.
- receive() external payable {}
+ receive() external payable {
+ require(msg.value > 0, "Zero value not allowed");
+ emit ZETAReceived(msg.sender, msg.value);
+ }
+
+ event ZETAReceived(address indexed sender, uint256 amount);📝 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.
| receive() external payable {} // Receive ZETA to pay for gas | |
| receive() external payable { | |
| require(msg.value > 0, "Zero value not allowed"); | |
| emit ZETAReceived(msg.sender, msg.value); | |
| } | |
| event ZETAReceived(address indexed sender, uint256 amount); |
| address payable gatewayAddress, // Include EVM gateway address | ||
| uint256 gas, // Set gas limit for universal NFT calls | ||
| address uniswapRouterAddress // Uniswap v2 router address for gas token swaps | ||
| ) public initializer { | ||
| __ERC721_init(name, symbol); | ||
| __ERC721Enumerable_init(); | ||
| __ERC721URIStorage_init(); | ||
| __ERC721Pausable_init(); | ||
| __Ownable_init(initialOwner); | ||
| __ERC721Burnable_init(); | ||
| __UUPSUpgradeable_init(); | ||
| if (gatewayAddress == address(0) || uniswapRouterAddress == address(0)) | ||
| revert InvalidAddress(); | ||
| if (gas == 0) revert InvalidGasLimit(); | ||
| gateway = GatewayZEVM(gatewayAddress); | ||
| uniswapRouter = uniswapRouterAddress; | ||
| gasLimitAmount = gas; | ||
| } | ||
|
|
||
| function setGasLimit(uint256 gas) external onlyOwner { | ||
| if (gas == 0) revert InvalidGasLimit(); | ||
| gasLimitAmount = gas; | ||
| } | ||
|
|
||
| function setConnected( | ||
| address zrc20, | ||
| address contractAddress | ||
| ) external onlyOwner { | ||
| connected[zrc20] = contractAddress; | ||
| emit SetConnected(zrc20, contractAddress); | ||
| } | ||
|
|
||
| function transferCrossChain( | ||
| uint256 tokenId, | ||
| address receiver, | ||
| address destination | ||
| ) public payable whenNotPaused { | ||
| 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 remaining = msg.value - out; | ||
|
|
||
| if (remaining > 0) { | ||
| IWETH9(WZETA).withdraw(remaining); | ||
| (bool success, ) = msg.sender.call{value: remaining}(""); | ||
| if (!success) revert TransferFailed(); | ||
| } | ||
|
|
||
| bytes memory message = abi.encode( | ||
| receiver, | ||
| tokenId, | ||
| uri, | ||
| 0, | ||
| msg.sender | ||
| ); | ||
| CallOptions memory callOptions = CallOptions(gasLimitAmount, false); | ||
|
|
||
| RevertOptions memory revertOptions = RevertOptions( | ||
| address(this), | ||
| true, | ||
| address(0), | ||
| abi.encode(tokenId, uri, msg.sender), | ||
| gasLimitAmount | ||
| ); | ||
|
|
||
| IZRC20(gasZRC20).approve(address(gateway), gasFee); | ||
| gateway.call( | ||
| abi.encodePacked(connected[destination]), | ||
| destination, | ||
| message, | ||
| callOptions, | ||
| revertOptions | ||
| ); | ||
|
|
||
| emit TokenTransfer(receiver, destination, tokenId, uri); | ||
| __UniversalNFTCore_init(gatewayAddress, gas, uniswapRouterAddress); // Initialize universal NFT core |
There was a problem hiding this comment.
Validate Uniswap router address during initialization
The ZetaChain implementation requires a Uniswap router for gas token swaps. Add validation for the router address.
function initialize(
address initialOwner,
string memory name,
string memory symbol,
address payable gatewayAddress,
uint256 gas,
address uniswapRouterAddress
) public initializer {
+ require(uniswapRouterAddress != address(0), "Invalid Uniswap router");
// ... rest of the initialization
__UniversalNFTCore_init(gatewayAddress, gas, uniswapRouterAddress);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| address payable gatewayAddress, // Include EVM gateway address | |
| uint256 gas, // Set gas limit for universal NFT calls | |
| address uniswapRouterAddress // Uniswap v2 router address for gas token swaps | |
| ) public initializer { | |
| __ERC721_init(name, symbol); | |
| __ERC721Enumerable_init(); | |
| __ERC721URIStorage_init(); | |
| __ERC721Pausable_init(); | |
| __Ownable_init(initialOwner); | |
| __ERC721Burnable_init(); | |
| __UUPSUpgradeable_init(); | |
| if (gatewayAddress == address(0) || uniswapRouterAddress == address(0)) | |
| revert InvalidAddress(); | |
| if (gas == 0) revert InvalidGasLimit(); | |
| gateway = GatewayZEVM(gatewayAddress); | |
| uniswapRouter = uniswapRouterAddress; | |
| gasLimitAmount = gas; | |
| } | |
| function setGasLimit(uint256 gas) external onlyOwner { | |
| if (gas == 0) revert InvalidGasLimit(); | |
| gasLimitAmount = gas; | |
| } | |
| function setConnected( | |
| address zrc20, | |
| address contractAddress | |
| ) external onlyOwner { | |
| connected[zrc20] = contractAddress; | |
| emit SetConnected(zrc20, contractAddress); | |
| } | |
| function transferCrossChain( | |
| uint256 tokenId, | |
| address receiver, | |
| address destination | |
| ) public payable whenNotPaused { | |
| 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 remaining = msg.value - out; | |
| if (remaining > 0) { | |
| IWETH9(WZETA).withdraw(remaining); | |
| (bool success, ) = msg.sender.call{value: remaining}(""); | |
| if (!success) revert TransferFailed(); | |
| } | |
| bytes memory message = abi.encode( | |
| receiver, | |
| tokenId, | |
| uri, | |
| 0, | |
| msg.sender | |
| ); | |
| CallOptions memory callOptions = CallOptions(gasLimitAmount, false); | |
| RevertOptions memory revertOptions = RevertOptions( | |
| address(this), | |
| true, | |
| address(0), | |
| abi.encode(tokenId, uri, msg.sender), | |
| gasLimitAmount | |
| ); | |
| IZRC20(gasZRC20).approve(address(gateway), gasFee); | |
| gateway.call( | |
| abi.encodePacked(connected[destination]), | |
| destination, | |
| message, | |
| callOptions, | |
| revertOptions | |
| ); | |
| emit TokenTransfer(receiver, destination, tokenId, uri); | |
| __UniversalNFTCore_init(gatewayAddress, gas, uniswapRouterAddress); // Initialize universal NFT core | |
| address payable gatewayAddress, // Include EVM gateway address | |
| uint256 gas, // Set gas limit for universal NFT calls | |
| address uniswapRouterAddress // Uniswap v2 router address for gas token swaps | |
| ) public initializer { | |
| require(uniswapRouterAddress != address(0), "Invalid Uniswap router"); | |
| __ERC721_init(name, symbol); | |
| __ERC721Enumerable_init(); | |
| __ERC721URIStorage_init(); | |
| __ERC721Pausable_init(); | |
| __Ownable_init(initialOwner); | |
| __ERC721Burnable_init(); | |
| __UUPSUpgradeable_init(); | |
| __UniversalNFTCore_init(gatewayAddress, gas, uniswapRouterAddress); // Initialize universal NFT core |
|
@zeta-chain/smart-contracts please, review. |
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
contracts/nft/contracts/evm/UniversalNFT.sol (1)
Line range hint
55-63: Improve token ID generation to prevent potential collisions.The current implementation using block number could lead to token ID collisions during chain reorganizations. Consider using a more robust approach.
- uint256 hash = uint256( - keccak256( - abi.encodePacked(address(this), block.number, _nextTokenId++) - ) - ); - - uint256 tokenId = hash & 0x00FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF; + uint256 tokenId = _nextTokenId++; + require(tokenId > 0, "Token ID overflow");
🧹 Nitpick comments (4)
contracts/token/contracts/evm/UniversalToken.sol (1)
32-40: Add validation for gas parameter.While the initialization sequence is correct, consider adding validation for the gas parameter to prevent setting extremely high or low values that could impact cross-chain operations.
function initialize( address initialOwner, string memory name, string memory symbol, address payable gatewayAddress, uint256 gas ) public initializer { + require(gas >= 21000 && gas <= 2000000, "Invalid gas limit"); __ERC20_init(name, symbol); __ERC20Burnable_init(); __ERC20Pausable_init(); __Ownable_init(initialOwner); __UUPSUpgradeable_init(); __UniversalTokenCore_init(gatewayAddress, address(this), gas); }contracts/nft/contracts/evm/UniversalNFT.sol (1)
16-25: Consider gas optimization through feature selection.The contract implements multiple OpenZeppelin extensions which could lead to higher gas costs. Consider if all features (Enumerable, URIStorage, Burnable, Pausable) are necessary for your use case.
contracts/token/contracts/zetachain/UniversalToken.sol (2)
57-59: Consider adding input validation and supply controls to mint function.While the basic implementation is functional, consider adding:
- Input validation for zero address and amount
- Maximum supply checks to control token economics
function mint(address to, uint256 amount) public onlyOwner { + require(to != address(0), "UniversalToken: mint to the zero address"); + require(amount > 0, "UniversalToken: mint amount must be positive"); + require(totalSupply() + amount <= maxSupply, "UniversalToken: max supply exceeded"); _mint(to, amount); }
Line range hint
20-75: Consider adding additional security measures.While the contract implements standard security features through OpenZeppelin's contracts, consider:
- Adding ReentrancyGuard for protection against reentrancy attacks
- Including a SECURITY.md file with disclosure policy
- Documenting upgrade procedures and timelock requirements
contract UniversalToken is Initializable, ERC20Upgradeable, ERC20BurnableUpgradeable, ERC20PausableUpgradeable, OwnableUpgradeable, UUPSUpgradeable, + ReentrancyGuardUpgradeable, UniversalTokenCore
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/nft/contracts/evm/UniversalNFT.sol(5 hunks)contracts/token/contracts/evm/UniversalToken.sol(2 hunks)contracts/token/contracts/zetachain/UniversalToken.sol(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test (contracts/token)
- GitHub Check: test (contracts/nft)
- GitHub Check: slither (contracts/token, token.sarif)
- GitHub Check: slither (contracts/nft, nft.sarif)
🔇 Additional comments (5)
contracts/token/contracts/evm/UniversalToken.sol (2)
11-12: Well-structured imports with clear organization.The imports are properly organized, using specific imports from OpenZeppelin's upgradeable contracts. The addition of UniversalTokenCore aligns with the PR's objective of extracting core logic.
21-21: Proper inheritance structure maintained.The addition of UniversalTokenCore maintains the correct inheritance order while preserving upgradeability.
contracts/nft/contracts/evm/UniversalNFT.sol (2)
38-48: Add validation for gas parameter.Similar to UniversalToken, consider adding validation for the gas parameter to prevent setting extremely high or low values that could impact cross-chain operations.
110-114: Correct implementation of override declarations.The override declarations properly handle the diamond inheritance pattern, including the new UniversalNFTCore contract.
Also applies to: 128-129
contracts/token/contracts/zetachain/UniversalToken.sol (1)
20-27: Well-structured inheritance and initialization.The contract follows OpenZeppelin's best practices for upgradeable contracts:
- Proper inheritance order
- Clear initialization pattern
- Good use of named parameters
Also applies to: 33-46
| } | ||
|
|
||
| function mint(address to, uint256 amount) public onlyOwner whenNotPaused { | ||
| function mint(address to, uint256 amount) public onlyOwner { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add whenNotPaused modifier to mint function.
The mint function should be disabled when the contract is paused to maintain consistent pause behavior across all state-changing operations.
- function mint(address to, uint256 amount) public onlyOwner {
+ function mint(address to, uint256 amount) public onlyOwner whenNotPaused {
_mint(to, amount);
}📝 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.
| function mint(address to, uint256 amount) public onlyOwner { | |
| function mint(address to, uint256 amount) public onlyOwner whenNotPaused { |
| @@ -1,45 +1,30 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity 0.8.26; | |||
| pragma solidity ^0.8.26; | |||
There was a problem hiding this comment.
Fix Solidity version to avoid potential issues.
Using a floating pragma (^) could expose the contract to bugs in newer compiler versions. Lock the version to a specific release.
-pragma solidity ^0.8.26;
+pragma solidity 0.8.26;📝 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.
| pragma solidity ^0.8.26; | |
| pragma solidity 0.8.26; |
| @@ -6,50 +6,26 @@ import "@zetachain/protocol-contracts/contracts/zevm/interfaces/UniversalContrac | |||
| import "@zetachain/protocol-contracts/contracts/zevm/interfaces/IGatewayZEVM.sol"; | |||
| import "@zetachain/protocol-contracts/contracts/zevm/GatewayZEVM.sol"; | |||
| import {SwapHelperLib} from "@zetachain/toolkit/contracts/SwapHelperLib.sol"; | |||
| import {ERC20BurnableUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20BurnableUpgradeable.sol"; | |||
There was a problem hiding this comment.
Remove duplicate import statement.
The ERC20BurnableUpgradeable contract is imported twice. Remove one of the duplicate imports to maintain clean and efficient code.
import {ERC20BurnableUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20BurnableUpgradeable.sol";
-import {ERC20BurnableUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20BurnableUpgradeable.sol";Also applies to: 11-11
| @@ -228,4 +71,6 @@ | |||
| ) internal override(ERC20Upgradeable, ERC20PausableUpgradeable) { | |||
| super._update(from, to, value); | |||
| } | |||
|
|
|||
| receive() external payable {} | |||
There was a problem hiding this comment.
💡 Codebase verification
Remove unnecessary receive() function
The codebase analysis shows no legitimate use cases requiring the UniversalToken contract to accept ETH. The empty receive() function should be removed to prevent accidental ETH transfers that could lead to permanently trapped funds.
🔗 Analysis chain
Clarify the purpose of the receive function.
An empty receive() function allows the contract to accept ETH without any handling logic, which could lead to trapped funds. Either:
- Document why the contract needs to accept ETH, or
- Remove the function if ETH acceptance is not required
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any functions in the codebase that require this contract to receive ETH
rg -l "transfer\(.*value.*\)" --type solidity
rg -l "call\{value:" --type solidity
Length of output: 152
Script:
#!/bin/bash
# Search for ETH transfers and value calls without file type restriction
rg -l "transfer\("
rg -l "\.send\("
rg -l "\.call\{value:"
# Also check for payable functions that might forward ETH
ast-grep --pattern 'function $FUNC($$$) payable $$$'
Length of output: 335
Script:
#!/bin/bash
# Check the context of value transfers in core contracts
rg "\.call\{value:" -A 2 -B 2 contracts/token/contracts/zetachain/UniversalTokenCore.sol contracts/token/contracts/evm/UniversalTokenCore.sol
rg "transfer\(" -A 2 -B 2 contracts/token/contracts/zetachain/UniversalTokenCore.sol contracts/token/contracts/evm/UniversalTokenCore.sol
# Check if UniversalToken is referenced in these transfers
rg "UniversalToken" contracts/token/contracts/zetachain/UniversalTokenCore.sol contracts/token/contracts/evm/UniversalTokenCore.sol
Length of output: 2169
| @@ -0,0 +1,164 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.26; | |||
There was a problem hiding this comment.
We might want to add comment documentation for all functions?
Would also make sense explaining why we create a Core contract aside from UniversalNFT contract
There was a problem hiding this comment.
Yes, the code is missing docs. Let me fix that 👍
| import {ERC721URIStorageUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721URIStorageUpgradeable.sol"; | ||
| import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; | ||
|
|
||
| abstract contract UniversalNFTCore is |
There was a problem hiding this comment.
Shouldn't owner be able to update the gateway address as well for more flexibility (even though gateway address should not change)
Owner can already upgrade the contract in any case so I don't see more attack surface from this
| gasLimitAmount = gas; | ||
| } | ||
|
|
||
| function setUniversal(address contractAddress) external onlyOwner { |
| function __UniversalNFTCore_init( | ||
| address gatewayAddress, | ||
| address universalAddress, | ||
| uint256 gas |
There was a problem hiding this comment.
| uint256 gas | |
| uint256 gasLimit |
I think it's better making variable as clear as possible
| * @param receiver The address on the destination chain that will receive the NFT. | ||
| * @param destination The contract address on the destination chain (or address(0) if same chain). |
There was a problem hiding this comment.
I think we could add a bit more explanation on the difference
| @@ -1,13 +1,6 @@ | |||
| // SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
I would personally name the zetachain directory zevm to be consistent with protocol-contracts
There was a problem hiding this comment.
I'm all for consistency, but in my opinion zEVM is confusing. I'm using zetachain instead of zevm intentionally to make it easier for developers to understand, which chain a contract is meant to be deployed on. zevm is somewhat ok inside the protocol contracts as an internal shorthand, but for a wider audience, I don't think it's suitable.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
contracts/nft/contracts/evm/UniversalNFTCore.sol (6)
11-17: Consider adding more inline documentation describing "why" vs. "what."While this docstring provides a good high-level overview, expanding on the motivations behind cross-chain NFT support could help future readers better understand the underlying rationale and potential pitfalls.
18-32: Align initializer naming with Solidity conventions, or clarify usage.The function
__UniversalNFTCore_initdoes not follow mixedCase naming. However, this pattern is recognized in OpenZeppelin upgradeable contracts. If adhering to standard naming is desired or mandated, consider renaming it. Otherwise, provide a comment stating that this naming follows OpenZeppelin’s upgradeable initialization convention.
64-92: Validate ownership or approval before burning tokens.
_burn(tokenId)will revert internally ifmsg.senderis not the owner or approved. However, providing an explicit check here might offer clearer error messaging for users. Currently, the function relies on_burnalone to handle incorrect ownership scenarios.
94-143: Consolidate reentrancy protection if multiple external calls are possible.The
transferCrossChainfunction makes external calls togatewayafter burning the NFT. Though it follows checks-effects-interactions, usingReentrancyGuardUpgradeablecould provide an additional layer of security against complex exploit scenarios.
145-175: Return a descriptive selector for cross-chain callback handlers.The function
onCallreturns an empty byte4 value. If a known interface or callback selector is expected, consider returning a dedicated signature (e.g.,bytes4(keccak256("onCall(...)))) to align with potential or future cross-chain standards.
193-225: Refine overrides for clarity and maintainability.Both
tokenURIandsupportsInterfaceoverride multiple parents. A short comment explaining that the function merges logic fromERC721UpgradeableandERC721URIStorageUpgradeablehelps future maintainers understand the rationale and any potential conflicts.contracts/token/contracts/evm/UniversalTokenCore.sol (2)
11-17: Enrich documentation explaining key differences from standard ERC20s.The contract is introducing cross-chain logic (e.g.,
gateway,universal,gasLimitAmount). Further elaboration on how this differs from a typical ERC20 can guide integrators.
32-35: Consider a specialized error message.
InvalidGasLimit()might be too generic. Using a more specific error message (e.g., "GasLimitCannotBeZero") can make debugging issues quicker when your codebase grows and complex edge-cases arise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/nft/contracts/evm/UniversalNFTCore.sol(1 hunks)contracts/nft/contracts/zetachain/UniversalNFTCore.sol(1 hunks)contracts/token/contracts/evm/UniversalTokenCore.sol(1 hunks)contracts/token/contracts/zetachain/UniversalTokenCore.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/token/contracts/zetachain/UniversalTokenCore.sol
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test (contracts/token)
- GitHub Check: test (contracts/nft)
- GitHub Check: slither (contracts/token, token.sarif)
- GitHub Check: slither (contracts/nft, nft.sarif)
🔇 Additional comments (11)
contracts/nft/contracts/zetachain/UniversalNFTCore.sol (6)
1-2: Consider verifying Solidity version compatibility.
Solidity^0.8.26is not yet an official release at the time of writing. Ensure that the compiler or deployed environment supports this exact version to prevent compilation or runtime issues.
14-21: Clean and comprehensive documentation.
The high-level contract description is clearly stated, aiding maintainability for future developers.
22-28: Interfaces and base contracts are well-structured.
The inheritance chain (UniversalContract,ERC721Upgradeable,ERC721URIStorageUpgradeable,OwnableUpgradeable,UniversalNFTEvents) is neatly composed and follows OpenZeppelin best practices for readability and maintainability.
63-74: Function name does not match recommended mixedCase style.
Although OpenZeppelin commonly uses double underscores for initialization, automated scanners flagged this. Keep it if following OpenZeppelin’s convention, otherwise rename to something like_universalNFTCoreInitto align with typical mixedCase.
248-257: Handling of NFT reverts is clear and robust.
TheonRevertfunction properly restores the NFT by minting it back to the sender, preventing potential asset loss.
264-274: Inheritance overrides are well-handled for metadata and interface checks.
OverridingtokenURIandsupportsInterfacewith explicit references to sibling parents is aligned with OpenZeppelin guidelines, ensuring consistent behavior and preventing overshadowing.Also applies to: 276-291
contracts/nft/contracts/evm/UniversalNFTCore.sol (2)
38-41: Ensure coverage for theonlyGatewaymodifier in test suites.Access-control logic helps ensure that only the Gateway can invoke certain functions. Verifying correct usage (and no unexpected side effects) in test suites is crucial to maintaining security guarantees.
177-191: Add tests foronRevertto ensure consistent fallback behavior.Since
onRevertis part of the cross-chain flow, verifying that NFT state is correctly restored upon a revert is vital. Ensure thorough test coverage of different revert scenarios, including partial mints, invalid addresses, and multiple consecutive failures.contracts/token/contracts/evm/UniversalTokenCore.sol (3)
66-84: Validate initialization in testing.Because this function is typically invoked only once, verifying that re-initialization or partial initialization does not break the contract is crucial. Use dedicated tests to ensure correct
gateway,universal, andgasLimitAmountsetup.
167-174: Ensure correct handling of revert messages.The
onRevertfunction decodes(uint256, address)fromcontext.revertMessage. Include negative or zero amounts, as well as invalid addresses, in testing to confirm that the revert logic fully aligns with cross-chain requirements.
141-160:⚠️ Potential issueAdopt safer value transfer patterns.
The usage of low-level
call{value: gasAmount}("")is prone to reentrancy. Consider usingReentrancyGuardUpgradeableorsendValuefromAddressUpgradeableto handle value transfers safely. This approach also simplifies error handling.+import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol"; +import {AddressUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol"; ... function onCall( MessageContext calldata context, bytes calldata message ) external payable onlyGateway returns (bytes4) + nonReentrant { ... if (gasAmount > 0) { if (sender == address(0)) revert InvalidAddress(); - (bool success, ) = payable(sender).call{value: gasAmount}(""); - if (!success) revert GasTokenTransferFailed(); + try AddressUpgradeable.sendValue(payable(sender), gasAmount) { + } catch { + revert GasTokenTransferFailed(); + } } ... }Likely invalid or redundant comment.
| function transferCrossChain( | ||
| address destination, | ||
| address receiver, | ||
| uint256 amount | ||
| ) external payable { | ||
| if (receiver == address(0)) revert InvalidAddress(); | ||
|
|
||
| _burn(msg.sender, amount); | ||
|
|
||
| bytes memory message = abi.encode( | ||
| destination, | ||
| receiver, | ||
| amount, | ||
| msg.sender | ||
| ); | ||
|
|
||
| emit TokenTransfer(destination, receiver, amount); | ||
|
|
||
| if (destination == address(0)) { | ||
| gateway.call( | ||
| universal, | ||
| message, | ||
| RevertOptions(address(this), false, address(0), message, 0) | ||
| ); | ||
| } else { | ||
| gateway.depositAndCall{value: msg.value}( | ||
| universal, | ||
| message, | ||
| RevertOptions( | ||
| address(this), | ||
| true, | ||
| address(0), | ||
| abi.encode(amount, msg.sender), | ||
| gasLimitAmount | ||
| ) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use reentrancy guards for cross-chain token transfers.
Similar to the NFT transfer logic, _burn followed by external calls to gateway can benefit from reentrancy protection, preventing potential complex reentrancy vectors.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
contracts/token/contracts/evm/UniversalTokenCore.sol (1)
151-170:⚠️ Potential issueUse OpenZeppelin's Address.sendValue for safer transfers.
Replace the low-level call with OpenZeppelin's
Address.sendValuefor better security.+import {Address} from "@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol"; if (gasAmount > 0) { if (sender == address(0)) revert InvalidAddress(); - (bool success, ) = payable(sender).call{value: gasAmount}(""); - if (!success) revert GasTokenTransferFailed(); + try Address.sendValue(payable(sender), gasAmount) { + } catch { + revert GasTokenTransferFailed(sender, gasAmount); + } }🧰 Tools
🪛 GitHub Check: Slither
[notice] 151-170: Reentrancy vulnerabilities
Reentrancy in UniversalTokenCore.onCall(MessageContext,bytes) (contracts/evm/UniversalTokenCore.sol#151-170):
External calls:
- (success,None) = address(sender).call{value: gasAmount}() (contracts/evm/UniversalTokenCore.sol#165)
Event emitted after the call(s):
- TokenTransferReceived(receiver,amount) (contracts/evm/UniversalTokenCore.sol#168)
[warning] 151-170: Low-level calls
Low level call in UniversalTokenCore.onCall(MessageContext,bytes) (contracts/evm/UniversalTokenCore.sol#151-170):
- (success,None) = address(sender).call{value: gasAmount}() (contracts/evm/UniversalTokenCore.sol#165)
🧹 Nitpick comments (3)
contracts/token/contracts/evm/UniversalTokenCore.sol (1)
23-43: Enhance error messages for better debugging.Consider adding more descriptive error messages to improve debugging experience.
- error InvalidAddress(); - error Unauthorized(); - error InvalidGasLimit(); - error GasTokenTransferFailed(); + error InvalidAddress(string reason); + error Unauthorized(address caller, address expected); + error InvalidGasLimit(uint256 provided); + error GasTokenTransferFailed(address recipient, uint256 amount);contracts/nft/contracts/zetachain/UniversalNFTCore.sol (1)
153-155: Use a safer pattern for returning leftover funds.
Performing a low-level call tomsg.senderposes a potential attack vector or reentrancy threat. Even though you revert on failure, consider implementing a withdrawal pattern or carefully ordering state changes to minimize risk.contracts/token/contracts/zetachain/UniversalTokenCore.sol (1)
153-155: Evaluate your low-level call approach.
Forwarding leftover ETH back tomsg.sendervia a low-level call can be error-prone or exploited by untrusted recipients. Consider alternative patterns (e.g., a withdrawal function) to reduce potential attack surfaces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/nft/contracts/zetachain/UniversalNFTCore.sol(1 hunks)contracts/token/contracts/evm/UniversalTokenCore.sol(1 hunks)contracts/token/contracts/zetachain/UniversalTokenCore.sol(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Slither
contracts/token/contracts/zetachain/UniversalTokenCore.sol
[warning] 72-83: Conformance to Solidity naming conventions
Function UniversalTokenCore.__UniversalTokenCore_init(address,uint256,address) (contracts/zetachain/UniversalTokenCore.sol#72-83) is not in mixedCase
[warning] 118-179: Low-level calls
Low level call in UniversalTokenCore.transferCrossChain(address,address,uint256) (contracts/zetachain/UniversalTokenCore.sol#118-179):
- (success,None) = msg.sender.call{value: remaining}() (contracts/zetachain/UniversalTokenCore.sol#153)
[notice] 193-239: Reentrancy vulnerabilities
Reentrancy in UniversalTokenCore.onCall(MessageContext,address,uint256,bytes) (contracts/zetachain/UniversalTokenCore.sol#193-239):
External calls:
- out = SwapHelperLib.swapExactTokensForTokens(uniswapRouter,zrc20,amount,destination,0) (contracts/zetachain/UniversalTokenCore.sol#213-219)
- ! IZRC20(destination).approve(address(gateway),out) (contracts/zetachain/UniversalTokenCore.sol#220)
- gateway.withdrawAndCall(abi.encodePacked(connected[destination]),out - gasFee,destination,abi.encode(receiver,tokenAmount,out - gasFee,sender),CallOptions(gasLimitAmount,false),RevertOptions(address(this),true,address(0),abi.encode(tokenAmount,sender),0)) (contracts/zetachain/UniversalTokenCore.sol#223-236)
Event emitted after the call(s):
- TokenTransferToDestination(destination,receiver,amount) (contracts/zetachain/UniversalTokenCore.sol#238)
contracts/token/contracts/evm/UniversalTokenCore.sol
[warning] 83-94: Conformance to Solidity naming conventions
Function UniversalTokenCore.__UniversalTokenCore_init(address,address,uint256) (contracts/evm/UniversalTokenCore.sol#83-94) is not in mixedCase
[notice] 151-170: Reentrancy vulnerabilities
Reentrancy in UniversalTokenCore.onCall(MessageContext,bytes) (contracts/evm/UniversalTokenCore.sol#151-170):
External calls:
- (success,None) = address(sender).call{value: gasAmount}() (contracts/evm/UniversalTokenCore.sol#165)
Event emitted after the call(s):
- TokenTransferReceived(receiver,amount) (contracts/evm/UniversalTokenCore.sol#168)
[warning] 151-170: Low-level calls
Low level call in UniversalTokenCore.onCall(MessageContext,bytes) (contracts/evm/UniversalTokenCore.sol#151-170):
- (success,None) = address(sender).call{value: gasAmount}() (contracts/evm/UniversalTokenCore.sol#165)
contracts/nft/contracts/zetachain/UniversalNFTCore.sol
[warning] 120-186: Low-level calls
Low level call in UniversalNFTCore.transferCrossChain(uint256,address,address) (contracts/zetachain/UniversalNFTCore.sol#120-186):
- (success,None) = msg.sender.call{value: remaining}() (contracts/zetachain/UniversalNFTCore.sol#154)
🔇 Additional comments (8)
contracts/token/contracts/evm/UniversalTokenCore.sol (3)
1-22: LGTM! Well-structured contract with clear documentation.The contract follows best practices with:
- Latest Solidity version
- Proper inheritance hierarchy
- Comprehensive NatSpec documentation
83-94: 🛠️ Refactor suggestionFollow OpenZeppelin's initialization naming pattern.
The initialization function name should follow OpenZeppelin's pattern.
- function __UniversalTokenCore_init( + function __UniversalTokenCore_init_unchained(Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: Slither
[warning] 83-94: Conformance to Solidity naming conventions
Function UniversalTokenCore.__UniversalTokenCore_init(address,address,uint256) (contracts/evm/UniversalTokenCore.sol#83-94) is not in mixedCase
105-142:⚠️ Potential issueAdd reentrancy protection for cross-chain transfers.
The
transferCrossChainfunction should be protected against reentrancy attacks.+import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol"; abstract contract UniversalTokenCore is ERC20Upgradeable, OwnableUpgradeable, - UniversalTokenEvents + UniversalTokenEvents, + ReentrancyGuardUpgradeable { // ... existing code ... - function transferCrossChain( + function transferCrossChain( address destination, address receiver, uint256 amount - ) external payable { + ) external payable nonReentrant {Likely invalid or redundant comment.
contracts/nft/contracts/zetachain/UniversalNFTCore.sol (3)
22-28: Contract structure looks solid.
Your abstract contract approach and inheritance hierarchy are well-organized, providing clear extension points for child contracts.
73-83: Consider renaming the initialization function for consistency.
Function__UniversalNFTCore_init(...)is not strictly in mixedCase. A more conventional naming, such as_initializeUniversalNFTCore(...), may better align with widely accepted Solidity style guides.
258-268: Validate minted token uniqueness on revert.
When reverting the cross-chain transfer, you mint a new token with the sametokenId. Although this can be correct in your flow, ensure that no edge case accidentally mints a duplicate or breaks invariants in an inherited contract.contracts/token/contracts/zetachain/UniversalTokenCore.sol (2)
72-83: Function name not in mixedCase.
Function__UniversalTokenCore_init(...)violates typical Solidity naming conventions (e.g., lowerCamelCase for function names). Prefer something like_initializeUniversalTokenCore(...)to avoid confusion and align with style guidelines.🧰 Tools
🪛 GitHub Check: Slither
[warning] 72-83: Conformance to Solidity naming conventions
Function UniversalTokenCore.__UniversalTokenCore_init(address,uint256,address) (contracts/zetachain/UniversalTokenCore.sol#72-83) is not in mixedCase
193-239: Confirm reentrancy safety inonCall.
Although_onCallis restricted byonlyGateway, it still invokes swaps and approvals that might be manipulated by external contracts. ReentrancyGuard (or a strict checks-effects-interactions approach) can help mitigate subtle attack vectors.🧰 Tools
🪛 GitHub Check: Slither
[notice] 193-239: Reentrancy vulnerabilities
Reentrancy in UniversalTokenCore.onCall(MessageContext,address,uint256,bytes) (contracts/zetachain/UniversalTokenCore.sol#193-239):
External calls:
- out = SwapHelperLib.swapExactTokensForTokens(uniswapRouter,zrc20,amount,destination,0) (contracts/zetachain/UniversalTokenCore.sol#213-219)
- ! IZRC20(destination).approve(address(gateway),out) (contracts/zetachain/UniversalTokenCore.sol#220)
- gateway.withdrawAndCall(abi.encodePacked(connected[destination]),out - gasFee,destination,abi.encode(receiver,tokenAmount,out - gasFee,sender),CallOptions(gasLimitAmount,false),RevertOptions(address(this),true,address(0),abi.encode(tokenAmount,sender),0)) (contracts/zetachain/UniversalTokenCore.sol#223-236)
Event emitted after the call(s):
- TokenTransferToDestination(destination,receiver,amount) (contracts/zetachain/UniversalTokenCore.sol#238)
| 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); | ||
|
|
||
| emit TokenTransfer(receiver, destination, tokenId, uri); | ||
|
|
||
| (address gasZRC20, uint256 gasFee) = IZRC20(destination) | ||
| .withdrawGasFeeWithGasLimit(gasLimitAmount); | ||
| if (destination != gasZRC20) revert InvalidAddress(); | ||
|
|
||
| address WZETA = gateway.zetaToken(); | ||
| IWETH9(WZETA).deposit{value: msg.value}(); | ||
| if (!IWETH9(WZETA).approve(uniswapRouter, msg.value)) { | ||
| revert ApproveFailed(); | ||
| } | ||
|
|
||
| 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(); | ||
| } | ||
|
|
||
| bytes memory message = abi.encode( | ||
| receiver, | ||
| tokenId, | ||
| uri, | ||
| 0, | ||
| msg.sender | ||
| ); | ||
| CallOptions memory callOptions = CallOptions(gasLimitAmount, false); | ||
|
|
||
| RevertOptions memory revertOptions = RevertOptions( | ||
| address(this), | ||
| true, | ||
| address(0), | ||
| abi.encode(tokenId, uri, msg.sender), | ||
| gasLimitAmount | ||
| ); | ||
|
|
||
| if (!IZRC20(gasZRC20).approve(address(gateway), gasFee)) { | ||
| revert ApproveFailed(); | ||
| } | ||
|
|
||
| gateway.call( | ||
| abi.encodePacked(connected[destination]), | ||
| destination, | ||
| message, | ||
| callOptions, | ||
| revertOptions | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a reentrancy guard to strengthen security.
This function contains multiple external calls (including Uniswap swaps, approvals, and low-level call) after state changes. A malicious or faulty external contract could exploit reentrancy.
Would you like me to add a ReentrancyGuard modifier around these operations or reorganize them to follow checks-effects-interactions more rigorously?
🧰 Tools
🪛 GitHub Check: Slither
[warning] 120-186: Low-level calls
Low level call in UniversalNFTCore.transferCrossChain(uint256,address,address) (contracts/zetachain/UniversalNFTCore.sol#120-186):
- (success,None) = msg.sender.call{value: remaining}() (contracts/zetachain/UniversalNFTCore.sol#154)
| function transferCrossChain( | ||
| address destination, | ||
| address receiver, | ||
| uint256 amount | ||
| ) public payable { | ||
| if (msg.value == 0) revert ZeroMsgValue(); | ||
| if (receiver == address(0)) revert InvalidAddress(); | ||
|
|
||
| _burn(msg.sender, amount); | ||
|
|
||
| emit TokenTransfer(destination, receiver, amount); | ||
|
|
||
| (address gasZRC20, uint256 gasFee) = IZRC20(destination) | ||
| .withdrawGasFeeWithGasLimit(gasLimitAmount); | ||
| if (destination != gasZRC20) revert InvalidAddress(); | ||
|
|
||
| address WZETA = gateway.zetaToken(); | ||
|
|
||
| IWETH9(WZETA).deposit{value: msg.value}(); | ||
| if (!IWETH9(WZETA).approve(uniswapRouter, msg.value)) { | ||
| revert ApproveFailed(); | ||
| } | ||
|
|
||
| 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(); | ||
| } | ||
|
|
||
| bytes memory message = abi.encode(receiver, amount, 0, msg.sender); | ||
|
|
||
| CallOptions memory callOptions = CallOptions(gasLimitAmount, false); | ||
|
|
||
| RevertOptions memory revertOptions = RevertOptions( | ||
| address(this), | ||
| true, | ||
| address(0), | ||
| abi.encode(amount, msg.sender), | ||
| gasLimitAmount | ||
| ); | ||
|
|
||
| if (!IZRC20(gasZRC20).approve(address(gateway), gasFee)) { | ||
| revert ApproveFailed(); | ||
| } | ||
| gateway.call( | ||
| abi.encodePacked(connected[destination]), | ||
| destination, | ||
| message, | ||
| callOptions, | ||
| revertOptions | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a reentrancy guard to critical token transfer logic.
Similar to the NFT core contract, this function makes multiple external calls (swaps, approvals, low-level calls). Use a reentrancy guard or carefully structure the function to reduce risk.
🧰 Tools
🪛 GitHub Check: Slither
[warning] 118-179: Low-level calls
Low level call in UniversalTokenCore.transferCrossChain(address,address,uint256) (contracts/zetachain/UniversalTokenCore.sol#118-179):
- (success,None) = msg.sender.call{value: remaining}() (contracts/zetachain/UniversalTokenCore.sol#153)
This refactor makes it possible to take an ERC-721 template from https://wizard.openzeppelin.com/#erc721, modify 5 lines and make it a universal NFT.
Turning any ERC-721 (assuming it's upgradeable) into a universal NFT is pretty cool.
We can create a non-upgradeable version in the future (sort of like OZ), but I don't think it's a priority. For one, how would you upgrade an existing ERC-721 if it's non-upgradeable?
Fixed error in the token contract, when
amountwas refunded instead ofgasAmount.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores