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

feat: check withdraw fee in Swap, callMulti example, universal NFT/FT #229

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
4 changes: 0 additions & 4 deletions .github/workflows/slither.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,6 @@ jobs:
file: "call.sarif"
- project: "examples/swap"
file: "swap.sarif"
- project: "examples/nft"
file: "nft.sarif"
- project: "examples/token"
file: "token.sarif"
permissions:
contents: read
security-events: write
Expand Down
2 changes: 1 addition & 1 deletion examples/call/contracts/Connected.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import "@zetachain/protocol-contracts/contracts/evm/GatewayEVM.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

contract Connected {
using SafeERC20 for IERC20; // Use SafeERC20 for IERC20 operations
using SafeERC20 for IERC20;

GatewayEVM public immutable gateway;

Expand Down
30 changes: 30 additions & 0 deletions examples/call/contracts/Universal.sol
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,36 @@
gateway.call(receiver, zrc20, message, callOptions, revertOptions);
}

function callMulti(
bytes[] memory receiverArray,
address[] memory zrc20Array,
bytes calldata messages,
CallOptions memory callOptions,
RevertOptions memory revertOptions
) external {
for (uint256 i = 0; i < zrc20Array.length; i++) {
(, uint256 gasFee) = IZRC20(zrc20Array[i])
.withdrawGasFeeWithGasLimit(callOptions.gasLimit);
if (
!IZRC20(zrc20Array[i]).transferFrom(
msg.sender,
address(this),
gasFee
)
) {
revert TransferFailed();
}
IZRC20(zrc20Array[i]).approve(address(gateway), gasFee);
gateway.call(
receiverArray[i],
zrc20Array[i],
messages,
callOptions,
revertOptions
);
}
}
Comment on lines +44 to +72
Comment on lines +44 to +72

Check warning

Code scanning / Slither

Unused return Medium

Comment on lines +44 to +72
Comment on lines +44 to +72
Comment on lines +44 to +72

Check notice

Code scanning / Slither

Calls inside a loop Low

Comment on lines +44 to +72

Check notice

Code scanning / Slither

Calls inside a loop Low

Comment on lines +44 to +72
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Address potential security and gas optimization concerns in the multi-call implementation.

The current implementation has several areas that need attention:

  1. No validation for array length matching between receiverArray and zrc20Array
  2. Multiple external calls in a loop could lead to high gas costs
  3. No checks for array length limits

Consider implementing these improvements:

 function callMulti(
     bytes[] memory receiverArray,
     address[] memory zrc20Array,
     bytes calldata messages,
     CallOptions memory callOptions,
     RevertOptions memory revertOptions
 ) external {
+    if (receiverArray.length != zrc20Array.length) {
+        revert("Array lengths mismatch");
+    }
+    if (receiverArray.length == 0) {
+        revert("Empty arrays");
+    }
+    if (receiverArray.length > 10) {  // Adjust limit based on gas considerations
+        revert("Too many calls");
+    }
+
+    uint256[] memory gasFees = new uint256[](zrc20Array.length);
+    // Batch all checks first
+    for (uint256 i = 0; i < zrc20Array.length; i++) {
+        (, gasFees[i]) = IZRC20(zrc20Array[i])
+            .withdrawGasFeeWithGasLimit(callOptions.gasLimit);
+        if (!IZRC20(zrc20Array[i]).transferFrom(
+            msg.sender,
+            address(this),
+            gasFees[i]
+        )) {
+            revert TransferFailed();
+        }
+    }
+
+    // Then batch all approvals and calls
+    for (uint256 i = 0; i < zrc20Array.length; i++) {
+        IZRC20(zrc20Array[i]).approve(address(gateway), gasFees[i]);
         gateway.call(
             receiverArray[i],
             zrc20Array[i],
             messages,
             callOptions,
             revertOptions
         );
     }
 }
