miaowu - The upper limit of clientID of `Reward::registerClient` is type(uint32).max. This value is not large enough and can be reached by attackers by creating a large number of accounts, so that the clientID created by others will be revert

[sherlock-admin4]

miaowu

high

The upper limit of clientID of Reward::registerClient is type(uint32).max. This value is not large enough and can be reached by attackers by creating a large number of accounts, so that the clientID created by others will be revert

Summary

The upper limit of clientID of Reward::registerClient is type(uint32).max. This value is not large enough and can be reached by attackers by creating a large number of accounts, so that the clientID created by others will be revert

Vulnerability Detail

According to the comment "return uint32 the newly assigned clientId", it can be seen that the upper limit of clientID that Reward::registerClient can create is type(uint32).max. This number is not large. An attacker can reach this upper limit by creating a large number of accounts. This will cause the clientID created by others to be revert (in version 0.8.19, overflow will be revert), which means that the attacker can exclude other people from the auction, making the auction unable to proceed normally.

Impact

Code Snippet

https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol#L187-L201

/**
     * @notice Register a client, mints an NFT and assigns a clientId
     * @return uint32 the newly assigned clientId
     */
function registerClient(string calldata name, string calldata description) external whenNotPaused returns (uint32) {
        RewardsStorage storage $ = _getRewardsStorage();

        uint32 tokenId = $.nextTokenId;
        $.nextTokenId = tokenId + 1;
        _mint(msg.sender, tokenId);

        ClientMetadata storage md = $._clientMetadata[tokenId];
        md.name = name;
        md.description = description;

        emit ClientRegistered(tokenId, name, description);

        return tokenId;
    }

Tool used

Manual Review

Recommendation

Change tokenID, nextID, clientID and other related variable types to uint256

Duplicate of #11

[sherlock-admin3]

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

takarez commented:

valid; medium(1)

[eladmallel]

unit32 ID range is ~4.29B. there is a least one SSTORE into a cold slot costing 22K gas (with more minor state updates we'll omit for now). let's calculate the attack cost assuming gas price is at 50 gwei:

• 22000 * 50 gwei = 1100000 gwei = 0.0011 ETH
• 0.0011 ETH * 4.29B = 4,719,000 ETH
• given say 1 ETH = 3K USD, this comes out to 14,157,000,000 USD.

we ran this analysis while designing the contract and concluded that the risk is very low.

wdyt?

[eladmallel]

@WangSecurity have you chosen a different primary issue to point to the same risk?

[WangSecurity]

Yes, I see that issue 11 has better explanation and it also has impact and better recommendation. But we can keep the discussion here, if it's more convenient

[WangSecurity]

My primary question is what do you think of the impact, it's not clear in other issues as well. If the uint32 value reaches its max, then there cannot be any other clients? And what is there functionality to delete the clients?

[eladmallel]

@WangSecurity I think the attack is very expensive, see my comment above. And even if someone is crazy enough to do it, I we can upgrade the contracts to grow the ID space.

IMO it's low priority.

wdyt?

[WangSecurity]

Refer to #11 for final decision.