thisvishalsingh - Potential `nextTokenId` Overflow in `registerClient` Function

[sherlock-admin4]

thisvishalsingh

medium

Potential nextTokenId Overflow in registerClient Function

Summary

The registerClient function is vulnerable to a potential overflow of the nextTokenId variable, which is a uint32. If this variable overflows, it could lead to token ID collisions and disrupt the integrity of client token tracking.

Vulnerability Detail

Current Implementation: The nextTokenId is incremented with each new client registration without bounds checks.

Issue: If nextTokenId reaches the maximum uint32 value (4,294,967,295) and overflows, it would reset to zero, leading to duplicate token IDs.

PoC:

function registerClient(string calldata name, string calldata description) external whenNotPaused returns (uint32) {
    RewardsStorage storage $ = _getRewardsStorage();

    uint32 tokenId = $.nextTokenId;
    $.nextTokenId = tokenId + 1; //@audit Potential overflow
    _mint(msg.sender, tokenId);
    // ...
}

Impact

An overflow would result in the minting of NFTs with duplicate IDs, which could cause loss of rewards for clients and disrupt the rewards distribution mechanism.

Code Snippet

• Refer to the PoC section above. https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/8f6879efaf831eb7fc9d4a4ad2b62b5334220d87/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol#L191

  Tool used

Manual Review

Recommendation

Implement a check to ensure nextTokenId does not exceed the maximum value for uint32. Alternatively, consider using a larger data type like uint256 for nextTokenId to avoid the overflow risk.

Duplicate of #11

[sherlock-admin2]

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

takarez commented:

valid; medium(1)

[WangSecurity]

Refer to #11 for the final decision.