KingNFT - Part of users might lose their NFTs permanently after ````L2Migration````

[sherlock-admin2]

KingNFT

high

Part of users might lose their NFTs permanently after L2Migration

Summary

After L2Migration, current design utilizes MerkleReserveMinter for users to claim NFTs back on L2. Smart wallet users would encounter issues in this design.

Vulnerability Detail

The issue here is that if members of the origin DAO are smart contract wallets such as the widely used Gnosis safe wallet, these is no guarantee that users also control the same wallet address on L2. The address on L2 might belong to others, or even doesn't exist at all.

File: src\minters\MerkleReserveMinter.sol
66:     struct MerkleClaim {
68:         address mintTo; // @audit a smart wallet address might belong to others or doesn't exist at all on L2
70:         uint256 tokenId;
72:         bytes32[] merkleProof;
73:     }

Impact

Part of users might lose their NFTs permanently after L2Migration

Code Snippet

https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/minters/MerkleReserveMinter.sol#L66

Tool used

Manual Review

Recommendation

Maybe make a new design that allow users to do cross chain NFT transfers by 1) burn on L1 2) send a cross chain message 3) mint NFT on L2 to a target address specified by the sender

[nevillehuang]

Invalid, it is users responsibility to provide a valid controlled mintTo address for admin to provide merkle proof for minting.

[ydspa]

Escalate

There is no existing docs or implemented functions to explain and let users to specify the mintTo before migration, I think the dev hasn't realize this problem yet

[sherlock-admin2]

Escalate

There is no existing docs or implemented functions to explain and let users to specify the mintTo before migration, I think the dev hasn't realize this problem yet

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.

[nevillehuang]

@ydspa

I maintain my previous judgement, but will loop in @neokry @Czar102 to take a look. Interesting find but ultimately I still feel like it doesn't constitute a medium given it is the users responsibility to check if they own the same wallet address in both L1 and L2.

[ydspa]

It's too harsh to require users to ensure same smart wallet address in both L1 and L2, in many cases, it's impossible. In my opinion, the protocol should provide some on chain solution to allow users specify their migration receipt.

[neokry]

we will be using aliased addresses for any smart contract wallets that hold tokens / are founders etc

https://docs.optimism.io/chain/differences#address-aliasing

[Czar102]

mintTo is sourced off-chain, so I don't see any programming error here. The issue would be a result of an admin (merkle tree/root) misconfiguration, which is out of scope.

Planning to reject the escalation and leave the issue as is.

[Czar102]

Result: Invalid Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• ydspa: rejected