EIP-2612: 712-signed token approvals

[MrChico]

This EIP is now final. Discussions about final EIPs should be on https://ethereum-magicians.org/

This is a place for discussing https://github.com/ethereum/EIPs/pull/2612, which proposes an additional method permit for making approvals by way of signed messages rather than direct transactions

[axic]

@MrChico this EIP is now in stagnant, but many projects seem to depend on it. I think it would be important getting this to Final to avoid even more fragmentation. Different behaviour can then be codified in a new EIP if a different interface is sought.

[MrChico]

@axic what is missing to Finalize it in your opinion?

[axic]

I haven't 100% checked the content, but from a process perspective it is a few steps. Now that it lapsed into Stagnant, it needs to be brought back to Draft/Review (it is agreed by the editors that it can go directly to Review). One you rallied enough relevant parties (i.e. the people who engaged here), it can be moved to Last Call for (at least 2 weeks) and if no substantial disagreement/comments come up it becomes Final.

i.e. your next step is to create a PR moving it to Draft or Review, but you need to make sure the format matches the requirements of EIP-1, which has undergone several changes since you created the draft (new preamble fields, different section requirements, etc.)

[MrChico]

I tried moving this ERC to last call before https://github.com/ethereum/EIPs/pull/3308, which was met with a series of bike shedding and bureaucratic problems relating to the EIP dependencies. I agree that it would be valuable to mark this EIP as Final. As far as I am concerned the spec is Final. I don't feel particularly motivated to address the numerous comments raised in the linked PR, which really have more to do with formatting rather than the actual spec

[axic]

Oh I see. That is very unfortunate.

[nventuro]

@axic could someone else address the comments to have this be finalized? The entire EIP space is ridden with 'draft' specs that people are afraid to build on, it'd be useful to mark at least some of the most widely used (like this one) as 'final'.

@MicahZoltu's comments seem simple enough to address, except for the request to move 712 to a 'final' stage. This is another of these huge forever-draft EIPs that still see widespread use but remain unfinalized despite pretty much no discussion happening. Is not linking to 712 really a hard requirement?

[frangio]

Seems like we should start by finalizing EIP-712 and then move on to this one.

[MicahZoltu]

Is not linking to 712 really a hard requirement?

