code423n4 commented 2 years ago

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.

GalloDaSballo commented 2 years ago

Historically Low, I think Low is more appropriate