Burning of council NFT may affect future minting of NFTs.

Summary

If a token id that is not the last token id is burned, future token id cannot be minted because totalSupply() is used when minting.

Vulnerability Detail

When a council member is minted his nft, the token id is the current totalSupply() of the NFT minted.

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

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

When the token is burned, the tokenId can be chosen and _burn is called.

    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);
    }

Imagine there are 10 token id for 10 council members, from token index 0 to 9. Council member with token id 2 is deemed malicious, and so his token id should be burned. TokenId 2 is therefore burned.

A new council member is selected to replace the council member. _mint() is called but because the totalSupply() is used as the tokenId, and tokenId 9 exists, the mint function cannot work.

ERC721Enumerable does update the index, so that means that the 10th council member gets token id 2 instead and the 10th token, token id 9 is burned.

    function _removeTokenFromAllTokensEnumeration(uint256 tokenId) private {
        // To prevent a gap in the tokens array, we store the last token in the index of the token to delete, and
        // then delete the last slot (swap and pop).

        uint256 lastTokenIndex = _allTokens.length - 1;
        uint256 tokenIndex = _allTokensIndex[tokenId];

        // When the token to delete is the last token, the swap operation is unnecessary. However, since this occurs so
        // rarely (when the last minted token is burnt) that we still do the swap here to avoid the gas cost of adding
        // an 'if' statement (like in _removeTokenFromOwnerEnumeration)
        uint256 lastTokenId = _allTokens[lastTokenIndex];

        _allTokens[tokenIndex] = lastTokenId; // Move the last token to the slot of the to-delete token
        _allTokensIndex[lastTokenId] = tokenIndex; // Update the moved token's index

        // This also deletes the contents at the last position of the array
        delete _allTokensIndex[tokenId];
        _allTokens.pop();
    }

However, ERC721 does not update the index.

ERC721.sol
    function _burn(uint256 tokenId) internal virtual {
        address owner = ERC721.ownerOf(tokenId);

        _beforeTokenTransfer(owner, address(0), tokenId, 1);

        // Update ownership in case tokenId was transferred by `_beforeTokenTransfer` hook
>       owner = ERC721.ownerOf(tokenId);

        // Clear approvals
        delete _tokenApprovals[tokenId];

Council Member 3 is still the owner of tokenId 2. Council Member 10 is still the owner of tokenId 9.

Impact

New council member tokens cannot be minted.

Code Snippet

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

Tool used

Manual Review

Recommendation

Don't use totalSupply() when minting token.

Duplicate of #199

sherlock-audit / 2024-01-telcoin-judging

Jaraxxus - Burning of council NFT may affect future minting of NFTs. #187