search

CallistoSecurity / Smart-contract-auditing

This is a working repo of @EthereumCommonwealth audits. We performed more than 400 security audits since 2018. Not even a single contract was hacked after our auditors approved the code. Accepting audit requests here.
https://audits.callisto.network/
2 stars 2 forks source link

Mint Manager v2 and ERC721 v2 Audit request #12

Closed zerongt9 closed 1 year ago

zerongt9 commented 1 year ago

Audit request

Here is two contract related

ERC721_nftPassV2.sol , a standard ERC721 with Royalty, blindbox function and limited supply MintManagerV2.sol , provide a whitelist mint(erc20 payment) method to minting a nftpass NFT, also there are a mint period and quota to limit the number of minting. Also allow switch to a public mint to welcome all user to minting with erc20 payment

Source code

https://github.com/technine-IT/live4well-smartcontractv2-for-audit

Payment plan

... Write [x] at the checkbox of the payment plan that suits your needs ...

Disclosure policy

... Do you want us to publish the report as it is or to notify you privately in case of finding critical mistakes? ...

... provide your conditions for publishing the report or leave only standard disclosure policy link ...

Standard disclosure policy.

Contact information (optional)

email: zero.ng@technine.io

Platform

polygon

yuriy77k commented 1 year ago

@zerongt9 You've added new functions and structures to the contract, so it's not eligible for free reaudit.

The audit fee with 50% re-audit discount is 1200 USDT. You may send USDT (ERC20 or BEP20) to: 0x6317c6944bd1cD3932d062cce39d7Fd602119529 (valid for Ethereum and Binance Smart Chain)

The estimated auditing time - is 7 days after payment.

zerongt9 commented 1 year ago

@yuriy77k can you please help to split it in TWO difference audit request to process.

we will pay the new audit request cost for this audit request (Mint Manager v2 and ERC721 v2 Audit request #12) and also like to submit a fix version ( Mint Manager and ERC721 Audit request #8 ) for free reaudit

yuriy77k commented 1 year ago

@zerongt9 Ok. In this case, the audit fee is 2400 USDT.

zerongt9 commented 1 year ago

@yuriy77k here is the payment tx hash https://etherscan.io/tx/0x6f672d9a92ff2f7f501f1a8ae899b20bce1e7e596e4c14538e25ead31467cc86

please help to arrange the audit

zerongt9 commented 1 year ago

@yuriy77k May I know the current status of audit?

chhajershrenik commented 1 year ago

@zerongt9 The audit has been completed, please wait for updates from @yuriy77k on final report.

yuriy77k commented 1 year ago

The high severity issue was found in the contract. The report was sent by email.

zerongt9 commented 1 year ago

@yuriy77k we have fixed the issue and please help to arrange the reaudit.

yuriy77k commented 1 year ago

Live4well V2 Security Audit Report (re-audit fixed contracts)

1. Summary

live4well-smartcontractv2-for-audit smart contract security audit report performed by Callisto Security Audit Department

2. In scope

Commit 4101aa7f1ae0f1880caba7f1bfd5f42266173a5d

3. Findings

In total, 0 issues were reported, including:

In total, 13 notes were reported, including:

3.1. SectionDetail and publicMintSection quotas can exceed MAX_SUPPLY.

Severity: owner privileges

Description

The functions initializeSectionInfo(), changeSectionQuota(), and changePublicMintQuota() do not check if the allowed number of tokens to be minted (quota) in SectionDetail and publicMintSection exceeds MAX_SUPPLY. The function publicMintERC20() would revert if there is still a quota availability and the amount of MAX_SUPPLY tokens are minted.

Code Snippet

Recommendation

The owner should take into account MAX_SUPPLY so that the sum of quotas does not exceed it.

Note The variable quota is redundant in the SectionDetail as each whitelisted user is only allowed to mint one token and the user is removed from the whitelist, the owner can control the number of tokens that can be minted by the SectionDetail by managing the addressWhitelist.

3.2. Owner Privileges

Severity: owner privileges

Description

The ERC721-nftpassV2.sol and MintManagerV2.sol contract inherits features of the OpenZeppelin AccessControl contract, allowing the administrator to manage administrators. This role can be abandoned, and it will result in blocking access to critical functions of the contract.

Recommendation

Since the owner has unlimited rights to do everything, the ownership must be transferred to a multi-sig contract. And also the addresses to whom the rights PRIVATE_MINT, MINT_ROLE are given must be trusted.

3.3. Tokens can be read URI even if Blind Box is not opened

Severity: minor observations

Description

The function tokenURI() returns blindboxURI if the Blind Box is not opened. But all data that are stored in the smart contract can be read from the blockchain. So, if your security model expects that baseURI_ or tokenURIOverride[tokenId] should stay unknown for users until you openBlindbox you shouldn't set it before that time.

Code snippet

3.4. Optimization Tips

Severity: minor observation

Description

  1. Resetting allowance to 0 before setting a new value makes sense only for protection from front-running attacks if another amount is already approved. Since it's the first approval, no sense to reset it.

Recommend to remove erc20Token.safeApprove(address(this), 0); from constructor.

Code snippet

  1. A function transferForm uses more gas than function transfer, so if needs to transfer from the address(this) better to use transfer(to, value) instead of transferFrom(address(this), to, value).

To reduce gas cost on contract deployment and use the function transferERC20Token() you should remove from the constructor:

            IERC20 erc20Token = IERC20(_erc20AddressList[i]);
            erc20Token.safeApprove(address(this), 0);
            erc20Token.safeApprove(address(this), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF);

And replace in the function transferERC20Token() 

        erc20Contract.safeTransferFrom(address(this), _to, _amount);

with

        erc20Contract.safeTransfer(_to, _amount);

Code snippet

  1. Use custom errors to optimize gas usage and reduce deployment costs.

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deployment cost, and it is difficult to use dynamic information in them.

3.5. Follow good coding practice

Severity: minor observation

Description

  1. Unlocked Pragma.

Contracts should be deployed using the same compiler version/flags with which they have been tested. Locking the floating pragma, i.e. by not using ^ in pragma solidity ^0.8.4, ensures that contracts do not accidentally get deployed using a compiler version with unfixed bugs.

  1. Missing docstrings.

The contracts in the code base lack documentation. This hinders reviewers’ understanding of the code’s intention, which is fundamental to correctly assessing not only security but also correctness. Additionally, docstrings improve readability and ease maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, the values returned, and the events emitted.

Consider thoroughly documenting all functions (and their parameters) that are part of the contracts’ public API. Functions implementing sensitive functionality, even if not public, should be documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).

  1. Missing test suite.

The contract is missing a test suite to validate and verify the behavior of the contract functionalities. Add tests are recommended to ensure that the contract functions and behaves as expected.

  1. Functions do not emit Events.

Function setBaseURI(), setTokenURI(), and openBlindbox() in the NFTPass contract and functions initializeSectionInfo(), changeSectionPeriod(), changeSectionQuota(), changeERC20Price(), changePublicMintERC20Price(), changePublicMintQuota(), addSectionWhitelist(), and removeSectionWhitelist() in the MintManager contract does not emit any events. Events are a way to log and notify external entities (such as user interfaces or other smart contracts) about specific occurrences within a smart contract. They serve as a mechanism for emitting and recording data onto the blockchain, making it transparent and easily accessible.

4. Security practices

5. Conclusion

The audited smart contract can be deployed. No security issues were found during the audit.

Users should pay attention to the contract owner's privileges.

It is recommended to adhere to the security practices described in pt. 4 of this report to ensure the contract's operability and prevent any issues that are not directly related to the code of this smart contract.

6. Previous audit report