OpenZeppelin / openzeppelin-contracts

OpenZeppelin Contracts is a library for secure smart contract development.
https://openzeppelin.com/contracts
MIT License
24.97k stars 11.8k forks source link

EIP-2612: Add ERC20 permit() function #2206

Closed nventuro closed 3 years ago

nventuro commented 4 years ago

There's been some discussion about an ERC20 extension with the permit function, popularized by MakerDAO's Dai token (see the code here).

This recently became even more relevant in light of hacks revolving ERC777 and reentrancy, since permit could help solve some of the UX issues that drive people to using ERC777 improperly. It looks like a proper ERC might be in the works: we should make sure we're compliant if that ends up happening. In all likelihood the spec will be identical to the Dai code, however.

Link to EIP-2612

miohtama commented 4 years ago

I agree that permit() needs its own ERC.

However, the ERC-777 solves UX issues better than permit(). permit() requires all wallets to have a special flow to sign + send. In ERC-777 we can just send() tokens to the smart contract. ERC-777 t is semantically more correct and also from the user perspective much easier to understand.

I think it is a separate discussion if people are using ERC-777 improperly or not, and maybe we should not go in there on Github.

miohtama commented 4 years ago

For the underlying issue of re-entrance:

Currently the reason why these hacks happen are "bad defaults". Re-entrance should be opt-in, not opt-out.

nventuro commented 4 years ago

Yes, I wasn't suggesting ERC777 should be removed or avoided (on the contrary, I think it opens the door to many interesting possibilities), but it does require a deeper understanding of the EVM and the Ethereum ecosystem, partly due to the 'bad defaults' you've mentioned.

In that sense, permit would let people get a better UX than plain ERC20 offers without having to worry about the dangers of misusing it.

abcoathup commented 4 years ago

EIP: https://github.com/ethereum/EIPs/issues/2613

mds1 commented 4 years ago

In all likelihood the spec will be identical to the Dai code, however.

I don't think this is necessarily true. There's two potential improvements I've seen discussed, which I'm sure you're aware of, but figured I'd mention anyway:

Regardless of the spec decided upon, I still think it's valuable addition

nventuro commented 4 years ago

Thanks for mentioning those improvements @mds1! The ERC is very detailed - I encourage people interested in this to go read it.

The ERC references an xDai staking token with an alternative use, where expiry instead refers to how long the allowance is valid for, which helps prevent allowances from lasting indefinitely

That implementation has permit with a boolean argument, which greatly simplifies working with expiry: we'd otherwise need to e.g. track how much allowance each permit granted, and then remove that. I'm not sure permit(uint256 value, uint256 expiry) would work.

Both permit(bool, uint256 expiry and permit(uint256 value, uint256 deadline) make sense to me, but in the interest of reducing fragmentation I think it'd be probably better if we stick to a single one (whatever the EIP goes with). Should we move forward with an implementation before the EIP is finalized however, or wait for it to mature a little?

abcoathup commented 4 years ago

An alternative could be: https://eips.ethereum.org/EIPS/eip-3009

https://twitter.com/smpalladino/status/1313526048143945729?s=20

Ro5s commented 4 years ago

Yes, I wasn't suggesting ERC777 should be removed or avoided (on the contrary, I think it opens the door to many interesting possibilities), but it does require a deeper understanding of the EVM and the Ethereum ecosystem, partly due to the 'bad defaults' you've mentioned.

In that sense, permit would let people get a better UX than plain ERC20 offers without having to worry about the dangers of misusing it.

@nventuro what are your thoughts on combining ERC777 features (like transferAndCall-type deposits) and permit() signature txs? Does having both add value to an ERC-20 token?

nventuro commented 4 years ago

I'd say it depends on what you intend to use the token for. For 'regular' ERC20 usage (e.g. exchange, participation in DeFi platforms), the bare interface is enough since there's no feature discovery.

Ro5s commented 4 years ago

Thanks for response @nventuro. Here, wondering how an updated WETH contract might use both permit() to establish more gasless transactions, as well erc677 to reduce number of txs, metamask popups: https://github.com/wETH-v2/wETH-v2/pull/19

alfredolopez80 commented 3 years ago

Another focus of the problem may be this new method of open zeppelin can help https://docs.openzeppelin.com/contracts/4.x/api/utils#SignatureChecker

@abcoathup @nventuro

frangio commented 3 years ago

@alfredolopez80 Please ask in the forum. https://forum.openzeppelin.com

alfredolopez80 commented 3 years ago

@alfredolopez80 Please ask in the forum. https://forum.openzeppelin.com

Sure i will do today