search

sherlock-audit / 2024-01-telcoin-judging

6 stars 5 forks source link

sonny2k - mint() function using totalSupply() as the id for newly minted NFT will always be reverted if some NFTs were burnt #249

Closed sherlock-admin closed 8 months ago

sherlock-admin commented 8 months ago

sonny2k

high

mint() function using totalSupply() as the id for newly minted NFT will always be reverted if some NFTs were burnt

Summary

In mint(), the function use totalSupply() as the id for the future minted token, but not beware of the case if some of the NFTs were burnt affecting totalSupply().

Vulnerability Detail

If some NFTs were burnt, the totalSupply() will decrease and make the Id duplicate with the other NFTs that were minted before.

POC:

  1. protocol minted 3 NFTs, totalSupply() is now 3
  2. protocol burned the NFTs of id 1
  3. totalSupply() is now 2
  4. protocol minted a new NFTs, Id now would be 2 as the totalSupply() is 2
  5. mint() function will be always reverted as NFT with Id 2 already exists

Impact

Protocol can't mint new NFT forever as it would duplicate the existed NFT

Code Snippet

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

Tool used

Manual Review

Recommendation

Use Auto Increment Ids like the one OpenZeppelin suggests for there ERC721 implementation https://wizard.openzeppelin.com/#erc721

Duplicate of #199

sherlock-admin2 commented 8 months ago

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

takarez commented:

valid because { valid and a dupp of 109 with minimal impact}