Nouns Builder contest - ElKu's results

Lines of code

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

Vulnerability details

Impact

In the rare case that, the totalOwnership of the founders is 100, the whole DAO will fail. Because with such a setting, the Token contract wont be able to mint any tokens. Any call on the mint() method will result in an infinite while loop, eventually reverting because of gas limit.

Proof of Concept

I added the following function to Token.t.sol for POC.

    function test_pct100Founder() public {
        createUsers(10, 1 ether);

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

        uint256 pct = 10;
        uint256 end = 4 weeks;

        unchecked {
            for (uint256 i; i < 10; ++i) {
                wallets[i] = otherUsers[i];
                percents[i] = pct;
                vestExpirys[i] = end;
            }
        }
        // 10 founders. each with a pct  of 10. which means totalOwnership is 10*10=100.
        deployWithCustomFounders(wallets, percents, vestExpirys);
        // some routine checking
        assertEq(token.totalFounders(), 10);
        assertEq(token.totalFounderOwnership(), 100);

        vm.prank(token.auction());
        token.mint();   //this will result in a infinite while loop
    }

The following forge test command was ran:

forge test --match-contract Token --match-test test_pct100Founder -vvvv --block-gas-limit 28000000

And the output looked like this: Test Result

The issue is caused by the condition of the while loop in mint() method.

            do {
                // Get the next token to mint
                tokenId = settings.totalSupply++;

                // Lookup whether the token is for a founder, and mint accordingly if so
            } while (_isForFounder(tokenId));

Since 100 out of 100 tokens belongs to the founders, _isForFounder always returns true, resulting in an Infinite loop.

Once set, the founder parameters cannot be edited, which makes the whole system useless and would require a fresh deployment.

Tools Used

VSCode, Foundry, Manual analysis

Recommended Mitigation Steps

This can be avoided by simply making the below change to line 88 of Token.sol.

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

This will cause the _addFounders method to revert if the totalOwnership is 100 or more.

Lines of code

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

Vulnerability details

Impact

In the _addFounders internal function, the wallet addresses of the founders are not checked for zero address. If the wallet address is zero, the contract will not emit any warnings, but simply will skip minting the token for that particular founder.

Proof of Concept

I added the following function to Token.t.sol for POC.

    function test_NullWalletFounder() public {
        createUsers(10, 1 ether);

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

        uint256 pct = 2;
        uint256 end = 4 weeks;

        unchecked {
            for (uint256 i; i < 10; ++i) {
                wallets[i] = otherUsers[i];
                percents[i] = pct;
                vestExpirys[i] = end;
            }
            wallets[5] = address(0);  //make one wallet address zero
        }
        // 10 founders. each with a pct  of 2. which means totalOwnership is 10*2=20.
        deployWithCustomFounders(wallets, percents, vestExpirys);
        // some routine checking
        assertEq(token.totalFounders(), 10);
        assertEq(token.totalFounderOwnership(), 20);

        vm.prank(token.auction());
        token.mint();  //6th founder will not receive any token. and he/she wont be warned about it.
    }

The following forge test command was ran:

forge test --match-contract Token --match-test test_NullWalletFounder -vvvv

And the output looked like this: Test Result

As you can see the 6th founder is skipped silently in the mint() method because the founder's wallet address was zero.

This is because in _isForFounder method, we have the following if condition which skips any zero wallet addresses.

        if (tokenRecipient[baseTokenId].wallet == address(0)) {
            return false;

Though it is fine to assume that a wallet address with zero value means he/she doesnt have any right over tokens, since their ownershipPct is more than zero, we should consider the possibility that it was more of a typo rather than their actual intention.

Once set, the founder parameters cannot be edited. Then we have only two options. Either to redeploy everything with correct settings or let that unfortunate founder miss out on his dear tokens.

Tools Used

VSCode, Foundry, Manual analysis

Recommended Mitigation Steps

This can be avoided by simply adding a new condition below line 85 of Token.sol.

      if (founderPct == 0) continue;
      if ( _founders[i].wallet == address(0) ) revert INVALID_FOUNDER_WALLET();

This will cause the _addFounders method to revert if the wallet address is zero for a founder with non-zero ownershipPct.