0xG0P1 - A user can claim tokens from the MerkleReserveMinter contract multiple times because the burned tokens can be minted again.

[sherlock-admin2]

0xG0P1

medium

A user can claim tokens from the MerkleReserveMinter contract multiple times because the burned tokens can be minted again.

Summary

A user can claim tokens from the MerkleReserveMinter after migration by providing a Merkle proof within the claiming period. The claimed tokens can be transferred to a minter, who has the capability to burn the tokens. If this burning process occurs within the claiming period, the user can subsequently reclaim the burned tokens by submitting the original Merkle proof utilized during the initial claiming.

Vulnerability Detail

"The process of claiming tokens through the MerkleReserveMinter contract involves users calling the mintFromReserve function, which executes the following tasks:

1. Calculates the fee based on the token price.
2. Verifies the Merkle proof provided by the user.
3. Mints the corresponding token if the provided proof is legitimate.

However, a critical vulnerability exists in the absence of a sanity check to ascertain whether the token has already been claimed. This loophole poses a security risk, allowing users to exploit the following sequence of events:

1. Bob claims tokenId(5) by providing the necessary Merkle proof, establishing ownership of tokenId(5).
2. Subsequently, Bob transfers tokenId(5) to Charlie, who is a designated minter.
3. As a minter, Charlie has the ability to burn tokens, and he proceeds to burn tokenId(5) within the claiming period.
4. Despite the burning of tokenId(5), the absence of a check in the MerkleReserveMinter contract allows Bob to reclaim tokenId(5) by submitting the same Merkle proof used initially. This oversight enables Bob to regain ownership of tokenId(5).

