Nouns Builder contest - elad's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L157 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L179-L190

Vulnerability details

Impact

In the case of founder shares totalOwnership reaching 100, e.g. with a single founder owning the entire 100%, the mint function seems stuck in an infinite loop because all token IDs are founder tokens, at least until the founder vesting timestamp is reached.

Proof of Concept

I made minor changes to Token to allow my test to include minimal setup, then wrote a short test function sharing it below.

Modifications

• Removed the initializer modifier from the constructor
• Removed the call to metadataRenderer.onMinted to avoid having to provide a renderer or mock one

The test

function test_mint() public {
    address founder = address(0xabc);
    address renderer = address(0xbbb);
    address auction = address(0xccc);

    Token token = new Token(address(this));

    IManager.FounderParams[] memory _founders = new IManager.FounderParams[](1);
    _founders[0] = IManager.FounderParams({ wallet: founder, ownershipPct: 100, vestExpiry: block.timestamp + 30 seconds });

    token.initialize(_founders, abi.encode("name", "symbol", "desc", "image", "base"), renderer, auction);

    vm.prank(auction);
    token.mint();
}

Tools Used

forge test

Recommended Mitigation Steps

• Make it so the token sends founder rewards every X tokens to a single destination, similar to Nouns
• Have that destination be a new contract that handles the split among founders, to separate that concern out
• In this new contract:
  • keep count of how many NFTs have been received so far
  • keep count of how many NFTs each founder has withdrawn
  • have a view function of how many NFTs a founder should have received so far: founderNFTsTotal = totalReceived * founderShare
  • allow founder to withdraw founderNFTsTotal - founderReceivedSoFar (each of these vars being founder specific)
  • bonus: let founders transfer NFTs out with a n/m signing scheme like a Safe, this can help them liquidate NFTs when no one can withdraw a whole NFT (e.g. 3 equal founders after the first token sent to them only have 1/3 each)

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L363 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L128

Vulnerability details

Impact

It seems a bug that the propose function lets you propose when you have exactly proposalThreshold votes, while cancel lets anyone cancel your proposal. Such a proposer should not be considered as spammy.

Proof of Concept

It's easy to see from the lines of code linked above that the comparisons don't match well between the two functions.

Tools Used

Github

Recommended Mitigation Steps

In the cancel function change the following code:

if (msg.sender != proposal.proposer && getVotes(proposal.proposer, block.timestamp - 1) > proposal.proposalThreshold)
                revert INVALID_CANCEL();

to be

if (msg.sender != proposal.proposer && getVotes(proposal.proposer, block.timestamp - 1) >= proposal.proposalThreshold)
                revert INVALID_CANCEL();

making it an INVALID_CANCEL when getVotes returns exactly proposal.proposalThreshold.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L194 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L144-L160

Vulnerability details

Impact

It's confusing UX to allow users to add more items than 65535 in the addProperties function, while limiting the ID range in use in onMinted in the referenced line. They might think they added items that will be used but that won't be the case.

Recommended Mitigation Steps

I think the limit of 65535 is fine, I would make it clear in the docs, and I would revert in addProperties if that number is reached.