popeye - Multiple owners could show "ownership" of the same token ID in `CouncilMember.sol`

[sherlock-admin2]

popeye

high

Multiple owners could show "ownership" of the same token ID in CouncilMember.sol

Summary

This could allow duplicate token IDs to be created. Specifically, when an NFT is burned, its token ID is not invalidated. This allows the token ID to be reused in future mints, violating the ERC-721 standard and creating two tokens with the same ID.

Vulnerability Detail

The vulnerability stems from how CouncilMember::mint function rely on totalSupply to determine the next token ID.

In mint(), the new token ID is simply the current totalSupply:

// CouncilMember.sol
function mint(address newMember) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
  // ...
  _mint(newMember, totalSupply());
}

When burning in burn(), totalSupply is reduced but the specific burned tokenId is not tracked:

// CouncilMember.sol
function burn(uint256 tokenId, address recipient) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
  // ...
  _burn(tokenId);
}

This allows the burned tokenId to be reused if totalSupply reaches that number again.

On ERC721Upgradeable::_mint openzeppelin mentioned in the natspec:

* Requirements:
*
* - `tokenId` must not exist.

Impact

This vulnerability breaks the fundamental ERC-721 requirement that each token ID must be unique and not assigned multiple owners.

If exploited, it would be possible for multiple wallet addresses to show "ownership" of the same token ID. This destroys the reliability of the NFT's identity and ownership tracking.

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#L210-L222

Proof of Concept

Alice: A member with the GOVERNANCE_COUNCIL_ROLE, responsible for minting and burning tokens Bob: A council member and the owner of (tokenId 5)

1. Alice mints 10 NFTs to 10 different council members. The token IDs minted are 0, 1, 2, 3, 4, 5, 6, 7, 8, 9.
2. Let's say the NFT with token ID 5 belongs to Bob.
3. Alice burns Bob's NFT (token ID 5).
4. totalSupply is now 9.
5. Alice mints an NFT to a new council member.
6. The contract assigns the new NFT a token ID of 9, since the current totalSupply is 9.
7. But token ID 9 already exists from the first minting!

So at the end of this example:

• Council member #9 still holds token ID 9 from the initial minting
• The new council member also receives token ID 9 after the burn and re-mint

Tool used

Manual Review

Recommendation

Independent Counter for Token IDs:

• Implement a separate counter for token IDs to ensure uniqueness.

  uint256 private _tokenIdCounter;

  Modify Mint Function:

• Update the mint function to use _tokenIdCounter for new token IDs.

  function mint(address newMember) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
  _mint(newMember, _tokenIdCounter);
  _tokenIdCounter += 1;
  }

Duplicate of #199

[sherlock-admin2]

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

takarez commented:

valid because { This is valid and the watson explained how a the exploit looks like; which is of two token ids; but i consider it a dupp of 109 because of the same underlying cause }