Note: although there is another discussion on whether to change the implementation to mapping, I'm posting this issue in case you decide to stay with the array.

At the moment, when minting, this is the whitelist process:

        // For whitelist only collections
        if (_metaData.whitelistOnly) {
            require(isWhitelistWallet(msg.sender), "NOT_IN_WTHIELIST");

            _removeWhitelist(msg.sender); // decrease allowance by 1
        }

This means the contract will iterate once over the whitelist array in isWhitelistWallet, and then iterate again on _removeWhitelist.
While the second iteration will be cheaper, it will still be 100 gas for every address that is read from the whitelist. So address #200 will have to pay unnecessary 20000 gas.

Instead of doing this, you can skip calling isWhitelistWallet, and just call _removeWhitelist, and change it so it will revert if the address it not found.

So you can change the _removeWhitelist from this:

    function _removeWhitelist(address wallet) private {
        for (uint256 i = 0; i < _whitelist.length; i++) {
            if (_whitelist[i] == wallet) {
                _whitelist[i] = _whitelist[_whitelist.length - 1]; // put the last element into the delete index
                _whitelist.pop(); // delete the last element to decrease array length;

                break; // delete the first matching one and stop
            }
        }
    }

To this:

    function _removeWhitelist(address wallet) private {
        for (uint256 i = 0; i < _whitelist.length; i++) {
            if (_whitelist[i] == wallet) {
                _whitelist[i] = _whitelist[_whitelist.length - 1]; // put the last element into the delete index
                _whitelist.pop(); // delete the last element to decrease array length;

                return; // delete the first matching one and stop
            }
        }
        require(false, "NOT_IN_WHITELIST")
    }

And change the minting whitelist block to this:

        // For whitelist only collections
        if (_metaData.whitelistOnly) {
            _removeWhitelist(msg.sender); // decrease allowance by 1, revert if not in whitelist
        }

And this will save you O(n) of iteration on the list.

Note: removeWhitelist also uses _removeWhitelist, so make sure you're ok with it reverting if the owner supplies an address which is not whitelisted. If not, just change removeWhitelist implementation to not revert if not found... You probably get the picture.

Description

Line 119: Mint the NFT, and then call the _safeMint function.
Line 124: Increment the tokenId

_safeMint calls _checkOnERC721Received which has a re-entrancy opportunity.

Impact

When an attacker has an opportunity to re-enter the NFT contract, the attacker can emit the Mint events twice with the same tokenId, by following steps:

1. Attacker call the mint with to as an attacker's contract address.
2. NFT contract calls onERC721Received function of the attacker's contract. (tokenID = X)
3. Attacker's contract calls burn with tokenID = X.
4. Attacker's contract calls the mint function again. (tokenID = X)
5. Since the incrementation of the tokenId executes after the _safeMint function returns, so attacker can mint twice with the same tokenId.

Recommendation

Add a nonReentrant modifier on the Mint/Burn functions. (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol)

cc/ @Jinmo

contract DixelClubV2NFT is ERC721Enumerable, Ownable, SVGGenerator {
	...
	address[] private _whitelist;
	...
}

The whitelist of the DixelClubV2NFT contract is implemented as an Array.

Each function uses the following computational cost.

• addWhitelist() is O(1)
• removeWhitelist() O(N)
• isWhitelistWallet() O(N)

How about using mapping to save gas cost?

mapping(addres => bool) public whitelist;

• add O(1)
• remove O(1)
• isWhitelistWallet O(1)

Please let me know if there's anything I didn't think of.

Hi, can't _initializedAt at

I saw it's used very little, and I don't get clearly its function. Can you clarify it?

Description

Factory calls init when the user creates the collection.

init function transfers its ownership to the owner which is the first argument of the function (msg.sender of the factory).

There is missing check of the owner should not be the contract.

Impact

If the owner is a contract, the owner can control the possibility that user can mint or can not mint by reverting on the recieve function.

