Metacraft

In the contract `MCTPStake`, the role `owner` has authority over the following functions. - `pause()`: Used to pause the contract. - `unpause()`: Used to unpause the contract. - `setWithDuration()`: Used to change the status of `withDuration`. - `setWithdrawAddress()`: Used to create a pending transaction for setting `withdrawAddress`. - `addOwner()`: Used to create a pending transaction for adding a new `validOwner`. - `removeOwner()`: Used to create a pending transaction for removing a existed `validOwner`. In the contract `MCTPStake`, the role `owners` (`validOwner`) has authority over the following functions. - `signTransaction()`: Used to sign/execute the pending transactions of modifying the `withdrawAddress`. - `signUserAdd()`: Used to sign/execute the pending transactions of adding a new `validOwner`. - `signUserDel()`: Used to sign/execute the pending transactions of removing a new `validOwner`. In the contract `MCTPStake`, the role `withdrawAddress` has authority over the following function. - `withdrawFunds()`: Used to withdraw the specified itemToken in the `MCTPStake` contract, but it has a protection to ensure that the remaining token balance should equal or more than the total stake amount of users. Any compromise to the `owner` account or `withdrawAddress` account may allow the hacker to take advantage of this authority, change the status of `withDuration`, pause/unpause the staking operation and withdraw the tokens for reward in the contract. --- In the contract `MetaCraftVNode` the role `owner` has authority over the functions shown in the list below. - `addOwner()`: Initiates a pending transaction to add a new `validOwner`. - `removeOwner()`: Initiates a pending transaction to remove an existing `validOwner`. - `setParameters()`: Initiates a pending transaction to change critical operational parameters, including locking and halting the contract's functionalities. - `setSalesAddress()`: Initiates a pending transaction to update the `feeAddress`. Any compromise to the `owner` account may allow the hacker to take advantage of this authority and initialize a pending transaction. The role `_validOwners` has authority over the functions shown in the list below. - `signSetTransaction()`: Used to sign pending transactions initiated by the `owner`. - `executeSetTransaction()`: Executes pending transactions once the required signatures are collected. Any compromise to the `_validOwners` account may allow the hacker to take advantage of this authority and disrupt the normal execution of critical pending transactions in the contract. The role `publicKey` has the authority to sign the signature for minting Node ERC721 token. - `buyNode`: Used to mint the ERC721 token. Only the `publicKey` can sign the signatures for minting Node ERC721 tokens, acting as the sole authority to authenticate mint requests. Any compromise to the `publicKey` account may allow the hacker to perform the unauthorized minting of tokens.

The role `withdrawAddress` has the authority to withdraw tokens from the contract. There are no controls over when this function can be run and the amount that can be withdrawn. ```solidity=174 function withdrawFunds(address _itemToken) external whenNotPaused withdrawAddressOnly() { IERC20(_itemToken).safeTransfer(msg.sender, IERC20(_itemToken).balanceOf(address(this))); emit StakeEvents(block.timestamp,msg.sender, "withdrawFunds Coin"); } modifier withdrawAddressOnly() { require(msg.sender == withdrawAddress, "only withdrawer can call this"); _; } ``` A hacker can take advantage of this function to withdraw all assets within the contract.

When users stake or distribute items, they specify the `itemToken`. Therefore, we need to check the following points with the team: 1. If there is documentation specifying the `itemToken` type when users stake or claim? Using arbitrary tokens may pose potential risks, such as **Unchecked ERC-20 `transfer()`/`transferFrom()` Call** and **Potential Reentrancy Issue**. 2. The source of the reward token sent to the user when staking or unstaking. If there is no sufficient deposited `itemToken` in the contract, this scenario creates a situation where the payouts for the initial users (like User A) are dependent on the deposits made by the newer users (like User B). If the inflow of new stakes ceases or slows, the contract will not have enough tokens to pay out the rewards or return the stakes, characterizing a Ponzi scheme dynamic.

The project design may not be compatible with non-standard ERC20 tokens, such as deflationary tokens or rebase tokens.

The `unStake()` function and `reStake()` function impose a restriction that prevents its execution by contracts, however, such a requirement is not present in the `openStake()` function. Consequently, contracts engaging in staking might encounter difficulties when attempting to unstake or restake their staked tokens. ```solidity function openStake( address _itemToken, uint256 _tokenAmount, uint256 _duration ) public whenNotPaused virtual{ ... } function unStake(address _itemToken,uint256 _tokenAmount,uint256 _duration) public whenNotPaused noReentrancy //anti re-entrancy notContract(msg.sender) //anti contract address virtual{ ... } ```

One or more parameters with `memory` data location are never modified in their functions and those functions are never called internally within the contract. Thus, their data location can be changed to `calldata` to avoid the gas consumption copying from calldata to memory. ```solidity=398 function setBaseURI(string memory baseURI_) external { _baseURIextended = baseURI_; } ``` - `setBaseURI` has memory location parameters: `baseURI_`. ```solidity=402 function _setTokenURI(uint256 tokenId, string memory _tokenURI) internal virtual //override(ERC721URIStorage) { require(tokenId <= _tokenIds.current(), "ERC721Metadata: URI set of nonexistent token"); _tokenURIs[tokenId] = _tokenURI; } ``` - `_setTokenURI` has memory location parameters: `tokenURI_`. ```solidity=433 function buyNode( address _to, string memory tokenURI_, address parentAddress //uint256 tokenPrice ){ ... } ``` - `buyNode()` has memory location parameters: `tokenURI_`.

