No I see, but I don’t think this function is necessary. Just permit is enough. You grant a permit to a smart contract that will first invoke permit and then invoke either of the standard ERC721 transfer functions.
1 Like
I’m not sure if we shouldn’t add it.
I had already added an example of safeTransferWithPermit() in the example of implementation I created for the EIP (erc721-with-permits/NFTMock.sol at main · dievardump/erc721-with-permits · GitHub)
It can be really useful since it allows a user to use a permit and transfer the NFT in one go. Which is great to save gas and time.
Problem is, it adds another method to a standard that is already not so small.
I think there are pro and cons for this idea.
One thing I haven’t seen mentioned here is the mismatch between who is allowed to approve versus permit. approve allows an approved-for-all sender, while permit only allows the owner to sign. The maximally general version of permit should have the following form:
function permit(
address ownerOrApprovedForAll,
address spender,
uint256 tokenId,
uint256 deadline,
bytes memory signature
) external;
in order to support 1271, 2098, and to homogenize approve/permit. Alternatively, to more closely mirror 2612, the form could be:
function permit(
address ownerOrApprovedForAll,
address spender,
uint256 tokenId,
uint256 deadline,
uint8 v,
bytes32 r,
bytes32 s
) external;
but this makes the compatibility with 1271 somewhat less useful.
And just to add my $0.02, I think safeTransferFromWithPermit and its ilk are unnecessary since the same behavior can be achieved with a simple helper contract. ERC721 is already a conceptually/bytecode large standard to support; let’s not bloat it further with features that can be achieved simply through other means. The focus of the standard should be to build minimal blocks that enable useful applications, not to build the applications themselves.
1 Like
The example of implementation coming with the EIP allows both Approved on the tokenId and approvedForAll of the owner to create permits on a token id.
This doesn’t have to come in the method signature, it’s how the signer address is checked that matters here
However I am of the idea that this is just a sample, and implementers should decide themselves if they want any approved, approved for all or only owners to sign permits.
i.e: In the implementation I already use, I only add ApprovedForAll and owners
About mirroring 2612, as discussed before, I think using (v, r, s) was a bad idea and that we should stick to bytes memory, to accommodate all present and future types of signatures (v, r, s) (r, vs), …
Ahh, but unfortunately it does. In order to support 1271 with approved-for-all permits, the “signer” has to be an explicit argument to the function. Otherwise, how do we know which contract to call isValidSignature on? The implementation you provide above only allows the owner to use 1271.
1 Like
Relying on this Solidity quirk as a solution doesn’t seem like a good idea. I don’t think there is any additional overhead to specifying it as tokenNonces(uint256 tokenId) instead in order to prevent conflict with other uses of this function.
Note that ERC2098 would have the same size calldata as uint8 v, bytes32 r, bytes32 s since there is only 64 byes (plus length). uint8 takes a full 32 bytes to encode in calldata.
I like that ERC2098 is also more flexible (at the expense of extra gas cost to process)
I like EIP-3074, but conversations around it have really slowed down since EIP-3074 was first discussed. It makes me think that it’s at least a few years away from going live.
Overloading like this isn’t the best solution I think.
@frangio is right @wschwab, smart contracts can natively batch txns very easily, so there is no necessity for this use case of transfer w/ permit in one function.
Why doesn’t a straight transfer work then?
2 Likes
Additional questions/comments:
- Why does nonce need to change when the token transfers?
It sees like the nonce will no longer be valid if you change the owner. I suppose the only caveat to this would be if you transfer it, and transfer it back, and no nonce change has occured, then older signatures are now valid (assuming their deadline hasn’t expired)
-
ERC721 approvals are action-based, meaning they are only good for one action and not a limited number of tokens. This might batch interactions much harder to replicate (in a use case I am working on, the NFT owner can decide to have their NFT mutated a number of times in order to fulfill a rebalancing need)
-
I think allowing the
ApprovedForAlloperator have permit authority might be really dangerous, especially given multiple NFT ownership where the nonces might be the same. Additionally, it’s unlikely that this type of operator would be an EOA to send signatures in the first place, nevermind the potential for requiring another nonces field to track per-account nonces alongside per-token ones.
2 Likes
NOTE I believe due to the recent AnySwap hack, this EIP should incorporate a bool return value (that is just True) so that the permit call can be authenticated as passing.
1 Like
This is the reason, and I think it’s primordial that a permit could not be used after any transfer.
And we can see it now with the OpenSea debacle on OrderBooks with old orders still valid, it has to be 100% clear that after a transfer, even if the nft comes back to you, any Permit is revoked.
The idea was to allow X to transfer/sell the NFT they have in their Gnosis-Safe, without having to transfer them back in their main account.
However as I mentioned, for me it’s up to the implementers. I did add it like this because I think ApprovedForAll should have as much right as an owner (because with Permits, there will be much less need to add accounts as ApprovedForAll, only the bare minimum)
I see Permits as a perfect use case for selling stuff on MarketPlaces and never have to do an approve/setApprovalForAll for a contract anymore.
- Select your NFT sitting in your Vault
- fill what is required from the marketplace
- Sign a Permit
- Sign a Sell Order
If you change your mind, just up the nonce, revoke the permit or transfer your NFT to yourself and you’re good. All from outside your Vault.
I don’t really understand this. Can you explain?
I think that would be wise yes.
Totally agreed on this now, yes.
This is actually in conflict with the above re: OpenSea hack, because what really happens is that people sign orders and forget about their existence. Permits are best used as a short-term use case where a metatxn service provider might batch process a bunch of permits for a given contract (say for example, an auction contract). You are approving the token for transfer by another entity, there is no concept of an actual “exchange” in the permit use case (nor should there be).
These should be left up to separate use cases to define.
This will make the logic more complicated, because you will have to have a per-account nonce, and a per-token nonce, and updates to both will have to be handled whenever an action occurs. Also, ApprovedForAll is more a “forever permit” (along the lines of EIP-2612) vs. this standard, which is doing single token transfer permits. The contexts are so mixed.
It’s more likely that an operator would be a contract than an EOA. Not sure the comment makes as much sense upon reading it again.
Upon further conversations, this is not really a security critical issue (it was a red herring in the disclosure), but for gas efficiency’s sake, it is a bit cheaper to return a value than not.
I sure disagree with this. I see permit as a way to secure tokens by suppressing the need to give approvals/approvalForAll to exchanges contracts.
That’s actually the use case I’m the most interested in with the ability to let someone “take an NFT from my account” without me having to spend one cent in gas.
I see those use cases way more “day to day use case” than a metatxn service batch processing stuff. and that’s why I brought it up.
I don’t understand that. Except for the contracts approved-for-all permit problem pointed by duncan, this implementation here works as expected with owner or approvedForAll without needing any other nonce. The nonce is per-token, not per-signer. And it shouldn’t be, else one signer couldn’t sign permits on different tokens at the same time.
That’s why Permits would have totally make this “hack” impossible, since all those Orders would have been revoked after any transfer, since the nonce increases every time the NFT is transferred.
You can sign as much Permits as you want and forget them. You can even sign the same Permits (nonce) to several actors at once, and be sure that after any of them transfers the nft, any other signed permit for this token at this nonce, will be revoked.
For one thing, you’ve created a huge race condition where an MEV opportunity exists to capitalize on executing a trade. But disregarding that, these permits don’t have any concept or representation of an “order”, and no way to tie one in. You are approving an address to transfer for a tokenId on your behalf. I believe this ERC to be well-formed for that use case.
It is most ideal to permit approve some sort of exchange contract, as the “terms and conditions” of that contract can be known ahead of time. That contract could additionally define an order book structure, allow lockup/deposit calls, perform fractionalization, you name it. That design space is super complicated, but having this ERC means I can permit approve an operator to deposit on my behalf to some sort of exchange.
If the use case is thinking that two signed messages will be executed together, e.g. 1 permit message and 1 order, well there is an assumption they “have” to be processed together, which is a false assumption. Unless they are committed to the blockchain (e.g. The permit is “used up”) it is still valid, and can be replayed in any venue that will accept it. You could let it linger for longer, and have permits for multiple such contracts at the same time, but the longer it’s valid for, the more likely it is that you’ll forget about signing the message, which can come around and bite you at a later point in time a la BAYC “thefts”.
Either way, I don’t think this is a big issue, your use case is still totally possible with an alternative design leveraging this ERC. I think we are perhaps talking past each other on this point.
The nonce value for per-token approvals is okay to be per-token. The nonce value for approvedForAll would have to be per-account because the concept of tokenId doesn’t apply. That was my point.
Again, this ERC doesn’t describe “orders” at all, and are best left out of scope for security purposes. NFT order book exchanges seem like a very complicated topic to try and standardize here. I think this ERC makes sufficient space available for different types of exchange contracts to be built without being prescriptive about how they do it.
Oh I think there has been a misunderstanding here.
I’m talking about the fact that for me, an operator that is already approvedForAll for an account, should be able to sign simple approve permits, on any token of this account (i.e My token is in my vault, I am an approvedForAll operator on all tokens of my vault, I should be able to sign an approve permit on those tokens)
I’m not talking about permits that gives approvalForAll, only approve permits.
About the rest: I’m mentioning use cases for permits, I’m not trying to standardize those things in here.
Just mentioning “this is a use case where this would work and bring a good solution to a problem that today makes people lose hundreds of thousands of dollars”.
We’re in discussions, I did not mention order books in the EIP itself, because the eip is about permits. But the different use cases of permits that people can see will decide what we add in the eip.
Several people asked about - and already implemented for some - Permits that gives approvalForAll.
I’m of the idea that it’s not necessary because using a permit sets approve and deletes it in the same transaction, which in term of gas brings a refund. The simplicity of use, the low gas cost and the fact it brings security is for me enough to not have approvalForAll which can be considered dangerous in some way.
1 Like
I also came across that, but I don’t think the standard here should be bloated even more for this. And there is a workaround for it. An approved for all contract could simply approve himself first using ERC721.approve. And in the signature verification one would do
require(
isValidEOASignature
|| _isValidContractERC1271Signature(ownerOf(tokenId), hash, signature)
|| _isValidContractERC1271Signature(getApproved(tokenId), hash, signature),
"ERC721Permit: invalid signature"
);
@dievardump I know this is an early draft and I saw you discussed changing the signature passing as bytes for example. I just published Advancing the NFT standard: ERC721-Permit which includes a little library, so if anything changes in the standard I’d be happy to get notified to change it. Thanks.
1 Like
_isValidContractERC1271Signature(getApproved(tokenId), hash, signature),
This seems quite dangerous because it breaks existing intuitions about how ERC721 approvals work. An approved (but not approved-for-all) address can spend the token, but not create additional approvals. With this scheme, if the approved address is a contract, then it breaks this intuition. Of course, the net permission granted is the same because the contract could just spend the token to itself and then do whatever it wants, but subtle bugs are introduced when peoples’ intuitions are violated. I can’t presently anticipate what that might be, but I advise conservatism and adherence to existing patterns.
I also dislike this workaround because it is cumbersome. Rather than giving blanket permission for the approved-for-all contract to take implicit actions through 1271 signatures, which encourages contracts to be generic and composable, this requires the approved-for-all contract to make a non-static call to the token contract to get the approval before it can take a 1271-enabled action.
I reiterate that I believe the correct signature for this standard ought to be:
function permit(
address ownerOrApprovedForAll,
address spender,
uint256 tokenId,
uint256 deadline,
bytes memory signature
) external;
with the option of whether to support ERC1271 left up to the implementer. This does not mandate ERC1271 support, but leaves open the option for those implementations that wish to support it. If an extra word of calldata is considered “bloat”, then I suppose I am advocating for a bloated standard 
I am mostly neutral on the tokenId-nonce vs. owner-nonce distinction, but slightly prefer the owner-nonce concept because it doesn’t penalize non-4494 users’ gas for the existence of the 4494 feature in the token contract.
For EIP-4973, if we introduced a function mintWithPermission(/*msg.sender*/uint256 tokenId, uint256 deadline, bytes memory signature) (where address receiver would have to be the msg.sender) this would allow for a very interesting mechanic where:
- account-bound tokens could at first be minted purely offline by the issuer sending a permission signature and the metadata to an ABT receiver.
- and then the ABT receiver (in case they need the ABT on-chain), to “lazy-mint” the token with permission of the issuer to their account using EIP-4494.
It’d solve one problem elegantly which Twitter and others in the space like https://erc1238.notion.site/ (see section on “Consent”, actually they implement a variant of EIP-712) have talked about:
- By default NOT putting any credential immediately on-chain. But only those that the user deems necessary to put on-chain.
- Minting only towards an account if a mutual agreement between two accounts can be representatively sent to the Ethereum signature validation mechanism aka PoW consensus.
EIP-4494: Permit for ERC-721 NFTs reference implementation points to external link and seems to be licensed differently than specification: erc721-with-permits/package.json at a2afe09fe93e4bf41cb4e99e910796ea40c257af · dievardump/erc721-with-permits · GitHub
Why not include reference implementation in assets directory?
Proposing to add EIP-1271 as required to document header: Include EIP-1271 as required by TimDaub · Pull Request #5178 · ethereum/EIPs · GitHub
I believe there is an error in the specification for the EIP-712 typed signature.
I think "value": value, here should be "tokenId"; tokenId, instead?
I had mentioned this in ERC-6956, but since your proposal’s status I had to remove it in order to proceed to Review. Please ping me in case this proposal moves forward.