Nouns Builder contest - tonisives's results

Lines of code

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

Vulnerability details

Impact

This happens for bigger founder allocation, like 70% and 10% for 2 founders. The multiplier has to be divisible by the same amount, for instance 10.

In other configurations that have the same total owner percentage, the nouns are vested correctly. For instance 40% and 40% for 2 founders allocates correctly, while 70% and 10% (same 80%), doesn’t.

Test results for 70% and 10%(founder 2 gets 2% instead of 10):

forge test -vv -m test_SimilarMultiplier5
total 879
founder 1 630 71%
founder 2 25 2%
bidder 224 25%

Test results for 40% and 40%

forge test -vv -m test_SimilarMultiplier7
total 1184
founder1 480 40%
founder2 480 40%
bidder 224 18%

Proof of Concept

Add to the test folder

// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;

import { console2 } from "forge-std/console2.sol";
import { NounsBuilderTest } from "./utils/NounsBuilderTest.sol";
import { TokenTypesV1 } from "../src/token/types/TokenTypesV1.sol";
import { IERC721 } from "../src/lib/interfaces/IERC721.sol";
import { IBaseMetadata } from "../src/token/metadata/interfaces/IBaseMetadata.sol";

contract VestingExpiryTest is NounsBuilderTest, TokenTypesV1 {
    address internal bidder1;
    address internal bidder2;

    function setUp() public virtual override {
        super.setUp();

        bidder1 = vm.addr(0xB1);
        bidder2 = vm.addr(0xB2);

        vm.deal(bidder1, 100 ether);
        vm.deal(bidder2, 100 ether);
    }

    function deployWithCustomFounders(
        address[] memory _wallets,
        uint256[] memory _percents,
        uint256[] memory _vestExpirys
    ) internal virtual {
        setFounderParams(_wallets, _percents, _vestExpirys);

        setMockTokenParams();

        // setMockAuctionParams();
        // every 3 hours auction = (24/3) * 7 = 56 per week
        setAuctionParams(0.01 ether, 3 hours);

        setMockGovParams();

        deploy(foundersArr, tokenParams, auctionParams, govParams);
    }

    // following have same amount of auctions:
    // every 3 hours = (24/3) * 7 = 56 per week

    //  2 founders, 70% and 10% ❗️ founder 2 gets 3% only
    function test_SimilarMultiplier5() public {
        createUsers(2, 1 ether);

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

        wallets[0] = otherUsers[0];
        percents[0] = 70;
        vestExpirys[0] = 4 weeks;

        wallets[1] = otherUsers[1];
        percents[1] = 10;
        vestExpirys[1] = 4 weeks;

        deployWithCustomFounders(wallets, percents, vestExpirys);

        console2.log(auction.duration());
        assertEq(token.totalFounders(), 2);

        vm.prank(wallets[0]);
        auction.unpause();
        uint256 timePass = 4 weeks / 3 hours;

        uint256 auctionInterval = 1 seconds;
        vm.startPrank(bidder1);
        for (uint256 i = 0; i < timePass; ++i) {
            auction.createBid{ value: 0.01 ether }(token.totalSupply() - 1);
            auctionInterval += 3 hours;
            vm.warp(auctionInterval);
            auction.settleCurrentAndCreateNewAuction();
        }
        vm.stopPrank();

        IERC721 nft = IERC721(IBaseMetadata(token.metadataRenderer()).token());
        uint256 wallet0Owned = nft.balanceOf(wallets[0]);
        uint256 wallet1Owned = nft.balanceOf(wallets[1]);
        uint256 bidderOwned = nft.balanceOf(bidder1);

        uint256 total = wallet0Owned + wallet1Owned + bidderOwned;
        console2.log("total %d", total);
        console2.log("founder 1 %d %d%", wallet0Owned, (wallet0Owned * 100) / total);
        console2.log("founder 2 %d %d%", wallet1Owned, (wallet1Owned * 100) / total);
        console2.log("bidder %d %d%", bidderOwned, (bidderOwned * 100) / total);

        /**
        results
          total 879
          founder 1 630 71%
          founder 2 25 2%
          bidder 224 25%
         */
    }

    // 2 founders, 40% and 40 % ✅
    function test_SimilarMultiplier7() public {
        createUsers(2, 1 ether);

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

        wallets[0] = otherUsers[0];
        percents[0] = 40;
        vestExpirys[0] = 4 weeks;

        wallets[1] = otherUsers[1];
        percents[1] = 40;
        vestExpirys[1] = 4 weeks;

        deployWithCustomFounders(wallets, percents, vestExpirys);

        console2.log(auction.duration());
        assertEq(token.totalFounders(), 2);

        vm.prank(wallets[0]);
        auction.unpause();
        uint256 timePass = 4 weeks / 3 hours;

        uint256 auctionInterval = 1 seconds;
        vm.startPrank(bidder1);
        for (uint256 i = 0; i < timePass; ++i) {
            auction.createBid{ value: 0.01 ether }(token.totalSupply() - 1);
            auctionInterval += 3 hours;
            vm.warp(auctionInterval);
            auction.settleCurrentAndCreateNewAuction();
        }
        vm.stopPrank();

        IERC721 nft = IERC721(IBaseMetadata(token.metadataRenderer()).token());
        uint256 wallet0Owned = nft.balanceOf(wallets[0]);
        uint256 wallet1Owned = nft.balanceOf(wallets[1]);
        uint256 bidderOwned = nft.balanceOf(bidder1);
        uint256 total = wallet0Owned + wallet1Owned + bidderOwned;

        console2.log("total %d", total);
        console2.log("founder1 %d %d%", wallet0Owned, (wallet0Owned * 100) / total);
        console2.log("founder2 %d %d%", wallet1Owned, (wallet1Owned * 100) / total);
        console2.log("bidder %d %d%", bidderOwned, (bidderOwned * 100) / total);

        /**
          total 1184
          founder1 480 40%
          founder2 480 40%
          bidder 224 18%
        */
    }
}

Tools Used

vscode

Recommended Mitigation Steps

I think the problem might be caused by the Founder allocation logic that uses a multiplier of 100% to calculate the founder tokens here. Is it possible that the baseTokenId multiplier overlaps for both 70% and 10%, and thus misses one founders tokens?

uint256 baseTokenId = _tokenId % 100;

• https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/manager/Manager.sol/#L80 _owner variable is shadowed by the storage variable. New developer might confuse it with the function parameter.

• remove nounsDAO ivar. it is unused https://github.com/code-423n4/2022-09-nouns-builder/blob/main/test/utils/NounsBuilderTest.sol/#L33

• add onlyTreasury modifier. This check is used in 3 functions, and could be refactored into a single modifier.

  if (msg.sender != address(this)) revert ONLY_TREASURY();

  https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol/#L224