The owner can initialize maxSupply only once which limits the maximum NFT counts.

Nevertheless the maxSupply sets properly, reverting the owner contract allows the owner can control the miting possibility whenever the owner want.

Recommendation

There is a _transferOwnership function which has an opportutnity to transfer the ownership to the contract address. So the design should be changed to the owner claims the mintingCost instead of sending the mintingCost to the owner diretly. Also, it could reduce the gas when the user mints the NFT.

cc/ @Jinmo

Ref. : https://medium.com/@ItsCuzzo/using-merkle-trees-for-nft-whitelists-523b58ada3f9

Don't use for loop for whitelisting.
It is not efficient, and when the number of whilelist increases, the for loop must be reverted.

DixelClubV2NFT has an ether receive function:

    receive() external payable {}

It is not necessary for the functioning of the contract - as ether should only be sent during a mint call.
The receive function will enable sending ether to the contract outside of the mint function.
This ether will be stuck in the contract, as there's no way to pull ether.
While this is unlikely to happen, this means that if somebody accidently sends ether (/native token) to the contract without calling mint, his funds would be lost.

Since the contract does not need this method, and it might cause harm, I suggest removing this receive function.

test

AS-IS)

function updateBeneficiary(address newAddress, uint256 newCreationFee, uint256 newMintingFee) external onlyOwner {
  if(newAddress == address(0)) revert DixelClubV2Factory__ZeroAddress();
  if(newMintingFee > FRICTION_BASE) revert DixelClubV2Factory__InvalidFee();
  
  beneficiary = newAddress;
  mintingFee = newMintingFee;
  creationFee = newCreationFee;
}

if exist case that update only one of the beneficiary or fees, how about split each parameters to functions?
It seems that human error can be reduced.

TO-BE)

function updateBeneficiary(address newAddress) external onlyOwner {
  if(newAddress == address(0)) revert DixelClubV2Factory__ZeroAddress();
  beneficiary = newAddress;
}
    
function updateMintingFee(uint256 newMintingFee) external onlyOwner {
  if(newMintingFee > FRICTION_BASE) revert DixelClubV2Factory__InvalidFee();
  mintingFee = newMintingFee;
}
    
function updateCreationFee(uint256 newCreationFee) external onlyOwner {
  creationFee = newCreationFee;
}

On createCollection() method and updateDescription() method, they filter double quote character (0x22) to make it impossible to return invalid JSON string on tokenJSON() method and contractJSON() method.

function createCollection(
    string memory name,
    string memory symbol,
    Shared.MetaData memory metaData,
    uint24[PALETTE_SIZE] memory palette,
    uint8[TOTAL_PIXEL_COUNT] memory pixels
) external payable returns (address payable) {
    ...
    // Validate `symbol`, `name` and `description` to ensure generateJSON() creates a valid JSON
    require(!StringUtils.contains(name, 0x22), "NAME_CONTAINS_MALICIOUS_CHARACTER");
    require(!StringUtils.contains(symbol, 0x22), "SYMBOL_CONTAINS_MALICIOUS_CHARACTER");
    require(!StringUtils.contains(metaData.description, 0x22), "DESCRIPTION_CONTAINS_MALICIOUS_CHARACTER");
    ...
}

function updateDescription(string memory description) external onlyOwner {
    require(bytes(description).length <= 1000, "DESCRIPTION_TOO_LONG"); // ~900 gas per character
    require(!StringUtils.contains(description, 0x22), "DESCRIPTION_CONTAINS_MALICIOUS_CHARACTER");

    _metaData.description = description;
}

function tokenJSON(uint256 tokenId) public view checkTokenExists(tokenId) returns (string memory) {
    return string(abi.encodePacked(
        '{"name":"',
        _symbol, ' #', ColorUtils.uint2str(tokenId),
        '","description":"',
        _metaData.description,
        '","external_url":"https://dixel.club/collection/',
        ColorUtils.uint2str(block.chainid), '/', StringUtils.address2str(address(this)), '/', ColorUtils.uint2str(tokenId),
        '","image":"',
        generateBase64SVG(tokenId),
        '"}'
    ));
}

