Ignite - The mint() function may fail if the totalSupply() is equal to the number of already minted NFT

[sherlock-admin]

Ignite

high

The mint() function may fail if the totalSupply() is equal to the number of already minted NFT

Summary

The mint() function uses totalSupply() as a token ID when minting, which may duplicate with an already minted NFT, resulting in a failed minting process.

Vulnerability Detail

The CouncilMember contract has imported the ERC721EnumerableUpgradeable abstract to make a fungible token contract.

import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721EnumerableUpgradeable.sol";

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L4

The mint() function is designed to mint new NFTs for governance council members. It then mints a new NFT for the provided newMember address with a token ID equal to the current total supply.

function mint(
    address newMember
) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
    if (totalSupply() != 0) {
        _retrieve();
    }

    balances.push(0);
    _mint(newMember, totalSupply());
}

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L173-L182

However, the token ID can be duplicated if the governance burns an NFT and the totalSupply() matches an already minted NFT.

function burn(
    uint256 tokenId,
    address recipient
) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
    require(totalSupply() > 1, "CouncilMember: must maintain council");
    _retrieve();
    _withdrawAll(recipient, tokenId);

    uint256 balance = balances[balances.length - 1];
    balances[tokenId] = balance;
    balances.pop();
    _burn(tokenId);
}

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L210-L222

For example:

1. The governance council mints 4 NFTs with token IDs 0, 1, 2, and 3, resulting in a totalSupply() of 4.
2. The governance council burns token ID 1, reducing the totalSupply() to 3.
3. If the governance wants to mint a new NFT, this function may be reverted at lines 316-318 in the ERC721Upgradeable contract from OpenZeppelin since the previousOwner of tokenId 3 is not the zero address.

function _mint(address to, uint256 tokenId) internal {
    if (to == address(0)) {
        revert ERC721InvalidReceiver(address(0));
    }
    address previousOwner = _update(to, tokenId, address(0));
    if (previousOwner != address(0)) {
        revert ERC721InvalidSender(address(0));
    }
}

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/dee3ce0d2ab61c1fd6641595d3067c6ca1ce117c/contracts/token/ERC721/ERC721Upgradeable.sol#L316-L318

As a result, the governance council may need to burn the NFT before minting the new NFT to control the next token ID; otherwise, they cannot mint a new NFT.

Impact

Govenance council may not be able to mint a new council member NFT.

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L181

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L210-L222

Tool used

Manual Review

Recommendation

I suggest using counter instead of totalSupply() to ensure that tokenId cannot be duplicated.

Remove the balances array and directly transfer allocated Telcoin rewards to the NFT owner when called _retrieve().

Duplicate of #199

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid because { This is also a dupp of (014) but 014 stays the best}