[M2] Founders can schedule unlimited number of tokens for minting

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L82 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L108

Vulnerability details

Proof of Concept

When you call _addfounders() you pass FounderParams[].

The variable foundersPct is defined as a uint256

   uint256 founderPct = _founders[i].ownershipPct;

This variable is unsafely casted to calculate that the totalOwnership is never more that 100 percent

    if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();

Then is ok when you assign

            newFounder.ownershipPct = uint8(founderPct);

However, when you enter the loop to schedule minting you dont limit the loop to the casted value

That means for example if you set founderPct to 256 then it is stored as ownershipPct = 1. But the founder will be scheduling 256 tokens.

for (uint256 j; j < founderPct; ++j) {@audit high
                // Get the available token id
               baseTokenId = _getNextTokenId(baseTokenId);
                // Store the founder as the recipient
                tokenRecipient[baseTokenId] = newFounder;
                emit MintScheduled(baseTokenId, founderId, newFounder);
                // Update the base token id
                (baseTokenId += schedule) % 100;
            }   

Recommended

It is better to do one safecast at the begining

[-] uint256 founderPct = _founders[i].ownershipPct;
[+] uint8 founderPct = SafeCast.toUint8(_founders[i].ownershipPct);

Even better, I believe, is just to change the variable length is the definition of the struct FounderParams

[-] uint256 ownershipPct;
[+] uint8 ownershipPct;

[GalloDaSballo]

Dup of #303