IvanFitro - CouncilMember.sol :: Burning a NFT impossibilities minting new NFTs (DOS).

[sherlock-admin]

IvanFitro

high

CouncilMember.sol :: Burning a NFT impossibilities minting new NFTs (DOS).

Summary

mint() is used to create new NFTs for users. However, a problem arises when an NFT is burned, making it impossible to mint new NFTs due to the calculation of the nftID being dependent on the totalSupply().

Vulnerability Detail

When the mint() is called to create a new NFT for a user, the calculation of the nftID relies on the totalSupply().

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

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

Initially, this process works as expected. However, a problem arises when an NFT is burned using the burn() , as it decrements the totalSupply.

The issue becomes apparent when the burned NFT is not the latest one minted. In such a scenario, the subsequent call to mint() reverts. This failure occurs because it attempts to use an nftID that already exists (owned by other user).

As a consequence, the transaction reverts with the ERC721 custom error ERC721InvalidSender("0x0000000000000000000000000000000000000000").
This is because for the successful minting of a new NFT, the previous owner must be the zero address. This situation provocates a Denial of Service (DOS).

POC

To run the POC, copy the provided code into the CouncilMember.test.ts file.

describe("Burn custom", () => {

            beforeEach(async () => {
                telcoin.transfer(await stream.getAddress(), 100000);
                await expect(councilMember.mint(member.address)).to.not.reverted;
                await expect(councilMember.mint(support.address)).to.not.reverted;
                await expect(councilMember.mint(await stream.getAddress())).to.not.reverted;
            });

            it("if a NFT is burned impossibilities mint new NFT", async () => {
                await councilMember.burn(0, support.address);
                await expect(councilMember.mint(member.address)).to.revertedWithCustomError(councilMember, "ERC721InvalidSender");
            });
        });

Impact

New NFTs can't be minted (DOS).

Code Snippet

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

Tool used

Manual Review.

Recommendation

To address this issue, introduce a state variable that increments with each minted NFT.

uint256 nftID;

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

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

Duplicate of #199

[amshirif]

https://github.com/telcoin/telcoin-audit/pull/23
https://github.com/telcoin/telcoin-audit/pull/31

[amshirif]

This is an issue, however the changes here have been reversed due to issue #32. A new fix has been included to address both issues with fewer changes.

[amshirif]

Duplicate issue #36

[sherlock-admin2]

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

takarez commented:

valid because {The watson explained how minting a new token will be prevented after burning; its a dupp of 109 with the same underlying cause}

[sherlock-admin]

The protocol team fixed this issue in PR/commit https://github.com/telcoin/telcoin-audit/pull/31.

[sherlock-admin]

The Lead Senior Watson signed off on the fix.