Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NFT: improvements to make production-ready #217

Merged
merged 13 commits into from
Nov 8, 2024
Merged

NFT: improvements to make production-ready #217

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
57bc55a
NFT: fixes
fadeev Nov 7, 2024
a674ace
refactor
fadeev Nov 7, 2024
3c638dc
should throw an error
fadeev Nov 7, 2024
d54d7ca
should also throw an error, because address is zero
fadeev Nov 7, 2024
14dd3e2
should be passing the test
fadeev Nov 7, 2024
73347ae
remove unused imports
fadeev Nov 7, 2024
edca64c
refactor
fadeev Nov 7, 2024
bddf609
set gasLimit in constructor
fadeev Nov 7, 2024
f191e37
add events
fadeev Nov 7, 2024
bed8230
events in a separate file
fadeev Nov 7, 2024
c779848
events in a separate file
fadeev Nov 7, 2024
b77396f
Ownable2Step
fadeev Nov 7, 2024
6ba78a0
invalid gas limit error
fadeev Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 37 additions & 22 deletions examples/nft/contracts/Connected.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,86 +8,100 @@ import "@openzeppelin/contracts/access/Ownable.sol";
import "@openzeppelin/contracts/token/ERC721/extensions/ERC721Burnable.sol";
import "@zetachain/protocol-contracts/contracts/evm/GatewayEVM.sol";
import {RevertContext} from "@zetachain/protocol-contracts/contracts/Revert.sol";

