No council member can be created after burning a NFT token

Summary

Vulnerability Detail

CouncilMember#burn() function is used to burn a council member 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);
}

It call _burn() function in the ERC721EnumerableUpgradeable libary to burn NFT:

function _burn(uint256 tokenId) internal {
    address previousOwner = _update(address(0), tokenId, address(0));
    if (previousOwner == address(0)) {
        revert ERC721NonexistentToken(tokenId);
    }
}

_update() function:

function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) {
    address previousOwner = super._update(to, tokenId, auth);

    if (previousOwner == address(0)) {
        _addTokenToAllTokensEnumeration(tokenId);
    } else if (previousOwner != to) {
        _removeTokenFromOwnerEnumeration(previousOwner, tokenId);
    }
    if (to == address(0)) {
        _removeTokenFromAllTokensEnumeration(tokenId);
    } else if (previousOwner != to) {
        _addTokenToOwnerEnumeration(to, tokenId);
    }

    return previousOwner;
}

And because to address is 0, function _removeTokenFromAllTokensEnumeration will be called:

function _removeTokenFromAllTokensEnumeration(uint256 tokenId) private {
    ERC721EnumerableStorage storage $ = _getERC721EnumerableStorage();
    // 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(); // <---- this will reduce total supply
}

It will reduce total supply, because it will rely on length of _allToken

function totalSupply() public view virtual returns (uint256) {
    ERC721EnumerableStorage storage $ = _getERC721EnumerableStorage();
    return $._allTokens.length;
}

This will break function mint() because token id is rely on totalSupply(). which will be duplicated with old council member when a NFT is burned:

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

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

Consider scenario:

1, User A, B, C, D is minted council member with ID = 0, 1, 2, 3. totalSupply() = 4 2, User B's nft is burned, only have A nd C left with id = 0, 2, 3. totalSupply() = 3. 3, Governance try to mint council member for user E with ID = 3 but failed because it is duplicate with user D.

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)) {  // <--- this condition will true
        revert ERC721InvalidSender(address(0));
    }
}

Impact

New council member can't be created if NFT is burn

Code Snippet

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

Tool used

Manual Review

Recommendation

New tokenId should be selected by governance, not rely on totalSupply()

Duplicate of #199

sherlock-audit / 2024-01-telcoin-judging

grearlake - No council member can be created after burning a NFT token #231