Nouns Builder contest - teawaterwire's results

Lines of code

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

Vulnerability details

Impact

When adding more than one founder, if the first founder has more than 50% of the allocation then if the second founder has more than 2% they will be missing tokens

Proof of Concept

adding this test in Token.t.sol

function test_Ownership2Founders() public {
        createUsers(2, 1 ether);

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

        uint256 end = 4 weeks;

        unchecked {
            for (uint256 i; i < 2; ++i) {
                wallets[i] = otherUsers[i];
                percents[i] = i == 0 ? 60 : 40;
                vestExpirys[i] = end;
            }
        }

        deployWithCustomFounders(wallets, percents, vestExpirys);

        assertEq(token.totalFounders(), 2);

        Founder memory founder;

        unchecked {
            for (uint256 i; i < 100; ++i) {
                founder = token.getScheduledRecipient(i);

                if (i < 60) assertEq(founder.wallet, otherUsers[0]);
                else assertEq(founder.wallet, otherUsers[1]);
            }
        }
    }

in that case the allocation is 60% / 40% and we can see that the second founder gets only 20 tokens instead of 40 (when running the test)

forge test --match test_Ownership2Founders

Tools Used

foundry forge

Recommended Mitigation Steps

it has to do with the vesting schedule and the way the next token is not computed modulo 100.

maybe easier to drop the vesting schedule and just mint "linearly" from first founder to last founder (this would improve gas cost as well)