Myrault - DOS in Rewards::registerClient

[sherlock-admin3]

Myrault

medium

DOS in Rewards::registerClient

Summary

DOS in Rewards::registerClient

Vulnerability Detail

Malicious accounts can call the Rewards::registerClient function without restriction, causing tokenId to reach type(uint32).max. At this time, when any normal user wants to registerClient function, the sentence $.nextTokenId = tokenId + 1; will be revert, because the solidity 0.8.19 compiler has overflow checking

Impact

DOS

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-L191

/**
     * @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; // @audit: will revert when maximum value is reached
        _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

1. Replacing the variable type from uint32 to uint256 makes the attack cost for malicious accounts almost infinite.
2. Or provide the function of deleting maliciously registered Clients.

[sherlock-admin4]

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

takarez commented:

this should be valid seeing there is no restriction on who to call thw function; medium(1)

[eladmallel]

Please see my comments on #12 - we think this attack is expensive and therefore not likely, and it's pretty easy to solve by upgrading contracts. we think it's low priority, not sure why it's labeled as high?

[WangSecurity]

Initially labeled it as high because there are no pre-reqs for that issue, but since it can be easily mitigated with upgrades, then I believe the impact should be Low (at max Med) and likelihood is also low, due to high cost of the attack and no profit or even incentive from the attack, cause again it's easily mitigated with an upgrade.

Likelihood: Low + Impact: Low = Low