Nouns Builder contest - rbserver's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L473-L477 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L116-L175 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L413-L456

Vulnerability details

Impact

At the early stage of the deployed DAO, it is possible that the following quorum function returns 0 because the token supply is low.

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L473-L477

    function quorum() public view returns (uint256) {
        unchecked {
            return (settings.token.totalSupply() * settings.quorumThresholdBps) / 10_000;
        }
    }

When calling the following propose function, proposal.quorumVotes = uint32(quorum()) is executed. If quorum() returns 0, proposal.quorumVotes is set to 0.

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L116-L175

    function propose(
        address[] memory _targets,
        uint256[] memory _values,
        bytes[] memory _calldatas,
        string memory _description
    ) external returns (bytes32) {
        // Get the current proposal threshold
        uint256 currentProposalThreshold = proposalThreshold();

        // Cannot realistically underflow and `getVotes` would revert
        unchecked {
            // Ensure the caller's voting weight is greater than or equal to the threshold
            if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();
        }

        // Cache the number of targets
        uint256 numTargets = _targets.length;

        // Ensure at least one target exists
        if (numTargets == 0) revert PROPOSAL_TARGET_MISSING();

        // Ensure the number of targets matches the number of values and calldata
        if (numTargets != _values.length) revert PROPOSAL_LENGTH_MISMATCH();
        if (numTargets != _calldatas.length) revert PROPOSAL_LENGTH_MISMATCH();

        // Compute the description hash
        bytes32 descriptionHash = keccak256(bytes(_description));

        // Compute the proposal id
        bytes32 proposalId = hashProposal(_targets, _values, _calldatas, descriptionHash);

        // Get the pointer to store the proposal
        Proposal storage proposal = proposals[proposalId];

        // Ensure the proposal doesn't already exist
        if (proposal.voteStart != 0) revert PROPOSAL_EXISTS(proposalId);

        // Used to store the snapshot and deadline
        uint256 snapshot;
        uint256 deadline;

        // Cannot realistically overflow
        unchecked {
            // Compute the snapshot and deadline
            snapshot = block.timestamp + settings.votingDelay;
            deadline = snapshot + settings.votingPeriod;
        }

        // Store the proposal data
        proposal.voteStart = uint32(snapshot);
        proposal.voteEnd = uint32(deadline);
        proposal.proposalThreshold = uint32(currentProposalThreshold);
        proposal.quorumVotes = uint32(quorum());
        proposal.proposer = msg.sender;
        proposal.timeCreated = uint32(block.timestamp);

        emit ProposalCreated(proposalId, _targets, _values, _calldatas, _description, descriptionHash, proposal);

        return proposalId;
    }

When determining the proposal's state, the following state function is called, which can execute else if (proposal.forVotes < proposal.againstVotes || proposal.forVotes < proposal.quorumVotes) { return ProposalState.Defeated; }. If proposal.quorumVotes is 0, the proposal.forVotes < proposal.quorumVotes condition would always be false. Essentially, quorum votes have no effect at all for determining whether the proposal is defeated or succeeded when the token supply is low. Hence, critical proposals, such as for updating implementations or withdrawing funds from the treasury, that should not be passed if there are effective quorum votes for which the for votes fail to reach can be passed, or vice versa, so the impact can be huge.

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L413-L456

    function state(bytes32 _proposalId) public view returns (ProposalState) {
        // Get a copy of the proposal
        Proposal memory proposal = proposals[_proposalId];

        // Ensure the proposal exists
        if (proposal.voteStart == 0) revert PROPOSAL_DOES_NOT_EXIST();

        // If the proposal was executed:
        if (proposal.executed) {
            return ProposalState.Executed;

            // Else if the proposal was canceled:
        } else if (proposal.canceled) {
            return ProposalState.Canceled;

            // Else if the proposal was vetoed:
        } else if (proposal.vetoed) {
            return ProposalState.Vetoed;

            // Else if voting has not started:
        } else if (block.timestamp < proposal.voteStart) {
            return ProposalState.Pending;

            // Else if voting has not ended:
        } else if (block.timestamp < proposal.voteEnd) {
            return ProposalState.Active;

            // Else if the proposal failed (outvoted OR didn't reach quorum):
        } else if (proposal.forVotes < proposal.againstVotes || proposal.forVotes < proposal.quorumVotes) {
            return ProposalState.Defeated;

            // Else if the proposal has not been queued:
        } else if (settings.treasury.timestamp(_proposalId) == 0) {
            return ProposalState.Succeeded;

            // Else if the proposal can no longer be executed:
        } else if (settings.treasury.isExpired(_proposalId)) {
            return ProposalState.Expired;

            // Else the proposal is queued
        } else {
            return ProposalState.Queued;
        }
    }

Proof of Concept