📝 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
function callMulti(
bytes[] memory receiverArray,
address[] memory zrc20Array,
bytes calldata messages,
CallOptions memory callOptions,
RevertOptions memory revertOptions
) external {
for (uint256 i = 0; i < zrc20Array.length; i++) {
(, uint256 gasFee) = IZRC20(zrc20Array[i])
.withdrawGasFeeWithGasLimit(callOptions.gasLimit);
if (
!IZRC20(zrc20Array[i]).transferFrom(
msg.sender,
address(this),
gasFee
)
) {
revert TransferFailed();
}
IZRC20(zrc20Array[i]).approve(address(gateway), gasFee);
gateway.call(
receiverArray[i],
zrc20Array[i],
messages,
callOptions,
revertOptions
);
}
}
function callMulti(
bytes[] memory receiverArray,
address[] memory zrc20Array,
bytes calldata messages,
CallOptions memory callOptions,
RevertOptions memory revertOptions
) external {
if (receiverArray.length != zrc20Array.length) {
revert("Array lengths mismatch");
}
if (receiverArray.length == 0) {
revert("Empty arrays");
}
if (receiverArray.length > 10) { // Adjust limit based on gas considerations
revert("Too many calls");
}
uint256[] memory gasFees = new uint256[](zrc20Array.length);
// Batch all checks first
for (uint256 i = 0; i < zrc20Array.length; i++) {
(, gasFees[i]) = IZRC20(zrc20Array[i])
.withdrawGasFeeWithGasLimit(callOptions.gasLimit);
if (!IZRC20(zrc20Array[i]).transferFrom(
msg.sender,
address(this),
gasFees[i]
)) {
revert TransferFailed();
}
}
// Then batch all approvals and calls
for (uint256 i = 0; i < zrc20Array.length; i++) {
IZRC20(zrc20Array[i]).approve(address(gateway), gasFees[i]);
gateway.call(
receiverArray[i],
zrc20Array[i],
messages,
callOptions,
revertOptions
);
}
}
🧰 Tools
🪛 GitHub Check: Slither

