Add a suggestion to disallow transfers to the contract address

[qbzzt]

Fixes #3157 (or at least reduces the risk)

PR Checklist

• [ ] Tests
• [X] Documentation
• [ ] Changelog entry

[Amxx]

Hello @qbzzt

I'm not sure if we want to include such a recommendation in our docs, but if we want, then the piece of code in the example is really not how people should do it.

In your example, you modify the ERC20.sol contract, which come from some package manager and should not be tampered with. If you want to disable transfers to some address, you should probably do it by overriding the "beforeTokenTransfer" hook

function _beforeTokenTransfer(address from, address to, uint256 amount) internal virtual override {
    require(to != address(this));
    super._beforeTokenTransfer(from, to, amount);
}

This is simpler to do than changing the core code, and it will also prevent minting directly to the contract.

[qbzzt]

What would be the proper way to do this? Add ERC20SafetyRails.sol to https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ ?

[Amxx]

I'm not sure an extensions is needed.

IMO developers can just add this _beforeTokenTransfer override directly in their contract. We could add a toogle to the wizard that adds this check.

[qbzzt]

The reason I'm trying to get this added somewhere is that in many cases developers don't add this feature, even though they don't need the contract to hold tokens and it is a common usage mistake. Mass adoption requires being clueless user friendly.

Where is the source code for the wizard? Adding it there could also solve the problem.

[frangio]

I'm really thinking we should just add ERC20SafetyRails (naming TBD) into the library. If we think this should be standard, that is one of the strongest ways to make the recommendation.

@Amxx Do you see issues with adding this as a separate contract that can be opted in? It can have a Wizard toggle, and we can consider enabling it by default.

By the way the source for for Wizard is at OpenZeppelin/contracts-wizard.

[Amxx]

I'm afraid that having to many extensions will hurt discoverability.

We currently do not include an ERC1363 extension, despite the ERC being useful and final ... yet we would have this, which any decent dev could write faster than it would take them to find it in our doc.

I have nothing against this module, just doesn't feel like the most important thing we can offer to our users

[MarkP42]

Hi Guys, I'm a programmer but new to crypto. I'm wondering why you wouldn't put the contract address check in the before..Transfer function in the main (super) ERC20 contract? I've probably missed something haven't I?

[frangio]

@MarkP42 No, you're right. This PR needs to be edited so that the suggested check is added in a _beforeTokenTransfer hook as opposed to _transfer.

[qbzzt]

I decided to write this as an ethereum.org article instead.