MUMUBIT TOKEN security record

Update the value of `itemToken` will cause the old trades in the `trades` array fail to be processed due to the failure of transferring the NFTs.

The project design may not be compatible with non-standard ERC20 tokens, such as deflationary tokens or rebase tokens. The functions use `transferFrom()` to move funds from the sender to the recipient but fail to verify if the received token amount matches the transferred amount. This could pose an issue with fee-on-transfer tokens, where the post-transfer balance might be less than anticipated, leading to balance inconsistencies. There might be subsequent checks for a second transfer, but an attacker might exploit leftover funds (such as those accidentally sent by another user) to gain unjustified credit.

The `withdrawAllFunds()` function in the contract allows the withdrawal of project coins under the condition that all users have withdrawn their allocated tokens. This mechanism creates a potential deadlock where the withdrawal of project coins by the managing entity can be indefinitely blocked if even a single user fails to withdraw their tokens. This design inadvertently grants any user the power to prevent the withdrawal of project coins, posing a significant risk to lock the project tokens in the contract. ```solidity function withdrawAllFunds(address tokenAddress_, uint256 amount_) external nonReentrant notContract withdrawAddressOnly() { uint256 balance = IERC20(tokenAddress_).balanceOf(address(this)); require(balance >= amount_, "balance insufficent!"); require(investCoinAddress == tokenAddress_ || projectCoinAddress == tokenAddress_, "Coin Address Not Correct!"); require(block.timestamp - claimTimestamp >= 3 days, "Withdraw Fund Until 72 hours after Claim "); uint256 investAmount = investedAmount[investCoinAddress]; uint256 claimAmount = claimedAmount[projectCoinAddress]; if(tokenAddress_ == investCoinAddress){ refundedAmount[tokenAddress_] += amount_; emit IDOStatusChange(tokenAddress_, amount_, msg.sender, "Manager Withdraw invest Coins"); } else if(tokenAddress_ == projectCoinAddress){ require((investAmount * 10 ** decimals / projectCoinPrice - claimAmount == 0 ),"Some others not claimed, can not withdraw so many!"); projectCoinAmount[tokenAddress_] -= amount_; claimedAmount[tokenAddress_] += amount_; emit IDOStatusChange(tokenAddress_, amount_, msg.sender, "Manager Withdraw invest Coins"); } else revert("withDraw TokenAddress ERROR"); IERC20(tokenAddress_).safeTransfer(msg.sender, amount_); } ```

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; } ```