Casinoverse Land
Casino Metaverse
Date
August 2022
Daniel Luca
@cleanunicorn
Andrei Simion
@andreiashu
- Details
- Issues Summary
- Executive summary
- Scope
- Recommendations
- Issues
- Whitelisted addresses can mint more than maxMintPerAddress
- Modifier isValidAmount should check for strict equality
- Some methods should be locked while the sale is active
- Use uint256 instead of uint8 in loops
- Simplify withdraw and withdrawAll by sending the funds directly to withdrawalRecipient
- Modifier isBalanceEnough is not needed
- Artifacts
- Sūrya's Description Report
- License
- Client Casino Metaverse
- Date August 2022
- Lead reviewer Daniel Luca (@cleanunicorn)
- Reviewers Daniel Luca (@cleanunicorn), Andrei Simion (@andreiashu)
- Repository: Casinoverse Land
- Commit hash
bf607afe5ff68b91990649567347f333bb27f6be - Technologies
- Solidity
- Node.JS
| SEVERITY | OPEN | CLOSED |
|---|---|---|
| Informational | 0 | 0 |
| Minor | 3 | 0 |
| Medium | 2 | 0 |
| Major | 1 | 0 |
This report represents the results of the engagement with Casino Metaverse to review Casinoverse Land.
The review was conducted over the course of 1 week from August the 1st to August the 5th, 2022. A total of 5 person-days were spent reviewing the code.
The initial review focused on the Casinoverse Land repository, identified by the commit hash bf607afe5ff68b91990649567347f333bb27f6be.
We focused on manually reviewing the codebase, searching for security issues such as, but not limited to, re-entrancy problems, transaction ordering, block timestamp dependency, exception handling, call stack depth limitation, integer overflow/underflow, self-destructible contracts, unsecured balance, use of origin, costly gas patterns, architectural problems, code readability.
Includes:
- code/contracts/Land.sol
We identified a few possible general improvements that are not security issues during the review, which will bring value to the developers and the community reviewing and using the product.
A good rule of thumb is to have 100% test coverage. This does not guarantee the lack of security problems, but it means that the desired functionality behaves as intended. The negative tests also bring a lot of value because not allowing some actions to happen is also part of the desired behavior.
Description
A user can call the method batchMint to mint a group of tokens. This method enforces a maximum number of tokens per whitelisted address using the modifier isNFTBalanceExceedsMaxMintPerAddress:
code/contracts/Land.sol#L419-L432
function batchMint(
uint256[] calldata _tokenIds,
string[] calldata _tokenURIs,
bytes32[] memory _merkleProof
)
external
payable
whenNotPaused
isCallerValid
isTotalMintedExceedsMaxSupply(_tokenIds.length)
isValidAmount(_tokenIds.length)
isNFTBalanceExceedsMaxMintPerAddress(msg.sender, _tokenIds.length)
isWhitelisted(_merkleProof)
{Each user is allowed to mint up to a maximum number of tokens defined as maxMintPerAddress as enforced by the modifier isNFTBalanceExceedsMaxMintPerAddress:
code/contracts/Land.sol#L308-L321
/**
* @dev Throws if caller balance + amount of nft to mint
* exceeds maxMintPerAddress
*
* @param _address address of minter
* @param _nftQty amount of nft to mint
*/
modifier isNFTBalanceExceedsMaxMintPerAddress(address _address, uint256 _nftQty) {
require(
(balanceOf(_address) + _nftQty) <= maxMintPerAddress,
"Max nft per address reached"
);
_;
}This modifier is added to both methods that mint tokens batchMint and mint.
However, the check uses the user's current balance, not how many tokens they minted. The user's balance can be modified by sending the tokens to a different address and calling batchmintMint again, minting up to maxMintPerAddress. The user can send the tokens to a different address and repeat the process.
Recommendation
Use a mapping that counts how many tokens were minted for each address. This way, the user has no option to decrease the number used when making the verification in isNFTBalanceExceedsMaxMintPerAddress.
A suggestion is to use the current modifier isNFTBalanceExceedsMaxMintPerAddress to increase the count and do the check.
// Define a mapping that counts how many tokens were minted per whitelisted address
mapping(address => uint256) mintedTokensPerWhitelistedAddress;
modifier isNFTBalanceExceedsMaxMintPerAddress(address _address, uint256 _nftQty) {
// Increment the number of minted tokens
mintedTokensPerWhitelistedAddress[_address] += _nftQty;
// Check if the total number of minted tokens is allowed
require(
mintedTokensPerWhitelistedAddress[_address] <= maxMintPerAddress,
"Max nft per address reached"
);
_;
}
Description
When a user calls the methods mint or batchMint, they need to provide the correct amount of ether in order to buy the tokens. The ether amount is checked in the modifier isValidAmount.
code/contracts/Land.sol#L239-L247
/**
* @dev Throws if amount is not enough
*
* @param _nftQty quantity of nft to mint
*/
modifier isValidAmount(uint256 _nftQty) {
require(msg.value >= (mintPrice * _nftQty), "Invalid Amount");
_;
}The check makes sure that the user sent the ether amount that is equal to or higher than the necessary one.
require(msg.value >= (mintPrice * _nftQty), "Invalid Amount");Recommendation
Change the check to make sure the amount sent is exactly equal to the required amount. Making this change will protect the users from making mistakes and sending too much ether into the contract.
Description
The owner has permission to change some important parameters while the sale is active:
The parameters controlled by these methods are very important for the users.
A few examples of why these methods should be inactive while the sale is happening:
- owner can change the token price
- owner can change the merkle root
- owner can change the token limit sale per address
The users assume the contract they are interacting with is safe. This includes coding bugs and centralization issues, such as the owner being able to change the price mid-auction or changing the token limit sale.
Recommendation
Do not allow the methods to be called while the sale is active.
Description
The EVM works with 256bit/32byte words. For smaller data types, further operations are performed to downscale from 256 bits to the required lower bites type, and therefore having uint8 as an iterator consumes more gas than keeping it to uint256.
pragma solidity ^0.6.12;
/**
* Show the difference in gas costs between a loop that uses a uint8 variable
* and one that uses uint256 variable.
*
* Both contracts compiled with `Enable Optimization` set to 200 runs.
*/
contract LoopUint8 {
address[] internal arr;
// 1st call; arr.length == 0: gas cost 42719
// 2nd call; arr.length == 1: gas cost 30322
// 3rd call; arr.length == 2: gas cost 32925
function add(address _new) public {
for (uint8 i = 0; i < arr.length; i++) {
if (arr[i] == _new) {
require(false, 'exists');
}
}
arr.push(_new);
}
}
contract LoopUint256 {
address[] internal arr;
// 1st call; arr.length == 0: gas cost 42713
// 2nd call; arr.length == 1: gas cost 30304
// 3rd call; arr.length == 2: gas cost 32895
function add(address _new) public {
for (uint256 i = 0; i < arr.length; i++) {
if (arr[i] == _new) {
require(false, 'exists');
}
}
arr.push(_new);
}
}Recommendation
Use uint256 for the loop iterators.
Description
code/contracts/Land.sol#L249-L260
/**
* @dev Throws if withdrawalRecipient is invalid
*
* @param _withdrawalRecipient withdrawalRecipient passed by caller
*/
modifier isValidRecipient(address _withdrawalRecipient) {
require(
withdrawalRecipient == _withdrawalRecipient,
"Invalid Recipient"
);
_;
}There's only 1 valid account that funds can be sent to. Thus, the argument _withdrawalRecipient is not required.
function withdraw(address _withdrawalRecipient, uint256 _amount) function withdraw(address _withdrawalRecipient, uint256 _amount)Recommendation
Remove modifiers, send directly to _withdrawalRecipient
Description
This is not needed since the tx will fail anyway if not enough ether is available.
code/contracts/Land.sol#L262-L270
/**
* @dev Throws if _amount exceeds contract balance
*
* @param _amount amount to withdraw
*/
modifier isBalanceEnough(uint256 _amount) {
require(_amount <= address(this).balance, "Not Enough Balance");
_;
}Recommendation
Remove modifier since this will not change how the contract functions.
Keep the modifier isBalanceNotZero since this will not create a "no-operation" transaction that doesn't transfer any ether.
Sūrya is a utility tool for smart contract systems. It provides a number of visual outputs and information about the structure of smart contracts. It also supports querying the function call graph in multiple ways to aid in the manual inspection and control flow analysis of contracts.
| File Name | SHA-1 Hash |
|---|---|
| ./code/contracts/Land.sol | a326ec1307afd3a679ceaddaca7ecba5b73d9733 |
| Contract | Type | Bases | ||
|---|---|---|---|---|
| └ | Function Name | Visibility | Mutability | Modifiers |
| Land | Implementation | ERC721, ERC2981, ReentrancyGuard, Ownable, Pausable | ||
| └ | Public ❗️ | 🛑 | ERC721 | |
| └ | setPreviewURI | External ❗️ | 🛑 | onlyOwner |
| └ | setMaxMintPerAddress | External ❗️ | 🛑 | onlyOwner |
| └ | mint | External ❗️ | 💵 | whenNotPaused isCallerValid isTotalMintedExceedsMaxSupply isValidAmount isNFTBalanceExceedsMaxMintPerAddress isWhitelisted |
| └ | batchMint | External ❗️ | 💵 | whenNotPaused isCallerValid isTotalMintedExceedsMaxSupply isValidAmount isNFTBalanceExceedsMaxMintPerAddress isWhitelisted |
| └ | ownerMint | External ❗️ | 🛑 | onlyOwner isTotalMintedExceedsMaxSupply isNFTBalanceExceedsMaxMintPerAddress |
| └ | _batchMintNFT | Private 🔐 | 🛑 | |
| └ | _mintNFT | Private 🔐 | 🛑 | isTokenIdValid |
| └ | setMintPrice | External ❗️ | 🛑 | onlyOwner |
| └ | setWithdrawalRecipient | External ❗️ | 🛑 | onlyOwner |
| └ | setBaseTokenURI | External ❗️ | 🛑 | onlyOwner |
| └ | setContractURI | External ❗️ | 🛑 | onlyOwner |
| └ | tokenURI | Public ❗️ | isTokenExist | |
| └ | setMerkleRoot | External ❗️ | 🛑 | onlyOwner |
| └ | pause | External ❗️ | 🛑 | onlyOwner |
| └ | unpause | External ❗️ | 🛑 | onlyOwner |
| └ | enableBaseURIMode | External ❗️ | 🛑 | onlyOwner |
| └ | disableBaseURIMode | External ❗️ | 🛑 | onlyOwner |
| └ | withdraw | External ❗️ | 🛑 | onlyOwner nonReentrant isValidRecipient isBalanceEnough |
| └ | withdrawAll | External ❗️ | 🛑 | onlyOwner isValidRecipient isBalanceNotZero |
| └ | _withdraw | Private 🔐 | 🛑 | |
| └ | getContractBalance | External ❗️ | onlyOwner | |
| └ | getMerkleRoot | External ❗️ | onlyOwner | |
| └ | getMaxMintPerAddress | External ❗️ | onlyOwner | |
| └ | isValidMerkleProof | Private 🔐 | ||
| └ | supportsInterface | Public ❗️ | NO❗️ | |
| └ | setRoyaltyInfo | Public ❗️ | 🛑 | onlyOwner |
| Symbol | Meaning |
|---|---|
| 🛑 | Function can modify state |
| 💵 | Function is payable |
$ npx surya describe code/contracts/Land.sol
npx: installed 64 in 7.099s
+ Land (ERC721, ERC2981, ReentrancyGuard, Ownable, Pausable)
- [Pub] <Constructor> #
- modifiers: ERC721
- [Ext] setPreviewURI #
- modifiers: onlyOwner
- [Ext] setMaxMintPerAddress #
- modifiers: onlyOwner
- [Ext] mint ($)
- modifiers: whenNotPaused,isCallerValid,isTotalMintedExceedsMaxSupply,isValidAmount,isNFTBalanceExceedsMaxMintPerAddress,isWhitelisted
- [Ext] batchMint ($)
- modifiers: whenNotPaused,isCallerValid,isTotalMintedExceedsMaxSupply,isValidAmount,isNFTBalanceExceedsMaxMintPerAddress,isWhitelisted
- [Ext] ownerMint #
- modifiers: onlyOwner,isTotalMintedExceedsMaxSupply,isNFTBalanceExceedsMaxMintPerAddress
- [Prv] _batchMintNFT #
- [Prv] _mintNFT #
- modifiers: isTokenIdValid
- [Ext] setMintPrice #
- modifiers: onlyOwner
- [Ext] setWithdrawalRecipient #
- modifiers: onlyOwner
- [Ext] setBaseTokenURI #
- modifiers: onlyOwner
- [Ext] setContractURI #
- modifiers: onlyOwner
- [Pub] tokenURI
- modifiers: isTokenExist
- [Ext] setMerkleRoot #
- modifiers: onlyOwner
- [Ext] pause #
- modifiers: onlyOwner
- [Ext] unpause #
- modifiers: onlyOwner
- [Ext] enableBaseURIMode #
- modifiers: onlyOwner
- [Ext] disableBaseURIMode #
- modifiers: onlyOwner
- [Ext] withdraw #
- modifiers: onlyOwner,nonReentrant,isValidRecipient,isBalanceEnough
- [Ext] withdrawAll #
- modifiers: onlyOwner,isValidRecipient,isBalanceNotZero
- [Prv] _withdraw #
- [Ext] getContractBalance
- modifiers: onlyOwner
- [Ext] getMerkleRoot
- modifiers: onlyOwner
- [Ext] getMaxMintPerAddress
- modifiers: onlyOwner
- [Prv] isValidMerkleProof
- [Pub] supportsInterface
- [Pub] setRoyaltyInfo #
- modifiers: onlyOwner
($) = payable function
# = non-constant function
No coverage available.
$ npm run test
> casinoverse-land@1.0.0 test /home/daniel/Development/github.com/akiratechhq/review-casinoverse-land-2022-08/code
> hardhat test
Land
deployment
✔ deployer is owner
✔ should return the expected name and symbol
✔ should return MAX_SUPPLY
✔ should return withdrawalRecipient address
✔ should return the initial base uri
✔ should return the uri suffix
✔ should return merkle root
✔ should return max mint per wallet address
setMintingPrice
✔ should return mintPrice
✔ should be able to set new minting price
✔ should emit event MintPriceChanged
setMerkleRoot
✔ should be able to set new merkle root
✔ should emit event MerkleRootChanged (38ms)
setWithdrawalRecipient
✔ sould be able to set new recipient address
✔ should emit event WithdrawalRecipientChanged
setMaxMintPerAddress
✔ sould be able to set new max mint per wallet address (154ms)
✔ sould emit event MaxMintPerAddressChanged
setBaseTokenURI
✔ sould be able to set new base token uri
✔ sould emit event BaseTokenURIChanged
getContractBalance
✔ sould return contract balance
pausing
✔ should return if contract is paused
✔ should be able to pause contract
✔ should be able to unpause contract
enable disable base uri mode
✔ should be able to enable base uri mode
✔ should be able to disable base uri mode
✔ should emit event BaseURIModeChanged
✔ should return base uri if base uri mode is true
✔ should return token specific uri if base uri mode is false (76ms)
✔ should return preview uri if token uri is empty
batch minting
✔ should be able to batch mint nft
✔ should emit event NFTBatchMint
✔ should be able to batch mint nft from using contract if owner (97ms)
✔ should not be able to batch mint if contract is paused, should be reverted with error message 'Pausable: paused' (42ms)
✔ duplicate token id, should be reverted with error message 'ERC721: token already minted'
✔ token id should not exceed max supply, should be reverted with error message 'Invalid token id'
✔ invalid mint price amount, should be reverted with error message 'Invalid Amount'
✔ maxed supply, should be reverted with error message 'Max Reached' (101ms)
✔ max mint per address, should be reverted with error message 'Max nft per address reached'
✔ not whitelisted, should be reverted with error message 'Not on whitelist'
✔ should not allow contract to call batch mint, should be reverted with error message 'Not allowed' (41ms)
owner minting
✔ should be able to owner mint nft
✔ should emit event NFTBatchMint
✔ duplicate token id, should be reverted with error message 'ERC721: token already minted'
✔ token id should not exceed max supply, should be reverted with error message 'Invalid token id'
✔ maxed supply, should be reverted with error message 'Max Reached' (86ms)
✔ max mint per address, should be reverted with error message 'Max nft per address reached'
minting
✔ should be able to mint nft
✔ should emit event NFTSingleMint
✔ should be able to mint nft using contract if owner (55ms)
✔ should be able to mint nft with different accs (316ms)
✔ token id should not exceed max supply, should be reverted with error message 'Invalid token id'
✔ token already minted, should be reverted with error message 'ERC721: token already minted'
✔ should not be able to mint if contract is paused, should be reverted with error message 'Pausable: paused'
✔ maxed supply, should be reverted with error message 'Max Reached' (89ms)
✔ invalid mint price amount, should be reverted with error message 'Invalid Amount'
✔ not whitelisted, should be reverted with error message 'Not on whitelist'
✔ max mint per address, should be reverted with error message 'Max nft per address reached' (77ms)
✔ should not allow contract to call mint, should be reverted with error message 'Not allowed'
withdrawAll
✔ should be able to withdraw all balance (79ms)
✔ should emit event Withdraw (75ms)
✔ invalid recipient, should be reverted with error message 'Invalid Recipient'
✔ invalid balance, should be reverted with error message 'No Balance'
✔ caller is not owner, should be reverted with errror message 'Ownable: caller is not the owner'
withdraw
✔ should be able to withdraw balance (49ms)
✔ should be able to withdraw balance (49ms)
✔ should be able to withdraw all balance (38ms)
✔ invalid recipient, should be reverted with error message 'Invalid Recipient'
✔ invalid balance, should be reverted with error message 'Not Enough Balance'
✔ caller is not owner, should be reverted with errror message 'Ownable: caller is not the owner'
royalty info
✔ should return current royalty info
✔ should be able to set new royalty info
✔ should emit event RoyaltyInfoChanged
setContractURI
✔ should be able to set new contract URI
✔ should emit event ContractURIChanged
setPreviewURI
✔ should be able to set new preview URI
✔ should emit event PreviewURIChanged
76 passing (9s)
This report falls under the terms described in the included LICENSE.
<script src="//cdnjs.cloudflare.com/ajax/libs/highlight.js/10.4.1/highlight.min.js"></script> <script>hljs.initHighlightingOnLoad();</script> <script type="text/javascript" src="https://cdn.jsdelivr.net/npm/highlightjs-solidity@1.0.20/solidity.min.js"></script> <script type="text/javascript"> hljs.registerLanguage('solidity', window.hljsDefineSolidity); hljs.initHighlightingOnLoad(); </script>