contract Connected is ERC721, ERC721Enumerable, ERC721URIStorage, Ownable {
import "./shared/Events.sol";

contract Connected is
ERC721,
ERC721Enumerable,
ERC721URIStorage,
Ownable,
Events
{
GatewayEVM public immutable gateway;
uint256 private _nextTokenId;
address public counterparty;

error InvalidAddress();
error Unauthorized();

function setCounterparty(address contractAddress) external onlyOwner {
if (contractAddress == address(0)) revert InvalidAddress();
counterparty = contractAddress;
emit SetCounterparty(contractAddress);
}

modifier onlyGateway() {
require(msg.sender == address(gateway), "Caller is not the gateway");
if (msg.sender != address(gateway)) revert Unauthorized();
_;
}

constructor(
address payable gatewayAddress,
address initialOwner
) ERC721("MyToken", "MTK") Ownable(initialOwner) {
address owner,
string memory name,
string memory symbol
) ERC721(name, symbol) Ownable(owner) {
if (gatewayAddress == address(0) || owner == address(0))
revert InvalidAddress();
gateway = GatewayEVM(gatewayAddress);
fadeev marked this conversation as resolved.
Show resolved Hide resolved
}

function safeMint(address to, string memory uri) public onlyOwner {
uint256 hash = uint256(
keccak256(
abi.encodePacked(address(this), block.number, _nextTokenId++)
)
);
if (to == address(0)) revert InvalidAddress();

uint256 tokenId = hash & 0x00FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF;
uint256 tokenId = _nextTokenId++;

_safeMint(to, tokenId);
_setTokenURI(tokenId, uri);
emit TokenMinted(to, tokenId, uri);
fadeev marked this conversation as resolved.
Show resolved Hide resolved
}

function transferCrossChain(
uint256 tokenId,
address receiver,
address destination
) external payable {
if (receiver == address(0)) revert InvalidAddress();

string memory uri = tokenURI(tokenId);
_burn(tokenId);
bytes memory encodedData = abi.encode(
tokenId,
receiver,
uri,
destination
);
bytes memory message = abi.encode(tokenId, receiver, uri, destination);

RevertOptions memory revertOptions = RevertOptions(
address(this),
true,
address(0),
encodedData,
message,
0
);

if (destination == address(0)) {
gateway.call(counterparty, encodedData, revertOptions);
gateway.call(counterparty, message, revertOptions);
} else {
gateway.depositAndCall{value: msg.value}(
counterparty,
encodedData,
message,
revertOptions
);
}

emit TokenTransfer(tokenId, receiver, destination, uri);
fadeev marked this conversation as resolved.
Show resolved Hide resolved
}

function onCall(
MessageContext calldata messageContext,
MessageContext calldata context,
bytes calldata message
) external payable onlyGateway returns (bytes4) {
if (messageContext.sender != counterparty) revert("Unauthorized");
if (context.sender != counterparty) revert Unauthorized();

(uint256 tokenId, address receiver, string memory uri) = abi.decode(
message,
(uint256, address, string)
);

_safeMint(receiver, tokenId);
_setTokenURI(tokenId, uri);
emit TokenTransferReceived(tokenId, receiver, uri);
return "";
}

Expand All @@ -99,6 +113,7 @@ contract Connected is ERC721, ERC721Enumerable, ERC721URIStorage, Ownable {

_safeMint(sender, tokenId);
_setTokenURI(tokenId, uri);
emit TokenTransferReverted(tokenId, sender, uri);
}

receive() external payable {}
Expand Down
38 changes: 26 additions & 12 deletions examples/nft/contracts/Universal.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,37 +12,45 @@ 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 {SystemContract} from "@zetachain/toolkit/contracts/SystemContract.sol";
import "./shared/Events.sol";

contract Universal is
ERC721,
ERC721Enumerable,
ERC721URIStorage,
Ownable,
UniversalContract
UniversalContract,
Events
{
GatewayZEVM public immutable gateway;
SystemContract public immutable systemContract =
SystemContract(0xCf7Ed3AccA5a467e9e704C703E8D87F634fB0Fc9);
uint256 private _nextTokenId;
bool public isUniversal = true;
uint256 public gasLimit = 700000;
uint256 public gasLimit;
fadeev marked this conversation as resolved.
Show resolved Hide resolved

error TransferFailed();
error Unauthorized();
error InvalidAddress();

mapping(address => bytes) public counterparty;

event CounterpartySet(address indexed zrc20, bytes indexed contractAddress);

modifier onlyGateway() {
require(msg.sender == address(gateway), "Caller is not the gateway");
if (msg.sender != address(gateway)) revert Unauthorized();
_;
}

constructor(
address payable gatewayAddress,
address initialOwner
) ERC721("MyToken", "MTK") Ownable(initialOwner) {
address owner,
string memory name,
string memory symbol,
uint256 gas
) ERC721(name, symbol) Ownable(owner) {
if (gatewayAddress == address(0) || owner == address(0))
revert InvalidAddress();
gateway = GatewayZEVM(gatewayAddress);
gasLimit = gas;
fadeev marked this conversation as resolved.
Show resolved Hide resolved
}

function setCounterparty(
Expand All @@ -58,6 +66,7 @@ contract Universal is
address receiver,
address destination
) public {
if (receiver == address(0)) revert InvalidAddress();
string memory uri = tokenURI(tokenId);
_burn(tokenId);

Expand All @@ -68,25 +77,27 @@ contract Universal is
!IZRC20(destination).transferFrom(msg.sender, address(this), gasFee)
) revert TransferFailed();
IZRC20(destination).approve(address(gateway), gasFee);
bytes memory encodedData = abi.encode(tokenId, receiver, uri);
bytes memory message = abi.encode(tokenId, receiver, uri);

CallOptions memory callOptions = CallOptions(gasLimit, false);

RevertOptions memory revertOptions = RevertOptions(
address(this),
true,
address(0),
encodedData,
message,
gasLimit
);

gateway.call(
counterparty[destination],
destination,
encodedData,
message,
callOptions,
revertOptions
);

emit TokenTransfer(tokenId, receiver, destination, uri);
}

function safeMint(address to, string memory uri) public onlyOwner {
Expand Down Expand Up @@ -121,9 +132,10 @@ contract Universal is
if (destination == address(0)) {
_safeMint(sender, tokenId);
_setTokenURI(tokenId, uri);
emit TokenTransferReceived(tokenId, sender, uri);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Reentrancy protection is recommended for external calls

The onCall function in Universal.sol contains external calls through gateway.call after state modifications (_safeMint, _setTokenURI). While the function is protected by onlyGateway modifier, implementing the following safeguards is recommended:

  • Add OpenZeppelin's ReentrancyGuard and use nonReentrant modifier
  • Alternatively, strictly follow checks-effects-interactions pattern by:
    • Moving the TokenTransferReceived event emission after all state changes
    • Ensuring gateway.call is the last operation in all execution paths

Example implementation:

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

contract Universal is ..., ReentrancyGuard {
    function onCall(...) external override onlyGateway nonReentrant {
        // existing implementation
    }
}
🔗 Analysis chain

Consider reentrancy protection for external calls.

The function makes external calls to swapExactTokensForTokens and gateway.call. Consider implementing a reentrancy guard or ensuring proper checks-effects-interactions pattern.

Also applies to: 140-140, 156-156, 159-159

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for ReentrancyGuard usage in the codebase
ast-grep --pattern 'contract $_ is $$$, ReentrancyGuard, $$$ {'

Length of output: 65

Script:

#!/bin/bash
# Let's check for ReentrancyGuard imports and usage with different patterns
rg -i "reentrancy|ReentrancyGuard" -A 3

# Check the specific onCall function implementation
ast-grep --pattern 'function onCall($$$) {
  $$$
}'

# Check for nonReentrant modifier usage
ast-grep --pattern 'nonReentrant'

# Look for the gateway.call pattern to understand the external call context
rg "gateway.*call" -A 3

Length of output: 1514

Script:

#!/bin/bash
# Let's examine the full context of the onCall function to understand the control flow
rg "function onCall" -A 20

# Check for existing modifiers on the contract and function
ast-grep --pattern 'function onCall($$$) $_ {
  $$$
}'

# Look for existing checks-effects-interactions pattern
rg "require\(.*\)" -A 2 examples/nft/contracts/Universal.sol

Length of output: 7522

} else {
(, uint256 gasFee) = IZRC20(destination).withdrawGasFeeWithGasLimit(
700000
gasLimit
);

SwapHelperLib.swapExactTokensForTokens(
Expand All @@ -139,9 +151,10 @@ contract Universal is
counterparty[destination],
destination,
abi.encode(tokenId, sender, uri),
CallOptions(700000, false),
CallOptions(gasLimit, false),
RevertOptions(address(0), false, address(0), "", 0)
);
emit TokenTransferToDestination(tokenId, sender, destination, uri);
}
}

