Bad practices in example contract

[0xsublime]

The example contract (which we should expect will be forked by plenty of developers) uses some bad practices.

1. Disallowing contracts to mint
2. Limiting per-address mints

Both of these lend a veneer of extra security but offer absolutely no protection against sophisticated users and bots, while limiting casual and cautious users.

Blocking contracts from minting: Holding NFTs in an EOA is not necessarily the best practice. Smart contracts wallets and multisigs are sensible options for most users but they suffer because many contracts restrict them arbitrarily. It also confers no security benefits and doesn't stop bots, who can just as well programatically submit many transactions, or transaction bundles.

Limiting per-address mint: Any sophisticated user or bot can easily set up several addresses and bypass the minting limit as much as they want. This restriction may inflate the number of listed holders and make it look like engagement is being driven, or fairness is implemented, but that is clearly not the case. It may make sense for a whitelist mint, but not for a public mint.

A third problem:

3. Allowing users to send more than quantity * mintPrice to public mint.

This adds no value to users, and can really only cause unsophisticated users to overpay. The UI should set the exact value to be sent in the transaction.

[elie222]

Thanks for the feedback.

1 and 2 are debatable. Agree that maybe it shouldn't be in the basic example. Stopping contracts mass mint makes it slightly harder but agree this won't stop someone that wants to do it.

Regarding limits on the public mint, it stops a regular user minting 50 which is helpful. Bots aren't the only users of these contracts. Regular minters are too and this limit will make it hard for the vast majority of users to mint a large amount.

To be clear regarding 3, you mean this line should be == instead: https://github.com/exo-digital-labs/ERC721R/blob/main/contracts/ERC721RExample.sol#L61

[0xsublime]

I think 1 and 2 both contribute very little to actual security, while being the kind of approach that many new developers assume is a good security practice, which is why I find it strange to keep in the example. It's not that they are never reasonable if you have a very specific idea of why you are doing it.

I guess I've never come across an example of when it would make sense to limit per-address mints in this way. Anyone willing to buy a bulk of NFTs likely is also very aware that they can just create another address easily. Limitations work when there is a whitelist, obviously, so that the default user is allowed to mint 0.

Re: 3 Yes, that's right. Note that == is already used in the preSaleMint. Again, this is one of those gotchas I've seen new devs falling for: they are used to thinking that you can't use == on very small numbers (I guess they think they are floats) and use >= instead, and sometimes send the extra funds back, creating a reentrancy vuln.

[lawweiliang]

@elie222 , I agree with @0xsublime points.

Indeed, I myself believe that putting assets under smart contract is way more safer than putting it in EOA.

Point 1

@elie222 Would you mind to share your perception on why would you stop contract from minting? What are the key problem from doing that?

For my case, I didn't do that. Because as I think it from the business perspectives. I am a company, I am selling product. I don't care what wallet my customers want to use. As long as they ping me the money, I will send them the product.(Keep it as simple as that) By restricting user to use any wallet( in our case is smart contract wallet), this is like "Oh, you don't have this wallet, please go away, this is extremely bad for business stand points."

Point 2

@0xsublime For this part Limiting per-address. How did you make the project more decentralize if we did not set a maximum cap for the user? If we don't have a maximum cap, one user might buy 50% of the total supply. Maybe 2 whales for all 10,000 nfts. Kind of weird is't? Whole community only have 2 people.

For my case, I usually set the maximum cap of 5 for each user.

Point 3

@0xsublime , I made mistake on this as well. Yup, I should put ==instead of >= when come to minting. Control of the price should be done over UI site. Adding refund in this section will trigger more problem(reentrancy). This one I totally agree with your points. Thanks bro.

[elie222]

Regarding stopping SC from minting let's remove it. If a team want to put it in themselves they can.

Would you mind to share your perception on why would you stop contract from minting? What are the key problem from doing that?

There's a bot attack where an SC can create 500 other SCs and mint out an entire project in one go. Stopping SC minting stops this. But as @0xsublime mentioned you can bot attack with regular accounts too. It just means it's across many wallets.

For my case, I usually set the maximum cap of 5 for each user.

Around this point I agree it's totally standard to have a max mint per user. If a bot wanted to get around it they could but it's not an easy process and it helps a lot of projects. For a regular user even minting from 3 wallets is a lot of work.

In terms of action items for this issue:

1. remove isContract check
2. change to ==

[0xsublime]

Point 2

@0xsublime For this part Limiting per-address. How did you make the project more decentralize if we did not set a maximum cap for the user? If we don't have a maximum cap, one user might buy 50% of the total supply. Maybe 2 whales for all 10,000 nfts. Kind of weird is't? Whole community only have 2 people.