To address this vulnerability, it is crucial to implement a check within the mintFromReserve function to ensure that a token has not been previously claimed before allowing the transaction to proceed.
Note : Ive Already Discussed this issue with the dev @neokry and he confirmed its a valid issue`
https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/minters/MerkleReserveMinter.sol#L154-L167
https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L211-L217
https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/lib/token/ERC721.sol#L191-L207
https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L293-L300

Impact

This vulnerability has significant implications, particularly in the context of voting power. Assuming a one-to-one correspondence between votes and tokens (1 vote = 1 token), the impact on voting power is notable. If Bob initially owns only one token and transfers it to a minter, resulting in the token being burned, Bob theoretically should have no votes. However, due to the identified vulnerability, Bob can exploit the situation by reclaiming the burned token, thereby increasing his voting power by one vote.

The potential consequences are particularly pronounced in scenarios where users are required to secure tokens through competitive auctions, bidding the highest amount. The vulnerability introduces the risk of users obtaining tokens at a significantly lower cost by reclaiming tokens that were initially burned by the minter. This undermines the integrity of the token acquisition process and compromises the expected correlation between token value and the investment made during the auction.

Code Snippet

Test :

    function test_MintFlow() public {
        deployAltMock(20);

        bytes32 root = bytes32(0x5e0da80989496579de029b8ad2f9c234e8de75f5487035210bfb7676e386af8b);

        MerkleReserveMinter.MerkleMinterSettings memory settings = MerkleReserveMinter.MerkleMinterSettings({
            mintStart: 0,
            mintEnd: uint64(block.timestamp + 1000),
            pricePerToken: 0 ether,
            merkleRoot: root
        });

        vm.prank(address(founder));
        minter.setMintSettings(address(token), settings);

        (uint64 mintStart, uint64 mintEnd, uint64 pricePerToken, bytes32 merkleRoot) = minter.allowedMerkles(address(token));
        assertEq(mintStart, settings.mintStart);
        assertEq(mintEnd, settings.mintEnd);
        assertEq(pricePerToken, settings.pricePerToken);
        assertEq(merkleRoot, settings.merkleRoot);

        TokenTypesV2.MinterParams memory params = TokenTypesV2.MinterParams({ minter: address(minter), allowed: true });
        TokenTypesV2.MinterParams memory params1 = TokenTypesV2.MinterParams({ minter: claimer1, allowed: true }); // Setting the 
 // Claimer1 as the minter so that he can burn the claimed token for making the test simple.
        TokenTypesV2.MinterParams[] memory minters = new TokenTypesV2.MinterParams[](2);
        minters[0] = params;
        minters[1] = params1;
        vm.prank(address(founder));
        token.updateMinters(minters);

        bytes32[] memory proof = new bytes32[](1);
        proof[0] = bytes32(0xd77d6d8eeae66a03ce8ecdba82c6a0ce9cff76f7a4a6bc2bdc670680d3714273);

        MerkleReserveMinter.MerkleClaim[] memory claims = new MerkleReserveMinter.MerkleClaim[](1);
        claims[0] = MerkleReserveMinter.MerkleClaim({ mintTo: claimer1, tokenId: 5, merkleProof: proof });

        minter.mintFromReserve(address(token), claims); //Initial claim 

        vm.startPrank(token.ownerOf(5));
        token.burn(5); // burning the claimed token
        vm.stopPrank();

        MerkleReserveMinter.MerkleClaim[] memory claims1 = new MerkleReserveMinter.MerkleClaim[](1);
        claims1[0] = MerkleReserveMinter.MerkleClaim({ mintTo: claimer1, tokenId: 5, merkleProof: proof });

        minter.mintFromReserve(address(token), claims1); // Second claim

        assertEq(token.ownerOf(5), claimer1);
    }

Result :

Running 9 tests for test/MerkleReserveMinter.t.sol:MerkleReserveMinterTest
[PASS] testRevert_InvalidProof() (gas: 3310228)
[PASS] testRevert_InvalidValue() (gas: 3318272)
[PASS] testRevert_MintEnded() (gas: 3307581)
[PASS] testRevert_MintNotStarted() (gas: 3307495)
[PASS] test_MintFlow() (gas: 3490797) //Passing Test
[PASS] test_MintFlowSetFromToken() (gas: 3437856)
[PASS] test_MintFlowWithValue() (gas: 3488261)
[PASS] test_MintFlowWithValueMultipleTokens() (gas: 3617296)
[PASS] test_ResetMint() (gas: 3241395)
Test result: ok. 9 passed; 0 failed; finished in 38.09ms

Tool used

Manual Review, Foundry

Recommendation

Create a mapping that tracks the claiming of tokens.
mapping(address user=> uint tokenId => bool isClaimed) public TokenClaims;

[neokry]

This is a valid issue but is unlikely to cause any major problems imo. There are no current or future plans for us or anyone in the community to develop a minter contract that uses the burn function but we plan on fixing this in case one is ever developed. we see this as more of a future issue than something that could be immediately exploited. The example given where an EOA is set as a minter and is responsible for burning tokens is a valid scenario but one that would likely never happen as the minter role is tightly restricted in most DAOs

[nevillehuang]

Similar to #144, situation is unlikely, but possible to allow unintended re-minting of tokens, so I believe this issue is definitely worth consideration. Leaving as medium severity for now on the criteria that it is more of a future look issue and not immediately exploitable.

[Czar102]

I feel like this may be a design choice – any burning mechanics on top shouldn't assume that the burns will be permanent if the claiming period hasn't ended. I have mixed feelings whether ability to mint only once is clearly a property of the code users should expect. Is it communicated anywhere that minting the same token multiple times is undesired?
Also, this issue seems to be speculating on future integrations.

Per docs:

Future issues: Issues that result out of a future integration/implementation that was not intended (mentioned in the docs/README) (…) are not valid issues.

[nevillehuang]

I feel like this may be a design choice – any burning mechanics on top shouldn't assume that the burns will be permanent if the claiming period hasn't ended. I have mixed feelings whether ability to mint only once is clearly a property of the code users should expect. Is it communicated anywhere that minting the same token multiple times is undesired? Also, this issue seems to be speculating on future integrations.

Per docs:

Future issues: Issues that result out of a future integration/implementation that was not intended (mentioned in the docs/README) (…) are not valid issues.

@Czar102 It needs to be explicitly mentioned in the README/docs though which is not, and based on @neokry comments above, it is certainly not a desired and intended scenario since he recognizes it is a valid issue, that is while unlikely, still could be possible

Also, I don’t see the scenario where burning shouldn’t be permanent, if not why burn in the first place?

[neokry]

I feel like this may be a design choice – any burning mechanics on top shouldn't assume that the burns will be permanent if the claiming period hasn't ended. I have mixed feelings whether ability to mint only once is clearly a property of the code users should expect. Is it communicated anywhere that minting the same token multiple times is undesired? Also, this issue seems to be speculating on future integrations.
Per docs:

Future issues: Issues that result out of a future integration/implementation that was not intended (mentioned in the docs/README) (…) are not valid issues.

@Czar102 It needs to be explicitly mentioned in the README/docs though which is not, and based on @neokry comments above, it is certainly not a desired and intended scenario since he recognizes it is a valid issue, that is while unlikely, still could be possible

Also, I don’t see the scenario where burning shouldn’t be permanent, if not why burn in the first place?

Like I mentioned it is unlikely this scenario would occur as it requires us to build out a minter with burn functionality something neither us nor the community has any plans for and it would require that burn minter and the merkle minter to be enabled at the same time which is something a DAO can easily avoid by enabling / disabling the merkle minter before using the burn minter.

[nevillehuang]

Again, unlikely but possible + not stated in the READ.ME/Docs, have to give watsons the benefit of the doubt here. My stance is to keep medium severity.

[neokry]

Again, unlikely but possible + not stated in the READ.ME/Docs, have to give watsons the benefit of the doubt here. My stance is to keep medium severity.

its unlikely + relying on a future integration which is out of scope imo

[nevillehuang]

There is so many conditions and assumptions in play here that I feel like should have been made known to the watsons in the contest details.

Future integrations are not out of scope unless explicitly mentioned in the contest details, which you can see based on the rules above.

Additionally, the issue is present in the code logic of the contracts that are currently inscope. It is completely fair that watsons assume the burn minter is being utilised since it is not mentioned in the contest docs as a future integration

There is a clear bug here in my eyes, because I really don’t see why burning shouldn’t be permanent and why there exists a mechanism to allow reminting that could be potentially exploited.

[neokry]

sorry missed that part of the docs about a future integration needing to be explicitly mentioned @nevillehuang . I agree something about this should have explicitly mentioned in the docs.

re your question on why a burn might not be permanent the optimism NFT bridge is a usecase where tokens are minted when deposited to L2 and burned when they are withdrawn back to L1. If a developer wanted to implement an NFT bridge like that for this protocol burning would not be a permanent state for a given tokenId.

[nevillehuang]

sorry missed that part of the docs about a future integration needing to be explicitly mentioned @nevillehuang . I agree something about this should have explicitly mentioned in the docs.

re your question on why a burn might not be permanent the optimism NFT bridge is a usecase where tokens are minted when deposited to L2 and burned when they are withdrawn back to L1. If a developer wanted to implement an NFT bridge like that for this protocol burning would not be a permanent state for a given tokenId.

Thanks for the clarification, based on that use case, I definitely am inclined to keep medium. Since the burn minter cannot be disabled in such use cases, the user can simply do the following to own the 2 NFTs again.

1. Deposit to L2
2. Withdraw to L1
3. Mint from L2 again

[neokry]

sorry missed that part of the docs about a future integration needing to be explicitly mentioned @nevillehuang . I agree something about this should have explicitly mentioned in the docs.
re your question on why a burn might not be permanent the optimism NFT bridge is a usecase where tokens are minted when deposited to L2 and burned when they are withdrawn back to L1. If a developer wanted to implement an NFT bridge like that for this protocol burning would not be a permanent state for a given tokenId.

Thanks for the clarification, based on that use case, I definitely am inclined to keep medium. Since the burn minter cannot be disabled in such use cases, the user can simply do the following to own the 2 NFTs again.

1. Deposit to L2
2. Withdraw to L1
3. Mint from L2 again

I agree there is a potential issue here but this is speculating on the design of the burn minter and a fix on the burn contract is it could only allow users to burn tokens if the merkle minter is not enabled or only allow tokens to be burned outside of the reserve ie tokens the merkle minter cannot access. since there is potential for causing issues with a future burn contract we are planning to fix this but I want to be sure that the severity makes sense here since the reasoning is based on how a future contract may or may not be built which I feel is speculative.

[nevillehuang]

Like I mentioned in my previous comments, I feel like the speculation is fair due to:

1. Lack of concrete explanation in contest details
2. Code logic in in scope contracts allow the potential issue to occur, as evident by the PoC. If unintended reminting is allowed, it constitutes undesired loss of funds to the DAO in my eyes.

Suggest to maintain as medium severity

[Czar102]

@nevillehuang

Future integrations are not out of scope unless explicitly mentioned in the contest details, which you can see based on the rules above.

Please read the rule again. Maybe I am misunderstanding something there, but there are out of scope if there is no explicit mention in the docs/README. Hence, this issue should not classify for a medium, because it speculates on inexistent future integrations.
It can be also considered an admin misconfiguration, because:

it would require that burn minter and the merkle minter to be enabled at the same time which is something a DAO can easily avoid by enabling / disabling the merkle minter before using the burn minter.

And this is out of scope.

[nevillehuang]

@Czar102 How about you read the sherlock rules again. I will quote it here:

Future issues: Issues that result out of a future integration/implementation that was not intended (mentioned in the docs/README) or because of a future change in the code (as a fix to another issue) are not valid issues.

Lets break it down:
Issues-that-result-out-of-a-future-integration/implementation-that-was-not-intended-(mentioned in the docs/README)-or because-of-a-future-change-in-the-code-(as a fix to another issue)-are-not-valid-issues.

The above means, THE FUTURE INTEGRATION NEEDS TO BE EXPLICITLY MENTIONED AS NOT INTENDED FOR IT TO BE INVALID . You can correct me if I'm wrong here, but no matter how many times i read it that is my conclusion. This is especially so when the inscope contract code logic allows the issue described to happen. It could be understandable if there is speculation based on other external integrations, but in this case, the contracts involved are all in scope.

It doesn't make sense to me that sherlock would completely invalidate future integration type issues, because I believe some protocols are still interested, depending on context.

I hope you can hear my frustration here. If I am misunderstanding something here, I apologize, but if not, better revisit this gray area of the sherlock rules and make it more concrete, especially the language involved in describing future type issues.

[Czar102]

I see the reason of the confusion. Note that it's not "implementation that was not intended (not mentioned in the docs/README)", but "implementation that was not intended (mentioned in the docs/README)". In other words, "mentioned in the docs/README" means "intended". So If the integration was not mentioned, then it's invalid, which is the case here.

It doesn't make sense to me that sherlock would completely invalidate future integration type issues, because I believe some protocols are still interested, depending on context.

They should audit the new codebase with us then, and Sherlock with Watsons will find the bug. If we were to accept any issue like that, then practically all info/low severity issues would be considered medium/high because usually when there is an assumption that can be broken, there is an idea that would break on top of it. We don't want the severity to be inflated like that, so we need this rule.

[nevillehuang]

@Czar102 Agree with you stance, but like sure I could have interpret it your way but I hope you see where I'm coming from by using such confusing language? There is no point of me arguing with you since you will be the one deciding the final severity anyways, so I would have to be inclined to agree with you.

I can assure you this will likely be brought up by watsons during the escalations period, and hope your explanation give them clarification from potential confusion similar to mine. Closing this issue as of now.

PS: I once again strongly urge you to implement certain changes to the language use in sherlocks rules asap. I understand its difficult work but hope you are actively working on it.

[nevillehuang]

Escalate

Escalating for watsons to leave comments for discussion, because ultimately I am in support of them for validating this issue. I believe my above comments to be sufficient, will leave it in watsons and @Czar102 hands. Open to having my escalations rejected.

[sherlock-admin2]

Escalate

Escalating for watsons to leave comments for discussion, because ultimately I am in support of them for validating this issue. I believe my above comments to be sufficient, will leave it in watsons and @Czar102 hands. Open to having my escalations rejected.

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.

[SteveHarrington0]

@Czar102 let me clarify this. The claimed token can be again claimed if the token got burned and only minter role can burn the token if he own it. So its it requires some circumstances which are possible and the impact that this issue causes is considerable. IF THE PROTOCOL IS NOT IMPLEMENTING THE MINTER ROLE SPECIFICALLY THEY SHOULD MENTION THAT IN THE README. Since they never mentioned and the SHERLOCK rule that you mentioned can be understandable in multiple ways. If they are particularly not implementing the Minter implementation they should and must mention it in the Readme or Docs but they never did so that any watson will assume there is a minter role and that makes the issue exploitable. Sponsors said that "we don’t have any plans to build minters with burn functionality" then why in the first place the role exist and they had the ability to burn the tokens. If they wanted to implement the minter role in future then they MUST have mention that its a future integration and then i can agree to invalidating the issue but they never did. So this issue is within the protocol and in SCOPE and can be exploitable. So its a valid issue.

[Oot2k]

I want to add that "Future issues" are invalid by sherlock rules.
And because there is currently no impact to this I think this should be more an low/info.

Please see point 22 on: https://docs.sherlock.xyz/audits/judging/judging

Mainly for this reason this should be invalid. + there is currently no intended way to burn tokens -> so this will never accrue in real life. (the argument of DAO setting minter is not valid, DAO can do anything to damage them selfs)

For the future it may make sense to add some info to this sherlock rule.

Best regards

[SteveHarrington0]

Its "Future issues" even though its no where mentioned in the docs or contest details. and you guys defending it by showing a rule thats not clear. Is there any rule that mentioning "If the rule is not clear by any chance it must be the watson that suffer who worked tirelessly"..?. i could have agreed if the mentioned so called "Rule" is solid. but its not. any way i can clearly say that its sponsor`s and sherlocks fault. cause sponsor should have mention it in contest details and sherlock should have clear rule. Due to these to things today my issue got invalidated. I really appreciate @nevillehuang for defending my issue. atleast someone in sherlock judged it fairly. I strongly disagree with this invalidation. Thanks thats all i can say. Really disappointed in this one. As in seneca's case you distributed funds based on leaderboard points then why are you having the "ONLY USDC" option in the first place. And yet its watsons fault. This is not at all makes any sense. anyway at the end of the day i only argue about my finding but cant make it valid. I think its not a fair judgement as you can see so obviously.