Expand All @@ -153,6 +166,7 @@ contract Universal is

_safeMint(sender, tokenId);
_setTokenURI(tokenId, uri);
emit TokenTransferReverted(tokenId, sender, uri);
}

// The following functions are overrides required by Solidity.
Expand Down
30 changes: 30 additions & 0 deletions examples/nft/contracts/shared/Events.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.26;

contract Events {
event SetCounterparty(address indexed newCounterparty);
event TokenMinted(address indexed to, uint256 indexed tokenId, string uri);
event TokenTransfer(
uint256 indexed tokenId,
address indexed receiver,
address indexed destination,
string uri
);
event TokenTransferReceived(
uint256 indexed tokenId,
address indexed receiver,
string uri
);
event TokenTransferReverted(
uint256 indexed tokenId,
address indexed sender,
string uri
);
event CounterpartySet(address indexed zrc20, bytes indexed contractAddress);
event TokenTransferToDestination(
uint256 indexed tokenId,
address indexed sender,
address indexed destination,
string uri
);
}
fadeev marked this conversation as resolved.
Show resolved Hide resolved
26 changes: 12 additions & 14 deletions examples/nft/scripts/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,16 @@

set -e

if [ "$1" = "localnet" ]; then
npx hardhat localnet --exit-on-error & sleep 10
fi
if [ "$1" = "localnet" ]; then npx hardhat localnet --exit-on-error & sleep 10; fi
fadeev marked this conversation as resolved.
Show resolved Hide resolved