Please append the following test in test\Gov.t.sol. This test will pass to demonstrate the described scenario.

    function test_QueueProposalWithZeroQuorumVotes() public {
        mintVoter1();

        (address[] memory targets, uint256[] memory values, bytes[] memory calldatas) = mockProposal();

        bytes32 descriptionHash = keccak256(bytes("test"));

        vm.warp(1 days);

        // token supply is only 4 at this moment
        assertEq(token.totalSupply(), 4);

        // the calculated quorum votes is 0 because the token supply is low
        assertEq((token.totalSupply() * governor.quorumThresholdBps()) / 10_000, 0);

        // voter1 creates the proposal
        vm.prank(voter1);
        governor.propose(targets, values, calldatas, "test");

        bytes32 proposalId = governor.hashProposal(targets, values, calldatas, descriptionHash);

        vm.warp(block.timestamp + governor.votingDelay());

        vm.prank(voter1);
        governor.castVote(proposalId, 1);

        vm.warp(block.timestamp + governor.votingPeriod());

        // quorum votes corresponding to the proposal is 0
        Proposal memory proposal = governor.getProposal(proposalId);
        assertEq(proposal.quorumVotes, 0);

        // the proposal is succeeded
        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Succeeded));

        // voter1 is able to queue the proposal
        vm.prank(voter1);
        governor.queue(proposalId);
        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Queued));
    }

Tools Used

VSCode

Recommended Mitigation Steps

A minimum quorum votes governance configuration that is at least 1 can be added. When quorum() returns 0 because the token supply is low, calling propose could set proposal.quorumVotes to the minimum quorum votes.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L116-L175 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L353-L377

Vulnerability details

Impact

When User B calls the following propose function for creating a proposal, it checks that User B's past number of votes is larger than the proposal threshold through executing if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD(). This means that User B can create the proposal when the past number of votes and the proposal threshold are the same.

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L116-L175

    function propose(
        address[] memory _targets,
        uint256[] memory _values,
        bytes[] memory _calldatas,
        string memory _description
    ) external returns (bytes32) {
        // Get the current proposal threshold
        uint256 currentProposalThreshold = proposalThreshold();

        // Cannot realistically underflow and `getVotes` would revert
        unchecked {
            // Ensure the caller's voting weight is greater than or equal to the threshold
            if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();
        }

        // Cache the number of targets
        uint256 numTargets = _targets.length;

        // Ensure at least one target exists
        if (numTargets == 0) revert PROPOSAL_TARGET_MISSING();

        // Ensure the number of targets matches the number of values and calldata
        if (numTargets != _values.length) revert PROPOSAL_LENGTH_MISMATCH();
        if (numTargets != _calldatas.length) revert PROPOSAL_LENGTH_MISMATCH();

        // Compute the description hash
        bytes32 descriptionHash = keccak256(bytes(_description));

        // Compute the proposal id
        bytes32 proposalId = hashProposal(_targets, _values, _calldatas, descriptionHash);

        // Get the pointer to store the proposal
        Proposal storage proposal = proposals[proposalId];

        // Ensure the proposal doesn't already exist
        if (proposal.voteStart != 0) revert PROPOSAL_EXISTS(proposalId);

        // Used to store the snapshot and deadline
        uint256 snapshot;
        uint256 deadline;

        // Cannot realistically overflow
        unchecked {
            // Compute the snapshot and deadline
            snapshot = block.timestamp + settings.votingDelay;
            deadline = snapshot + settings.votingPeriod;
        }

        // Store the proposal data
        proposal.voteStart = uint32(snapshot);
        proposal.voteEnd = uint32(deadline);
        proposal.proposalThreshold = uint32(currentProposalThreshold);
        proposal.quorumVotes = uint32(quorum());
        proposal.proposer = msg.sender;
        proposal.timeCreated = uint32(block.timestamp);

        emit ProposalCreated(proposalId, _targets, _values, _calldatas, _description, descriptionHash, proposal);

        return proposalId;
    }

After User B's proposal is created, User A can try to cancel it by calling the following cancel function. Calling cancel executes if (msg.sender != proposal.proposer && getVotes(proposal.proposer, block.timestamp - 1) > proposal.proposalThreshold) revert INVALID_CANCEL(). This means that if User B's past number of votes at that moment is the same as the proposal threshold, User A is able to cancel User B's proposal. However, this contradicts the fact that User B actually can create this proposal when the same condition holds true. In other words, if User B can create this proposal when her or his past number of votes and the proposal threshold are the same, User A should not be able to cancel User B's proposal under the same condition but it is not true, which is unfair to User B.

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L353-L377

    function cancel(bytes32 _proposalId) external {
        // Ensure the proposal hasn't been executed
        if (state(_proposalId) == ProposalState.Executed) revert PROPOSAL_ALREADY_EXECUTED();

        // Get a copy of the proposal
        Proposal memory proposal = proposals[_proposalId];

        // Cannot realistically underflow and `getVotes` would revert
        unchecked {
            // Ensure the caller is the proposer or the proposer's voting weight has dropped below the proposal threshold
            if (msg.sender != proposal.proposer && getVotes(proposal.proposer, block.timestamp - 1) > proposal.proposalThreshold)
                revert INVALID_CANCEL();
        }

        // Update the proposal as canceled
        proposals[_proposalId].canceled = true;

        // If the proposal was queued:
        if (settings.treasury.isQueued(_proposalId)) {
            // Cancel the proposal
            settings.treasury.cancel(_proposalId);
        }

        emit ProposalCanceled(_proposalId);
    }

Proof of Concept

