Add back in src/ for review #1

dbmikus · 2023-01-30T06:50:39Z

No description provided.

dbmikus

LGTM! I'm not too much of a Solidity expert, but I had a couple inline questions/suggestions.

dbmikus · 2023-01-30T07:19:02Z

src/DraupMembershipERC721.sol

+    error TransfersNotAllowed();
+
+    function mint(bytes32[] calldata merkleProof) public {
+        bytes32 leaf = keccak256(bytes.concat(keccak256(abi.encode(msg.sender, 1))));


Someone with more merkle tree experience should confirm this, but if you'er just trying to check that an address is part of the tree, you could probably do:

bytes32 leaf = keccak256(bytes.concat(keccak256(abi.encode(msg.sender))));

I don't know what the gas savings would be. Probably pretty small.

I would suggest:

bytes32 leaf = keccak256(abi.encodePacked(msg.sender));

I've used this in the past and it requires hashing only once.

I was curious why the OZ code code recommended double-usage of keccak256. They mention that it is to prevent second preimage attacks, which are best described here. In the naive case, an attacker could provide data and a proof for an intermediate node instead of a leaf node and trick the system into thinking that the proof passes.

However, here you do not have that problem because:

Most importantly, the input value (msg.sender) is 20 bytes long, while the inputs to an internal hash combine to be 64 bytes long. So a value that could be msg.sender could never equal the value for an internal node.

The attack can only control the input value by calling from specific addresses. Finding these addresses would be time intensive and the attacker's benefit is negligible: they would only be able to cause an NFT to get sent to one of those random addresses.

In the general case, there are two ways to solve the problem:

Ensure that the data in a leaf node could never collide with the data in an internal node. Using different width types for leaves vs internal solves this, as does prefixing internal nodes and leaf nodes with different data.

Ensure that leaves use a different hashing algorithm than the internal hashing algorithm when computing their node hash. So if a user supplies an internal node as a leaf, it will be double-hashed as part of the proof, which returns a different value than when it is single-hashed in the normal internal case.

OZ chooses the second approach and does double-hashing of leaves, while internal nodes are hashed once with keccak256.

A few more links that were useful for me:

MerkleProof.sol source code

A bug report about this kind of vulnerability

OZ's JS library for merkle proofs, which kicked off my whole investigation about why they hash twice

Actually, it might be possible that an internal tree node could be equal to a msg.sender value extended with 44 zeroes, and then you would have data collisions between leaves and internal nodes. Someone might want to double check me on that vulnerability as it contradicts a bug report in one of my reference links.

Anyways, very unlikely and double hashing leaves eliminates the problem.

If you decide to go with the single-hashed version that banksian suggested above, a reminder that you won't be able to use OZ's JS library StandardMerkleTree to generate the proof since it does a double hash.

Lanyard has some example code that uses the single hash approach.
https://lanyard.org/tree/sample

dbmikus · 2023-01-30T07:33:36Z

src/DraupMembershipERC721.sol

+    error MaxSupplyReached();
+    error TransfersNotAllowed();
+
+    function mint(bytes32[] calldata merkleProof) public {


I think it's generally nice to have a mintTo function where you can specify a recipient address. ie

function mintTo(address to, bytes32[] calldata merkleProof) public;

This allows flexibility in how the mint occurs. Some benefits:

Can mint gaslessly with relayers/meta-transactions

Could mint from a hot wallet into a cold wallet

.. other things?

Of course, if you do this, you need to change logic around msg.sender. What addresses go into the merkle tree? The minters or the recipients of mints? Probably better to verify the recipient is in the tree, that way relayers could mint on behalf of recipients, etc.

Not sure if it's worth it to you to add a recipient address!

You know what, you would definitely want to do merkle proof checks on the recipient if you had a mintTo. Otherwise if it was on a msg.sender that could be different than the recipient, I could see your in flight transaction, get the merkle proof, and then submit a different recipient.

dbmikus · 2023-01-30T07:46:05Z

src/DraupMembershipERC721.sol

+    }
+
+    function withdrawAll() external {
+        require(address(this).balance > 0, "Zero balance");


Is there a point in doing this check vs doing a zero-balance transfer?

bankisan · 2023-01-30T14:37:29Z

src/DraupMembershipERC721.sol

+    error TransfersNotAllowed();
+
+    function mint(bytes32[] calldata merkleProof) public {
+        bytes32 leaf = keccak256(bytes.concat(keccak256(abi.encode(msg.sender, 1))));


I would suggest:

bytes32 leaf = keccak256(abi.encodePacked(msg.sender));

I've used this in the past and it requires hashing only once.

bankisan · 2023-01-30T14:41:17Z

src/DraupMembershipERC721.sol

+        if (!transfersAllowed && from != address(0)) {
+            revert TransfersNotAllowed();
+        }
+        super._beforeTokenTransfer(from, to, firstTokenId, batchSize);


It doesn't seem necessary to call this base method. The base method's logic is only executed if batchSize > 1 which is never the case here.

bankisan · 2023-01-30T14:41:52Z

src/DraupMembershipERC721.sol

+        returns (bool)
+    {
+        return
+            _interfaceId == type(IERC2981).interfaceId ||


bankisan · 2023-01-30T14:45:21Z

src/DraupMembershipERC721.sol

+
+contract DraupMembershipERC721 is ERC721, Ownable, DefaultOperatorFilterer {
+    uint256 public immutable MAX_SUPPLY;
+    uint256 public immutable ROYALTY = 750;


Change immutable to constant as ROYALTY isn't set in the constructor.

bankisan · 2023-01-30T14:49:58Z

src/DraupMembershipERC721.sol

+
+    function mint(bytes32[] calldata merkleProof) public {
+        bytes32 leaf = keccak256(bytes.concat(keccak256(abi.encode(msg.sender, 1))));
+        if (!MerkleProof.verify(merkleProof, merkleRoot, leaf)) {


nit: Instead of MerkleProof.verify, you could add the using MerkleProof for bytes32[] directive at the top level of your contract and then do merkleProof.verify(merkleRoot, leaf). It's a trick I use to clean the verify line but totally optional.

reggieag · 2023-02-01T04:02:35Z

src/DraupMembershipERC721.sol

+    }
+
+    // Admin actions
+    function setMerkleRoot(bytes32 _merkleRoot) external onlyOwner {


Definitely a good idea to have an escape hatch in case the merkle root generation breaks off contract!

Another approach you can take is to have additive Merkle roots by storing a hash map of roots to bools {merkleRoot: bool}. You can find the root from the proof and check to see if it's in the hash map. Maybe not worth the complexity, but throwing it out there just in case.

Here's a little helper function that we built at Foundation to do the root lookup.

import "@openzeppelin/contracts/utils/cryptography/MerkleProof.sol"; /** * @title Helper library for interacting with Merkle trees & proofs. * @author batu-inal & HardlyDifficult & reggieag */ library MerkleAddressLibrary { using MerkleProof for bytes32[]; /** * @notice Gets the root for a merkle tree comprised only of addresses, using the msg.sender. */ function getMerkleRootForSender(bytes32[] calldata proof) internal view returns (bytes32 root) { bytes32 leaf = keccak256(abi.encodePacked(msg.sender)); // single hash approach root = proof.processProofCalldata(leaf); } }

reggieag · 2023-02-01T04:31:50Z

src/DraupMembershipERC721.sol

+        }
+        _claimed[msg.sender] = true;
+        nextTokenId++;
+        if (nextTokenId > MAX_SUPPLY) {


Gas nit, I recommend following the checks, effects, interactions pattern here to save users some gas in the reversion case. If you're expecting a high heat mint, some transactions may be in flight when the NFT mints out. This means that some minters will revert and have to pay gas. To minimize gas, I recommend moving the mutations above below the MaxSupplyReached() check. I also recommend moving the MaxSupplyReached to the top of the function since merkle tree lookups are logn where n is the height of the tree.

Add back in src/ for review

47fcba4

dbmikus commented Jan 30, 2023

View reviewed changes

bankisan reviewed Jan 30, 2023

View reviewed changes

reggieag reviewed Feb 1, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add back in src/ for review #1

Add back in src/ for review #1

dbmikus commented Jan 30, 2023

dbmikus left a comment

dbmikus Jan 30, 2023 •

edited

bankisan Jan 30, 2023

dbmikus Jan 30, 2023

dbmikus Jan 30, 2023

reggieag Feb 1, 2023

dbmikus Jan 30, 2023

dbmikus Jan 30, 2023

dbmikus Jan 30, 2023

bankisan Jan 30, 2023

bankisan Jan 30, 2023

bankisan Jan 30, 2023

bankisan Jan 30, 2023

bankisan Jan 30, 2023

reggieag Feb 1, 2023 •

edited

reggieag Feb 1, 2023

Add back in src/ for review #1

Are you sure you want to change the base?

Add back in src/ for review #1

Conversation

dbmikus commented Jan 30, 2023

dbmikus left a comment

Choose a reason for hiding this comment

dbmikus Jan 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reggieag Feb 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbmikus Jan 30, 2023 •

edited

reggieag Feb 1, 2023 •

edited