For my case, I usually set the maximum cap of 5 for each user.

It doesn't matter. You're trying to solve sybil resistance, and you can't without some serious work. This is purely an optics thing: it looks better if you have 2,000 different owners, because you use 2,000 different addresses. But everyone knows how to make new addresses, and a whale has 0 problem buying the assets under different addresses. You have the same problem any way you look at it. You achieve decentralization by those whales selling their tokens (I mean, they didn't buy them to hold them forever). They get the secondary market sales if your token was mispriced at launch. That's basic economics, they act as a wholesaler.

Around this point I agree it's totally standard to have a max mint per user. If a bot wanted to get around it they could but it's not an easy process and it helps a lot of projects. For a regular user even minting from 3 wallets is a lot of work.

Still disagree on this, it's actually fairly trivial. If you can code a bot to frontrun mints using Flashbots, then making the bot set up 100 different accounts to do so is just a few more lines of code and no big deal at all. Like I said above, it just makes the optics better if you have a large number of addresses holding tokens instead of a single address, but in the end, the result is just as centralized. You're forcing the bots to spend a little more on gas to transfer funds between accounts and doing several mints instead of a single one, but that's it. The beauty of the ERC721A token contract is that you can save a lot of gas to do big mints, so restricting the mint size just means more ETH burned, not more decentralization or helping real users. But I know my position here is controversial, many communities don't get or agree with this point (again, I think it's all about the optics), so let's agree to disagree on this one.

[0xsublime]

Keep in mind that with this implementation, you should be almost sure that bots will mint at any price you give them, and simply refund if they can't sell. So pricing will become a lot harder. Whitelists may be a way around this.

[elie222]

The key point I believe you're missing on this is that max mint per account isn't only for bots. Most mints aren't botted. So by setting a limit of 10 mints per account, 5 per account, or 2 per account, it does make a difference. Very, very few are. I agree that ERC721R mints are more likely to be botted.

[0xsublime]

Anything a bot can do, a human can do. It only makes a difference for people who don't know they can create more than one address. How many people are that, seriously? And does it seem cool to use their ignorance against them? Why not just tell people that "we prefer if you mint at most X so that everyone in the community gets a chance"? It's as effective, and more hones. At the end of the day, 1 person != 1 address, and we should encourage everyone to learn that.

[elie222]

Anything a bot can do, a human can do. It only makes a difference for people who don't know they can create more than one address. How many people are that, seriously? And does it seem cool to use their ignorance against them? Why not just tell people that "we prefer if you mint at most X so that everyone in the community gets a chance"? It's as effective, and more hones. At the end of the day, 1 person != 1 address, and we should encourage everyone to learn that.

Once again, most mints are not botted. It doesn't make sense to bot them.

Popular projects: People will try bot. Then maybe what you say makes sense.

Most projects: Not botted. Therefore it helps to have this condition in.

It's this latter case you're missing.

Not sure there's too much to add here. The reality is it has a real impact on a huge number of projects. Whether you put limit of 2, 5, or 20.

It's not directly related to ERC721R, so it may only remain in the examples, but the actual package people use won't have this included in any case.

[0xsublime]

Both cases are the same, is my point. Humans and bots can both (extremely easily) mint from several addresses. So the limit only serves to create an illusion of more people minting, it doesn't protect against anyone minting more than they should.

[elie222]

Both cases are the same, is my point. Humans and bots can both (extremely easily) mint from several addresses. So the limit only serves to create an illusion of more people minting, it doesn't protect against anyone minting more than they should.

Reality is different. To mint more than the max mint on a project you have to move eth to other wallets to mint.

Nothing I can add here. You should run an experiment if you don't believe there's a difference but there clearly is. It's a lot of extra effort to mint more than the max and for that reason most people won't. There's also an extra gas cost to do it (unless you already have multiple wallets set up).

[0xsublime]

Can you tell me about the experiments you have run? What were the outcomes?

[elie222]

You should run an experiment

Can you stop trolling us, please. Sorry it's hard for you to understand that creating 10 wallets, sending eth to 10 wallets, clicking mint 10 times is not something people do. Therefore a limit of 5 instead of 50 makes a difference.

[0xsublime]

I'm not trolling. I'm here because I care deeply about NFTs, I know what I'm talking about, and I know that security and good practice is sorely lacking in the space. Your project is a great idea that will get good traction, and lots of people will copy what you do, assuming that it's good practice.

As for the experiment comment, I genuinely believed you have some data that I didn't. Minting from many wallets is certainly something people do all the time to get around these restrictions. The gas cost of transferring is also negligible compared to the mint costs. Anyone interested and well-funded enough to buy 50 will not be stopped by the address restriction.

Thanks for your time.