function nft_balance() {
function balance() {
local ZETACHAIN=$(cast call "$CONTRACT_ZETACHAIN" "balanceOf(address)(uint256)" "$SENDER")
local ETHEREUM=$(cast call "$CONTRACT_ETHEREUM" "balanceOf(address)(uint256)" "$SENDER")
local BNB=$(cast call "$CONTRACT_BNB" "balanceOf(address)(uint256)" "$SENDER")
echo -e "\n🖼️ NFT Balance"
echo "---------------------------------------------"
echo "🟢 ZetaChain: $ZETACHAIN"
echo "🔵 EVM Chain: $ETHEREUM"
echo "🔵 Ethereum: $ETHEREUM"
echo "🟡 BNB Chain: $BNB"
echo "---------------------------------------------"
fadeev marked this conversation as resolved.
Show resolved Hide resolved
}
Expand All @@ -27,11 +25,11 @@ GATEWAY_ETHEREUM=$(jq -r '.addresses[] | select(.type=="gatewayEVM" and .chain==
GATEWAY_BNB=$(jq -r '.addresses[] | select(.type=="gatewayEVM" and .chain=="bnb") | .address' localnet.json)
SENDER=0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266

CONTRACT_ZETACHAIN=$(npx hardhat deploy --network localhost --json | jq -r '.contractAddress')
CONTRACT_ZETACHAIN=$(npx hardhat deploy --network localhost --json --gas-limit 1000000 | jq -r '.contractAddress')
echo -e "\n🚀 Deployed NFT contract on ZetaChain: $CONTRACT_ZETACHAIN"

CONTRACT_ETHEREUM=$(npx hardhat deploy --name Connected --json --network localhost --gateway "$GATEWAY_ETHEREUM" | jq -r '.contractAddress')
echo -e "🚀 Deployed NFT contract on EVM chain: $CONTRACT_ETHEREUM"
echo -e "🚀 Deployed NFT contract on Ethereum: $CONTRACT_ETHEREUM"

CONTRACT_BNB=$(npx hardhat deploy --name Connected --json --network localhost --gateway "$GATEWAY_BNB" | jq -r '.contractAddress')
echo -e "🚀 Deployed NFT contract on BNB chain: $CONTRACT_BNB"
Expand All @@ -45,30 +43,30 @@ npx hardhat universal-set-counterparty --network localhost --contract "$CONTRACT
npx hardhat universal-set-counterparty --network localhost --contract "$CONTRACT_ZETACHAIN" --counterparty "$CONTRACT_BNB" --zrc20 "$ZRC20_BNB" --json &>/dev/null

npx hardhat localnet-check
nft_balance
balance

NFT_ID=$(npx hardhat mint --network localhost --json --contract "$CONTRACT_ZETACHAIN" --token-uri https://example.com/nft/metadata/1 | jq -r '.tokenId')
echo -e "\nMinted NFT with ID: $NFT_ID on ZetaChain."

npx hardhat localnet-check
nft_balance
balance

echo -e "\nTransferring NFT: ZetaChain → Ethereum..."
npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_ZETACHAIN" --to "$ZRC20_ETHEREUM"
npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_ZETACHAIN" --to "$ZRC20_ETHEREUM"

npx hardhat localnet-check
nft_balance
balance

echo -e "\nTransferring NFT: Ethereum → BNB..."
npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_ETHEREUM" --to "$ZRC20_BNB" --gas-amount 0.1

npx hardhat localnet-check
nft_balance
balance

echo -e "\nTransferring NFT: BNB → ZetaChain..."
npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_BNB"

npx hardhat localnet-check
nft_balance
balance
fadeev marked this conversation as resolved.
Show resolved Hide resolved

npx hardhat localnet-stop
if [ "$1" = "localnet" ]; then npx hardhat localnet-stop; fi
8 changes: 5 additions & 3 deletions examples/nft/tasks/connectedSetCounterparty.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { task } from "hardhat/config";
import { HardhatRuntimeEnvironment } from "hardhat/types";
import { ethers } from "ethers";
import { Connected } from "@/typechain-types";

const main = async (args: any, hre: HardhatRuntimeEnvironment) => {
const [signer] = await hre.ethers.getSigners();
Expand All @@ -10,7 +10,10 @@ const main = async (args: any, hre: HardhatRuntimeEnvironment) => {
);
}

const contract = await hre.ethers.getContractAt(args.name, args.contract);
const contract: Connected = await hre.ethers.getContractAt(
"Connected",
args.contract
);

const tx = await contract.setCounterparty(args.counterparty);
const receipt = await tx.wait();
Expand All @@ -34,5 +37,4 @@ const main = async (args: any, hre: HardhatRuntimeEnvironment) => {
task("connected-set-counterparty", "Sets the universal contract address", main)
.addParam("contract", "The address of the deployed contract")
.addParam("counterparty", "The address of the universal contract to set")
.addOptionalParam("name", "The contract name to interact with", "Connected")
.addFlag("json", "Output the result in JSON format");
Loading
Loading