Please append the following test in test\Gov.t.sol. This test will pass to demonstrate the described scenario.

    function testRevert_cancelProposalWhenPastVotesOfProposerAndProposalThresholdAreSame() public {
        createUsers(1, 0);
        mintVoter1();

        vm.warp(block.timestamp + 1 seconds);

        vm.prank(address(treasury));
        governor.updateProposalThresholdBps(2500);

        address[] memory targets = new address[](1);
        uint256[] memory values = new uint256[](1);
        bytes[] memory calldatas = new bytes[](1);
        targets[0] = address(auction);
        calldatas[0] = abi.encodeWithSignature("");

        // voter1's past number of votes and proposal threshold are the same before creating the proposal
        assertEq(token.balanceOf(voter1), governor.proposalThreshold());

        // voter1 creates the proposal successfully
        vm.prank(voter1);
        bytes32 proposalId = governor.propose(targets, values, calldatas, "");

        vm.warp(block.timestamp + governor.votingDelay());

        // voter1's past number of votes and proposal threshold are still the same after creating the proposal and before canceling it
        assertEq(token.balanceOf(voter1), governor.proposalThreshold());

        // otherUsers[0] who is not the proposer cancels the proposal successfully
        vm.prank(otherUsers[0]);
        governor.cancel(proposalId);
        Proposal memory proposal = governor.getProposal(proposalId);
        assertTrue(proposal.canceled);
    }

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L128 can be changed to the following code.

            if (getVotes(msg.sender, block.timestamp - 1) <= proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();

or

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L363-L364 can be changed to the following code.

            if (msg.sender != proposal.proposer && getVotes(proposal.proposer, block.timestamp - 1) >= proposal.proposalThreshold)
                revert INVALID_CANCEL();

but not both.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L385-L405

Vulnerability details

Impact

The settings.vetoer, which is the first founder defined by the FounderParams, can call the following veto function to veto any proposals that are not yet executed, which immediately blocks these proposals from execution. Because the vetoer is just one founder, which can just be a single EOA, the chance of losing its private key and being compromised is not low. There is also no guarantee that the vetoer will not become malicious in the future. When the vetoer becomes compromised or malicious, all critical proposals, such as for updating implementations or withdrawing funds from the treasury, can be vetoed so the negative impact can be very high.

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L385-L405

    function veto(bytes32 _proposalId) external {
        // Ensure the caller is the vetoer
        if (msg.sender != settings.vetoer) revert ONLY_VETOER();

        // Ensure the proposal has not been executed
        if (state(_proposalId) == ProposalState.Executed) revert PROPOSAL_ALREADY_EXECUTED();

        // Get the pointer to the proposal
        Proposal storage proposal = proposals[_proposalId];

        // Update the proposal as vetoed
        proposal.vetoed = true;

        // If the proposal was queued:
        if (settings.treasury.isQueued(_proposalId)) {
            // Cancel the proposal
            settings.treasury.cancel(_proposalId);
        }

        emit ProposalVetoed(_proposalId);
    }

Proof of Concept

Please append the following tests in test\Gov.t.sol. These tests will pass to demonstrate the vetoer's power for vetoing pending, active, and queued proposals.

    function test_VetoPendingProposal() public {
        bytes32 proposalId = createProposal();

        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Pending));

        // vetoer can veto a pending proposal without a delay
        vm.prank(founder);
        governor.veto(proposalId);
        assertEq(uint8(governor.state(proposalId)), uint8(ProposalState.Vetoed));
    }

    function test_VetoActiveProposal() public {
        mintVoter1();

        bytes32 proposalId = createProposal();

        uint256 votingDelay = governor.votingDelay();
        vm.warp(block.timestamp + votingDelay + 1);

        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Active));

        // vetoer can veto an active proposal without a delay
        vm.prank(founder);
        governor.veto(proposalId);
        assertEq(uint8(governor.state(proposalId)), uint8(ProposalState.Vetoed));
    }

    function test_VetoQueuedProposal() public {
        mintVoter1();

        bytes32 proposalId = createProposal();

        vm.warp(block.timestamp + governor.votingDelay());

        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Active));

        vm.prank(voter1);
        governor.castVote(proposalId, 1);

        vm.warp(block.timestamp + governor.votingPeriod());

        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Succeeded));

        vm.prank(voter1);
        governor.queue(proposalId);

        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Queued));

        // vetoer can veto a queued proposal without a delay
        vm.prank(founder);
        governor.veto(proposalId);
        assertEq(uint8(governor.state(proposalId)), uint8(ProposalState.Vetoed));
    }

Tools Used

VSCode

Recommended Mitigation Steps

A token supply threshold governance configuration can be added. Before the token supply exceeds this threshold, the vetoer can remain in full power for protecting against the 51% attack on the deployed DAO while the token supply is low.

After the token supply exceeds this threshold, the following changes can be considered for restricting the vetoer's power.

1. The vetoer is only allowed to veto a proposal during a defined period after the voting is done. The proposal is not allowed to be executed during this period.
2. The vetoer is only allowed to veto the passed proposal when the number of support and against votes are very close or the number of support votes is much higher than the against votes.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L90-L154 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L323-L327

Vulnerability details

Impact

