m4ttm - Burning any CouncilMember other than the last can prevent minting

[sherlock-admin2]

m4ttm

high

Burning any CouncilMember other than the last can prevent minting

Summary

CouncilMember is an ERC-721 which contains a mint function that mints at an index according to the supply. Since the burn function allows any token to be burned, the next id to mint can be inconsistent with the supply and can cause mint to revert since the id already exists.

Vulnerability Detail

Add the following test to [https://github.com/sherlock-audit/2024-01-telcoin/blob/0954297f4fefac82d45a79c73f3a4b8eb25f10e9/telcoin-audit/test/sablier/CouncilMember.test.ts#L127](https://github.com/sherlock-audit/2024-01-telcoin/blob/0954297f4fefac82d45a79c73f3a4b8eb25f10e9/telcoin-audit/test/sablier/CouncilMember.test.ts)

describe("mintBurnMint", () => {
            it("mint 2, burn the first, then mint", async () => {
            	// mint two NFTS
                await expect(councilMember.mint(member.address)).emit(councilMember, 'Transfer');
                await expect(councilMember.mint(member.address)).emit(councilMember, 'Transfer');
                
                // burn NFT at index 0
                await councilMember.burn(0, member.address);
                
                // try and mint another NFT - reverts
                await councilMember.mint(member.address);
            });
        });

The mint function mints the next NFT according to the current supply, assuming all consecutive NFTs exist but the burn function allows burning by any index.

Impact

This can brick the mint function temporarily. This state can be corrected, but will require burning and reminting such that all ids are consecutive.

Code Snippet

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

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/0954297f4fefac82d45a79c73f3a4b8eb25f10e9/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L210-L222

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

Tool used

Manual Review

Recommendation

Change the burn function to only allow burning the last ID.

Duplicate of #199

[sherlock-admin2]

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

takarez commented:

valid because { dupp of 109}