Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EIP-2612: 712-signed token approvals #2613

Closed
MrChico opened this issue Apr 24, 2020 · 108 comments
Closed

EIP-2612: 712-signed token approvals #2613

MrChico opened this issue Apr 24, 2020 · 108 comments

Comments

@MrChico
Copy link
Member

MrChico commented Apr 24, 2020

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

This is a place for discussing #2612, which proposes an additional method permit for making approvals by way of signed messages rather than direct transactions

@maurelian
Copy link
Contributor

IMO the 'security considerations' section should mention the front running attack that can occur when an allowance is reduced, either by approve or the new permit function. I don't think this proposal changes the risk or possible mitigations in any way, but I haven't thought hard about it.

If you have a strong desire to do some bike shedding, you could even suggest a specific mitigation for it. 😝

@Nipol
Copy link

Nipol commented Apr 24, 2020

I was looking forward to talking about this. I'd like to talk about the need for a "deadline"

What aspects of Tx Relayer do we need to take into account "deadline"? The Tx Relayer may have a response that causes tx while setting tx fee low. or no response.

And when a user signs for an interface, it requires more information. or more knowledge. Is usability not compromised? and more complexity for Tx Relayer.

So I think the deadline is unnecessary. 🤔

@maurelian
Copy link
Contributor

I like deadline, as it solves the problem of worrying about all the approvals one has floating out in the world.
That said, I am sympathetic to the argument that it is inconsistent with the existing approve interface.

@MrChico
Copy link
Member Author

MrChico commented Apr 24, 2020

