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.

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

ERC-20 token transfer example and improvements to NFT #209

Merged
merged 27 commits into from
Nov 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion examples/hello/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,4 @@
"@solana/web3.js": "^1.95.2",
"@zetachain/protocol-contracts": "10.0.0-rc11"
}
}
}
10 changes: 10 additions & 0 deletions examples/hello/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2434,6 +2434,16 @@
"@zetachain/networks" "^10.0.0"
ethers "^6.13.1"

"@zetachain/[email protected]":
version "10.0.0-rc11"
resolved "https://registry.yarnpkg.com/@zetachain/protocol-contracts/-/protocol-contracts-10.0.0-rc11.tgz#53f55ead492f7b5802b1feae4e51abc75730af33"
integrity sha512-qWazjqnIGRngf4OmyeSIv7sHICQRdMQ1CKPIQIqxA8qFR+gHhDHSfvMdRAvgWbsfkimXOIFiHVIATypyWhviJw==
dependencies:
"@openzeppelin/contracts" "^5.0.2"
"@openzeppelin/contracts-upgradeable" "^5.0.2"
"@zetachain/networks" "^10.0.0"
ethers "^6.13.1"

"@zetachain/[email protected]":
version "9.0.0"
resolved "https://registry.yarnpkg.com/@zetachain/protocol-contracts/-/protocol-contracts-9.0.0.tgz#c20ad5da43f6f3676f31556b303d1cb4ea17357e"
Expand Down
16 changes: 8 additions & 8 deletions examples/nft/contracts/Connected.sol
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ contract Connected is

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

RevertOptions memory revertOptions = RevertOptions(
address(this),
Expand All @@ -85,7 +85,7 @@ contract Connected is
);
}

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

function onCall(
Expand All @@ -94,26 +94,26 @@ contract Connected is
) external payable onlyGateway returns (bytes4) {
if (context.sender != counterparty) revert Unauthorized();

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

_safeMint(receiver, tokenId);
_setTokenURI(tokenId, uri);
emit TokenTransferReceived(tokenId, receiver, uri);
emit TokenTransferReceived(receiver, tokenId, uri);
Comment on lines +97 to +104
Copy link

Choose a reason for hiding this comment

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

๐Ÿ› ๏ธ Refactor suggestion

Add address validation in onCall function.

While the parameter reordering is correct, consider adding validation for the decoded receiver address to maintain consistency with other functions that check for zero addresses.

 (address receiver, uint256 tokenId, string memory uri) = abi.decode(
     message,
     (address, uint256, string)
 );
+if (receiver == address(0)) revert InvalidAddress();
 
 _safeMint(receiver, tokenId);

Committable suggestion skipped: line range outside the PR's diff.

return "";
}

function onRevert(RevertContext calldata context) external onlyGateway {
(uint256 tokenId, address sender, string memory uri) = abi.decode(
(address sender, uint256 tokenId, string memory uri) = abi.decode(
context.revertMessage,
(uint256, address, string)
(address, uint256, string)
);

_safeMint(sender, tokenId);
_setTokenURI(tokenId, uri);
emit TokenTransferReverted(tokenId, sender, uri);
emit TokenTransferReverted(sender, tokenId, uri);
Comment on lines +109 to +116
Copy link

Choose a reason for hiding this comment

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

๐Ÿ› ๏ธ Refactor suggestion

Add address validation in onRevert function.

For consistency with onCall and other functions, add validation for the decoded sender address.

 (address sender, uint256 tokenId, string memory uri) = abi.decode(
     context.revertMessage,
     (address, uint256, string)
 );
+if (sender == address(0)) revert InvalidAddress();
 
 _safeMint(sender, tokenId);
๐Ÿ“ 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.

Suggested change
(address sender, uint256 tokenId, string memory uri) = abi.decode(
context.revertMessage,
(uint256, address, string)
(address, uint256, string)
);
_safeMint(sender, tokenId);
_setTokenURI(tokenId, uri);
emit TokenTransferReverted(tokenId, sender, uri);
emit TokenTransferReverted(sender, tokenId, uri);
(address sender, uint256 tokenId, string memory uri) = abi.decode(
context.revertMessage,
(address, uint256, string)
);
if (sender == address(0)) revert InvalidAddress();
_safeMint(sender, tokenId);
_setTokenURI(tokenId, uri);
emit TokenTransferReverted(sender, tokenId, uri);

}

receive() external payable {}
Expand Down
24 changes: 12 additions & 12 deletions examples/nft/contracts/Universal.sol
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ contract Universal is
!IZRC20(destination).transferFrom(msg.sender, address(this), gasFee)
) revert TransferFailed();
IZRC20(destination).approve(address(gateway), gasFee);
bytes memory message = abi.encode(tokenId, receiver, uri);
bytes memory message = abi.encode(receiver, tokenId, 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

Parameter ordering inconsistency detected between Universal and Connected contracts

The parameter ordering differs between the contracts:

  • Universal.sol: abi.encode(receiver, tokenId, uri)
  • Connected.sol: abi.encode(destination, receiver, tokenId, uri)

This inconsistency in message encoding could lead to cross-chain transfer failures. The Connected contract expects an additional destination parameter in the encoded message that Universal contract does not provide.

๐Ÿ”— Analysis chain

Parameter reordering looks good, verify cross-chain compatibility.

The reordering of parameters in message encoding and event emission maintains internal consistency. The sequence (receiver, tokenId, uri) is now uniform throughout the contract.

Let's verify the parameter order consistency in connected contracts:

Also applies to: 102-102

๐Ÿ Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for message encoding patterns in connected contracts
rg -A 2 "abi\.encode.*receiver.*tokenId.*uri" --type sol

Length of output: 86


Script:

#!/bin/bash
# Search for message encoding patterns in all files
rg -A 2 "abi\.encode.*receiver.*tokenId.*uri"

# Also search for message decoding patterns
rg -A 2 "abi\.decode.*receiver.*tokenId.*uri"

# Search for any abi.encode patterns to analyze parameter ordering
rg -A 2 "abi\.encode"

Length of output: 3355


CallOptions memory callOptions = CallOptions(gasLimit, false);

Expand All @@ -99,7 +99,7 @@ contract Universal is
revertOptions
);

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

