Metacraft.

CertiK5 Sep 2022Files named in the report

CertiK reviewed MUMUBIT TOKEN (Metacraft.) on 5 Sep 2022: 36 findings, 5 critical; 19 marked fixed, 3 acknowledged, 14 open.

5 critical findings, fixed

Open the report Open MUMUBIT TOKEN’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings36

Statuses are the security firm’s own, from its report.

Severity	Open	Acknowledged	Fixed
Critical	0	0	5
High	8	0	1
Medium	0	3	1
Low	6	0	12

CriticalA Way To Steal `bnb`Fixed

The seller can finish the auction with different tokens in the function `finishAuction(string memory symbol_,uint256 _tokenId)`. Consider the following situation: 1. Bob creates an auction by account A. 2. Bob bids the auction with 100e18 `SHIBA` by account B. 3. At this time, Bob finishes the auction with the symbol `USDT`, and then 100e18 `USDT` will be paid to Bob. 4. Bob gets 100e18 `USDT` and pays 100e18 `SHIBA`. He pays less and gets more. ```solidity function finishAuction(string memory symbol_,uint256 _tokenId) public { ... ... IERC20(tokens[symbol_]).transferFrom(address(this),recipientAddr, receipientAmount); IERC20(tokens[symbol_]).transferFrom(address(this),auction.seller,sellerAmount); ... ... } ``` Round 2 **[Metacraft.pro]**: This function of finishAuction without a symbol of the chain, is set the bids with the default symbol of the chain(BNB in BSC, MATIC in Polygon and so on. **[Certik]**: The native token does not support the `transferFrom` function. Consider adding an native symbol and checking it in the function `finishAuction(uint256 _tokenId)`, for example: ```solidity function finishAuction(uint256 _tokenId) public payable { Auction memory auction = tokenIdToAuction[_tokenId]; require( msg.sender == auction.seller, "Should only be called by the seller" ); require(keccak256(abi.encodePacked("Native")) == keccak256(abi.encodePacked(auction.symbol)),"Symbol is not correct"); ... ... ```

CriticalPotential Risk For Money Lost In `bidAuction`Fixed

The users can bid the auction with different tokens in the function `bidAuction(string memory symbol_,uint256 price_,uint256 _tokenId)`. Consider the following situation: 1. Alice firstly bids the auction with 100e18 `USDT` and she is the highest bidder now. 2. Bob bids the auction with 200e18 `SHIBA` and 200e18 is greater than 100e18. Bob becomes the highest bidder and it will return 100e18 `SHIBA` to Alice. 3. Alice lost his 100e18 `USDT` and only gets 100e18 `SHIBA`. She almost lost all of the money. ```solidity function bidAuction(string memory symbol_,uint256 price_,uint256 _tokenId) public { .... .... if (price_ > highestBid) { ... ... if( highestBid > 0 ) { require(IERC20(tokens[symbol_]).approve(address(this), highestBid), "Approve has failed"); IERC20(tokens[symbol_]).transferFrom(address(this),highestBidder, highestBid); //payable(highestBidder).transfer(highestBid); } ```

CriticalPotential Risk For Money Lost In `cancelAuction`Fixed

The seller can cancel the auction with different tokens in the function `cancelAuction(string memory symbol_,uint256 _tokenId)`. Consider the following situation: 1. Bob creates an auction. 2. Alice firstly bids the auction with 100e18 `USDT`. 3. At this time, Bob cancels the auction with the symbol `SHIBA`, and then 100e18 `SHIBA` will be returned to Alice. 4. Alice pays 100e18 `USDT` and only gets 100e18 `SHIBA`. She pays more and gets less. ```solidity function cancelAuction(string memory symbol_,uint256 _tokenId) public { .... .... // if there are bids refund the last bid if( amount > 0 ) { require(IERC20(tokens[symbol_]).approve(address(this), amount), "Approve has failed"); IERC20(tokens[symbol_]).transferFrom(address(this),bidder,amount); // payable(bidder).transfer(amount); } .... .... } ```

CriticalPotential Risk For Stealing Money In `finishAuctiFixed

CriticalWrong check for the balance of `msg.sender`Fixed

```solidity require(payable(msg.sender).balance >=msg.value, "insufficient balance"); ``` The above check is meaningless since the native tokens have already been sent to this contract. For example, a user has 1 BNB and the price the user paid to bid the auction is also 1 BNB, then, in the above statement, the value of `payable(msg.sender)` is zero. As a result, the program is probably blocked.

HighSymbol Not CheckFixed

The given symbol is not checked in the function `executeTrade`.

MediumLack Of Access ControlFixed

The function `setBaseURI` can be called by anyone as it has no access restriction. This enables anyone to call this and update the URI info of the contract.

LowAuction Status Not Check In `finishAuction`Fixed

Auction status should be checked in the function `finishAuction`.

LowAuction Status Not Check In `isBidValid`Fixed

Auction status is not checked in the function `isBidValid`. In this case, biding a canceled auction is allowed.

LowImproper Usage of `public` and `external` TypeFixed

`public` functions that are never called by the contract could be declared as `external`. `external` functions are more efficient than `public` functions.

Show 9 more

LowMissing Check For `halted_`Fixed

In the function `transfer()` of `MetaCraftToken`, it is noted that the contract checks the variable `halted_` first and then does the transfer. But in the function `transferFrom()` of `MetaCraftToken`, it missing the check for `halted_`. As a result, users can invoke the `approve()` and `transferFrom()` functions to work around the forbid of the calling on the `transfer()` function.

LowRedundant CheckFixed

```solidity if(block.timestamp> auction.endTime) revert(); ``` The above check is already done in the function `isBidValid`.

LowSafeMath Not UsedFixed

SafeMath from OpenZeppelin is not used in the following functions which makes them possible for overflow/underflow and will lead to an inaccurate calculation result. ```solidity payable(trade.poster).transfer(msg.value-perf); ``` ```solidity require(IERC20(tokens[symbol_]).approve(trade.poster, price_-perf), "Approve has failed"); IERC20(tokens[symbol_]).transferFrom(msg.sender, trade.poster, price_-perf); ```

LowUnlocked compiler versionFixed

The contract has unlocked compiler version. An unlocked compiler version in the source code of the contract permits the user to compile it at or above a particular version. This, in turn, leads to differences in the generated bytecode between compilations due to different compiler versions. This can lead to an ambiguity when debugging as compiler specific bugs may occur in the codebase that would be hard to identify over a span of multiple compiler versions rather than a specific one.

LowUnused VariableFixed

The variable `currToken` is declared but never used or updated.

LowUnused VariableFixed

The variable `tradeCounter` and `currencyToken` are declared but never used or updated.

LowUsage of `transfer()` for sending EtherFixed

After [EIP-1884](https://eips.ethereum.org/EIPS/eip-1884) was included in the Istanbul hard fork, it is not recommended to use `.transfer()` or `.send()` for transferring ether as these functions have a hard-coded value for gas costs making them obsolete as they are forwarding a fixed amount of gas, specifically `2300`. This can cause issues in case the linked statements are meant to be able to transfer funds to other contracts instead of EOAs.

LowVariables that could be declared as `constant`Fixed

The linked variables could be declared as `constant` since these state variables are never modified.

LowVariables That Could Be Declared as `constant`Fixed

The linked variables could be declared as `constant` since these state variables are never modified.