The `tokenDecimals` variable is used to calculate the mint price for `buyNode()`. However, there is no consistency check between `tokenDecimals` and the provided ERC20 token's decimals during initialization, which may result in an incorrect mint fee if they are misaligned. ```solidity ERC20Token = IERC20(_erc20Token); tokenDecimals = _decimals; ``` --- In the `MCTPStakeNew` contract, the `addOwner` and `removeOwner` never checks if the address to be added is already an owner, nor does it check if the address to be removed is truly an existing owner. It only ensures that the `_owner` is not a zero address. If an address that is already on the owner's list is added, the contract may contain inconsistent or duplicated data. ```solidity function addOwner(address _owner) public { ... require(_owner != address(0),"Zero Address Error!"); ... } function removeOwner(address _owner) public { ... require(_owner != address(0),"Zero Address Error!"); ... } ```

According to the team's message, the team plans to upgrade the contract in the future. The team may need to redeploy the `MCTPvNode` contract to upgrade it since the contract itself is not upgradeable. However, redeploying the contract may introduce new attack vectors. The current signature design in the `MCTPvNode` contract is not properly verified with the current chain ID and the address of the contract, thus allowing attackers to perform replay attacks across chains or contracts (from the old version to the new version). Especially if the team plans to deploy contracts on multiple chains or if the team has deployed multiple versions of contracts, the signature and message hash can be reused in the future.

To optimize the `MetaCraftVNode` contract for handling administrative actions like adding and removing owners, setting parameters, and updating addresses, we can utilize a generic transaction mechanism. This contract can streamline the process by using a single structure to manage different types of transactions, reducing redundancy and enhancing maintainability. ### Administrative Functions Ensure that administrative functions such as `setOwner`, `setParams`, and similar functions can only be executed by `address(this)`. ```solidity modifier onlySelf() { require(msg.sender == address(this), "Unauthorized: can only be called by the contract itself"); _; } function setOwner(address owner, uint8 status) public onlySelf { _validOwners[owner] = status; } function setParameters(/* Parameters */) public onlySelf { // Implementation for setting parameters } ``` ### Data Structures: - `TransactionType`: An enum to categorize transaction types. - `Transaction`: A struct to hold transaction details, including type, data payload, signature count, and execution status. ```solidity enum TransactionType { AddOwner, RemoveOwner, SetParameter, SetSalesAddress } struct Transaction { TransactionType type; bytes data; uint8 signatureCount; uint256 timestamp; bool executed; } ``` ### Functions - **Transaction Creation**: Function to add transactions with data validation. - **Signing Transactions**: Function for authorized owners to sign pending transactions. - **Executing Transactions**: Function to execute signed transactions using dynamic method invocation. ```solidity mapping(uint => Transaction) private _transactions; uint[] private _pendingTransactions; mapping(uint => mapping(address => bool)) private signatures; modifier notExecuted(uint transactionId) { require(!_transactions[transactionId].executed, "Transaction already executed"); _; } function addTransaction(TransactionType _type, bytes memory _data) public onlyValidOwner{ ... } function signTransaction(uint transactionId) public onlyValidOwner{ } function executeTransaction(uint transactionId) public onlyValidOwner notExecuted(transactionId){ ... } ``` ### Security Measures: - **Function Selector Verification**: Before adding a transaction, verify that the function selector matches the expected function for the transaction type. - **Execution Control**: Ensure that transactions can only be executed once they have the required number of signatures and are verified for correct method invocation. ```solidity function addTransaction(TransactionType _type, bytes memory _data) public onlyValidOwner { // Ensure data is at least 4 bytes long require(_data.length >= 4, "Data too short for a valid function selector"); require(isValidFunctionSelector(_type, bytes4(_data)), "Invalid function selector for transaction type"); ... } function executeTransaction(uint transactionId) onlyValidOwner notExecuted(transactionId) { ... (bool success,) = address(this).call(_transactions[transactionId].data); require(success, "Transaction execution failed"); ... } function isValidFunctionSelector(TransactionType _type, bytes4 selector) private pure returns (bool) { if (_type == TransactionType.AddOwner && selector == this.setOwner.selector) return true; if (_type == TransactionType.RemoveOwner && selector == this.setOwner.selector) return true; if (_type == TransactionType.SetParameter && selector == this.setParameters.selector) return true; if (_type == TransactionType.SetSalesAddress && selector == this.setSalesAddress.selector) return true; return false; } ```

The `timestamp` of a transaction is updated each time a valid owner signs the transaction. This is observed in functions like `signTransaction`, `signUserAdd`, and `signUserDel` where after a signature, the transaction's timestamp is set to the current block time (`transaction.timestamp = block.timestamp`). Our understanding is that if a transaction does not accumulate enough signatures (reaching the `MIN_SIGNATURES` threshold) within 96 hours, it should be considered expired and deleted, making room for a new transaction. However, given the timestamp refresh mechanism on each signature, the real pending time (the time a transaction remains in a pending state without reaching the minimum required signatures) can indeed exceed the intended 96 hours. This pending time would reset with each new signature, potentially leading to scenarios where a transaction remains pending for considerably longer than intended, especially if signatures are spaced out over time.