[nevillehuang]

I agree with @SteveHarrington0, the language used on sherlock rules revolving future issues is too ambiguous, and as such is the root cause of why we are even having this discussion in the first place.

While I agree with @Czar102 stance revolving future issues, it is quite disappointing that the fault of such ambiguity is being put on the watsons submitting this issue and the lead judge judging this issue, even more so ESPECIALLY when the code logic of the protocol allows the issue to occur.

[Czar102]

This issue may be considered a result of an admin misconfiguration – adding minters (who can burn) while the claim period hasn't concluded. I believe that the issues revolving around admin misconfiguration are quite clear.

To make sure I am not biased, I consulted another core team member and they independently arrived at the same conclusion, so I believe this is judgment is not a mistake and is a general Sherlock's stance on this kind of issues.

[nevillehuang]

This issue may be considered a result of an admin misconfiguration – adding minters (who can burn) while the claim period hasn't concluded. I believe that the issues revolving around admin misconfiguration are quite clear.

To make sure I am not biased, I consulted another core team member and they independently arrived at the same conclusion, so I believe this is judgment is not a mistake and is a general Sherlock's stance on this kind of issues.

@Czar102 It is not a mistake when it is intended, but it is not according to sponsors public comments AFTER the contest ended not BEFORE/DURING. Above, the sponsor even highlights a potential use case of a burn minter.

