0x52 - MerkleReserveMinter minting methodology is incompatible with current governance structure and can lead to migrated DAOs being hijacked immediately

[sherlock-admin2]

0x52

medium

MerkleReserveMinter minting methodology is incompatible with current governance structure and can lead to migrated DAOs being hijacked immediately

Summary

MerkleReserveMinter allows large number of tokens to be minted instantaneously which is incompatible with the current governance structure which relies on tokens being minted individually and time locked after minting by the auction. By minting and creating a proposal in the same block a user is able to create a proposal with significantly lower quorum than expected. This could easily be used to hijack the migrated DAO.

Vulnerability Detail

MerkleReserveMinter.sol#L154-L167

unchecked {
    for (uint256 i = 0; i < claimCount; ++i) {
        // Load claim in memory
        MerkleClaim memory claim = claims[I];

        // Requires one proof per tokenId to handle cases where users want to partially claim
        if (!MerkleProof.verify(claim.merkleProof, settings.merkleRoot, keccak256(abi.encode(claim.mintTo, claim.tokenId)))) {
            revert INVALID_MERKLE_PROOF(claim.mintTo, claim.merkleProof, settings.merkleRoot);
        }

        // Only allowing reserved tokens to be minted for this strategy
        IToken(tokenContract).mintFromReserveTo(claim.mintTo, claim.tokenId);
    }
}

When minting from the claim merkle tree, a user is able to mint as many tokens as they want in a single transaction. This means in a single transaction, the supply of the token can increase very dramatically. Now we'll take a look at the governor contract as to why this is such an issue.

Governor.sol#L184-L192

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

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

Governor.sol#L495-L499

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

When creating a proposal, we see that it uses a snapshot of the CURRENT total supply. This is what leads to the issue. The setup is fairly straightforward and occurs all in a single transaction:

1) Create a malicious proposal (which snapshots current supply) 2) Mint all the tokens 3) Vote on malicious proposal with all minted tokens

The reason this works is because the quorum is based on the supply before the mint while votes are considered after the mint, allowing significant manipulation of the quorum.

Impact

DOA can be completely hijacked

Code Snippet

MerkleReserveMinter.sol#L129-L173

Tool used

Manual Review

Recommendation

Token should be changed to use a checkpoint based total supply, similar to how balances are handled. Quorum should be based on that instead of the current supply.

[neokry]

This is a valid issue but makes a big assumption that a malicious user is included in the merkle tree with a significant share of reserved tokens and the DAO has no veto set. This might be too much of an edge case to make the recommended changes to the token and governor contracts.

[neokry]

Fixed here: https://github.com/ourzora/nouns-protocol/pull/124

As noted in this PR we added a governance delay to fix the more significant issue of a DAO without veto potentially being hijacked. A delay will also help alleviate some of the issues related to claiming and quorum but not outright fix. For quorum issues we will warn DAOs around the risks of setting a high amount of votes for single users.

[Oot2k]

Escalate

I think this issue should be high instead of medium. The attack path can be simply executed by anyone and there is no way to prevent it.

The only requirement would be that there is no veto, but this is a feature not a requirement, which makes this a high instead of medium. Also see #155 for reasoning

As @nevillehuang mentions in the comment on #52 this should be more an High instead.

[sherlock-admin2]

Escalate

I think this issue should be high instead of medium. The attack path can be simply executed by anyone and there is no way to prevent it.

The only requirement would be that there is no veto, but this is a feature not a requirement, which makes this a high instead of medium. Also see #155 for reasoning

As @nevillehuang mentions in the comment on #52 this should be more an High instead.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[neokry]

Escalate

I think this issue should be high instead of medium. The attack path can be simply executed by anyone and there is no way to prevent it.

The only requirement would be that there is no veto, but this is a feature not a requirement, which makes this a high instead of medium. Also see #155 for reasoning

As @nevillehuang mentions in the comment on #52 this should be more an High instead.

the attack can only be executed by users in the merkle tree which is controlled by the admin. most DAOs also have veto set. there is no loss of funds if the merkle claim has no price per token and deployer can simply redeploy the DAO and remove the malicious user form the merkle tree.

[nevillehuang]

I see your point @Oot2k but I agree with sponsor, this issue is dependent on a NounsDAO type governance with no veto which I believe is unlikely, so will leave it up to @Czar102 to decide severity.

[Czar102]

Is the new DAO being created holding any funds? Or is it said that it shouldn't hold any until settings.mintEnd? @neokry @nevillehuang

[neokry]

DAOs are not holding any funds upon creation but it could receive funds if price per token is set and users have claimed tokens

[nevillehuang]

@Czar102 I only assigned this medium severity on the condition that veto are not set for a nounsDAO type governance which is unrealistic, but if you disagree, I could also see why this could be high severity given the impact it has.

[Oot2k]

I wanted to add to sponsor comment that after migration the DAO indeed holds funds, the total layer 1 treasury.

So for example: DAO with 100 ETH is migrated, all old users get chance to mint tokens for free and no veto set because nouns is designed to delete the veto at some point.

In this case the 100 ETH can be instantly drained.

[nevillehuang]

@Oot2k Can you outline how the funds can be immediately drained? If true this definitely could be high severity.

From my understanding it has something to do with adjusting the parameters to allow instant execution of proposals correct?

[Oot2k]

In #155 we tried to outline it.

Migration happens like this:

