code-423n4 / 2022-09-nouns-builder-findings

10 stars 6 forks source link

Upgraded Q -> M from 418 [1665255821676] #737

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Judge has assessed an item in Issue #418 as Medium risk. The relevant finding follows:

GalloDaSballo commented 2 years ago

DAO can be deployed with 100% allocation to founders It's possible to deploy a DAO with an exact 100% allocation to the founders. If a DAO is deployed with these parameters, all auction bids will revert with out of gas errors, when they search for the next available unallocated token.

See the following test case:

// in Token.t.sol
function testHundredPercentFunderOwnership() public {
    // setup
    address[] memory wallets = new address[](1);
    uint256[] memory percents = new uint256[](1);
    uint256[] memory vestingEnds = new uint256[](1);

    wallets[0] = founder;
    percents[0] = 100;
    vestingEnds[0] = 4 weeks;

    setFounderParams(wallets, percents, vestingEnds);
    setMockTokenParams();
    setMockAuctionParams();
    setMockGovParams();
    deploy(foundersArr, tokenParams, auctionParams, govParams);

    vm.prank(founder);
    auction.unpause();

    address bidder = makeAddr("bidder");
    vm.prank(bidder);
    auction.createBid{ value: 1 ether }(2);

    // causes an infinite loop, hence eventually an out of gas error
    // [FAIL. Reason: EvmError: Revert] testHundredPercentFunderOwnership() (gas: 8660281895701201590)
}

Recommendation:

Change the following condition in Token#addFounders from:

            // Update the total ownership and ensure it's valid
            if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();

to:

            // Update the total ownership and ensure it's valid
            if ((totalOwnership += uint8(founderPct)) >= 100) revert INVALID_FOUNDER_OWNERSHIP();
GalloDaSballo commented 2 years ago

Dup of https://github.com/code-423n4/2022-09-nouns-builder-findings/issues/269