bitsurfer - Wrong logic on minting using `totalSupply` as tokenId can cause minting DoSed

[sherlock-admin2]

bitsurfer

high

Wrong logic on minting using totalSupply as tokenId can cause minting DoSed

Summary

The mint logic is flawed, due to minting a token using totalSupply(). when burn a tokenId, it will remove the tokenId, and the length (total supply) is now decreased by 1 from previous totalSupply. Therefore, when minting a new one, it will mint a tokenId which already exist.

Vulnerability Detail

In CouncilMember mint function, the _mint is using totalSupply() as value for the tokenId instead of common variable increment.

This can cause issue because when there was a tokenId being burned, which then decreasing the totalSupply() then the next minting it will use the already existing tokenId.

For example if there are 5 tokenId: 0, 1, 2, 3, 4, totalSupply() is 5 then next mint would be minting this tokenId 5.

But, then before minting again, a user or owner burn tokenId #2 thus the tokens would be: 0, 1, 3, 4 and totalSupply() is now 4

Now, the next mint, which by the code provided will be totalSupply() as the tokenId, which is 4, will be reverted due to it's already exist.

File: CouncilMember.sol
173:     function mint(
174:         address newMember
175:     ) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
176:         if (totalSupply() != 0) {
177:             _retrieve();
178:         }
179:
180:         balances.push(0);
181:         _mint(newMember, totalSupply());
182:     }

Impact

Unable to mint new tokenId, breaking the contract

Code Snippet

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

Tool used

Manual Review

Recommendation

Consider to use a common incremental value for tokenId

Duplicate of #199

[sherlock-admin2]

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

takarez commented:

valid because { This is also a valid findings and a dupp of 109 issue}