Nouns Builder contest - volky's results

Judge has assessed an item in Issue #357 as Medium risk. The relevant finding follows:

_addFounders allow founders to own all tokens

The _addFounders method allows setting the founders percent ownership to 100%, which then makes minting tokens impossible because it hangs on an infinite loop.

To fix that and reject the contract creation with 100% funders ownership, this line should be changed to

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

That is, use a >= comparison instead of >

_isForFounder function name is misleading

The name and return type of the function makes it look like it is read-only and has no side effects on the contract, while it actually mints tokens.

I suggest changing it to _performScheduledMint to be consistent with the terminology used on other parts of the contract

Unused ITreasury error EXECUTION_EXPIRED

The EXECUTION_EXPIRED error was declared but never used

use string.concat for concatenation on MetadataRenderer

use string.concat instead of abi.encodePacked where simple string concatenation is needed for better readability:

• https://github.com/code-423n4/2022-09-nouns-builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/token/metadata/MetadataRenderer.sol#L208-L213
• https://github.com/code-423n4/2022-09-nouns-builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/token/metadata/MetadataRenderer.sol#L243-L244
• https://github.com/code-423n4/2022-09-nouns-builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/token/metadata/MetadataRenderer.sol#L258-L260
• https://github.com/code-423n4/2022-09-nouns-builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/token/metadata/MetadataRenderer.sol#L272-L280
• https://github.com/code-423n4/2022-09-nouns-builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/token/metadata/MetadataRenderer.sol#L290-L303
• https://github.com/code-423n4/2022-09-nouns-builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/token/metadata/MetadataRenderer.sol#L309

typos

psuedo-random => pseudo-random

Use less storage on Auction contract

Considering the following assumptions:

• an uint40 is enough for storing the auctions timestamps
• auctions must last at least 1 second to receive bids, and in practice they have to last at least 12 seconds each, considering the block times post-merge
• only the auction contract can mint tokens
• an auction can realistically mint at most 100 tokens, considering 99 for founders with 1% ownership each

It's safe to assume that a tokenId won't realistically overflow an uint48 in the contract operational timespan.

With that assumption, we can usa a uint48 instead of uint256 on Auction.tokenId and move it down above uint40 startTime to save storage gas with the struct packing.

For consistency, it can be made an uint96 to match the Token Setting.totalSupply while still fitting in a packed uint256

use less storage on Governor.settings

The Governor Settings struct can be tightly packed and use one less storage slot (4 instead of 5) if its members are reordered to:

    struct Settings {
        Token token;
        Treasury treasury;
        address vetoer;
        uint16 proposalThresholdBps;
        uint16 quorumThresholdBps;
        uint48 votingDelay;
        uint48 votingPeriod;
    }

redundant/unreachable EXECUTION_NOT_READY revert on Treasury.execute

On Treasury.execute there's a check for the execution readiness of the proposal, which will never be truthy because the only possible execution of this function (onlyOwner modifier) is via Governor.execute, which already checks if the proposal is queued and reverts otherwise