Mintra 2

BaseCollection.sol#L181-L187

In BaseCollection, there’s additionalFeatures.erc20AndErc721IsAndvariable that its purpose is for token-gated minting. More specifically:

• If erc20AndErc721IsAnd == false, this means the minter only needs to satisfy either ERC20 holding requirement or ERC721 holding requirement
• if erc20AndErc721IsAnd == true, this means the minter MUST need to satisfy both ERC20 holding requirement and ERC721 holding requirement

But in the code, when erc20AndErc721IsAnd == false, minter has to satisfy both requirements:

bool hasErc20Check = additionalFeatures.erc20TokenAddresses.length > 0;
bool hasErc721Check = additionalFeatures.erc721TokenAddresses.length > 0;

        
if (additionalFeatures.erc20AndErc721IsAnd) {
    require(isERC20CheckSatisfied && isERC721CheckSatisfied, "Not enough ERC20 and ERC721 tokens");
} else {
    if (hasErc20Check) {
        require(isERC20CheckSatisfied, "Not enough ERC20 tokens");
    }

    if (hasErc721Check) {
        require(isERC721CheckSatisfied, "Not enough ERC721 tokens");
    }
}

If erc20AndErc721IsAnd == false and erc20TokenAddresses.length > 0 and erc721TokenAddresses.length > 0, minter has to satisfy both requirements to pass.

Remediations to Consider

} else {
-    if (hasErc20Check) {
-        require(isERC20CheckSatisfied, "Not enough ERC20 tokens");
-    }
-    if (hasErc721Check) {
-        require(isERC721CheckSatisfied, "Not enough ERC721 tokens");
-    }

+		 require(isERC20CheckSatisfied || isERC721CheckSatisfied, "Not enough ERC20 or ERC721 tokens");
}

RoyaltySplitter.sol#L54

In distrubuteRoyalty() function in the RoyaltySplitter contract, it will send native token to all receivers:

function distrubuteRoyalty(address[] memory erc20Addresses) public payable nonReentrant {
        ...
    if (plsBalance > MIN_PLS) {
            (bool success,) = to[i].call{value: amount}("");
            require(success, "ETH_TRANSFER_FAILED");
                        ...
        }
    }
    ...
}

If there’s an address in the to array that can’t receive native tokens, it will make the function reverted, which leads to all the funds held (including native token and ERC20s) in this contract being locked forever

Remediations to Consider

This issue can be resolved in different ways:

1. Have an owner-controlled function that can change to and amountBps arrays. Note that this requires the contracts to be Ownable, adding centralization risks.
2. Adding a function where each recipient can withdraw it’s own share. Note that would increase complexity, as the contract would to keep track about the withdrawn amount of each recipient.

ERC1155Collection.sol#L140-L145

In ERC1155Collection contract, we have a tokenSupply mapping variable, its purpose is to track the supply of each tokenId:

mapping(uint256 => uint256) public tokenSupply;

As per the definition, the token supply increases on minting and should be decreased on burn.

However, in the burn() function, tokenSupply of the burned tokenId will not be decreased:

function burn(uint256 _tokenId, uint256 _amount) external {
require(additionalFeatures.isBurnable, "Collection is not burnable");

require(balanceOf(msg.sender, _tokenId) >= _amount, "Token not owned");
_burn(msg.sender, _tokenId, _amount);
}

This will lead to the totalSupply of each tokenId will be higher than the actual underlying total supply of that tokenId. As a consequence, minters are not being able to mint more tokens even though some tokens have been burned

Remediations to Consider

Decrease the token supply by the amount burned.

function burn(uint256 _tokenId, uint256 _amount) external {
require(additionalFeatures.isBurnable, "Collection is not burnable");

require(balanceOf(msg.sender, _tokenId) >= _amount, "Token not owned");

+	  tokenSupply[_tokenId] -= _amount;

_burn(msg.sender, _tokenId, _amount);
}

In RoyaltySplitter.distributeRoyalty, native tokens can only be distributed to defined recipients when the contract’s balance exceeds MIN_PLS.

if (plsBalance > MIN_PLS) {
for (uint256 i = 0; i < to.length; i++) {
    uint256 amount = plsBalance * amountBps[i] / MAX_BPS;
    ...

However, the constant MIN_PLS is set to 1000 ether.

While this number might not be particular large when deployed on Pulsechain (1 PLS ~ $0.00005), it can get very large when the contracts are considered to be deployed on other chains. This can even lead to locked funds in the contract, as the amount is never reached.

Remediation to Consider

Remove the check plsBalance > MIN_PLS entirely to allow native balance to be distributed in any case.-

BaseCollection.sol#L73

In BaseCollection’s constructor(), it doesn’t validate startOfMint and endOfMint. If incorrect values are provided, such as startOfMint > endOfMint or endOfMint > block.timestamp, invalid collections are deployed, where minting of tokens will revert.

function verifyAndProcessPayment(uint256 _amount, address _tokenToPayIn) internal {
  ...

  if (additionalFeatures.isTimed) {
      require(
          block.timestamp >= additionalFeatures.startOfMint && block.timestamp <= additionalFeatures.endOfMint,
          "Minting not started or ended"
      );
  }
        
        ...

Remediations to Consider

Consider adding proper validation for startOfMint and endOfMint when the collection are deployed.

In BaseCollector’s constructor(), we have this check to make sure the listing price is not too small:

BaseCollection.sol#L103

for (uint256 i = 0; i < _priceListToken.length; i++) {
isFreeMint = false;

@>  require(_priceListAmount[i] >= 100000, "Price too low");
price[_priceListToken[i]] = _priceListAmount[i];
}	

However, some tokens have decimals set to 6 or smaller, which can make the minimum mint price higher than wanted:

• For USDC which has decimals = 6 , 100_000 means 0.1 USD
• For GeminiUSD which has decimals = 2, 100_000 means 1000USD

Remediations to Consider

Since this check doesn’t add much value, consider removing this line completely:

- require(_priceListAmount[i] >= 100000, "Price too low");

ERC1155Collection.sol#L102

ERC1155Collection.sol#L128-L129

ERC721Collection.sol#L120

In ERC721Collection and ERC1155Collection contracts, we have [ERC721Collection.mint](http://ERC721Collection.mint)(), ERC1155Collection.mint() and ERC1155Collection.devMint() functions that doesn’t follow CEI (Checks-Effects-Interact) pattern. Meaning that in ERC721Collection.mint for example, amountMinted is updated after calling _safeMint (which makes an external call).

Even though the function uses nonReentrant modifier, it can still lead to issues like view-only reentrancy attacks.

Remediations to Consider

Follow best practices by adhering to the CEI pattern for aforementioned functions.

EXPLANATION

LaunchpadFactory.sol#L81

In LauchpadFactory contract, we have a setMarketPercent() function:

function setMarketPercent(uint256 _marketPercent) public onlyWizard {
    require(_marketPercent >= 0 && _marketPercent <= 369, "Fee not in range");

    marketPercent = _marketPercent;
}

Notice here that LaunchpadFactory’s marketPercent can be set in [0, 369] range. But in BaseCollection’s constructor(), BaseCollection’s marketPercent can only be set in [0,225]:

BaseCollection.sol#L71

constructor(
    string memory _name,
    string memory _symbol,
    string memory _baseURI,
    address[] memory _priceListToken,
    uint256[] memory _priceListAmount,
    address _feeSplitterAddress,
    uint256 _marketPercent,
    bytes memory data
) {
    ...
    require(_marketPercent >= 0 && _marketPercent <= 225, "Market percent not in range");

    ...

As a result, if marketPercent in LaunchpadFactory is set in [226, 369], LaunchpadFactory can’t deploy any collections.

Remediations to Consider

function setMarketPercent(uint256 _marketPercent) public onlyWizard {
-   require(_marketPercent >= 0 && _marketPercent <= 369, "Fee not in range");
+   require(_marketPercent >= 0 && _marketPercent <= 225, "Fee not in range");

    marketPercent = _marketPercent;
}

In LaunchpadFactory’s deployCollection, there is the following check on the maximum tokenId allowed:

require(_maxTokenId > 0 && _maxTokenId <= 10000, "maxTokenId not in range");

While a lot of NFT projects use a collection quantity of 10_000 tokens, there can be valid use cases where one wants to create collections consisting of more than 10_000 tokens, both for ERC721 and ERC1155. Restricting the maxTokenId to 10_000 unnecessarily limits the use case of those collections.

Remediation to Consider

Remove the upper limit of 10_000 for _maxTokenId.

LaunchpadFactory.sol#L25-L28

In the LaunchpadFactory contract, we have a collections mapping variable:

// mapping of collection addresses to their types
mapping(address => CollectionType) public collections; 

Enum CollectionType looks like this:

enum CollectionType {
ERC721,
ERC1155
}

The default value for CollectionType is ERC721, which leads to reporting misleading information when calling the view function of collections mapping for addresses that have not been deployed via the factory.

Remediations to Consider

Use a spacer value as a default type

enum CollectionType {
+   NO_OP,
ERC721,
ERC1155
}

Since the code might be deployed to other chains, it should be considered to be reorgs protected.

About reorgs: also known as block reorganization, reorgs occur when network nodes fall out of sync with each other, and two distinct chains of blocks are produced concurrently. This may be due to a bug, network latency, or even malicious activity. When nodes sync once again, one canonical version of the chain is kept, and the blocks included in the invalid ‘fork’ are ignored.

Polygon historically got 157-block reorg, which is ~ 5 minutes: https://protos.com/polygon-hit-by-157-block-reorg-despite-hard-fork-to-reduce-reorgs/

Reorg can happen when a new Collection contract is deployed, and plenty of minters are trying to mint as much as possible. The reorgs that happen will make the Collection contract get undeployed. Malicious users take advantage of that to deploy new Collection contracts with the same address, minter didn’t notice that and still trying to mint NFT, but now malicious users take profit for that instead of the victim project.

Remediations to Consider

The root cause of this issue is that people can have the ability to deploy the contract with identical addresses. This can be solved by using CREATE2 and a carefully done salt to deploy the contract

CREATE is a deploying method that the deployed contract address depend on the msg.sender's address and msg.sender's nonce:

new_address = hash(sender, nonce)

CREATE2 is a deploying method that the deployed contract address depend on the msg.sender's address, an arbitrary salt, and the deployed contract’s bytecode:

new_address = hash(0xFF, sender, salt, bytecode)

Further reading: https://docs.openzeppelin.com/cli/2.8/deploying-with-create2

To make sure deployers can’t deploy a contract with collided address with each other, we must include deployer’s address into the salt

ERC721Collection.sol#L130-L135

ERC1155Collection.sol#L140-L145

In ERC721Collection contract, burners have to own the token in order to burn it. This can lead burners who have permission from other addresses, or have permission to use the burning token, to transfer it to their own wallet in order to burn() :

function burn(uint256 tokenId) external {
    require(additionalFeatures.isBurnable, "Collection is not burnable");
    
    // msg.sender must be token owner
    require(ownerOf(tokenId) == msg.sender, "Token not owned");
    _burn(tokenId);
}

The same issue appears in ERC1155Collection, the burner who has permission from another address need to transfer the tokens to their own wallet first in order to burn it:

function burn(uint256 _tokenId, uint256 _amount) external {
    require(additionalFeatures.isBurnable, "Collection is not burnable");

        // msg.sender must be token owner
    require(balanceOf(msg.sender, _tokenId) >= _amount, "Token not owned");
    _burn(msg.sender, _tokenId, _amount);
}

Remediations to Consider

For ERC721Collection, consider putting burn permission for burners that have an allowance of the burning token, or burners that have permission to other addresses that hold the burning token. In ERC721A contract, there’s a function function _burn(uint256 tokenId, bool approvalCheck) that we can use for this case (ERC721A.sol#L1130). If approvalCheck == true, it will check if msg.sender has the allowance of the burning token or msg.sender has permission to other addresses that hold the burning token:

function burn(uint256 tokenId) external {
    require(additionalFeatures.isBurnable, "Collection is not burnable");
    
-   require(ownerOf(tokenId) == msg.sender, "Token not owned");
-   _burn(tokenId);
+   _burn(tokenId, true);
}

For ERC1155Collection, consider putting burn permission for the burner having permission to other addresses that hold the burning token.

-	function burn(uint256 _tokenId, uint256 _amount) external {
+	function burn(uint256 _tokenId, uint256 _amount, address _burner) external {
    require(additionalFeatures.isBurnable, "Collection is not burnable");
    
-   require(balanceOf(msg.sender, _tokenId) >= _amount, "Token not owned");
+		require(_burner == msg.sender || isApprovedForAll(_burner, msg.sender), "Token not owned or approved");		

-    _burn(msg.sender, _tokenId, _amount);
+    _burn(_burner, _tokenId, _amount);
}

In BaseCollection, the constructor checks that erc20TokenAddresses == erc20TokenAmounts and erc721TokenAddresses == erc721TokenAmounts. However, this check is only done in the case erc20AndErc721IsAnd == true, but should be done in any case.

BaseCollection.sol#L80-L91

Remediations to Consider

constructor(
    string memory _name,
    string memory _symbol,
    string memory _baseURI,
    address[] memory _priceListToken,
    uint256[] memory _priceListAmount,
    address _feeSplitterAddress,
    uint256 _marketPercent,
    bytes memory data
) {
    ...

-   if (_additionalFeatures.erc20AndErc721IsAnd) {
        require(
            _additionalFeatures.erc20TokenAddresses.length == _additionalFeatures.erc20TokenAmounts.length,
            "ERC20 list mismatch"
        );
        require(
            _additionalFeatures.erc721TokenAddresses.length == _additionalFeatures.erc721TokenAmounts.length,
            "ERC721 list mismatch"
        );
        
+   if (_additionalFeatures.erc20AndErc721IsAnd) {

        require(_additionalFeatures.erc20TokenAddresses.length > 0, "ERC20 list empty");
        require(_additionalFeatures.erc721TokenAddresses.length > 0, "ERC721 list empty");
    }

    ...
}

RoyaltySplitter.sol#L47

Currently, the only way to send native token to RoyaltySplitter contract is by calling to distrubuteRoyalty() function:

function distrubuteRoyalty(address[] memory erc20Addresses) public payable nonReentrant {
    ...
}

Remediations to Consider

Consider adding a fallback or receive function to the RoyaltySplitter contract in order to receive native token directly.

In BaseCollection contract, the collection owner can configure the contract to be ERC20 and ERC721 token-gated for minters when minting tokens. But currently, the contract doesn’t support ERC1155 token-gated.

Remediations to Consider

For a more flexible contract, consider adding ERC1155 token-gated feature

BaseCollection.sol#L229-L246

In BaseCollection contract, getAdditionalFeatures() , onAllowList() , getPrice() view functions can be removed because due to the public state variables, we have additionalFeatures(), allowlist(), price() view functions that return exactly the same value.

ERC721Collection.sol#L169-L178

In ERC721Collection contract, name() and symbol() don’t need to be overriden. Because ERC721Collection's myName and mySymbol have the same value as ERC721A’s _name and _symbol, ERC721A’s and ERC721Collection’s name() and symbol() functions return the same value

Remediations to Consider

Consider removing those unnecessary view functions

On the following occurrences, a number is used instead of a constant:

• 225 for max market percentage
• 100000 for minimum price
• 369 for max market percentage (should be 225, see L-4)
• 10000 for max token id

Remediations to Consider

To improve readability, consider using constants for the number mentioned above.

In ERC721Collection, _maxTokenId is passed to the constructor and defined as follows:

* @param _maxTokenId The maximum supply of tokens that can be minted for the collection. 

However, the name _maxTokendId implies, that this is the maximum number of token id that can be minted, rather than the maximum number of supply. It is verified in the mint function:

require(supply + _amount <= maxTokenId, "Max maxTokenId reached");

As per above, maxTokenId is treated as the maximum allowed supply. This can create confusions amongst users, as tokens can be minted with tokenIds > maxTokenId.

Remediation to Consider

Rename _maxTokenId to a more meaningful name such as _maxTokenMinted.

1. In ERC721Collection and ERC1155Collection, supportsInterface() can be refactored to me more readable:

   + return ERC2981.supportsInterface(interfaceId) || ERC721A.supportsInterface(interfaceId);
   - return ERC2981.supportsInterface(interfaceId) ? true : ERC721A.supportsInterface(interfaceId);
   

2. Currently, in the BaseCollection.sol#L101 constructor(), isFreeMint variable gets set to false multiple times every loop:

   for (uint256 i = 0; i < _priceListToken.length; i++) {
       isFreeMint = false;
   
       require(_priceListAmount[i] >= 100000, "Price too low");
       price[_priceListToken[i]] = _priceListAmount[i];
   }
   

   Move isFreeMint = false; outside of the loop to save gas.

3. In BaseCollection.withdraw() function, a payable tag is not necessary, since this function doesn’t need to hold native token:

   -    function withdraw(address _tokenAddress) external payable onlyOwner {
   +    function withdraw(address _tokenAddress) external onlyOwner {
           ...
   

4. In BaseCollection.verifyAndProcessPayment(), required ERC20 will be pulled from msg.sender to the contract first, and then distributed to feeSplitterAddress and owner() . We can reduce one external call here by sending ERC20 directly from msg.sender to feeSplitterAddress and owner()

   -	    IERC20(_tokenToPayIn).safeTransferFrom(msg.sender, address(this), totalCost);
   -	    IERC20(_tokenToPayIn).safeTransfer(feeSplitterAddress, launchpadFee);
   -	    IERC20(_tokenToPayIn).safeTransfer(owner(), totalCost - launchpadFee);
           
   +	    IERC20(_tokenToPayIn).safeTransferFrom(msg.sender, feeSplitterAddress, launchpadFee);
   +	    IERC20(_tokenToPayIn).safeTransferFrom(msg.sender, owner(), totalCost - launchpadFee);
   

5. In BaseCollection, import of Strings and ReentrancyGuard is not used and can be removed

6. In BaseCollection, for isERC20CheckSatisfied and isERC721CheckSatisfied, it is not necessary to set default value.

7. Inaccurate natspec in ERC1155Collection:

   • missing @param for _maxSupplytoken

ERC721Collection and ERC1155Collection use the var maxMintableAmount to restrict the number of tokens allowed to be minted per address. However, this check can be easily bypassed, as a malicious user can just create as much addresses as they want.

Additionally, both ERC721 and ERC1155 collections implement token gating to restrict minting of tokens only to addresses holding certain tokens. This can also be easily bypassed by users utilizing flashloans to borrow assets.