[warning] 44-72: Unused return
Universal.callMulti(bytes[],address[],bytes,CallOptions,RevertOptions) (contracts/Universal.sol#44-72) ignores return value by (None,gasFee) = IZRC20(zrc20Array[i]).withdrawGasFeeWithGasLimit(callOptions.gasLimit) (contracts/Universal.sol#52-53)


[warning] 44-72: Unused return
Universal.callMulti(bytes[],address[],bytes,CallOptions,RevertOptions) (contracts/Universal.sol#44-72) ignores return value by IZRC20(zrc20Array[i]).approve(address(gateway),gasFee) (contracts/Universal.sol#63)


[notice] 44-72: Calls inside a loop
Universal.callMulti(bytes[],address[],bytes,CallOptions,RevertOptions) (contracts/Universal.sol#44-72) has external calls inside a loop: (None,gasFee) = IZRC20(zrc20Array[i]).withdrawGasFeeWithGasLimit(callOptions.gasLimit) (contracts/Universal.sol#52-53)


[notice] 44-72: Calls inside a loop
Universal.callMulti(bytes[],address[],bytes,CallOptions,RevertOptions) (contracts/Universal.sol#44-72) has external calls inside a loop: gateway.call(receiverArray[i],zrc20Array[i],messages,callOptions,revertOptions) (contracts/Universal.sol#64-70)


[notice] 44-72: Calls inside a loop
Universal.callMulti(bytes[],address[],bytes,CallOptions,RevertOptions) (contracts/Universal.sol#44-72) has external calls inside a loop: ! IZRC20(zrc20Array[i]).transferFrom(msg.sender,address(this),gasFee) (contracts/Universal.sol#55-59)


[notice] 44-72: Calls inside a loop
Universal.callMulti(bytes[],address[],bytes,CallOptions,RevertOptions) (contracts/Universal.sol#44-72) has external calls inside a loop: IZRC20(zrc20Array[i]).approve(address(gateway),gasFee) (contracts/Universal.sol#63)


function withdraw(
bytes memory receiver,
uint256 amount,
Expand Down
22 changes: 8 additions & 14 deletions examples/call/hardhat.config.ts
Original file line number Diff line number Diff line change
@@ -1,22 +1,16 @@
import "./tasks/deploy";
import "./tasks/universalCall";
import "./tasks/connectedCall";
import "./tasks/connectedDeposit";
import "./tasks/connectedDepositAndCall";
import "./tasks/universalWithdraw";
import "./tasks/universalWithdrawAndCall";
import "@zetachain/localnet/tasks";
import "@nomicfoundation/hardhat-toolbox";
import { HardhatUserConfig } from "hardhat/config";
import * as dotenv from "dotenv";

import "./tasks";
import "@zetachain/localnet/tasks";
import "@zetachain/toolkit/tasks";
import { getHardhatConfig } from "@zetachain/toolkit/client";

import { getHardhatConfigNetworks } from "@zetachain/networks";
import { HardhatUserConfig } from "hardhat/config";
dotenv.config();

const config: HardhatUserConfig = {
networks: {
...getHardhatConfigNetworks(),
},
solidity: "0.8.26",
...getHardhatConfig({ accounts: [process.env.PRIVATE_KEY] }),
};
Comment on lines +10 to +13
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance environment variable validation.

The configuration directly uses process.env.PRIVATE_KEY without validation. Add checks to ensure the private key is properly set.

Add this validation before the config:

dotenv.config();

+if (!process.env.PRIVATE_KEY) {
+  throw new Error("PRIVATE_KEY environment variable is required");
+}
+
const config: HardhatUserConfig = {
📝 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
dotenv.config();
const config: HardhatUserConfig = {
networks: {
...getHardhatConfigNetworks(),
},
solidity: "0.8.26",
...getHardhatConfig({ accounts: [process.env.PRIVATE_KEY] }),
dotenv.config();
if (!process.env.PRIVATE_KEY) {
throw new Error("PRIVATE_KEY environment variable is required");
}
const config: HardhatUserConfig = {
...getHardhatConfig({ accounts: [process.env.PRIVATE_KEY] }),
🧰 Tools
🪛 GitHub Actions: Run Tests

[error] Compiler configuration needs to be updated to support Solidity versions 0.8.20, 0.8.21, and 0.8.26

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 compiler configuration to resolve pipeline failures.

The pipeline is failing due to missing Solidity compiler configuration. Add the compiler settings to support the required versions.

Apply this diff to resolve the compiler configuration:

const config: HardhatUserConfig = {
  ...getHardhatConfig({ accounts: [process.env.PRIVATE_KEY] }),
+ solidity: {
+   version: "0.8.21",
+   settings: {
+     optimizer: {
+       enabled: true,
+       runs: 200,
+     },
+   },
+ },
};

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

🧰 Tools
🪛 GitHub Actions: Run Tests

[error] Compiler configuration needs to be updated to support Solidity versions 0.8.20, 0.8.21, and 0.8.26


export default config;
6 changes: 4 additions & 2 deletions examples/call/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
"@typescript-eslint/eslint-plugin": "^5.59.9",
"@typescript-eslint/parser": "^5.59.9",
"@zetachain/localnet": "4.0.0-rc6",
"@zetachain/toolkit": "13.0.0-rc7",
"@zetachain/toolkit": "13.0.0-rc13",
"axios": "^1.3.6",
"chai": "^4.2.0",
"dotenv": "^16.0.3",
Expand Down Expand Up @@ -57,6 +57,8 @@
"@solana-developers/helpers": "^2.4.0",
"@solana/spl-memo": "^0.2.5",
"@solana/web3.js": "^1.95.8",
"@zetachain/protocol-contracts": "11.0.0-rc3"
"@zetachain/networks": "10.0.0-rc4",
"@zetachain/protocol-contracts": "11.0.0-rc3",
"@zetachain/protocol-contracts-solana": "2.0.0-rc1"
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Remove or document unused Solana dependency

The @zetachain/protocol-contracts-solana dependency appears to be unused in the codebase. Either:

  • Remove the dependency if it's not needed
  • Document its intended usage and implement the Solana integration
🔗 Analysis chain

Verify Solana contract integration requirements.

The addition of @zetachain/protocol-contracts-solana suggests new Solana integration capabilities. Ensure this dependency is required and properly documented in the project's README.

Run this script to check for Solana-related configuration and documentation:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for Solana configuration and documentation

# Check for Solana configuration files
echo "Checking for Solana configuration files:"
fd -e yaml -e json "solana" examples/call/

# Check for Solana documentation
echo -e "\nChecking for Solana documentation:"
rg -i "solana" examples/call/README.md || echo "No Solana documentation found in README"

Length of output: 346


Script:

#!/bin/bash
# Check for imports and usage of Solana contracts
echo "Checking for Solana contract imports and usage:"
rg -i "@zetachain/protocol-contracts-solana" examples/call/ -A 2

echo -e "\nChecking for Solana-related code patterns:"
rg -i "solana" examples/call/ --type ts --type js --type tsx --type jsx

Length of output: 1163

}
}
2 changes: 2 additions & 0 deletions examples/call/scripts/localnet.sh
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#!/bin/bash

set -e
set -x
set -o pipefail

if [ "$1" = "start" ]; then npx hardhat localnet --exit-on-error & sleep 10; fi

Expand Down
22 changes: 2 additions & 20 deletions examples/call/tasks/connectedCall.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,6 @@ const main = async (args: any, hre: HardhatRuntimeEnvironment) => {
const { ethers } = hre;
const [signer] = await ethers.getSigners();

const txOptions = {
gasPrice: args.txOptionsGasPrice,
gasLimit: args.txOptionsGasLimit,
};

const revertOptions = {
abortAddress: "0x0000000000000000000000000000000000000000", // not used
callOnRevert: args.callOnRevert,
Expand Down Expand Up @@ -54,8 +49,7 @@ const main = async (args: any, hre: HardhatRuntimeEnvironment) => {
const tx = await contract.call(
args.receiver,
encodedParameters,
revertOptions,
txOptions
revertOptions
);

await tx.wait();
Expand All @@ -68,18 +62,6 @@ task(
main
)
.addParam("contract", "The address of the deployed contract")
.addOptionalParam(
"txOptionsGasPrice",
"The gas price for the transaction",
10000000000,
types.int
)
.addOptionalParam(
"txOptionsGasLimit",
"The gas limit for the transaction",
7000000,
types.int
)
.addFlag("callOnRevert", "Whether to call on revert")
.addOptionalParam(
"revertAddress",
Expand All @@ -94,7 +76,7 @@ task(
.addOptionalParam(
"onRevertGasLimit",
"The gas limit for the revert transaction",
7000000,
500000,
types.int
)
.addParam("name", "The name of the contract", "Connected")
Expand Down
27 changes: 3 additions & 24 deletions examples/call/tasks/connectedDeposit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,6 @@ const main = async (args: any, hre: HardhatRuntimeEnvironment) => {
const { ethers } = hre;
const [signer] = await ethers.getSigners();

const txOptions = {
gasPrice: args.txOptionsGasPrice,
gasLimit: args.txOptionsGasLimit,
};

const revertOptions = {
abortAddress: "0x0000000000000000000000000000000000000000", // not used
callOnRevert: args.callOnRevert,
Expand Down Expand Up @@ -43,16 +38,12 @@ const main = async (args: any, hre: HardhatRuntimeEnvironment) => {
args.receiver,
value,
args.erc20,
revertOptions,
txOptions
revertOptions
);
} else {
const value = hre.ethers.utils.parseEther(args.amount);
const method = "deposit(address,(address,bool,address,bytes,uint256))";
tx = await contract[method](args.receiver, revertOptions, {
...txOptions,
value,
});
tx = await contract[method](args.receiver, revertOptions, { value });
}

await tx.wait();
Expand All @@ -61,18 +52,6 @@ const main = async (args: any, hre: HardhatRuntimeEnvironment) => {

task("connected-deposit", "Deposit tokens to ZetaChain", main)
.addParam("contract", "The address of the deployed contract")
.addOptionalParam(
"txOptionsGasPrice",
"The gas price for the transaction",
10000000000,
types.int
)
.addOptionalParam(
"txOptionsGasLimit",
"The gas limit for the transaction",
7000000,
types.int
)
.addFlag("callOnRevert", "Whether to call on revert")
.addOptionalParam(
"revertAddress",
Expand All @@ -87,7 +66,7 @@ task("connected-deposit", "Deposit tokens to ZetaChain", main)
.addOptionalParam(
"onRevertGasLimit",
"The gas limit for the revert transaction",
7000000,
500000,
types.int
)
.addOptionalParam("erc20", "The address of the ERC20 token to deposit")
Expand Down
27 changes: 3 additions & 24 deletions examples/call/tasks/connectedDepositAndCall.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,6 @@ const main = async (args: any, hre: HardhatRuntimeEnvironment) => {
const { ethers } = hre;
const [signer] = await ethers.getSigners();

const txOptions = {
gasPrice: args.txOptionsGasPrice,
gasLimit: args.txOptionsGasLimit,
};

const revertOptions = {
abortAddress: "0x0000000000000000000000000000000000000000", // not used
callOnRevert: args.callOnRevert,
Expand Down Expand Up @@ -72,8 +67,7 @@ const main = async (args: any, hre: HardhatRuntimeEnvironment) => {
value,
args.erc20,
encodedParameters,
revertOptions,
txOptions
revertOptions
);
} else {
const value = hre.ethers.utils.parseEther(args.amount);
Expand All @@ -83,10 +77,7 @@ const main = async (args: any, hre: HardhatRuntimeEnvironment) => {
args.receiver,
encodedParameters,
revertOptions,
{
...txOptions,
value,
}
{ value }
);
}

Expand All @@ -100,18 +91,6 @@ task(
main
)
.addParam("contract", "The address of the deployed contract")
.addOptionalParam(
"txOptionsGasPrice",
"The gas price for the transaction",
10000000000,
types.int
)
.addOptionalParam(
"txOptionsGasLimit",
"The gas limit for the transaction",
7000000,
types.int
)
.addFlag("callOnRevert", "Whether to call on revert")
.addOptionalParam(
"revertAddress",
Expand All @@ -126,7 +105,7 @@ task(
.addOptionalParam(
"onRevertGasLimit",
"The gas limit for the revert transaction",
7000000,
500000,
types.int
)
.addParam("amount", "The amount of tokens to deposit")
Expand Down
8 changes: 8 additions & 0 deletions examples/call/tasks/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import "./deploy";
import "./universalCall";
import "./universalCallMulti";
import "./connectedCall";
import "./connectedDeposit";
import "./connectedDepositAndCall";
import "./universalWithdraw";
import "./universalWithdrawAndCall";
28 changes: 5 additions & 23 deletions examples/call/tasks/universalCall.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,6 @@ const main = async (args: any, hre: HardhatRuntimeEnvironment) => {
const { ethers } = hre;
const [signer] = await ethers.getSigners();

const txOptions = {
gasPrice: args.txOptionsGasPrice,
gasLimit: args.txOptionsGasLimit,
};

const callOptions = {
isArbitraryCall: args.callOptionsIsArbitraryCall,
gasLimit: args.callOptionsGasLimit,
Expand Down Expand Up @@ -60,10 +55,10 @@ const main = async (args: any, hre: HardhatRuntimeEnvironment) => {
ethers.utils.concat([functionSignature, encodedParameters])
);

const gasLimit = hre.ethers.BigNumber.from(args.txOptionsGasLimit);
const gasLimit = hre.ethers.BigNumber.from(callOptions.gasLimit);
const zrc20 = new ethers.Contract(args.zrc20, ZRC20ABI.abi, signer);
const [, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit);
const zrc20TransferTx = await zrc20.approve(args.contract, gasFee, txOptions);
const zrc20TransferTx = await zrc20.approve(args.contract, gasFee);

await zrc20TransferTx.wait();

Expand All @@ -75,8 +70,7 @@ const main = async (args: any, hre: HardhatRuntimeEnvironment) => {
args.zrc20,
message,
callOptions,
revertOptions,
txOptions
revertOptions
);

await tx.wait();
Expand All @@ -90,18 +84,6 @@ task(
)
.addParam("contract", "The address of the deployed universal contract")
.addParam("zrc20", "The address of ZRC-20 to pay fees")
.addOptionalParam(
"txOptionsGasPrice",
"The gas price for the transaction",
10000000000,
types.int
)
.addOptionalParam(
"txOptionsGasLimit",
"The gas limit for the transaction",
7000000,
types.int
)
.addFlag("callOnRevert", "Whether to call on revert")
.addOptionalParam(
"revertAddress",
Expand All @@ -116,14 +98,14 @@ task(
.addOptionalParam(
"onRevertGasLimit",
"The gas limit for the revert transaction",
7000000,
500000,
types.int
)
.addFlag("callOptionsIsArbitraryCall", "Call any function")
.addOptionalParam(
"callOptionsGasLimit",
"The gas limit for the call",
7000000,
500000,
types.int
)
.addParam("function", `Function to call (example: "hello(string)")`)
Expand Down
Loading
Loading