Yes. As I mentioned in the comments, one option is to just create a copy of that EIP (with a new number) and actually push it through the process. It would be better to get the authors to push 712 through the process or get the authors of 712 to hand over authorship to you so you can push it through the process, but creating a clone EIP is always a backup option (caveat being that you'll get a new number).

[wuya666]

It seems the current design of the nonces makes the permit functionality strictly sequential for a specific owner, ie. an owner cannot sign multiple approval permits to multiple spenders at the same time, as one spender calling the permit function will invalidate all the other signatures. I'm not too sure if there will be some severe security issues if I change the nonces[owner] mapping into nonces[owner][spender] mapping so that one owner can sign multiple permits to different spenders at a time?

[k06a]

@wuya666 it's possible to keep both:

require(nonce >= nonces[owner], "nonce is too old or cancelled);
require(nonce == noncesFromTo[owner][spender]++, "nonce is too old or cancelled");

function cancelNoncesLessThan(uint256 nonce_) external {
    require(nonce_ <= nonces[msg.sender] + 1_000_000);
    nonces[msg.sender] = nonce_;
}

function cancelNoncesFromTo(address spender, uint256 nonce_) external {
    require(nonce_ <= noncesFromTo[msg.sender][spender] + 1_000);
    noncesFromTo[msg.sender][spender] = nonce_;
}

[k06a]

But maybe we need to manage nonces on higher level, in EIP-712 ...

[k06a]

Or better have layer between EIP712 and EIP2612 to describe cancellable signature-based operations.

[frangio]

I believe permits are usually signed and immediately used, and in this case sequential nonces are not a problem.

[wuya666]

I believe permits are usually signed and immediately used, and in this case sequential nonces are not a problem.

well, it depends on how fast the transaction is mined, which can take seconds to hours (to forever) depending on the gas price provided and the network congestion situation. For standard ERC20 approvals, it's not a problem since I can just queue multiple approval transactions with incrementing transaction nonces and they'll be mined in that order when the gas price is accepted by miners, but for permitted approvals, there's no control over which one is mined first, so basically I need to be sure the last approval is mined, or the deadline is passed, before signing the next approval.

[wuya666]

@wuya666 it's possible to keep both:

require(nonce >= nonces[owner], "nonce is too old or cancelled);
require(nonce == noncesFromTo[owner][spender]++, "nonce is too old or cancelled");

function cancelNoncesLessThan(uint256 nonce_) external {
    require(nonce_ > nonces[msg.sender] + 1_000_000);
    nonces[msg.sender] = nonce_;
}

function cancelNoncesFromTo(address spender, uint256 nonce_) external {
    require(nonce_ > noncesFromTo[msg.sender][spender] + 1_000);
    noncesFromTo[msg.sender][spender] = nonce_;
}

@k06a I don't think it will work by checking the ">=" condition? Let's say I have signed 100 approvals for 100 people out there, with nonces being 0 to 99, how should I increment the nonce[owner] counter after one of them called the permit function?

[k06a]

@wuya666 nonces In my case are used for cancelling only and not requires incrementing at all.

[wschwab]

Seems like we should start by finalizing EIP-712 and then move on to this one.

I'm actively working on trying to get this done, fwiw, but will try putting in more effort. There's a discussion around external links in EIPs which is very relevant there.

One more thing: If it's too late in the game to ask, I totally understand, but wouldn't adding EIP-165 be a good idea? That way platforms would have an easy check if an ERC20 contract has permit or not. I'd be happy to PR myself to add this if @MrChico thinks it's a good idea.

[frangio]

Does anyone have stats on how many tokens already support permit? EIP-165 does sounds like it makes sense for this ERC, if it hadn't been in production for such a long time already.

[wuya666]

@wuya666 nonces In my case are used for cancelling only and not requires incrementing at all.

@k06a without incrementing nonce in the contract, the signature can be used to do permitted approvals multiple times as long as the deadline is not reached right? Isn't that a big security hole?

[k06a]

@wuya666 I meat I use same nonce in multiple ways. One of them increments, another ways is for bulk cancelling.

[vrypan]

Regarding the deadline. We are starting to see cases where an old approve() is used to steal tokens from users: https://twitter.com/dingalingts/status/1470095710888808449

I think it should be a deadline for transferFrom() and not for permit().

I have been drafting an EIP to address this, but do you think that we could/should integrate these ideas in 2612? Here is the draft: https://github.com/vrypan/EIPs/blob/master/EIPS/eip-draft_time_limited_token_allowances.md

[williamkmlau]

It seems the current design of the nonces makes the permit functionality strictly sequential for a specific owner, ie. an owner cannot sign multiple approval permits to multiple spenders at the same time, as one spender calling the permit function will invalidate all the other signatures. I'm not too sure if there will be some severe security issues if I change the nonces[owner] mapping into nonces[owner][spender] mapping so that one owner can sign multiple permits to different spenders at a time?

Hi all, I actually just wanted to throw some support for the nonce changes suggested by @wuya666 above during the discussions in November.

I've worked on several DeFi projects in the past 18 months or so and the existing implementation had always been sufficient, as most use cases only involve a user signing permits to be processed by platform contracts, which is unlikely to be submitted in parallel or in bulk.

However since Q3/Q4 2021 with the NFT boom, I've been working on multiple GameFi projects and have seen major needs instead for platform contracts to sign permits so individual users can collect/claim tokens based on off-chain data stored on game servers. This method is used as an alternative to having to oracle data on-chain, or directly initiating token transfers from a server managed wallet. This is (mostly) because most game projects would want to off-load the gas costs associated with the token transfers to users (without having to worry about users abusing reward collection mechanisms).

The current implementation of nonces[owner] is insufficient to handle signing permits to multiple users simultaenously. A change to nonces[owner][spender] solves this problem and is the method I've been using in my recent GameFi projects.

[frangio]

The recent Multichain/AnySwap vulnerability that put almost half a billion at risk was due to a combination of permit, infinite approvals, and "phantom functions" (i.e. a fallback function that accepts any function call).

A literal reading of the EIP shows that the last part is not strictly necessary. A compliant implementation of permit isn't required to revert if the permit is invalid or old, and this could lead to the same vulnerability that was found there.

I want to suggest adding to the specification that permit must revert if the signature is invalid.

[duckki]

I have the same concern as @frangio. Since permit returns no value, the caller contract can't detect if the permit was successful or the ERC-20 contract simply didn't implement permit. One way to mitigate this is to change permit to return a bool value indicating the success/failure, instead of relying on a token contract to implement permit and revert. In this way, at least, the caller contract can detect token contracts that didn't implement permit. Also, a wrapper function (say, safePermit()) could be implemented analogous to safeTransfer/safeTransferFrom.

[frangio]

Adding a boolean return value would be a breaking change for the many implementations already in production, so I don't think it's a good idea. But reverting on invalid signatures seems to be what current implementations are doing so we should just make that an explicit part of the specification.

A wrapper function could check that the account nonce is incremented, as suggested in https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3145.

[duckki]

Indeed, OpenZeppelin/openzeppelin-contracts#3145 is way to go.

[k06a]

@frangio increasing amount of returning data is not actually a “breaking” change. Solidity reverts when returned data size is less than expected by interface.

[k06a]

@frangio increasing amount of returning data is not actually “breaking” change. Solidity reverts when returned data size is less than expected by interface.

[frangio]

Depending on the specific change we're talking about, that may be true in in one direction (future implementations compatible with current users) but not in the other (future users not compatible with current implementations).

I mean that all current users of permit would work fine with a hypothetical permit implementation that returns a value. But if the EIP is changed so that a return value is required, future users of permit that expect a return value would break with all the current implementations that don't return anything.

The way around this is to specify the return value as optional. But this is not natively supported by Solidity at least, so it would be more complicated to use.

[k06a]

@frangio agree. Good thing that we already have experience in handling this: SafeERC20

[gonbad]

It would be better to put nonce in the permit function arguments. This way we can use a random number as nonce and prevent concurrency issues.

[gonbad]

One use-case of these contracts could be cheques; so it would be nice to have a form_timestamp argument.

[gonbad]

It's sad that we don't have a PKCS standard for encoding r, s and v to a single bytes argument.

[TimDaub]

The current document links to files of the Uniswap GitHub organization:

• https://eips.ethereum.org/EIPS/eip-2612#test-cases: https://github.com/Uniswap/uniswap-v2-core/blob/master/test/UniswapV2ERC20.spec.ts.
• https://eips.ethereum.org/EIPS/eip-2612#reference-implementation: https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2ERC20.sol

A few notes:

• When publishing EIP-4973 for DRAFT, I was asked to put such files as CC0 licensed in the EIPs repository's assets folder.
• Uniswap/v2-core is GPL3
• The links aren't permanent but they point to Uni's "master" directory, meaning those files could change over time
• Additionally, I think it's in the interest of the EIP process to be self-referential. So links pointing to external sources should be considered invalid. Otherwise, I'm happy to also host all of EIP-4973's files on my STARTUP'S GitHub so that everyone can see what awesome work I do etc.

Few more notes:

• Discussions-To points to this GitHub issue but should point to Ethereum Magicians
• There are other links referencing external sources. I don't know what is the general line of thinking in the EIP process, but IMO those links should at least be automatically snapshotted with e.g. the webarchive or idk uploaded to IPFS for posterity.

[MicahZoltu]

I agree that the external links should be removed.

This EIP was crafted before we moved discussions to Ethereum Magicians though, and we are generally grandfathering existing discussion links in GitHub to avoid discontinuity in discussions. If the authors want to move the discussion to Ethereum Magicians that is great, but not required.

[phil-ociraptor]

TL;DR: EIP appears stuck mostly because: 1) EIP-712 itself not yet finalized, 2) official support for EIP-1271 not agreed upon, 3) debate on if nonces should be nonces[owner] vs nonces[spender][owner].