function safeMint(address to, string memory uri) public onlyOwner {
Expand All @@ -125,16 +125,16 @@ contract Universal is
revert("Unauthorized");

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

if (destination == address(0)) {
_safeMint(sender, tokenId);
_setTokenURI(tokenId, uri);
emit TokenTransferReceived(tokenId, sender, uri);
emit TokenTransferReceived(sender, tokenId, uri);
} else {
(, uint256 gasFee) = IZRC20(destination).withdrawGasFeeWithGasLimit(
gasLimit
Expand All @@ -152,23 +152,23 @@ contract Universal is
gateway.call(
counterparty[destination],
destination,
abi.encode(tokenId, sender, uri),
abi.encode(sender, tokenId, uri),
CallOptions(gasLimit, false),
RevertOptions(address(0), false, address(0), "", 0)
);
emit TokenTransferToDestination(tokenId, sender, destination, uri);
emit TokenTransferToDestination(sender, destination, tokenId, uri);
}
}

function onRevert(RevertContext calldata context) external onlyGateway {
(uint256 tokenId, address sender, string memory uri) = abi.decode(
(address sender, uint256 tokenId, string memory uri) = abi.decode(
context.revertMessage,
(uint256, address, string)
(address, uint256, string)
);

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

// The following functions are overrides required by Solidity.
Expand Down
12 changes: 6 additions & 6 deletions examples/nft/contracts/shared/Events.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,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,
address indexed receiver,
uint256 indexed tokenId,
string uri
);
event TokenTransferReceived(
uint256 indexed tokenId,
address indexed receiver,
uint256 indexed tokenId,
string uri
);
event TokenTransferReverted(
uint256 indexed tokenId,
address indexed sender,
uint256 indexed tokenId,
string uri
);
event CounterpartySet(address indexed zrc20, bytes indexed contractAddress);
event TokenTransferToDestination(
uint256 indexed tokenId,
address indexed sender,
address indexed destination,
address indexed sender,
uint256 indexed tokenId,
string uri
);
Comment on lines 8 to 29
Copy link

Choose a reason for hiding this comment

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

๐Ÿ’ก Codebase verification

Parameter mismatch detected in TokenTransfer event between declaration and usage

The verification reveals inconsistencies in event parameter ordering:

  • In examples/nft/contracts/Universal.sol, TokenTransfer is emitted with parameters (receiver, destination, tokenId, uri)
  • However, in Events.sol, it's declared as (destination, receiver, tokenId, uri)
  • Similar parameter ordering should be maintained across the codebase to prevent event handling issues

This mismatch could lead to incorrect event handling by external listeners and should be corrected to maintain consistency with the event declaration in Events.sol.

๐Ÿ”— Analysis chain

Verify event parameter alignment with external contracts.

The parameter reordering appears to align with changes in the Connected and Universal contracts mentioned in the summary. However, it's crucial to verify this alignment to prevent any potential event handling issues.


Let me gather more information about the event declarations to complete the verification.

๐Ÿ Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify event parameter ordering consistency across related contracts
# Expected: Parameter ordering should match in event emissions and declarations

# Search for event emissions in related contracts
echo "Checking event emissions in contracts:"
rg -A 2 "emit Token(Transfer|TransferReceived|TransferReverted|TransferToDestination)"

# Search for interface definitions that might be listening to these events
echo "Checking interface definitions:"
ast-grep --pattern 'interface $_ {
  $$$
  event Token$_(address indexed $_, $$$);
  $$$
}'

Length of output: 2702


Script:

#!/bin/bash
# Search for event declarations in all contracts
echo "Checking event declarations in contracts:"
rg -A 3 "event Token(Transfer|TransferReceived|TransferReverted|TransferToDestination)" --type sol

# Search specifically in the shared Events.sol files
echo -e "\nChecking Events.sol files:"
fd "Events.sol" --exec cat {}

Length of output: 1911

}
10 changes: 10 additions & 0 deletions examples/swap/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2172,6 +2172,16 @@
"@zetachain/networks" "^10.0.0"
ethers "^6.13.1"

