VAD37 - Burn `CouncilMember` NFT mixed up rewards balance of other NFT members. Causing wrong rewards to user

[sherlock-admin2]

VAD37

high

Burn CouncilMember NFT mixed up rewards balance of other NFT members. Causing wrong rewards to user

Summary

CouncilMember.sol use unique NFT ID as index for array balances[]. NFT ID start from 0 and counting up to total NFT minted. So using NFT ID as index is safe as long as it share same length and never changing order.

But the burn NFT function swapping balances[] burning ID array index with the last element of array and popping last element from array.

Causing array out of order, not sorted same as by NFT ID. And balances[] length is not the same as total NFT minted anymore.

This result in last person minted NFT cannot claim rewards anymore as their NFT ID now higher than balances[] length. Also 1 person lost rewards as their NFT ID is swapped with the burning NFT ID.

Vulnerability Detail

Look at how NFT is minted

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

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

NFT ID is used as array index to keep track of rewards balance for each NFT. Each NFT is council member. The above implementation have a problem that balances[] and NFT ID must counting incremented and fixed order.

Just so when someone claim rewards, contract know which NFT owner to send rewards to. Using NFT ID as index to find rewards balance. https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L92-L111

    function claim(uint256 tokenId, uint256 amount) external {
        // Ensure the function caller is the owner of the token (council member) they're trying to claim for
        require(
            _msgSender() == ownerOf(tokenId),
            "CouncilMember: caller is not council member holding this NFT index"
        );
        // Retrieve and distribute any pending TELCOIN for all council members
        _retrieve();

        // Ensure the requested amount doesn't exceed the balance of the council member
        require(
            amount <= balances[tokenId],
            "CouncilMember: withdrawal amount is higher than balance"
        );

        // Deduct the claimed amount from the token's balance
        balances[tokenId] -= amount;
        // Safely transfer the claimed amount of TELCOIN to the function caller
        TELCOIN.safeTransfer(_msgSender(), amount);
    }

But the burn function while sending NFT to burn address, it also removing an index balances[] array by popping the last element.

By swap the burning NFT ID with the last element of array balances[], the array is now out of order and balances[] length is not the same as total NFT minted anymore. https://github.com/sherlock-audit/2024-01-telcoin/blob/main/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);
    }

This simply result in someone else balance is now swapped with the burning NFT balances which is 0.

Even the last NFT minted member cannot claim rewards anymore as their NFT ID now higher than balances[] length.

Impact

After a NFT is burned, lost rewards for a member of council. Also last NFT minted member cannot claim rewards anymore.

Code Snippet

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

Tool used

Manual Review

Recommendation

Here is a quick fix without changing much of the code.

Do not pop balances[] array when burning NFT.

Along with counter to keep track of total NFT minted and total NFT burned. Rewards divide evenly among NFT not burned yet. The _retrieve() function that spread rewards evenly should check if NFT is burned or not before sending rewards. Using balances[] array length as NFT ID.

Duplicate of #199

[sherlock-admin2]

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

takarez commented:

valid because { The watson was able to explain the flaw in the implementation of the balances array that stores the balance corresponding to a specific user; but its also a dupp of 109}