Suggestion is to finalize EIP-712 as is, as well as ERC-2612 as is, but also introduce an optional alternative i.e. ERC-2612A, that is more feature rich (i.e. has EIP-1271 support and nonces[spender][owner]). Unblocks us and can move forward on this very popular EIP

This EIP has been outstanding for over 2 years, despite having many implementations in the wild already. The longer the standard is not finalized, the longer there will be ambiguity, and create more reluctance to adopt this standard, which most people seem to want.

Many new tokens are already implementing EIP-2612, but the draft status of this EIP is a deterrent for adoption.

We have already resolved security concerns about tokens that do not revert, by recommending calling contracts use safePermit (https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3145) and specifying that tokens should revert if they wish to correctly implement EIP-2612 (https://github.com/ethereum/EIPs/pull/4728/files) (but contracts should still use safePermit)

It seems to me that the following points are blockers:

• EIP-712 is in Last Call, but not Final (as a representative from 0x - where EIP-712 originated, I can try to herd the cats on this one)
• nonces[owner] vs nonces[spender][owner]: I agree that the nonces[spender][owner] is preferable, however, this will result in a breaking change to the interface that exists for many tokens already deployed
• whether EIP-1271 support should be required such that smart contract wallets can benefit from permit support

It seems to me that we may need another ERC, i.e. ERC-2612A (or something better named, suggestions welcome), which codifies the nonces[spender][owner] and EIP-1271 behavior, but that should be a variation of the EIP-2612 standard. This variation might need some additional functions/flags (i.e. a function that identifies this contract as a 2612A variant, such that safePermit knows to type switch on the correct variation when checking the nonce increase)

This allows us to:

• get ERC-2612 finalized, such that all token contracts that already implement this can get "retroactive" status as being fully compliant with a finalized EIP, instead of being in the awkward position of implementing a standard that is still in review
• (stretch goal) produce a second ERC, i.e ERC-2612A for the nonces[spender][owner] and EIP-1271 behavior

The names of ERC-2612 and ERC-2612A can be improved to indicate that one is the legacy, widespread version and the other is the new and preferred version, but this allows us to push ahead.

How does this sound to everyone here, and am I missing anything?

As an aside, EIP-1271 support needs to be carefully considered, and I'm ok with dropping support for it in the alternative impl. Calling an arbitrary contract in the context of granting approvals seems like it's tempting fate - but with careful consideration, maybe we think carefully of the attack vectors and its ok to add

Once this is done, let's get this finalized for NFTs too 👀 https://eips.ethereum.org/EIPS/eip-4494 cc: @wschwab

[frangio]

I've already been doing the work to move EIP-712 to Final, by moving it to Last Call just a few weeks ago and going through the requisite editorial process. The Last Call period ends on Monday and the EIP should move to Final then.

Once that happens, I believe EIP-2612 should be moved to Last Call.

I also recently proposed EIP-5267 which tackles another blocker for EIP-712 adoption. Those interested should review that and give feedback and I will try to finalize it soon as well.

As for the nonces issue, I agree that at this point EIP-2612 is de facto Final and shouldn't be changed in any backwards incompatible way. I would support a "permit v2" ERC that could be implemented alongside EIP-2612 or on its own. I'd add support for EIP-1271 signatures to the wishlist.

[phil-ociraptor]

Amazing - EIP-5267 will solve one of the biggest pain points around using EIP-712: "how in the world do I generate the domain separator?".

Thank you for all the great work!

As an aside, I'd love to spec out a new reference impl of WETH9 for new EVM chains and L2s such that they can support permit - I think the major decision point there will be if this contract goes w/ permit support of this "permit v2" as well as the addition of gas saving functions such as withdrawTo

[frangio]

Proposing to move EIP-2612 to Last Call with deadline September 6th 2022. https://github.com/ethereum/EIPs/pull/5506

[frangio]

This EIP is now in Last Call until October 8th.

[k06a]

What about EIP-2098 (Compact Signatures Representation)?

[frangio]

@k06a What is your proposal?

[k06a]

Let’s store v component of the signature in top bit of s component since it’s always empty. This would allow to save 32 bytes of calldata (including padding)

[frangio]

In my opinion we should consider EIP-2612 frozen and start working on an improved EIP that extends this one, as suggested by @phil-ociraptor above.

Note that using EIP-2098 is at odds with EIP-1271 which is another thing that has been proposed as a needed improvement to permits.

[TimDaub]

why? In Eip-4973 we use both 1271 any 2098 and I didn't notice anything at odds

[frangio]

In EIP-1271 the signature has no specified semantics, it should be seen as a blob, so it's not correct to apply the EIP-2098 encoding to it since it's specific to ECDSA. Unless perhaps it's only applied as an optimization for the EOA case? I need to think about that more.

[TimDaub]

Cross linking to EIP-4973 on Magicians here for reference: https://ethereum-magicians.org/t/eip-4973-account-bound-tokens/8825/146?u=timdaub

[Pandapip1]

EIP-2612 need not be considered frozen. You can always change the spec until it's final (as was done with EIP-712).

[frangio]

That is true in theory, but in practice making normative changes to an EIP that's been widely deployed can be very problematic so I think it should be avoided.

[Pandapip1]

That is true in theory, but in practice making normative changes to an EIP that's been widely deployed can be very problematic so I think it should be avoided.

Fair enough. It should be pushed to final regardless, though.