Unsecuring ether transfer

Medium MM – 01 |  Resolved

In MegaNFT contract the function withdraw() use send method to transfer ether. Both send and transfer forward a stipend of 2300 gas, which is just enough to log an event at the receiving contract. In case the receiver requires a larger amount of gas to handle the reception of ether, call.value has to be used, as it forwards all remaining gas, unless specified otherwise with the help of the .gas() parameter.

Read more about this issue:

https://fravoll.github.io/solidity-patterns/secure_ether_transfer.html

Recommendation:

Use call to transfer ether.

(bool sent, ) = payable(msg.sender).call{value: amount}("");
require(sent, "Failed");

Useless require

Medium MM – 02 |  Resolved

In the contract, there is a case when the owner can mint a token and not pay the price. In this case, msg.value will be zero. But there is a require in the whenNotPausedAndValidSupply() modifier to check if the sent ether value by the user is above 0.  That's why the owner won't be able to call internalMint() function at any time.

Recommendation:

Consider removing the require in line 53.

Absent indexing in parameters of events

Low ML – 01 |  Resolved

In MegaNFT contract events are emitted without indexed parameter, which helps to log parameters in a transaction. The reason it is important to add indexed parameter to events is that it allows for more efficient and selective event filtering. By making certain parameters as indexed, Solidity can create an index of these values, making it easier and faster to search for events with the specific parameter value.

Recommendation:

Add indexed parameter in events.

event Withdraw(uint256 amount, address indexed addr);

Initialization with default values

Low ML – 02 |  Resolved

In MegaNFT in constructor contractPaused value set to false. This is a useless step that increases the use of gas, as a bool value by default is false.

Recommendation:

Remove the initialization of the variable from constructor.

Declaring default visibility

Low ML – 03 |  Resolved

In MegaNFT  the global values baseTokenURI and maxSupply are created without declaring their visibility. By default it is internal, but the best practice is to set visibility directly.

Recommendation:

Add visibility to baseTokenURI and maxSupply variables.

Require for inputs

Low ML – 04 |  Resolved

In MegaNFT contract most of the functions have absent to check input parameters for zero value.

Recommendation:

Add require for inputs.

Never reached require

Low ML – 05 |  Resolved

Require require(_tokenId != 0, "ERC721Metadata: token must not be 0") in tokenURI() function line 137 is never reached. The starting token id is 1 so it is impossible that the token with id 0 will ever exist. Its condition is checked in the require in  line 133. So that is why the mentioned previously require in line 137 is useless.

Recommendation:

Consider deleting the require in line 137.

Inappropriate function visibility

Informational  MI – 01 |  Resolved

In MegaNFT smart contract the functions mint(), mint_to(), internalMint(), pauseContract() and unpauseContract() have public visibility. The visibility can be changed to external, as it consumes less gas, and functions are not called outside the contract.

Recommendation:

Change functions visibility.

Re-Audit:

After re-audit, the function setBaseURI() still has public visibility, which can be changed to external.

Incorrect order of layout, functions

Informational  MI – 02 |  Resolved

In MegaNFT smart contract the order of functions and declared variables are not the same as in solidity documentation.

Recommendation:

Layouts, functions, and modifier order should be grouped according to Solidity documentation:

Layouts:

• interfaces;
• libraries;
• contracts.

Functions:

• constructor;
• receive;
• fallback;
• external;
• public;
• internal;
• private;
• view/pure.

Pay attention to the order of global variables and internal and public functions.

Re-Audit:

After re-audit there is still left one function setBaseURI(), which is public, between two external functions. It should be declared after all external and before internal.

Add Natspec

Informational  MI – 03 |  Resolved

MegaNFT smart contract has no explanation for functions and variables, which makes it harder to understand the contract’s logic.

Recommendation:

Add Natspec even for internal and private functions in all contracts that do not have it.

Optimization for requires

Informational  MI – 04 |  Resolved

In MegaNFT smart contract has mint and mint_to functions with the same requires. To reduce repetitive code, checking conditions can be moved to a separate function. At the same time this require require(!contractPaused, "Sale Paused!"); is used three times and can be moved to a modifier.

Recommendation:

Create a modifier for checking if the contract is paused, and add a separate function for requires.

Naming convention for mint_to()

Informational  MI – 05 |  Resolved

In MegaNFT smart contract the function mint_to() doesn’t follow naming conventions from official documentation:

https://docs.soliditylang.org/en/v0.8.17/style-guide.html#naming-styles

Recommendation:

Rename function to mintTo().

Floating pragma version

Informational  MI – 06 |  Resolved

In MegaNFT smart contract the current pragma Solidity directive is “^0.8.16”. It is recommended to specify a fixed compiler version to ensure that the bytecode produced does not vary between builds. This is especially important if you rely on bytecode-level verification of the code.

Recommendation:

Change the version to “0.8.16”.

Re-Audit:

The project team didn't change the pragma version or leave any comment about this issue. Hence, the issue is till unresolved.