"@zetachain/[email protected]":
version "10.0.0-rc11"
resolved "https://registry.yarnpkg.com/@zetachain/protocol-contracts/-/protocol-contracts-10.0.0-rc11.tgz#53f55ead492f7b5802b1feae4e51abc75730af33"
integrity sha512-qWazjqnIGRngf4OmyeSIv7sHICQRdMQ1CKPIQIqxA8qFR+gHhDHSfvMdRAvgWbsfkimXOIFiHVIATypyWhviJw==
dependencies:
"@openzeppelin/contracts" "^5.0.2"
"@openzeppelin/contracts-upgradeable" "^5.0.2"
"@zetachain/networks" "^10.0.0"
ethers "^6.13.1"

"@zetachain/[email protected]":
version "9.0.0"
resolved "https://registry.yarnpkg.com/@zetachain/protocol-contracts/-/protocol-contracts-9.0.0.tgz#c20ad5da43f6f3676f31556b303d1cb4ea17357e"
Expand Down
6 changes: 6 additions & 0 deletions examples/token/.eslintignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
.yarn
artifacts
cache
coverage
node_modules
typechain-types
47 changes: 47 additions & 0 deletions examples/token/.eslintrc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
const path = require("path");

/**
* @type {import("eslint").Linter.Config}
*/
module.exports = {
env: {
browser: false,
es2021: true,
mocha: true,
node: true,
},
extends: ["plugin:prettier/recommended"],
parser: "@typescript-eslint/parser",
parserOptions: {
ecmaVersion: 12,
},
plugins: [
"@typescript-eslint",
"prettier",
"simple-import-sort",
"sort-keys-fix",
"typescript-sort-keys",
],
rules: {
"@typescript-eslint/sort-type-union-intersection-members": "error",
camelcase: "off",
"simple-import-sort/exports": "error",
"simple-import-sort/imports": "error",
"sort-keys-fix/sort-keys-fix": "error",
"typescript-sort-keys/interface": "error",
"typescript-sort-keys/string-enum": "error",
},
fadeev marked this conversation as resolved.
Show resolved Hide resolved
settings: {
"import/parsers": {
"@typescript-eslint/parser": [".js", ".jsx", ".ts", ".tsx", ".d.ts"],
},
"import/resolver": {
node: {
extensions: [".js", ".jsx", ".ts", ".tsx", ".d.ts"],
},
typescript: {
project: path.join(__dirname, "tsconfig.json"),
},
},
},
};
17 changes: 17 additions & 0 deletions examples/token/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
node_modules
.env
coverage
coverage.json
typechain
typechain-types
dependencies

# Hardhat files
cache
artifacts

# Foundry files
out
cache_forge

access_token
21 changes: 21 additions & 0 deletions examples/token/LICENSE
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
MIT License

Copyright (c) 2023 ZetaChain

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.
4 changes: 4 additions & 0 deletions examples/token/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# NFT Example

This example currently only works with localnet `v4.0.0-rc*`, which supports
authenticated calls and multiple EVM chains.
89 changes: 89 additions & 0 deletions examples/token/contracts/Connected.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/access/Ownable2Step.sol";
import "@zetachain/protocol-contracts/contracts/evm/GatewayEVM.sol";
import {RevertContext} from "@zetachain/protocol-contracts/contracts/Revert.sol";
import "./shared/Events.sol";

