metaverse-user-dao---addendum II

There is a potential issue in the `depositMapping()` function of the `MudMiningEscrow` contract, where it could overwrite deposits made by users who called the `deposit()` function. This is because the function `depositMapping()` does not verify whether a user has already made a deposit at a particular index in the `_logbook`, but instead directly writes the mapped amounts from Ethereum to the `_logbook` storage. Consequently, the user's deposit amounts could be replaced, causing potential losses to users. ```solidity=80 if (_cursors[depositAddress].start == 0) { _cursors[depositAddress].start = orderIdArray[i]; _cursors[depositAddress].end = orderIdArray[i]; } else { if (orderIdArray[i] < _cursors[depositAddress].start) { _cursors[depositAddress].start = orderIdArray[i]; } if (orderIdArray[i] > _cursors[depositAddress].end) { _cursors[depositAddress].end = orderIdArray[i]; } } _logbook[depositAddress][orderIdArray[i]].startTime = starttimeArray[i]; _logbook[depositAddress][orderIdArray[i]].endTime = endtimeArray[i]; _logbook[depositAddress][orderIdArray[i]].amount = amountArray[i]; ```

In the `MudAngelRoundReleaseBank` contract, function `depositMapping()` violates the Check-Effects-Interactions pattern. This pattern is a best practice for writing secure smart contracts that involves performing all state changes before making any external function calls. ### External call(s) ```solidity=72 require(token.transferFrom(msg.sender, address(this), totalDepositToBeTransferred), "transferFrom failed!"); //check the return value, it should be true ``` ### State variables written after the call(s) ```solidity=73 _depositMappingTotal = _depositMappingTotal + totalDepositToBeTransferred;//to save gas ``` The same issue exists in the function `depositMapping()` of contract `MudABRoundReleaseBank()`. An external function call to the `token.transferFrom()` function is made before the state changes are made. This creates a potential vulnerability where an attacker could manipulate the contract's state during the external function call, which could lead to loss of funds or other malicious actions. To prevent such potential issue, it is important to ensure that all external function calls are made after the state changes have been made.

External calls are made inside a `for` loop. This might lead to a denial-of-service attack. If any of the calls fail, it will cause the entire loop to revert. ```solidity=71 require(token.transferFrom(msg.sender, address(this), balanceArray[i]), "transferFrom failed!"); //check the return value, it should be true ``` --- ```solidity=72 require(token.transferFrom(msg.sender, address(this), balanceArray[i]), "transferFrom failed!"); //check the return value, it should be true ``` Moreover, the code is to transfer `MUD` tokens from the sender to the contract multiple times inside a loop, this will also consume much more gas.

The `withdraw()` function logic restricts both the `admin` and `miningDappAddress` from withdrawing tokens from the current contract. However, the `poolInfoMapping()` function does not include a check to verify whether the `addressArray[i]` is equal to either the `admin` or `miningDappAddress`. This omission means that the `admin` and `miningDappAddress` could potentially be included in the withdrawal process, which may not be intended.

According to the code, variable `lastHalvingTime` was assigned an initial value during the construct process and reassigned a new value every 4 years. Therefore, the value of it always greater than 0. Condition `lastHalvingTime > 0` always true and hence that related check are meaningless.

The comment for the `depositMapping()` function of the `MudABRoundReleaseBank` contract in line#43 is misleading regarding the `icoBalanceArray` parameter. The comment states that it is an array of amounts of MUD coins received from the `Angel round`, which seems to be a leftover when copying from other code. ```solidity=39 /* only the contractor creator could deposit mapping to investor * map the MUD AB round from eth mainnet contract * parameters: * addressArray: Angel round investor addresses * icoBalanceArray: array of amount of MUD coin received from Angel round * balanceArray: array of amount of MUD coin mapped from eth mainnet contract * lasttimeArray: lasttime mapped from eth mainnet contract * return: (block time, total coins deposited in the contract) */ function depositMapping(address[] calldata addressArray, uint256[] calldata icoBalanceArray, uint256[] calldata balanceArray, uint[] calldata lasttimeArray) external returns (uint256, uint256){ require(msg.sender == admin, "Only admin can deposit."); require(!_depositMappingFinished, "Deposit Mapping finished!"); require(addressArray.length == balanceArray.length && icoBalanceArray.length == balanceArray.length && balanceArray.length == lasttimeArray.length, "Array length not match"); ``` However, based on the code implementation and context, it appears that the intended meaning of `icoBalanceArray` is the mapping amounts of the `AB round`, not the `Angel round`. This discrepancy between the comment and the actual usage of `icoBalanceArray` could potentially cause confusion for developers or auditors trying to understand the code.

There should always be events emitted in the sensitive functions that are controlled by centralization roles.

The **require** can be used to check for conditions and throw an exception if the condition is not met. It is better to provide a string message containing details about the error that will be passed back to the caller. ```solidity=147 require(balanceArray[i] > 0); ```

The issue with the current implementation is that although the owner of the token contract has the ability to freeze the token to prevent transfers, the token can still be burned even while it is frozen. This means that the total supply of the token can be decreased while it is still under frozen, which can impact the value of the token and the expectations of the token holders. If the token is frozen, the `_burn()` function should revert and not allow the tokens to be burned. This will ensure that the total supply of the token remains unchanged while it is under frozen and prevent any potential issues with token holders and the value of the token.

In the `checkBalance()` function of `MudABRoundReleaseBank` contract, the `require` statement in L#94 is unnecessary. ```solidity-94 require(block.timestamp > bank[addressToCheck].lastTime, "now time < lastTime"); ``` The `bank[addressToCheck].lastTime` is zero or a timestamp in the past.