Balance array misalignment and DoS on the next mint after calling CouncilMember.burn()

Summary

The CouncilMember contract, an implementation of ERC721, demonstrates a potential vulnerability in the management of token balances corresponding to token IDs. Specifically, the issue arises in the burn function, where the burning of a council member NFT can lead to a misalignment between the token IDs and their associated balances in the balances array. This misalignment could potentially cause out-of-bounds access in the claim function and incorrect token ID assignments in the mint function.

Vulnerability Detail

The contract maintains an array balances, where each index corresponds to a token ID that begins with 0, storing the balance of TELCOIN associated with that token. The problem arises when a token is burned, and its balance is reassigned the balance of the last token ID, followed by a reduction in the balances array size. This process creates a discrepancy between the existing token IDs and their corresponding indices in the balances array.

Let's say we have 10 NFT tokens with IDs 0, 1, 2, ... , 9 minted where totalSupply() == 10. Burning token ID 1 will first have balances[1] withdrawn before having balances[9] assigned to balances[1].

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

When the owner of token ID 9 call claim(), amount <= balances[tokenId] is going to revert because you are trying to access an out of bound array element, i.e. balances[9] that has already been popped.

         require(
            amount <= balances[tokenId],
            "CouncilMember: withdrawal amount is higher than balance"
        );

Additionally, minting a new token after burning an old one could lead to the reuse of a previously assigned token ID, causing further inconsistency. This is because totalSupply() == 9 after burning token ID 1, and you are attempting to mint token ID 9 that already exists.

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

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

Impact

The primary impact is the potential for out-of-bounds access in the claim function, which could result in transaction reverts when users attempt to claim TELCOIN for certain token IDs. Moreover, the reuse of token IDs in the mint function might cause confusion and inconsistency in the tracking of token ownership and balances. This misalignment poses a risk to the contract's reliability and the integrity of token management, potentially leading to loss of funds to the NFT owner and misfiring of the minting process.

Code Snippet

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

Tool used

Manual Review

Recommendation

In the example given above, consider reminting the burned token ID 1 to the owner of the last token and then burning the last token ID, i.e. 9. That way, token ID 9 is freed up. This will ensure continuous token IDs minting while aligning the balances array with the increasingly counting totalSupply(). Nevertheless, the original owner of the last token ID 9 will need to be aware he/she now owns the burned/reminted token ID 1.

Duplicate of #199

sherlock-audit / 2024-01-telcoin-judging

0xmystery - Balance array misalignment and DoS on the next mint after calling CouncilMember.burn() #109