1. DAO on layer 1 is stoped
2. Merkeltree with mint config is created
3. DAO on layer 2 is deployed
4. Treasury of DAO is transferred from layer1 to layer2
5. First user to mint quorum tokens can create proposal to drain DAO
6. Because of the nature of OZ governance only tokens minted at time of proposal can participate in governance. -> drain can not be stopped because only the malicious user can vote and there is no way to "save" treasury

[nevillehuang]

In #155 we tried to outline it.

Migration happens like this:

1. DAO on layer 1 is stoped
2. Merkeltree with mint config is created
3. DAO on layer 2 is deployed
4. Treasury of DAO is transferred from layer1 to layer2
5. First user to mint quorum tokens can create proposal to drain DAO
6. Because of the nature of OZ governance only tokens minted at time of proposal can participate in governance. -> drain can not be stopped because only the malicious user can vote and there is no way to "save" treasury

Is there a proposal delay/duration that can be changed by the malicious users? If so it will be helpful if you point me to the code logic. If not I think the other members of the DAO would have sufficient time to react to proposals.

[neokry]

The treasury migration step has to go through the full layer 1 DAO governance and the DAO can decide when to submit that proposal. it doesn’t happen immediately after the L2 DAO is deployed. An L1 DAO might also choose to only migrate a portion of their treasury as well to ensure the L2 DAO runs smoothly for a period of time

[Oot2k]

In #155 we tried to outline it. Migration happens like this:

1. DAO on layer 1 is stoped
2. Merkeltree with mint config is created
3. DAO on layer 2 is deployed
4. Treasury of DAO is transferred from layer1 to layer2
5. First user to mint quorum tokens can create proposal to drain DAO
6. Because of the nature of OZ governance only tokens minted at time of proposal can participate in governance. -> drain can not be stopped because only the malicious user can vote and there is no way to "save" treasury

Is there a proposal delay/duration that can be changed by the malicious users? If so it will be helpful if you point me to the code logic. If not I think the other members of the DAO would have sufficient time to react to proposals.

They cant react. OZ governance takes the votes from the timestamp. This report includes all important code snippets. Even if the proposal has an execution time of 10 weeks it does not matter because every proposal created after gets executed after the drain.

Sponsors comments are right, ofc the DAO can migrate only a part of funds, but that does not lower the severity. Its still highly realistic that all funds are send at ones.

And in reality I think no one will claim tokens in a DAO that has no treasury, so the possibility is quite high.

[neokry]

for full context on migration we have a bot setup to airdrop DAO tokens for migrated DAOs. Our recommendation to all DAOs will be to execute the migration call wait for tokens to be airdropped and pass a proposal to unpause auctions on L2 before sending the treasury to the L2 DAO. there is a chance this attack could be executed before the bot airdrops the tokens which is why we’ve added the governance delay as a fix. also a malicious user would need to immediately submit this proposal to the DAO making it obvious if a DAO will be captured ie they can easily choose not to send the treasury.

[Czar102]

I think sending any DAO funds to the L2 DAO would be irresponsible if the L2 DAO isn't set up yet. I would be considered an admin error in my opinion. @neokry would you agree?

DAOs are not holding any funds upon creation but it could receive funds if price per token is set and users have claimed tokens

I think mint fees could be stolen. The attacker could wait for as many tokens as possible would be bought and then mint all their tokens (so that they will have a majority and satisfy quorum) and create a proposal to steal all mint fees. From my understanding, they can also hijack the governance, but it wouldn't hold any other funds at that time.

Is my understanding accurate? I would also like to get to know why do you perceive having no veto as likely/unlikely.

[nevillehuang]

@Czar102 I am basing it off of the original NounsDAO governance, where veto is a core role present configured by the admin to prevent malicious proposals.

[neokry]

I agree on your first point @Czar102 . regarding stealing mint fees the attack only works if the attackers vote power is greater than the voting power of active voters. ie if they wait for the majority of users to claim their mint funds they have a higher chance of their proposal to steal funds be voted down.

also agree with @nevillehuang while veto is optional the veto is used by most DAOs and we strongly encouraged DAOs to set it up to prevent governance attacks like this.

[Czar102]

My thoughts after some internal discussions:

• This issue presents a loss of funds scenario, which is limited by the number of tokens an attacker can gather (when there is no frontrunning, frontrunning also has a limited impact). This is quite constrained, so is the loss. The DAO shouldn't hold any funds other than buy-ins at that time.
• If there is no vetoer and the attacker does the attack right, I believe there is no way to recover funds for addresses who bought in.
• It is reasonable to assume that there will be no vetoer because of this fragment of the code, hence this finding should be at least a medium. It feels like that role is highly advisable to be assigned at least in the very first moments of the DAO when it's being set up (and tokens are being bought, etc.), so I believe a lack of the vetoer is an assumption concerning the state of the DAO.

Based on the above points, I think it is a borderline Med/High. I am leaning towards leaving it a medium.

[Oot2k]

I want to quote the scenario where a DAOs treasury is transferred before the reserve mint ends. The code/docs do not mention in any way that the mint of tokens happens before the treasury is transferred. In this case the impact is detrimental. I still think this is a high considering the different ways the issue impacts governance manipulation.

[Czar102]

I think it would be a setup mistake to send funds to an "uninitialized" DAO.

[Czar102]

Result: Medium Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• Oot2k: rejected

[IAm0x52]

Fix looks good. A governance delay has been added to the governor contract. Proposals can only be created after either 1) all reserve tokens have been minted or 2) the delay has passed.