Calling the following createBid function executes extend = (_auction.endTime - block.timestamp) < settings.timeBuffer. A lower settings.timeBuffer would decrease the chance of the auction deadline extension and increase the chance that a bidder's bidding wins. With this in mind, bidders are more incentivized to bid when settings.timeBuffer is relatively short. However, when the setTimeBuffer function below is called after the auction starts and before it ends, a different settings.timeBuffer, which can be longer than the time buffer that is set previously when creating the auction, is set. As a result, the bidders who bid because of the short settings.timeBuffer that is set when creating the auction would unexpectedly face the new longer settings.timeBuffer for determining whether the corresponding auction's deadline should be extended or not, which is unfair to them.

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L90-L154

    function createBid(uint256 _tokenId) external payable nonReentrant {
        // Get a copy of the current auction
        Auction memory _auction = auction;

        // Ensure the bid is for the current token
        if (_auction.tokenId != _tokenId) revert INVALID_TOKEN_ID();

        // Ensure the auction is still active
        if (block.timestamp >= _auction.endTime) revert AUCTION_OVER();

        // Cache the address of the highest bidder
        address highestBidder = _auction.highestBidder;

        // If this is the first bid:
        if (highestBidder == address(0)) {
            // Ensure the bid meets the reserve price
            if (msg.value < settings.reservePrice) revert RESERVE_PRICE_NOT_MET();

            // Else this is a subsequent bid:
        } else {
            // Cache the highest bid
            uint256 highestBid = _auction.highestBid;

            // Used to store the minimum bid required
            uint256 minBid;

            // Cannot realistically overflow
            unchecked {
                // Compute the minimum bid
                minBid = highestBid + ((highestBid * settings.minBidIncrement) / 100);
            }

            // Ensure the incoming bid meets the minimum
            if (msg.value < minBid) revert MINIMUM_BID_NOT_MET();

            // Refund the previous bidder
            _handleOutgoingTransfer(highestBidder, highestBid);
        }

        // Store the new highest bid
        auction.highestBid = msg.value;

        // Store the new highest bidder
        auction.highestBidder = msg.sender;

        // Used to store if the auction will be extended
        bool extend;

        // Cannot underflow as `_auction.endTime` is ensured to be greater than the current time above
        unchecked {
            // Compute whether the time remaining is less than the buffer
            extend = (_auction.endTime - block.timestamp) < settings.timeBuffer;
        }

        // If the time remaining is within the buffer:
        if (extend) {
            // Cannot realistically overflow
            unchecked {
                // Extend the auction by the time buffer
                auction.endTime = uint40(block.timestamp + settings.timeBuffer);
            }
        }

        emit AuctionBid(_tokenId, msg.sender, msg.value, extend, auction.endTime);
    }

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L323-L327

    function setTimeBuffer(uint256 _timeBuffer) external onlyOwner {
        settings.timeBuffer = SafeCast.toUint40(_timeBuffer);

        emit TimeBufferUpdated(_timeBuffer);
    }

Proof of Concept

Please append the following test in test\Auction.t.sol. This test will pass to demonstrate the described scenario.

    function test_BidOnSameAuctionWithDifferentTimeBuffers() public {
        vm.prank(founder);
        auction.unpause();

        vm.prank(bidder1);
        auction.createBid{ value: 0.5 ether }(2);

        vm.warp(6 minutes);

        vm.prank(bidder2);
        auction.createBid{ value: 1 ether }(2);

        // extension by 5 minutes is as expected because it is set when creating the auction
        (, , , , uint256 endTime1, ) = auction.auction();
        assertEq(endTime1, 6 minutes + 5 minutes);

        vm.warp(10 minutes);

        // time buffer is changed before the auction is over
        vm.prank(address(treasury));
        auction.setTimeBuffer(1 days);

        vm.prank(bidder1);
        auction.createBid{ value: 2 ether }(2);

        // bidders now face extension by 1 day instead of the expected 5 minutes
        (, , , , uint256 endTime2, ) = auction.auction();
        assertEq(endTime2, 10 minutes + 1 days);
    }

Tools Used

VSCode

Recommended Mitigation Steps

The settings.timeBuffer used when creating the auction can be stored in the corresponding auction struct. When calling createBid, the stored time buffer for the auction, instead of the settings.timeBuffer at that moment, can be compared against for determing whether the auction's deadline should be extended or not.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L466-L470 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L116-L175

Vulnerability details

Impact

At the early stage of the deployed DAO, it is possible that the following proposalThreshold function returns 0 because the token supply is low.

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L466-L470

    function proposalThreshold() public view returns (uint256) {
        unchecked {
            return (settings.token.totalSupply() * settings.proposalThresholdBps) / 10_000;
        }
    }