I will accept your opinion, not because I agree, but because I am done arguing this.

[SteveHarrington0]

Sponsors only find out about this problem when someone brings it up. If no one mentions it, the admin doesn't know whether to set the minters before or after the claiming period. It was only because this problem was pointed out that sponsors became aware of it. The proposed solution of not setting minters before the claiming period is a way to fix this issue. It's a problem that needs to be resolved because the code logic is not correct, and the main purpose of an audit is to fix any issues in the code.I'm not sure why, but @Czar102 doesn't seem convinced about this issue. The sponsor has confirmed the problem, emphasizing its significant impact. The main concern raised by both the sponsor and the judge revolves around the ambiguity of the rule you mentioned.

[SteveHarrington0]

This is a valid issue but is unlikely to cause any major problems imo. There are no current or future plans for us or anyone in the community to develop a minter contract that uses the burn function but we plan on fixing this in case one is ever developed. we see this as more of a future issue than something that could be immediately exploited. The example given where an EOA is set as a minter and is responsible for burning tokens is a valid scenario but one that would likely never happen as the minter role is tightly restricted in most DAOs

@Czar102 Just for your reference. The sponsor confirmed its an valid issue.

[neokry]

This is a valid issue but is unlikely to cause any major problems imo. There are no current or future plans for us or anyone in the community to develop a minter contract that uses the burn function but we plan on fixing this in case one is ever developed. we see this as more of a future issue than something that could be immediately exploited. The example given where an EOA is set as a minter and is responsible for burning tokens is a valid scenario but one that would likely never happen as the minter role is tightly restricted in most DAOs

