Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add back in src/ for review #1

Open
wants to merge 1 commit into
base: review-base
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 159 additions & 0 deletions src/DraupMembershipERC721.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.17;

import {Ownable} from "openzeppelin-contracts/contracts/access/Ownable.sol";
import {IERC2981, IERC165} from "openzeppelin-contracts/contracts/interfaces/IERC2981.sol";
import {DefaultOperatorFilterer} from "operator-filter-registry/src/DefaultOperatorFilterer.sol";
import {ERC721} from "openzeppelin-contracts/contracts/token/ERC721/ERC721.sol";
import {IERC20} from "openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";
import {MerkleProof} from "openzeppelin-contracts/contracts/utils/cryptography/MerkleProof.sol";
import {PaddedString} from "draup-utils/src/PaddedString.sol";
import {IRenderer} from "./IRenderer.sol";

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bool public transfersAllowed = false;
IRenderer public renderer;
string public baseTokenURI;
uint256 public nextTokenId;
bytes32 public merkleRoot;

// Mapping to track who used their allowlist spot
mapping(address => bool) private _claimed;

constructor(uint256 maxSupply, string memory baseURI) ERC721("Draup Membership", "DRAUP") {
MAX_SUPPLY = maxSupply;
baseTokenURI = baseURI;
}

error InvalidProof();
error AlreadyClaimed();
error MaxSupplyReached();
error TransfersNotAllowed();

function mint(bytes32[] calldata merkleProof) public {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Can mint gaslessly with relayers/meta-transactions
  2. Could mint from a hot wallet into a cold wallet
  3. .. 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!

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

bytes32 leaf = keccak256(bytes.concat(keccak256(abi.encode(msg.sender, 1))));
Copy link
Owner Author

@dbmikus dbmikus Jan 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest:

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. 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:

  1. 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.
  2. 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:

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (!MerkleProof.verify(merkleProof, merkleRoot, leaf)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

revert InvalidProof();
}
if (_claimed[msg.sender]) {
revert AlreadyClaimed();
}
_claimed[msg.sender] = true;
nextTokenId++;
if (nextTokenId > MAX_SUPPLY) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

revert MaxSupplyReached();
}
_mint(msg.sender, nextTokenId - 1);
}

// token trading is disabled initially but will be enabled by the owner
function _beforeTokenTransfer(
address from,
address to,
uint256 firstTokenId,
uint256 batchSize
) internal virtual override {
if (!transfersAllowed && from != address(0)) {
revert TransfersNotAllowed();
}
super._beforeTokenTransfer(from, to, firstTokenId, batchSize);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// on-chain royalty enforcement integration
function setApprovalForAll(address operator, bool approved) public override onlyAllowedOperatorApproval(operator) {
super.setApprovalForAll(operator, approved);
}

function approve(address operator, uint256 tokenId) public override onlyAllowedOperatorApproval(operator) {
super.approve(operator, tokenId);
}

function transferFrom(address from, address to, uint256 tokenId) public override onlyAllowedOperator(from) {
super.transferFrom(from, to, tokenId);
}

function safeTransferFrom(address from, address to, uint256 tokenId) public override onlyAllowedOperator(from) {
super.safeTransferFrom(from, to, tokenId);
}

function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data)
public
override
onlyAllowedOperator(from)
{
super.safeTransferFrom(from, to, tokenId, data);
}

// upgradeable token renderer based on web3-scaffold example by frolic.eth
// https://github.com/holic/web3-scaffold/blob/main/packages/contracts/src/IRenderer.sol
function tokenURI(uint256 tokenId)
public
view
override
returns (string memory)
{
_requireMinted(tokenId);
if (address(renderer) != address(0)) {
return renderer.tokenURI(tokenId);
}
return
string(
abi.encodePacked(
baseTokenURI,
PaddedString.digitsToString(tokenId, 3),
".json"
)
);
}

// Royalty info provided via EIP-2981
// https://eips.ethereum.org/EIPS/eip-2981
function supportsInterface(bytes4 _interfaceId)
public
view
override
returns (bool)
{
return
_interfaceId == type(IERC2981).interfaceId ||

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

super.supportsInterface(_interfaceId);
}

function royaltyInfo(uint256, uint256 salePrice)
external
view
returns (address, uint256)
{
return (address(this), (salePrice * ROYALTY) / 10000);
}

// Admin actions
function setMerkleRoot(bytes32 _merkleRoot) external onlyOwner {
Copy link

@reggieag reggieag Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
  }
}

merkleRoot = _merkleRoot;
}

function enableTransfers() external onlyOwner {
transfersAllowed = true;
}

function setRenderer(IRenderer _renderer) external onlyOwner {
renderer = _renderer;
}

function setBaseTokenURI(string calldata _baseTokenURI) external onlyOwner {
baseTokenURI = _baseTokenURI;
}

function withdrawAll() external {
require(address(this).balance > 0, "Zero balance");
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(bool sent, ) = owner().call{value: address(this).balance}("");
require(sent, "Failed to withdraw");
}

function withdrawAllERC20(IERC20 token) external {
token.transfer(owner(), token.balanceOf(address(this)));
}
}

13 changes: 13 additions & 0 deletions src/ExampleRenderer.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.17;

import {IRenderer} from "./IRenderer.sol";
import {Strings} from "openzeppelin-contracts/contracts/utils/Strings.sol";


contract ExampleRenderer is IRenderer {
function tokenURI(uint256 tokenId) external pure override returns (string memory) {
return string(abi.encodePacked("https://www.example.com/", Strings.toString(tokenId), ".json"));
}
}

10 changes: 10 additions & 0 deletions src/IRenderer.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// SPDX-License-Identifier: Unlicense
pragma solidity ^0.8.9;

/// @author frolic.eth
/// @title Upgradeable renderer interface
/// @notice This leaves room for us to change how we return token metadata and
/// unlocks future capability like fully on-chain storage.
interface IRenderer {
function tokenURI(uint256 tokenId) external view returns (string memory);
}