When calling the following propose function, if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD() is executed. If proposalThreshold() returns 0, calling propose will not revert. As a result, even if the proposer owns no token, she or he can still be able to create a proposal when the token supply is low.

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L116-L175

    function propose(
        address[] memory _targets,
        uint256[] memory _values,
        bytes[] memory _calldatas,
        string memory _description
    ) external returns (bytes32) {
        // Get the current proposal threshold
        uint256 currentProposalThreshold = proposalThreshold();

        // Cannot realistically underflow and `getVotes` would revert
        unchecked {
            // Ensure the caller's voting weight is greater than or equal to the threshold
            if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();
        }

        // Cache the number of targets
        uint256 numTargets = _targets.length;

        // Ensure at least one target exists
        if (numTargets == 0) revert PROPOSAL_TARGET_MISSING();

        // Ensure the number of targets matches the number of values and calldata
        if (numTargets != _values.length) revert PROPOSAL_LENGTH_MISMATCH();
        if (numTargets != _calldatas.length) revert PROPOSAL_LENGTH_MISMATCH();

        // Compute the description hash
        bytes32 descriptionHash = keccak256(bytes(_description));

        // Compute the proposal id
        bytes32 proposalId = hashProposal(_targets, _values, _calldatas, descriptionHash);

        // Get the pointer to store the proposal
        Proposal storage proposal = proposals[proposalId];

        // Ensure the proposal doesn't already exist
        if (proposal.voteStart != 0) revert PROPOSAL_EXISTS(proposalId);

        // Used to store the snapshot and deadline
        uint256 snapshot;
        uint256 deadline;

        // Cannot realistically overflow
        unchecked {
            // Compute the snapshot and deadline
            snapshot = block.timestamp + settings.votingDelay;
            deadline = snapshot + settings.votingPeriod;
        }

        // Store the proposal data
        proposal.voteStart = uint32(snapshot);
        proposal.voteEnd = uint32(deadline);
        proposal.proposalThreshold = uint32(currentProposalThreshold);
        proposal.quorumVotes = uint32(quorum());
        proposal.proposer = msg.sender;
        proposal.timeCreated = uint32(block.timestamp);

        emit ProposalCreated(proposalId, _targets, _values, _calldatas, _description, descriptionHash, proposal);

        return proposalId;
    }

Proof of Concept

Please append the following test in test\Gov.t.sol. This test will pass to demonstrate the described scenario.

    function test_CreateProposalWithZeroTokens() public {
        createUsers(1, 0);
        mintVoter1();

        (address[] memory targets, uint256[] memory values, bytes[] memory calldatas) = mockProposal();

        bytes32 descriptionHash = keccak256(bytes(""));
        bytes32 proposalId = governor.hashProposal(targets, values, calldatas, descriptionHash);

        // otherUsers[0] does not own any tokens
        assertEq(token.balanceOf(otherUsers[0]), 0);

        // token supply is only 4 at this moment
        assertEq(token.totalSupply(), 4);

        // the calculated proposal threshold is 0 because the token supply is low
        assertEq((token.totalSupply() * governor.proposalThresholdBps()) / 10_000, 0);

        // otherUsers[0], who does not own any tokens, can create a proposal
        vm.prank(otherUsers[0]);
        bytes32 returnedProposalId = governor.propose(targets, values, calldatas, "");

        // the proposal is created successfully
        assertEq(proposalId, returnedProposalId);
        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Pending));

        Proposal memory proposal = governor.getProposal(proposalId);

        // proposalThreshold corresponding to the proposal is 0
        assertEq(proposal.proposalThreshold, 0);

        // proposer of the proposal is otherUsers[0]
        assertEq(proposal.proposer, otherUsers[0]);
    }

Tools Used

VSCode

Recommended Mitigation Steps

A minimum proposal threshold governance configuration that is at least 1 can be added. When proposalThreshold() returns 0 because the token supply is low, calling propose could still revert if getVotes(msg.sender, block.timestamp - 1) is less than the minimum proposal threshold.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/types/TokenTypesV1.sol#L16-L22 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L71-L126 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L235-L237

Vulnerability details

Impact

In the following TokenTypesV1.Settings struct, numFounders is an uint8. Its maximum value is 255.

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/types/TokenTypesV1.sol#L16-L22

    struct Settings {
        address auction;
        uint96 totalSupply;
        IBaseMetadata metadataRenderer;
        uint8 numFounders;
        uint8 totalOwnership;
    }

When an organization is crowdfunded or is a collaboration of multiple groups, it is possible that the the organization has hundreds of founders. If the deployed DAO has more than 255 founders for which at most 100 of them have the percentages of token ownership specified, then calling the following _addFounders function would assign an unexpected value to settings.numFounders. Because of the executions of uint256 numFounders = _founders.length and settings.numFounders = uint8(numFounders) within the unchecked block, the conversion from uint256 to uint8 occurs, and settings.numFounders could be overflowed to 0 when numFounders is 256. Therefore, calling the totalFounders function below would return an incorrect number of founders when there are more than 255 of them.

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

    function _addFounders(IManager.FounderParams[] calldata _founders) internal {
        // Cache the number of founders
        uint256 numFounders = _founders.length;

        // Used to store the total percent ownership among the founders
        uint256 totalOwnership;

        unchecked {
            // For each founder:
            for (uint256 i; i < numFounders; ++i) {
                // Cache the percent ownership
                uint256 founderPct = _founders[i].ownershipPct;

                // Continue if no ownership is specified
                if (founderPct == 0) continue;

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

                // Compute the founder's id
                uint256 founderId = settings.numFounders++;

                // Get the pointer to store the founder
                Founder storage newFounder = founder[founderId];

                // Store the founder's vesting details
                newFounder.wallet = _founders[i].wallet;
                newFounder.vestExpiry = uint32(_founders[i].vestExpiry);
                newFounder.ownershipPct = uint8(founderPct);

                // Compute the vesting schedule
                uint256 schedule = 100 / founderPct;

                // Used to store the base token id the founder will recieve
                uint256 baseTokenId;

                // For each token to vest:
                for (uint256 j; j < founderPct; ++j) {
                    // Get the available token id
                    baseTokenId = _getNextTokenId(baseTokenId);

                    // Store the founder as the recipient
                    tokenRecipient[baseTokenId] = newFounder;

                    emit MintScheduled(baseTokenId, founderId, newFounder);

                    // Update the base token id
                    (baseTokenId += schedule) % 100;
                }
            }

            // Store the founders' details
            settings.totalOwnership = uint8(totalOwnership);
            settings.numFounders = uint8(numFounders);
        }
    }

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

    function totalFounders() external view returns (uint256) {
        return settings.numFounders;
    }