contract Connected is ERC20, Ownable2Step, Events {
GatewayEVM public immutable gateway;
address public counterparty;

error InvalidAddress();
error Unauthorized();

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

function setCounterparty(address contractAddress) external onlyOwner {
counterparty = contractAddress;
emit CounterpartySet(contractAddress);
}
Comment on lines +22 to +25
Copy link

Choose a reason for hiding this comment

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

๐Ÿ› ๏ธ Refactor suggestion

Add address validation and improve event emission.

The function should validate the new counterparty address and include the old value in the event for better traceability.

Apply this improvement:

 function setCounterparty(address contractAddress) external onlyOwner {
+    if (contractAddress == address(0)) revert InvalidAddress("counterparty address is zero");
+    address oldCounterparty = counterparty;
     counterparty = contractAddress;
-    emit CounterpartySet(contractAddress);
+    emit CounterpartySet(oldCounterparty, contractAddress);
 }

Committable suggestion skipped: line range outside the PR's diff.


constructor(
address payable gatewayAddress,
address owner,
string memory name,
string memory symbol
) ERC20(name, symbol) Ownable(owner) {
if (gatewayAddress == address(0) || owner == address(0))
revert InvalidAddress();
gateway = GatewayEVM(gatewayAddress);
}

function mint(address to, uint256 amount) public onlyOwner {
_mint(to, amount);
}

function transferCrossChain(
address destination,
address receiver,
uint256 amount
) external payable {
_burn(msg.sender, amount);
bytes memory message = abi.encode(destination, receiver, amount);

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

if (destination == address(0)) {
gateway.call(counterparty, message, revertOptions);
} else {
gateway.depositAndCall{value: msg.value}(
counterparty,
message,
revertOptions
);
}
emit TokenTransfer(destination, receiver, amount);
}
Comment on lines +42 to +68
Copy link

Choose a reason for hiding this comment

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

โš ๏ธ Potential issue

Add security checks and follow CEI pattern.

The function has several security considerations that should be addressed:

  1. No validation if counterparty is set
  2. No validation of amount
  3. External calls after state modification (potential reentrancy)

Apply these security improvements:

 function transferCrossChain(
     address destination,
     address receiver,
     uint256 amount
 ) external payable {
+    if (counterparty == address(0)) revert InvalidAddress("counterparty not set");
+    if (receiver == address(0)) revert InvalidAddress("receiver is zero");
+    if (amount == 0) revert("amount cannot be zero");
+    
+    // Cache msg.value to follow CEI pattern
+    uint256 value = msg.value;
     _burn(msg.sender, amount);
     bytes memory message = abi.encode(destination, receiver, amount);

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

     if (destination == address(0)) {
         gateway.call(counterparty, message, revertOptions);
     } else {
-        gateway.depositAndCall{value: msg.value}(
+        gateway.depositAndCall{value: value}(
             counterparty,
             message,
             revertOptions
         );
     }
     emit TokenTransfer(destination, receiver, amount);
 }

Committable suggestion skipped: line range outside the PR's diff.


function onCall(
MessageContext calldata context,
bytes calldata message
) external payable onlyGateway returns (bytes4) {
if (context.sender != counterparty) revert Unauthorized();
(address receiver, uint256 amount) = abi.decode(
message,
(address, uint256)
);
_mint(receiver, amount);
emit TokenTransferReceived(receiver, amount);
return "";
}
Comment on lines +70 to +82
Copy link

Choose a reason for hiding this comment

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

๐Ÿ› ๏ธ Refactor suggestion

Add amount validation in onCall function.

The function should validate the minting amount to prevent potential issues.

Apply this improvement:

 function onCall(
     MessageContext calldata context,
     bytes calldata message
 ) external payable onlyGateway returns (bytes4) {
     if (context.sender != counterparty) revert Unauthorized();
     (address receiver, uint256 amount) = abi.decode(
         message,
         (address, uint256)
     );
+    if (receiver == address(0)) revert InvalidAddress("receiver is zero");
+    if (amount == 0) revert("amount cannot be zero");
     _mint(receiver, amount);
     emit TokenTransferReceived(receiver, amount);
     return "";
 }

Committable suggestion skipped: line range outside the PR's diff.


function onRevert(RevertContext calldata context) external onlyGateway {}
Copy link

Choose a reason for hiding this comment

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

โš ๏ธ Potential issue

Implement revert handling logic.

The empty onRevert function could lead to lost tokens in case of cross-chain transfer failures. Consider implementing proper revert handling logic to ensure tokens can be recovered.

Would you like me to provide an implementation for handling cross-chain transfer reverts?


receive() external payable {}

fallback() external payable {}
}
fadeev marked this conversation as resolved.
Show resolved Hide resolved
Loading
Loading