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 21 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
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
140 changes: 140 additions & 0 deletions examples/token/contracts/Universal.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.26;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/access/Ownable2Step.sol";
import {RevertContext, RevertOptions} from "@zetachain/protocol-contracts/contracts/Revert.sol";
import "@zetachain/protocol-contracts/contracts/zevm/interfaces/UniversalContract.sol";
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 ERC20, Ownable2Step, UniversalContract, Events {
GatewayZEVM public immutable gateway;
SystemContract public immutable systemContract =
SystemContract(0xCf7Ed3AccA5a467e9e704C703E8D87F634fB0Fc9);
Copy link

Choose a reason for hiding this comment

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

โš ๏ธ Potential issue

Avoid hardcoding the system contract address

Hardcoding the systemContract address reduces flexibility and can lead to issues if the address changes across different networks or environments. Consider passing the address as a constructor parameter or providing a setter function with appropriate access control to allow for configuration.

uint256 private _nextTokenId;
bool public isUniversal = true;
uint256 public gasLimit;

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

mapping(address => bytes) public counterparty;

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

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

function setCounterparty(
address zrc20,
bytes memory contractAddress
) external onlyOwner {
counterparty[zrc20] = contractAddress;
emit CounterpartyMappingSet(zrc20, contractAddress);
}

function transferCrossChain(
address destination,
address receiver,
uint256 amount
) public {
if (receiver == address(0)) revert InvalidAddress();
_burn(msg.sender, amount);

(, uint256 gasFee) = IZRC20(destination).withdrawGasFeeWithGasLimit(
gasLimit
);
if (
!IZRC20(destination).transferFrom(msg.sender, address(this), gasFee)
) revert TransferFailed();
IZRC20(destination).approve(address(gateway), gasFee);
bytes memory message = abi.encode(receiver, amount);

CallOptions memory callOptions = CallOptions(gasLimit, false);

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

gateway.call(
counterparty[destination],
destination,
message,
callOptions,
revertOptions
);
emit TokenTransfer(destination, receiver, amount);
}
Comment on lines +56 to +91
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 input validation and reentrancy protection

The transferCrossChain function needs additional security measures:

  1. Add a check for zero amount transfers
  2. Implement reentrancy protection using OpenZeppelin's ReentrancyGuard
  3. Consider implementing a maximum amount check

Apply this diff:

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

-contract Universal is ERC20, Ownable2Step, UniversalContract, Events {
+contract Universal is ERC20, Ownable2Step, UniversalContract, Events, ReentrancyGuard {
    // ...
-    function transferCrossChain(
+    function transferCrossChain(
         address destination,
         address receiver,
         uint256 amount
-    ) public {
+    ) public nonReentrant {
+        if (amount == 0) revert("Zero amount transfer");
         if (receiver == address(0)) revert InvalidAddress();

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


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

function onCall(
MessageContext calldata context,
address zrc20,
uint256 amount,
bytes calldata message
) external override onlyGateway {
if (keccak256(context.origin) != keccak256(counterparty[zrc20]))
revert Unauthorized();
(address destination, address receiver, uint256 tokenAmount) = abi
.decode(message, (address, address, uint256));
if (destination == address(0)) {
_mint(receiver, tokenAmount);
} else {
(, uint256 gasFee) = IZRC20(destination).withdrawGasFeeWithGasLimit(
gasLimit
);
SwapHelperLib.swapExactTokensForTokens(
systemContract,
zrc20,
amount,
destination,
0
);
IZRC20(destination).approve(address(gateway), gasFee);
gateway.call(
counterparty[destination],
destination,
abi.encode(receiver, tokenAmount),
CallOptions(gasLimit, false),
RevertOptions(address(0), false, address(0), "", 0)
);
}
emit TokenTransferToDestination(destination, receiver, amount);
}
Comment on lines +97 to +130
Copy link

Choose a reason for hiding this comment

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

โš ๏ธ Potential issue

Enhance security in onCall function

The onCall function needs improvements:

  1. Use a more gas-efficient comparison for origin verification
  2. Add slippage protection for the swap
  3. Validate tokenAmount

Apply this diff:

     function onCall(
         MessageContext calldata context,
         address zrc20,
         uint256 amount,
         bytes calldata message
     ) external override onlyGateway {
-        if (keccak256(context.origin) != keccak256(counterparty[zrc20]))
+        if (!BytesLib.equal(context.origin, counterparty[zrc20]))
             revert Unauthorized();
         (address destination, address receiver, uint256 tokenAmount) = abi
             .decode(message, (address, address, uint256));
+        if (tokenAmount == 0) revert("Zero amount transfer");
         if (destination == address(0)) {
             _mint(receiver, tokenAmount);
         } else {
+            uint256 minAmountOut = tokenAmount * 95 / 100; // 5% max slippage
             SwapHelperLib.swapExactTokensForTokens(
                 systemContract,
                 zrc20,
                 amount,
                 destination,
-                0
+                minAmountOut
             );

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


function onRevert(RevertContext calldata context) external onlyGateway {
(address sender, uint256 amount) = abi.decode(
context.revertMessage,
(address, uint256)
);
_mint(sender, amount);
emit TokenTransferReverted(sender, amount);
}
Comment on lines +132 to +139
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 validation in onRevert function

The onRevert function should validate the decoded parameters.

     function onRevert(RevertContext calldata context) external onlyGateway {
         (address sender, uint256 amount) = abi.decode(
             context.revertMessage,
             (address, uint256)
         );
+        if (sender == address(0)) revert InvalidAddress();
+        if (amount == 0) revert("Zero amount revert");
         _mint(sender, amount);
         emit TokenTransferReverted(sender, amount);
     }

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

}
24 changes: 24 additions & 0 deletions examples/token/contracts/shared/Events.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.26;

contract Events {
event CounterpartyMappingSet(
address indexed zrc20,
bytes indexed contractAddress
);
event CounterpartySet(address indexed contractAddress);

event TokenMinted(address indexed to, uint256 amount);
event TokenTransfer(
address indexed destination,
address indexed receiver,
uint256 amount
);
event TokenTransferReceived(address indexed receiver, uint256 amount);
event TokenTransferReverted(address indexed sender, uint256 amount);
event TokenTransferToDestination(
address indexed destination,
address indexed sender,
uint256 amount
);
}
Loading
Loading