Proof of Concept

Please append the following test in test\Token.t.sol. This test will pass to demonstrate the described scenario.

    function test_256Founders() public {
        uint256 founderCount = 256;

        createUsers(founderCount, 1 ether);

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

        unchecked {
            for (uint256 i; i < founderCount; ++i) {
                wallets[i] = otherUsers[i];
                if (i == 0) {
                    percents[i] = 20;
                } else if (i == founderCount - 1) {
                    percents[i] = 30;
                } else {
                    percents[i] = 0;
                }
                vestExpirys[i] = 4 weeks;
            }
        }

        deployWithCustomFounders(wallets, percents, vestExpirys);
        assertEq(token.totalFounderOwnership(), 50);

        // when there are 256 founders, the value returned by totalFounders function overflows to 0
        assertEq(token.totalFounders(), 0);
    }

Tools Used

VSCode

Recommended Mitigation Steps

Based on the current implementation, the founder and tokenRecipient mappings only include the founders who have the specified percentages of token ownership. To be consistent, settings.numFounders = uint8(numFounders) can be removed from the _addFounders function because settings.numFounders++ is executed in the _addFounders function's for (uint256 i; i < numFounders; ++i) loop already.

If the totalFounders function must return the number of all founders, which can be more than 255, then https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/types/TokenTypesV1.sol#L16-L22 can be updated to the following code,

    struct Settings {
        address auction;
        uint96 totalSupply;
        IBaseMetadata metadataRenderer;
        uint256 numFounders;
        uint8 totalOwnership;
    }

and, https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L124 can be changed to the following code.

            settings.numFounders = numFounders;

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L413-L456

Vulnerability details

Impact

When determining the proposal's state, the following state function is called, which can execute else if (proposal.forVotes < proposal.againstVotes || proposal.forVotes < proposal.quorumVotes) { return ProposalState.Defeated; }. If proposal.forVotes and proposal.againstVotes are the same, the proposal is not considered defeated when the quorum votes are reached by the for votes. However, many electoral systems require that the for votes to be more than the against votes in order to conclude that the proposal is passed because the majority of votes supports it. If the deployed DAO wants to require the majority of votes to support a proposal in order to pass it, the state function would incorrectly conclude that the proposal is not defeated when the for votes and against votes are the same at the end of voting. As a result, critical proposals, such as for updating implementations or withdrawing funds from the treasury, that should not be passed can be passed, or vice versa, so the impact can be huge.

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L413-L456

    function state(bytes32 _proposalId) public view returns (ProposalState) {
        // Get a copy of the proposal
        Proposal memory proposal = proposals[_proposalId];

        // Ensure the proposal exists
        if (proposal.voteStart == 0) revert PROPOSAL_DOES_NOT_EXIST();

        // If the proposal was executed:
        if (proposal.executed) {
            return ProposalState.Executed;

            // Else if the proposal was canceled:
        } else if (proposal.canceled) {
            return ProposalState.Canceled;

            // Else if the proposal was vetoed:
        } else if (proposal.vetoed) {
            return ProposalState.Vetoed;

            // Else if voting has not started:
        } else if (block.timestamp < proposal.voteStart) {
            return ProposalState.Pending;

            // Else if voting has not ended:
        } else if (block.timestamp < proposal.voteEnd) {
            return ProposalState.Active;

            // Else if the proposal failed (outvoted OR didn't reach quorum):
        } else if (proposal.forVotes < proposal.againstVotes || proposal.forVotes < proposal.quorumVotes) {
            return ProposalState.Defeated;

            // Else if the proposal has not been queued:
        } else if (settings.treasury.timestamp(_proposalId) == 0) {
            return ProposalState.Succeeded;

            // Else if the proposal can no longer be executed:
        } else if (settings.treasury.isExpired(_proposalId)) {
            return ProposalState.Expired;

            // Else the proposal is queued
        } else {
            return ProposalState.Queued;
        }
    }

Proof of Concept

Please append the following test in test\Gov.t.sol. This test will pass to demonstrate the described scenario.

    function test_ProposalIsSucceededWhenNumberOfForAndAgainstVotesAreSame() public {
        vm.prank(founder);
        auction.unpause();

        createVoters(7, 5 ether);

        vm.prank(address(treasury));
        governor.updateQuorumThresholdBps(2000);

        bytes32 proposalId = createProposal();

        vm.warp(block.timestamp + governor.votingDelay());

        // number of for and against votes are both 2
        castVotes(proposalId, 2, 2, 3);

        vm.warp(block.timestamp + governor.votingPeriod());

        // the proposal is considered succeeded when number of for and against votes are the same after voting ends
        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Succeeded));

        // the proposal can be queued afterwards
        governor.queue(proposalId);
        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Queued));
    }

Tools Used

VSCode

Recommended Mitigation Steps

