Nouns Builder contest - Tointer's results

Lines of code

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

Vulnerability details

Impact

There is a typo that can result in owners not getting part of their share. For example, with share percentages [1,1,1,1,1,1,20], last owner would actually get 19% instead of 20. And with 80 founders with 1% and one founder with 20%, last owner would get just 4%. This is happening because baseTokenId not wrapping when it goes beyond 99.

Recommended Mitigation Steps

change line to baseTokenId = (baseTokenId + schedule) % 100;

Foundry test

Paste this in Token.t.sol to test this issue (last assert would fail without fix) :

   function test_brokeFoundersShare() public {
    createUsers(7, 1 ether);

    address[] memory wallets = new address[](7);
    uint256[] memory percents = new uint256[](7);
    uint256[] memory vestExpirys = new uint256[](7);

    uint256 end = 4 weeks;

    unchecked {
        for (uint256 i; i < 6; ++i) {
            wallets[i] = otherUsers[i];
            percents[i] = 1;
            vestExpirys[i] = end;
        }

        wallets[6] = otherUsers[6];
        percents[6] = 20;
        vestExpirys[6] = end;
    }

    deployWithCustomFounders(wallets, percents, vestExpirys);

    assertEq(token.totalFounders(), 7);
    assertEq(token.totalFounderOwnership(), 26);

    Founder memory founder;
    uint actualTotalFounderOwnership;

    for (uint256 i; i < 100; ++i) {
        founder = token.getScheduledRecipient(i);
        if(founder.wallet != address(0)){
            actualTotalFounderOwnership++;
        }
    }

    assertEq(token.totalFounderOwnership(), actualTotalFounderOwnership);
} 

Lines of code

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

Vulnerability details

Overview

token.totalSupply() returning minted amount, but actual totalSupply can be lower, since auctions that ended without bidder will result in token burning. This wrong value used in Governor.sol to calculate proposalThreshold and quorumThreshold, which then assigned to proposal and used in governance.

Example and Impact

As an example we can imagine project where 50% of all auctions ends up without bidder. This can be either because of malicious attack, low activity, high minimum bid, incentive to ignore some tokens due to their metadata, or any combination of the above. In this example, when we minted token with id 1000, actual supply would be 500. But governance contract would calculate quorum and proposal thresholds as if there is 1000 tokens that can vote. This potentially can make DAO ungovernable, especially in high threshold setups. In less drastic situations, it still would affect governance in a non expected and possibly harmful way

Links

bug here: https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L280 totalSupply used here to calculate thresholds: https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L465-L477 then it gets to all proposals here: https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L168 and there: https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L123

Tools Used

VS Code

Recommended Mitigation Steps

Make counter that would count burned tokens, then subtract this value from totalSupply calculation. And rename totalSupply variable in settings struct to avoid confusion

wrong blockhash usage

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L251

blockhash(block.number) would always return 0. It should be changed to blockhash(block.number - 1). This is not affecting anything in it's current form, but can be dangerous if later someone would change the code

generate seed optimization

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L176

this line can be changed to uint256 seed = uint256(blockhash(block.number - 1)); This would cut 150 gas on average and reduce codebase a little (you could delete _generateSeed function after this change). There is no advantage to generate it the way _generateSeed function does it, blockhash provides sufficent amount of entropy, as it hashes whole block and it's metadata