function contractJSON() public view returns (string memory) {
    return string(abi.encodePacked(
        '{"name":"',
        _name,
        '","description":"',
        _metaData.description,
        '","image":"',
        generateBase64SVG(0),
        '","external_link":"https://dixel.club/collection/',
        ColorUtils.uint2str(block.chainid), '/', StringUtils.address2str(address(this)),
        '","seller_fee_basis_points":"',
        ColorUtils.uint2str(_metaData.royaltyFriction),
        '","fee_recipient":"',
        StringUtils.address2str(owner()),
        '"}'
    ));
}

However, backslash character (0x5c) can also be used to create invalid JSON string by escaping existing double quote character.
For example, when _metadata.description is A\ ([0x41, 0x5c]), tokenJSON() method would return like:

{"name":"symbol#0","description":"A\","external_url":"https://dixel.club/collection/0/0x0000000000000000000000000000000000000000/0","image":"data:image/svg+xml;base64,"}

which is not a valid JSON string.

Therefore, I suggest adding the validation code to filter backslash character (0x5c) like as below.

function createCollection(
    string memory name,
    string memory symbol,
    Shared.MetaData memory metaData,
    uint24[PALETTE_SIZE] memory palette,
    uint8[TOTAL_PIXEL_COUNT] memory pixels
) external payable returns (address payable) {
    ...
    // Validate `symbol`, `name` and `description` to ensure generateJSON() creates a valid JSON
    require(!StringUtils.contains(name, 0x22) && !StringUtils.contains(name, 0x5c), "NAME_CONTAINS_MALICIOUS_CHARACTER");
    require(!StringUtils.contains(symbol, 0x22) && !StringUtils.contains(symbol, 0x5c), "SYMBOL_CONTAINS_MALICIOUS_CHARACTER");
    require(!StringUtils.contains(metaData.description, 0x22) && !StringUtils.contains(metaData.description, 0x5c), "DESCRIPTION_CONTAINS_MALICIOUS_CHARACTER");
    ...
}

function updateDescription(string memory description) external onlyOwner {
    require(bytes(description).length <= 1000, "DESCRIPTION_TOO_LONG"); // ~900 gas per character
    require(!StringUtils.contains(description, 0x22) && !StringUtils.contains(description, 0x5c), "DESCRIPTION_CONTAINS_MALICIOUS_CHARACTER");

    _metaData.description = description;
}

Description

Impacts

Attackers can create the NFT contract as if minting had started in the past. (e.g. Setting metaData.mintingBeginsFrom to zero means minting had started on the 1970-01-01)

Recommendation

In createCollection() function, If the metaData.mintingBeginsFrom less than block.timestamp, it should be replaced with block.timestamp.

Example patch

    function createCollection(
        string memory name,
        string memory symbol,
        Shared.MetaData memory metaData,
        uint24[PALETTE_SIZE] memory palette,
        uint8[TOTAL_PIXEL_COUNT] memory pixels
    ) external payable returns (address payable) {
        
+      // Neutralize minting starts date.
+      if (metaData.mintingBeginsFrom < block.timestamp)
+          metaData.mintingBeginsFrom = block.timestamp

        address payable nftAddress = _createClone(nftImplementation);
        DixelClubV2NFT newNFT = DixelClubV2NFT(nftAddress);
        newNFT.init(msg.sender, name, symbol, metaData, palette, pixels);

The smallest uint type in solidity is uint8, and at the moment this is what Dixel2 uses to save each pixel's value.
This gives us 256 values to save in each uint8; but we need only 16, as the pallete size is 16.

This means that by using bit operations, we can utilize each uint8 to save 2 pixels.
For example, pixels[0] would be the colors of both the first and second pixel. The rightmost 4 bits would be the first pixel, and the leftmost 4 bits would be the second pixel.

While this will add some complexity to the front end and back end in creating/reading the pixels array, this saves 360,000 gas upon collection creation - a significant amount. (It is up to you to decide whether it is worth it.)

Basically, the formulas for getting the pixel values at _generateSVG (the only place that reads the values) will be:

uint256 prev = pixels[y * CANVAS_SIZE / 2] & 15;
uint256 current = (pixels[y * CANVAS_SIZE/2 + x/2] >> 4*(x%2)) & 15;

In a few hours I will try to create a patch/PR.

Description

There is a mint event on the mint function, but there isn't on the burn function.

Impact

The operator could not track the on-chain events easilty.

Recommendation

Add a Burn event and emit it as same as the mint function for better handling on the off-chain side.

cc/ @Jinmo

The SVGGenerator generates images which are 25 pixels wide. You can see it for example in test-svg.svg - we have the following SVG path:

M0 1h25

This tells the SVG to draw 25 pixels - but the size of the canvas is only 24 pixels.
I believe the culprit is this line, which is executed on the last pixel of the line:

                    paths[current] = string(abi.encodePacked(paths[current], "h", ColorUtils.uint2str(width + 1)));

If we change it to draw only width instead of width + 1, the resulting canvas would be 24 pixels wide, as it should.

At the moment it is drawing the last pixel of each row twice.

To see why - let's say the last pixel has changed in color. In the current implementation, this is the code:

                if (prev == current) {
                    width++;
                } else {
                    paths[prev] = string(abi.encodePacked(paths[prev], "h", ColorUtils.uint2str(width)));
                    width = 1;

                    paths[current] = string(abi.encodePacked(paths[current], "M", ColorUtils.uint2str(x), " ", ColorUtils.uint2str(y)));
                }

                if (x == CANVAS_SIZE - 1) {
                    paths[current] = string(abi.encodePacked(paths[current], "h", ColorUtils.uint2str(width + 1)));
                }

So the "else" block would set the width to 1, and then the if (x == CANVAS_SIZE - 1) block would increment the width to 2 - but the real width should be only 1.
This is why the width should be only width and not width + 1.

It is now my bed time, tomorrow if I'll have time I'll try adding some tests for this 🙂

AS-IS

function getWhitelistAllowanceLeft(address wallet) external view returns (uint256 allowance) {
  uint256 length = _whitelist.length; // gas saving
  for (uint256 i; i != length;) {
      if (_whitelist[i] == wallet) {
          allowance++;
      }
      unchecked {
          ++i;
      }
  }

  return allowance;
}

function getWhitelistIndex(address wallet) external view returns (uint256) {
  uint256 length = _whitelist.length; // gas saving
  for (uint256 i; i != length;) {
      if (_whitelist[i] == wallet) {
          return i;
      }
      unchecked {
          ++i;
      }
  }

  revert DixelClubV2__NotWhitelisted();
}

That clone _whitelist from storage to use in loop is expected the effect of reducing SLOAD calls.

First, this is the test result.

address[20] addrs = [ ... ]

function origin() external {
    uint256 dummy = 0;
    uint256 len = addrs.length;
    for (uint256 i; i < len;) {
        if (addrs[i] == address(0)) {
            dummy += i;
        }
        unchecked {
            i++;
        }
    }
}

function suggest1() external {
    uint256 dummy = 0;
    address[] memory temp = addrs;
    uint256 len = temp.length;
    for (uint256 i; i < len;) {
        if (temp[i] == address(0)) {
            dummy += i;
        }
        unchecked {
            i++;
        }
    }
}

function suggest2() external {
    uint256 dummy = 0;
    address[] storage temp = addrs;
    uint256 len = temp.length;
    for (uint256 i; i < len;) {
        if (temp[i] == address(0)) {
            dummy += i;
        }
        unchecked {
            i++;
        }
    }
}

origin() gasUsed : 57873
suggest1() gasUsed : 43636
suggest2() gasUsed : 57925

suggest1() what clone storage data to memory variable seems to be the best case.

TO-BE

function getWhitelistAllowanceLeft(address wallet) external view returns (uint256 allowance) {
    // gas saving
    address[] memory clone = _whitelist;
    uint256 length = clone.length;
    for (uint256 i; i != length;) {
        if (clone[i] == wallet) {
            allowance++;
        }
        unchecked {
            ++i;
        }
    }

    return allowance;
}

function getWhitelistIndex(address wallet) external view returns (uint256) {
    // gas saving
    address[] memory clone = _whitelist;
    uint256 length = clone.length;
    for (uint256 i; i != length;) {
        if (clone[i] == wallet) {
            return i;
        }
        unchecked {
            ++i;
        }
    }

    revert DixelClubV2__NotWhitelisted();
}

Without the Ownable contract, it is impossible to set essential options such as setting the 2nd fee in Opensea.

This is an important contract that must be implemented.

Description

If minting has already begun, the owner or any user can't mint the NFT. The code below ensures this.

However, the owner can bypass this because updateMetadata() allows the owner to change the minting starts date when mintingBeginsFrom equals block.timestamp.

To exploit the above issue, the owner creates a new NFT with a large enough mintingBeginsFrom in advance. And then, he will craft the below transactions in order. (All transactions in the block have the same block.timestamp)

1. The owner updates mintingBeginsFrom to block.timestamp.
2. The owner or any user mints the NFT.
3. The owner updates mintingBeginsFrom to large enough mintingBeginsFrom again.

Impacts

The minting starts date can be changed even after minting.

Recommendation

updateMetadata() should revert when _metaData.mintingBeginsFrom equals block.timestamp.

Example patch

    function updateMetadata(bool whitelistOnly, bool hidden, uint24 royaltyFriction, uint40 mintingBeginsFrom, uint152 mintingCost) external onlyOwner {
        if(royaltyFriction > MAX_ROYALTY_FRACTION) revert DixelClubV2__InvalidRoyalty(royaltyFriction);
-       if(_metaData.mintingBeginsFrom != mintingBeginsFrom && uint40(block.timestamp) > _metaData.mintingBeginsFrom) revert DixelClubV2__AlreadyStarted();
+       if(_metaData.mintingBeginsFrom != mintingBeginsFrom && uint40(block.timestamp) >= _metaData.mintingBeginsFrom) revert DixelClubV2__AlreadyStarted();

Community Audit for Dixel V2 contract is now over.
In total, 13 developers have contributed in this review process.

@contract-whale
@Nipol
@assafom
@syl-doo2
@dbadoy
@keon
@sirius651
@jonghwaC
@n1net4il
@kjsman
@junomonster
@kirasys
@inmarelibero

Thank you guys all for your valuable time and contributions 👍🏻

I'm going to evaluate all contributions as fair as possible considering the importance of the contributions and amount of work put into the reviewing process.

While I'm working on the distribution details, please leave your wallet address here for receiving your reward (USDT wallet, Please specify the network: Ethereum, BNB Smart Chain, Solana, AVAX C-Chain, Polygon, Tron network).
If you don't want to put your address here publicly, you can send it to our email instead: [email protected]

Hello,

How do you withdraw NFT sales from this contract?

I just had a cool idea to keep the whitelist as array, but save lots of gas 🙂
You can keep the current mint method if you want, but also add another one, which will take an additional parameter - the sender's position in the whitelist array.
It will then just verify that _whitelist[parameter] == msg.sender, and then delete that value.

This way it does not have to traverse the array at all! So obviously it's a huge difference.

How it will work is that the front end can search for an address's position in the whitelist array (free view function), and then pass that as parameter to the new mint function.

I think this is probably clear enough, let me know of any questions.