If there is no need to pass a proposal when proposal.forVotes and proposal.againstVotes are the same at the end of voting, then https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L441-L442 can be changed to the following code.

        } else if (proposal.forVotes <= proposal.againstVotes || proposal.forVotes < proposal.quorumVotes) {
            return ProposalState.Defeated;

Otherwise, a governance configuration can be added to indicate whether the majority of votes is needed or not for supporting and passing a proposal. The state function then could return ProposalState.Defeated when proposal.forVotes <= proposal.againstVotes if so and when proposal.forVotes < proposal.againstVotes if not.

[L-01] TOKEN CAN BE LOCKED IF SENDING TO A CONTRACT

Currently, as shown below, token.transferFrom is called to transfer the token to a receiver address. If the receiver is a contract that does not support the ERC721 protocol, the token can be locked and cannot be retrieved. To prevent this from happening, the ERC721 safeTransferFrom function could be used instead of the transferFrom function.

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L192

token.transferFrom(address(this), _auction.highestBidder, _auction.tokenId);

[L-02] TOKEN CAN BE LOCKED WHEN MINTING IT TO A CONTRACT

Currently, as shown below, super._mint is called to mint the token to a receiver address. If the receiver is a contract that does not support the ERC721 protocol, the token can be locked and cannot be retrieved. To mitigate this risk, the ERC721 _safeMint function could be used instead of the _mint function. It looks like that the _safeMint function is currently not found in the relevant contracts in the codebase; please consider adding one in the Token contract for using it.

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

    function _mint(address _to, uint256 _tokenId) internal override {
        // Mint the token
        super._mint(_to, _tokenId);

        // Generate the token attributes
        if (!settings.metadataRenderer.onMinted(_tokenId)) revert NO_METADATA_GENERATED();
    }

[L-03] CANCEL OR VETO FUNCTION CAN BE CALLED TO CANCEL OR VETO CANCELED, VETOED, AND EXPIRED PROPOSALS

The following cancel or veto function can be called to cancel a proposal that is already canceled, vetoed, or expired, which can be meaningless. Emitting the ProposalCanceled and ProposalVetoed events for these proposals that are already canceled, vetoed, and expired can spam the frontend with meaningless information. When calling these functions, please consider requiring the proposal to be not canceled, vetoed, or expired.

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L353-L377

    function cancel(bytes32 _proposalId) external {
        // Ensure the proposal hasn't been executed
        if (state(_proposalId) == ProposalState.Executed) revert PROPOSAL_ALREADY_EXECUTED();

        // Get a copy of the proposal
        Proposal memory proposal = proposals[_proposalId];

        // Cannot realistically underflow and `getVotes` would revert
        unchecked {
            // Ensure the caller is the proposer or the proposer's voting weight has dropped below the proposal threshold
            if (msg.sender != proposal.proposer && getVotes(proposal.proposer, block.timestamp - 1) > proposal.proposalThreshold)
                revert INVALID_CANCEL();
        }

        // Update the proposal as canceled
        proposals[_proposalId].canceled = true;

        // If the proposal was queued:
        if (settings.treasury.isQueued(_proposalId)) {
            // Cancel the proposal
            settings.treasury.cancel(_proposalId);
        }

        emit ProposalCanceled(_proposalId);
    }

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L385-L405

    function veto(bytes32 _proposalId) external {
        // Ensure the caller is the vetoer
        if (msg.sender != settings.vetoer) revert ONLY_VETOER();

        // Ensure the proposal has not been executed
        if (state(_proposalId) == ProposalState.Executed) revert PROPOSAL_ALREADY_EXECUTED();

        // Get the pointer to the proposal
        Proposal storage proposal = proposals[_proposalId];

        // Update the proposal as vetoed
        proposal.vetoed = true;

        // If the proposal was queued:
        if (settings.treasury.isQueued(_proposalId)) {
            // Cancel the proposal
            settings.treasury.cancel(_proposalId);
        }

        emit ProposalVetoed(_proposalId);
    }

[L-04] CONTRACT CONSTRUCTORS ARE PAYABLE

The following constructor are payable. For someone who is not familar with the protocol, it is possible to send ETH to these contracts during constructions. The sent ETH will be locked in these contracts. To prevent this, please consider removing payable from these constructor.

auction\Auction.sol
  39: constructor(address _manager, address _weth) payable initializer {  

governance\governor\Governor.sol
  41: constructor(address _manager) payable initializer {  

governance\treasury\Treasury.sol
  32: constructor(address _manager) payable initializer {  

manager\Manager.sol
  61: ) payable initializer {

token\metadata\MetadataRenderer.sol
  32: constructor(address _manager) payable initializer {  

[L-05] _proposalThresholdBps AND _quorumThresholdBps CAN BE FURTHER CHECKED

The sensible range of values for the following settings.proposalThresholdBps and settings.quorumThresholdBps would be > 0 and <= 10_000. Please consider checking the values of _proposalThresholdBps and _quorumThresholdBps before using them to set settings.proposalThresholdBps and settings.quorumThresholdBps to prevent unintended behaviors.

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L79-L80

        settings.proposalThresholdBps = SafeCast.toUint16(_proposalThresholdBps);
        settings.quorumThresholdBps = SafeCast.toUint16(_quorumThresholdBps);

[L-06] MISSING ZERO-ADDRESS CHECK FOR CRITICAL ADDRESSES

To prevent unintended behaviors, the critical address inputs should be checked against address(0). Please consider checking the address variables in the following constructor.

auction\Auction.sol
  39: constructor(address _manager, address _weth) payable initializer {  

governance\governor\Governor.sol
  41: constructor(address _manager) payable initializer {  

governance\treasury\Treasury.sol
  32: constructor(address _manager) payable initializer {  

lib\proxy\ERC1967Proxy.sol
  21: constructor(address _logic, bytes memory _data) payable {

manager\Manager.sol
  55: constructor(
        address _tokenImpl,
        address _metadataImpl,
        address _auctionImpl,
        address _treasuryImpl,
        address _governorImpl

token\Token.sol
  30: constructor(address _manager) payable initializer {

token\metadata\MetadataRenderer.sol
  32: constructor(address _manager) payable initializer {  

[N-01] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

To improve readability and maintainability, constants can be used instead of the magic numbers. Please consider replacing the magic numbers used in the following code with constants.

auction\Auction.sol
  80: settings.timeBuffer = 5 minutes;
  81: settings.minBidIncrement = 10;
  119: minBid = highestBid + ((highestBid * settings.minBidIncrement) / 100);

governance\governor\Governor.sol
  468: return (settings.token.totalSupply() * settings.proposalThresholdBps) / 10_000;
  475: return (settings.token.totalSupply() * settings.quorumThresholdBps) / 10_000;

governance\treasury\Treasury.sol
  57: settings.gracePeriod = 2 weeks;

[N-02] INCOMPLETE NATSPEC COMMENTS

NatSpec comments provide rich code documentation. @param or @return comments are missing for the following functions. Please consider completing NatSpec comments for them.

governance\governor\Governor.sol
  184: function castVote(bytes32 _proposalId, uint256 _support) external returns (uint256) {   
  192: function castVoteWithReason(   
  208: function castVoteBySig(   
  248: function _castVote(   
  305: function queue(bytes32 _proposalId) external returns (uint256 eta) {   
  324: function execute(   
  413: function state(bytes32 _proposalId) public view returns (ProposalState) {   
  461: function getVotes(address _account, uint256 _timestamp) public view returns (uint256) {    
  466: function proposalThreshold() public view returns (uint256) {    
  473: function quorum() public view returns (uint256) {    
  481: function getProposal(bytes32 _proposalId) external view returns (Proposal memory) {    
  487: function proposalSnapshot(bytes32 _proposalId) external view returns (uint256) {    
  493: function proposalDeadline(bytes32 _proposalId) external view returns (uint256) {    
  499: function proposalVotes(bytes32 _proposalId)    
  515: function proposalEta(bytes32 _proposalId) external view returns (uint256) {    
  524: function proposalThresholdBps() external view returns (uint256) {    
  529: function quorumThresholdBps() external view returns (uint256) {    
  534: function votingDelay() external view returns (uint256) {    
  539: function votingPeriod() external view returns (uint256) {    
  544: function vetoer() external view returns (address) {    
  549: function token() external view returns (address) {    
  554: function treasury() external view returns (address) {    

governance\treasury\Treasury.sol
  68: function timestamp(bytes32 _proposalId) external view returns (uint256) {   
  74: function isExpired(bytes32 _proposalId) external view returns (bool) {   
  82: function isQueued(bytes32 _proposalId) public view returns (bool) {   
  88: function isReady(bytes32 _proposalId) public view returns (bool) {   
  101: function hashProposal(   
  116: function queue(bytes32 _proposalId) external onlyOwner returns (uint256 eta) {   
  195: function delay() external view returns (uint256) {   
  200: function gracePeriod() external view returns (uint256) {   
  237: function onERC721Received(   
  247: function onERC1155Received(   
  258: function onERC1155BatchReceived(   

token\Token.sol
  221: function tokenURI(uint256 _tokenId) public view override(IToken, ERC721) returns (string memory) {  
  226: function contractURI() public view override(IToken, ERC721) returns (string memory) {  
  235: function totalFounders() external view returns (uint256) {  
  240: function totalFounderOwnership() external view returns (uint256) {  
  246: function getFounder(uint256 _founderId) external view returns (Founder memory) {  
  251: function getFounders() external view returns (Founder[] memory) {  
  270: function getScheduledRecipient(uint256 _tokenId) external view returns (Founder memory) {  
  279: function totalSupply() external view returns (uint256) {  
  284: function auction() external view returns (address) {  
  289: function metadataRenderer() external view returns (address) {  
  294: function owner() public view returns (address) {  

token\metadata\MetadataRenderer.sol
  77: function propertiesCount() external view returns (uint256) {    
  83: function itemsCount(uint256 _propertyId) external view returns (uint256) {    
  171: function onMinted(uint256 _tokenId) external returns (bool) {    
  206: function getAttributes(uint256 _tokenId) public view returns (bytes memory aryAttributes, bytes memory queryString) {    
  250: function _generateSeed(uint256 _tokenId) private view returns (uint256) {    
  255: function _getItemImage(Item memory _item, string memory _propertyName) private view returns (string memory) {    
  269: function contractURI() external view returns (string memory) {    
  286: function tokenURI(uint256 _tokenId) external view returns (string memory) {    
  308: function _encodeAsJson(bytes memory _jsonBlob) private pure returns (string memory) {    
  317: function token() external view returns (address) {    
  322: function treasury() external view returns (address) {    
  327: function contractImage() external view returns (string memory) {    
  332: function rendererBase() external view returns (string memory) {    
  337: function description() external view returns (string memory) {