Thanks @maurelian, added a comment about the approval front running attack.
@Nipol The deadline parameter is a way to mitigate the free option that the relaying party gets when receiving a signed message (it is just a deadline for the Permit itself, not for the approval so I don't think it solves the problem of random tokens being given allowance). In cases where the free option is not a concern, deadline can simply be set to uint(-1), so it should be seen as an optional parameter

@MrChico
Copy link
Member Author

MrChico commented Apr 24, 2020

Actually @maurelian I was just made aware of the xDai staking token implementation of permit which I think does what exactly what you are alluding to here. It shares the same ABI as Dai and Chai, but not the same semantics. I guess we better document all three flavors in the ERC

@Nipol
Copy link

Nipol commented Apr 27, 2020

In terms of "Free Options," I think deadline has sufficient requirements.

But in terms of EIP-712, deadline is not convincing.
deadline is a difficult option for most signer to understand. at now, no example of an interface receiving a deadline value exists.

We can't show you the "Free Options" article every time ask for a signature. If we cannot effectively explain deadline to the signer and Tx Relayer, uint256(-1) will be used in most cases.

Furthermore, deadline is a consideration that does not appear in the EIP-712 specification. I think it does not have to appear as an exception in Permit.

If all of these signatures necessary a "deadline" option, if all of EIP-712 signatures have the same security level, I think it's better to update the EIP-712 specification. 🙂

What is the best option? What do you think?

@MrChico
Copy link
Member Author

MrChico commented Apr 27, 2020

We can't show you the "Free Options" article every time ask for a signature. If we cannot effectively explain deadline to the signer and Tx Relayer, uint256(-1) will be used in most cases.

I'm perfectly happy with uint(-1) being used in most cases.

deadline is a difficult option for most signer to understand. at now, no example of an interface receiving a deadline value exists.

There are already interfaces which deal with deadline, uniswap being one of them:
Screen Shot 2020-04-27 at 12 21 12

or, for Permit https://stablecoin.services automatically sets deadline 4 minutes into the future.

I can agree with your point that it may a difficult point for users to understand, and one that may require some EIP-712 tweaking to improve. Another ux improving solution could also be to introduce Date as a type to 712, to improve how it is rendered to the user.

I do think that these options fall outside the scope of this ERC though, especially since this in some sense more of a descriptive erc than a prescriptive erc; I am highlighting what has been implemented and the motivations behind it. We can certainly make tweaks in new versions, but I am mainly interested in documenting what already exists here.

@axic
Copy link
Member

axic commented Apr 27, 2020

I've seen on twitter that apparently there are at least 3 different semantics contracts follow and still call it "permits". @MrChico you proposed to discuss all in the EIP/ERC.

I think an EIP should be unambiguous and promote a single way of doing things. Is it possible to do that while perhaps mentioning "semi-compatible" contracts? If not, would it make sense having 3 different EIPs?

@MrChico
Copy link
Member Author

MrChico commented Apr 27, 2020

@axic I'm planning on mentioning the Stake-flavored implementation under "backwards compatibility", making that, the dai and chai implementations all belong to this " semi-compatible" class, but keep the specification as is for specificity

@axic
Copy link
Member

axic commented Apr 27, 2020

Nice, sounds good!

@Nipol
Copy link

Nipol commented Apr 27, 2020

@MrChico Good! I think we've written quite a meaningful conversation in publicly.

It will be the data we or the next people will refer to when working in the future!
😉

@ptrwtts
Copy link

ptrwtts commented Apr 29, 2020

The standard ERC-20 race condition for approvals applies to permit as well.

Why not take the opportunity to fix this? Otherwise, we might end up with proliferations of safePermit too. Is the assumption that the allowances will be spent immediately, so it's not actually a problem?

@MrChico
Copy link
Member Author

MrChico commented Apr 29, 2020

The standard ERC-20 race condition for approvals applies to permit as well.

Why not take the opportunity to fix this? Otherwise, we might end up with proliferations of safePermit too. Is the assumption that the allowances will be spent immediately, so it's not actually a problem?

To be honest I don't find the approval race condition to be much of an issue in practice – it's only ever a problem when allowing addresses that can "spontaneously" spend your allowance. In most use cases, you give your allowance to a smart contract in such a way that only you can later initialize the transferFrom from your own address.

I'm all for people implementing safePermit if they feel that this is a concern, but in order to limit the scope of what I'm trying to do here I think such alternatives should be in a different ERC

@nventuro
Copy link
Contributor

nventuro commented May 15, 2020

It wasn't clear to me from a cursory read of the EIP that the following getter is also part of the required interface, I'd make it more explicit:

function nonces(address owner) external view returns (uint256);

I'd also consider changing its name to something else, since nonces is quite generic and non-descript.

@MrChico
Copy link
Member Author

MrChico commented May 15, 2020

I'd also consider changing its name to something else, since nonces is quite generic and non-descript.

Do you have a suggestion?

@MrChico
Copy link
Member Author

MrChico commented May 15, 2020

Keep in mind that a change in this name would make this etc incompatible with Uniswap v2 (and even more incompatible with dai and chai)

@petejkim
Copy link
Contributor

petejkim commented May 15, 2020

I'd also consider changing its name to something else, since nonces is quite generic and non-descript.

Do you have a suggestion?

permitNonces

@MrChico
Copy link
Member Author

MrChico commented May 15, 2020

Its okay for nonces to be generic, in some cases it can be an advantage. The erc doesn't specify other methods can't use nonces for other purpose

@MrChico
Copy link
Member Author

MrChico commented May 15, 2020

But most of the time, this doesn't seem like a particularly important concern

@wighawag
Copy link
Contributor

I think it would be useful to have a function that let smart contract know if the permit call will succeed if executed.
For example, when a contract called via static_call need to check the validity of the signature before handing the operation to another contract/call.

This could be like :

function isValidPermit(address owner, address spender, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s) external view returns (bool);

@MrChico
Copy link
Member Author

MrChico commented May 16, 2020

I think it would be useful to have a function that let smart contract know if the permit call will succeed if executed.
For example, when a contract called via static_call need to check the validity of the signature before handing the operation to another contract/call.

This could be like :

function isValidPermit(address owner, address spender, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s) external view returns (bool);

This is interesting. But if they were called with staticcall they wouldn't be able to execute the permit later anyway? In the cases I can imagine, it seems like you'd want to submit the permit whenever if it is well formed so it seems a try-catch pattern would be sufficient for this use case. Or is there some scenario you can think of in which you would NOT want to submit a well-formed permit?

@MrChico
Copy link
Member Author

MrChico commented May 16, 2020

Hmmm, I guess one could imagine a sort of pure validity checking contract which would be called with staticcall and return true to the original caller if all is ok, and the original caller would not be constrained to a STATIC environment....
Did you have something like this in mind? Do people actually write contracts like this?

@wighawag
Copy link
Contributor

Yes, that's basically it.
GSN (https://docs.opengsn.org/learn/index.html) have this architecture where a "view" call is made first to a "paymaster" contract that can answer whether it can pay for the metatx. If it answer no, the metatx do not proceed and it is not charged. If it says yes, another call will be made where the "paymaster" can now call permit. The gas consumed here will be charged

@MrChico
Copy link
Member Author

MrChico commented May 23, 2020

@wighawag couldn't you just as well check the validity of the permit in the "paymaster" in that case? As per the spec, you need to validate:

  • The current blocktime is less than or equal to deadline.
  • owner is not the zero address.
  • Token.nonces[owner] is equal to nonce provided nonce.
  • r, s and v is a valid secp256k1 signature from owner of an ERC-712 typed Permit message.

all of which can be verified in a static environment.

@MrChico
Copy link
Member Author

MrChico commented May 23, 2020

Although the nonce is not given as an argument in the permit function, it will equal to Token.nonces[owner] if it is indeed a valid signature.

@nventuro
Copy link
Contributor

I think this EIP can do without the view method described above. The reason why it's there in GSN is because part of that function is calling into a contract and performing an arbitrary query: by encapsulating the call there clients can get a boolean return value without having to know anything about the end recipient.

@wighawag
Copy link
Contributor

wighawag commented May 23, 2020

@MrChico @nventuro That's exactly what I do as a workaround, see here : https://github.com/wighawag/gsn-playground/blob/5f40cc949031129374a48e43c8a0b71b83e52bf1/contracts/src/DAIPaymaster.sol#L101-L119

but this does not scale if my paymaster wanted to support all token that support the permit standard (unless A DOMAIN_SEPARATOR view was also part of the standard), hence why in my opinion a function like isValidPermit should be part of the standard.

By the way in regard to EIP-712 domain. I am not sure this should be part of the standard. EIP-2612 Contract implementer should be free to set the EIP-712 domain they see fit.

@phil-ociraptor
Copy link

phil-ociraptor commented Aug 7, 2022

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
Copy link
Contributor

frangio commented Aug 19, 2022

Proposing to move EIP-2612 to Last Call with deadline September 6th 2022. #5506

@Pandapip1 Pandapip1 changed the title ERC-2612 permit: 712-signed token approvals EIP-2612: 712-signed token approvals Aug 28, 2022
@frangio
Copy link
Contributor

frangio commented Sep 23, 2022

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

@k06a
Copy link
Contributor

k06a commented Oct 4, 2022

What about EIP-2098 (Compact Signatures Representation)?

@frangio
Copy link
Contributor

frangio commented Oct 7, 2022

@k06a What is your proposal?

@k06a
Copy link
Contributor

k06a commented Oct 8, 2022

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
Copy link
Contributor

frangio commented Oct 8, 2022

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
Copy link
Contributor

TimDaub commented Oct 10, 2022

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

@frangio
Copy link
Contributor

frangio commented Oct 11, 2022

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
Copy link
Contributor

TimDaub commented Oct 11, 2022

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
Copy link
Member

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

@frangio
Copy link
Contributor

frangio commented Oct 13, 2022

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
Copy link
Member

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.

@TimDaub
Copy link
Contributor

TimDaub commented Oct 13, 2022

I think we should go full no-mercy here. If ppl implemented an EIP in non-final status that's their fault and I don't think we should give into "oh but they have implemented it already." We're EIP editors and our legitimacy comes from executing EIP-1 to the best of our capabilities.

I know this will make some enemies but I believe it sends the right signal to implementers to make breaking changes to EIP-2612.

Or alternatively move EIP-2612 into withdrawn unless someone steps up and moves it into final. And then create a new EIP with the breaking changes.

@frangio
Copy link
Contributor

frangio commented Oct 13, 2022

unless someone steps up and moves it into final. And then create a new EIP with the breaking changes.

This is exactly what was proposed and what is happening. I opened the PR to move to Final yesterday: #5782

@rube-de
Copy link

rube-de commented Oct 31, 2022

any reason (beside the CI error of missing ethereum-magicians.org thread) this is not Final now?

@Pandapip1
Copy link
Member

This has reached Final.

@invocamanman
Copy link

invocamanman commented Nov 17, 2022

Hi! i might be late, but i want to propose to add:
function DOMAIN_TYPEHASH() external view returns (bytes32)

The rationale behind is that there are right know 2 typical ways of creating the DOMAIN_TYPEHASH:

  • The one proposed on EIP-2612 as we can see in WETH10 impl
    • keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)")
  • And another one that basically retrieve the version field, which is used for example in UNI impl
    • keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)")

With the current standard view functions, a front-end cannot know which type of Domain uses the contract that want to interact with permit, and therefore cannot create properly the signature.
If we expose DOMAIN_TYPEHASH, the FE will be able to discern between these 2 implementations.
Currently some of contracts implement this as DOMAIN_TYPEHASH another ones EIP712DOMAIN_HASH and another ones do not implemented at all.
I think it will help add this to the standard to avoid confusions and help front-ends.

@frangio
Copy link
Contributor

frangio commented Nov 17, 2022

@invocamanman The domain hashes are not enough. Please see EIP-5267 for a proposal to tackle this problem.

@invocamanman
Copy link

invocamanman commented Nov 17, 2022

It's true, i was proposing a fix for a current common situation, but this solution has more general purpose and seems better, thanks for sharing!

@ethereum ethereum locked and limited conversation to collaborators Feb 24, 2023
@lightclient lightclient reopened this Jul 28, 2023
@ethereum ethereum unlocked this conversation Jul 29, 2023
@Pandapip1
Copy link
Member

@lightclient if you meant to unlock, fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests