KlimaIDONFT Manual Review Findings

KID-01M: Improper Enumerability

Description:

The _beforeTokenTransfer inheritence collision is not resolved properly, thereby never invoking the _beforeTokenTransfer hook of ERC721EnumerableUpgradeable and breaking enumerability.

Example:

Copy
122function _beforeTokenTransfer(
123     address from,
124     address to,
125     uint256 tokenId
126 ) internal virtual override(ERC721Upgradeable, ERC721EnumerableUpgradeable, ERC721PausableUpgradeable) {
127     
128     if(hasRole(MINTER_ROLE, _msgSender())){
129         ERC721Upgradeable._beforeTokenTransfer(from,to,tokenId);
130     }
131     else
132     {
133         ERC721PausableUpgradeable._beforeTokenTransfer(from, to, tokenId);
134     }
135 }

Recommendation:

We advise the _beforeTokenTransfer hooks of both ERC721EnumerableUpgradeable and ERC721PausableUpgradeable to be invoked in the final override implementation to ensure enumerability is accounted for correctly.

Alleviation:

The KlimaDAO team stated that enumerability operates properly in the compiled output. We still recommend the adjustment we have stated as currently it works due to the inheritence structure and the compiler's traits which can cause behaviour to change in future compiler versions.

KID-02M: Improper ERC721 Mint Operations

Description:

The _mint operations performed within the contract accept an argument that is incremented after the operation, allowing a re-entrancy to occur whereby the same NFT ID is minted over and over.

Example:

Copy
95function mint(address to) public onlyMinter whenPaused {
96    // We cannot just use balanceOf to create the new tokenId because tokens
97    // can be burned (destroyed), so we need a separate counter.
98    ERC721Upgradeable._mint(to, _tokenIdTracker.current());
99    _tokenIdTracker.increment();
100}
101
102function batchMint(address [] memory _list) public onlyMinter whenPaused {
103    // We cannot just use balanceOf to create the new tokenId because tokens
104    // can be burned (destroyed), so we need a separate counter.
105    require(_list.length <= 255, "Max 255 addresses at once");
106    
107    for (uint8 i = 0; i < _list.length; i++) {
108        ERC721Upgradeable._mint(_list[i], _tokenIdTracker.current());
109        _tokenIdTracker.increment();
110    }
111
112}

Recommendation:

We advise the incremention of the ID tracker to be performed prior to the actual _mint operation to ensure a consistent contract state.

Alleviation:

The KlimaDAO team stated that re-entrancy is only exploitable by the administrators that perform the mint operation and as such it is a non-issue.