Make permit compatible with smart-contract wallets like Gnosis and Argent

[juniset]

Motivation

Make the permit method compatible with smart-contract wallets that implement EIP1271, and make smart-contract wallets compatible with dapp flows that leverage the permit method (e.g. via WalletConnect).

Requested change

Replace ECDSA.recover by SignatureChecker. isValidSignatureNow in

openzeppelin-contracts/contracts/token/ERC20/extensions/draft-ERC20Permit.sol

Line 55 in aefcb3e

address signer = ECDSA.recover(hash, v, r, s);

.

[Amxx]

Thank you @juniset for raising this issue.

It is technically possible, and I would even say easy, to fix this issue for ERC20Permit.

One should note that ERC20Votes (our implementation of the Comp voting token) and Governor (base one Compound's Governor's ABI) do use ECDSA.recover in a context where the user address is unknown (recovered by ecdsa). This means that there will still be part of our code where we cannot unfortunately use SignatureChecker.isValidSignatureNow.

• ERC20.delegateBySig
• Governor.castVoteBySig

[frangio]

We really want to do this, but the current text of EIP-2612 is very explicit that the permit is an ECDSA signature by the token owner address:

• r, s and v is a valid secp256k1 signature from owner of the message [...]

So we don't feel comfortable making this change unilaterally, and the EIP is still in Draft so there is an opportunity to make this change directly in the spec. The fact that it is backwards compatible makes me hopeful that it should be accepted without much pushback.

We think the best course of action is to propose a pull request in ethereum/EIPs with the required change to EIP-2612. @juniset What do you think?

[juniset]

@frangio I agree that the spec of EIP-2612 should be clarified to include EIP-1271 signatures as being valid. I had raised that issue a while ago (ethereum/EIPs#2613 (comment)) and mentioned it again today.

However, one could argue that using SigantureChecker instead of ECDSA in your implementation today is compliant with the specification since a EIP-1721 signature is a valid secp256k1 signature from the owner of the message when the owner is a smart-contract, i.e. it depends of your definition of "valid".

Adding it today would also give a signal to the supporters of EIP-2612 and encourage them to clarify the definition of a valid signature.

[frangio]

@juniset Just to clarify I meant opening a pull request directly with the change ready to be accepted by the authors of the EIP. I think this will be a more sure way to get it accepted.