Founder minting infinite loop

[code423n4]

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)

[GalloDaSballo]

Dup of #347