@Czar102 Just for your reference. The sponsor confirmed its an valid issue.

If you read the comment your quoting I explain why I don’t believe it should be medium severity and should be low / informational

[Evert0x]

I believe this issue should not be judged medium or high severity as the ADMIN is considered TRUSTED by the protocol team. This admin is trusted to behave in the best interests of the protocol and would not burn tokens during this period to enable the bug described.

[nevillehuang]

@Evert0x @Czar102 Im not going to argue your stance, but I implore you to read the above comments where the sponsor highlighted a potential use case of a burn minter. If this issue wasn't highlighted by watsons, it can have a potential impact on any governance potentially using a burn minter. Are admin trusted to not make mistakes if they don't know about the potential impact of this issue at all?

#203 (comment)
#203 (comment)

• Sure it is "unlikely", but possible.
• Sure it can be seen as a future issue, and I might have misunderstood sherlock rules on future issues because the language used wasn't that clear.
• Whatever the severity is, this is still a valid issue so I am glad watsons brought it up.

[Czar102]

Result:
Low
Has duplicates

Taking all comments into account, I still believe this issue is not severe and concrete enough to be considered a medium. The comments mentioned above reference an implementation on top and should it be in scope of the audit, this submission would have been a high severity finding.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• nevillehuang: rejected

[neokry]

Fixed here: ourzora/nouns-protocol#126

[IAm0x52]

Fix looks good. MerkleReserveMinter now implements a mapping to prevent the same token from being minted twice.