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

ERC-1271 : Standard Signature Validation Method for Contracts #1271

Closed
Tracked by #5111
PhABC opened this issue Jul 28, 2018 · 164 comments
Closed
Tracked by #5111

ERC-1271 : Standard Signature Validation Method for Contracts #1271

PhABC opened this issue Jul 28, 2018 · 164 comments

Comments

@PhABC
Copy link
Contributor

PhABC commented Jul 28, 2018

Simple Description

Many blockchain based applications allow users to sign off-chain messages instead of directly requesting users to do an on-chain transaction. This is the case for decentralized exchanges with off-chain orderbooks like 0x and etherdelta. These applications usually assume that the message will be signed by the same address that owns the assets. However, one can hold assets directly in their regular account (controlled by a private key) or in a smart contract that acts as a wallet (e.g. a multisig contract). The current design of many smart contracts prevent contract based accounts from interacting with them, since contracts do not possess private keys and therefore can not directly sign messages. The proposal here outlines a standard way for contracts to verify if a provided signature is valid when the account is a contract.

See EIP draft.

@PhABC PhABC changed the title Standard Signature Validation Method for Contracts ERC-1271 : Standard Signature Validation Method for Contracts Jul 28, 2018
@shrugs
Copy link

shrugs commented Jul 28, 2018

Realizing that this is more than just validating signatures; we're actually asking the question "given this action, does the caller have the ability to it, given this proof"?

so perhaps the better metaphor is

isValidAction(bytes _action, bytes _proof) {}

where the default behavior might be to treat _action as a Bouncer data hash and _proof as a signature. But like we noticed before, they could be anything.

Although this implies that the proof handling and the access control are part of the same function/contract where they could potentially be different, although I'm not sure how that separation would generalize at all (i.e. in the case of ECDSA signatures, the validity is just the recovery, which produces an address and the access control is a "does this address have this permission" check against the _action data).

Thoughts, @PhABC?

@PhABC
Copy link
Contributor Author

PhABC commented Jul 28, 2018

That's a good point! A signature is usually used to guarantee that a given address allows something, but other schemes not relying on signature methods could also achieve the same goal.

Pinging @alexvandesande & @abandeali1 for comments.

@abandeali1
Copy link

I personally prefer using a 32 byte hash over an _action byte array. Any action should be able to be represented as a hash, and this really simplifies the implementation details. You're probably going to end up hashing _action and verifying a signature at some point anyways. Any extra logic could live in the caller contract.

I could see it making sense to use the word proof instead of signature, though.

@PhABC
Copy link
Contributor Author

PhABC commented Aug 2, 2018

@abandeali1

The reason why we opted for a dynamic byte array is because the called contract might request specific data that the caller contract is not aware of.

For instance, imagine you use a smart account that has some key management properties, where different keys have different roles (RBAC style). Now imagine that the private key on your phone can sell some game assets, but can't any other types of tokens. Your ledger private key can sell any type of asset. If the caller contract (e.g. 0x contract) only passes the hash of the message, the smart account has no way of knowing what the asset being traded is. The smart account would see two hashes that were indeed signed by two private key you own, but it would have no way of verifying if the action signed is valid based on it's own internal conditions. In this case, your smart account would either need to allow all your private keys to exchange assets via 0x (let them sign hashes) or prevent them all.

In general, what i'm trying to say is that I think there could always be additional rules not encompassed by the caller contract that the called contract requires.

@shrugs
Copy link

shrugs commented Aug 2, 2018

Likewise, my worry is that a hash loses information that may be necessary for validation.

That said, I'm also worried about the complexity of passing arbitrary data via a bytes call; we've just accidentally re-created the need for abi encoding/decoding and whoops, we should probably just use solidity for that, so now we're back where we started, creating context-specific methods.
It may make more sense to simply restrict this to hash + proof validation, and nothing more? that covers bouncer and identity contracts, the two main cases I'd like to use this technique for.

@abandeali1
Copy link

You can always encode any extra data about the hash in the signature field. For example:

function isValidSignature(bytes32 hash, bytes signature) external {
    uint8 signatureType = uint8(signature[0]);
    bytes32 msgHash = keccak256(hash, signatureType);
    if (signatureType == 0) {
        // Do something specific to signatureType
        return _isValidSignature(msgHash, signature);
    } else if ...
}

You can imagine adding any amount of extra data to signature in a similar way. This keeps the simplest implementation simple, but it's still pretty flexible if necessary.

@shrugs
Copy link

shrugs commented Aug 2, 2018

@abandeali1 I'm not convinced that overloading the signature ("proof") argument is a ton better than bytes action, bytes proof.

@shrugs
Copy link

shrugs commented Aug 4, 2018

@PhABC any feedback on restricting this to hash+signature based validation?

@abandeali1
Copy link

@shrugs my point is that it can be done with just a hash+signature, but I wouldn't expect that to be frequently used.

@frangio
Copy link
Contributor

frangio commented Aug 5, 2018

Thanks for writing this up @PhABC, it's quite good.

The discussion around @abandeali1's suggestion is very interesting, but we need to show some examples of usage in order to best decide on fixed or arbitrary size _data (i.e. the message that is signed).

I think a good example to analyze is that of an m-of-n multisig smart account. A valid signature for this account could be the concatenation of m (signature, signer) pairs, where the m signers are in the set of n signers, and each signature is valid for the message and respective signer. I think this is enough motivation for the signature to be of arbitrary size.

Now suppose that a family of messages can only be considered signed when the full set of n signers approve. It's common to increase the required number of signatures for large value transfers, so I think it would make sense for the signature scheme to have something analogous to that. The validity of the signature then depends on some parameter in the message being signed. Per @abandeali1's suggestion, the parameter should be included as part of the signature, but then the contract would have to additionally verify that the parameter in the signature is indeed the one contained in the message. If the message is hashed, then the entire message has to also be included in the signature in order to verify the hash. That would be a lot of redundant bytes being passed around. IMO this justifies having the message be of arbitrary size, and not necessarily a hash.

Do you all agree the scenario described makes sense?


A mostly unrelated question about the EIP. Would it be valid for an implementation of isValidSignature to change its behavior depending on the caller? I have the feeling that this should be forbidden by the spec ("MUST NOT"), but unfortunately I don't think there's a way to enforce it in the API.

@frangio
Copy link
Contributor

frangio commented Aug 5, 2018

I forgot to comment on the suggestion to use the names "action" and "proof". Although I kind of like "proof", I would prefer to keep the current names as they're consistent with those of cryptographic signing, and agnostic to any particular use case.

@abandeali1
Copy link

@frangio overloading the signature field should actually result in fewer bytes being passed around and should be cheaper, assuming that isValidSignature is being called by another contract (i.e the data is being copied to memory and is not included in the transaction calldata).

With bytes data, bytes signature, the ABI encoded bytes will look like: [dataOffset][signatureOffset][dataLength][data][signatureLength][signature], where everything is 32 bytes except for data and signature. This gives us a total of 128 + data.length + signature.length bytes.

With bytes32 hash, bytes signature, the ABI encoded bytes will look like: [hash][signatureOffset][signatureLength][signature]. This gives us a total of 96 + data.length + signature.length bytes (assuming that the signature field includes everything that would otherwise be in the data field).

So in the worst case scenario of using bytes32 hash, bytes signature, we save 32 bytes but add some extra hashing and complexity.

Now let's look that the case where we we use bytes data, bytes signature but data only contains a 32 bytes hash. The ABI encoded bytes are [dataOffset][signatureOffset][dataLength][hash][signatureLength][signature]. This means we add 64 bytes vs using bytes32 hash, bytes signature. There's an extra hidden cost here though - if calling isValidSignature from another contract, we now have to convert our 32 byte hash into a byte array before the call and then parse the hash out of byte array after the call. This isn't particularly easy without using some inline assembly (currently), and I'm guessing that the function will be used this way the majority of the time.

@abandeali1
Copy link

To be honest, the cost differences here are probably minimal. I think the main downside for the bytes data, bytes signature approach is converting a bytes32 to and from a byte array. With the bytes32 hash, bytes signature approach, the downside is confusion over potentially overloading signature. Which is better likely comes down to how often we think each case is likely to be used.

@shrugs
Copy link

shrugs commented Aug 5, 2018

The complexity of converting a bytes32 to bytes is not something I'd like people to have to deal with, great point, @abandeali1. And indeed, verifying the signature of a hash seems to be the primary use-case, @frangio.

I'm happy with, in order of preference:

  1. scoping this issue to just be hash + signature validation and encouraging people to overload signature for extra data
  2. scoping this issue to just be hash + signature validation and not encouraging people to overload signature, and instead encouraging a different signature (ideally without overloading the function name) for application-specific action validation
  3. using bytes, bytes as the arguments and providing a minimal assembly bytes32->bytes conversion library within OpenZeppelin to make it as easy as possible.

For naming, I'd prefer isValidAction, but am definitely fine with isValidSignature.

how about some fun voting, multiple choice ok:

  • 😀for 1
  • 🎉 for 2
  • ❤️ for 3
  • 👍 for isValidSignature
  • 👎 for isValidAction

(hmm, github should have twitter-style polls that only followers of an issue at time-of-poll can vote on)

@shrugs
Copy link

shrugs commented Aug 6, 2018

It seems like we'll need to include more information than just action/signature. I'm having trouble visualizing how to abstract signature validation like this so that contracts can recursively "sign". I think we need to add the "delegate" concept here so we can do "recursive" validity. i.e. I'm trying to sign something with my contract, but I want to proxy that validity to another contract.

I ended up writing something like this; does it make sense?

  // added this function to ECRecovery, so it's used as _hash.isSignedBy(signer, sig)
  function isSignedBy(bytes32 _hash, address _signer, bytes _sig)
    internal
    view
    returns (bool)
  {
    // if the signer address supports SignatureDelegation, delegate to it
    if (_signer.supportsInterface(InterfaceId_SignatureDelegate)) {
      return ISignatureDelegate(_signer).isValidAction(_signer, _hash, _sig);
    }

    // otherwise make sure the hash was personally signed by the EOA account
    return _signer == recover(toEthSignedMessageHash(_hash), _sig);
  }

  /**
   * @dev An action is valid iff the _sig of the _action is from an key with the ACTION role
   */
  function isValidAction(address _signer, bytes32 _action, bytes _sig)
    view
    public
    returns (bool)
  {
    // permission
    bool hasPermission = _signer == address(this) || hasRole(_signer, ROLE_ACTION);

    // validity
    bool isValid = _action.isSignedBy(_signer, _sig);

    return hasPermission && isValid;
  }

I haven't written any tests for this, but this allows, for example, a multisig to own an identity that owns another identity that can sign 0x orders and operate with off-chain tooling that expects signatures.

@frangio
Copy link
Contributor

frangio commented Aug 6, 2018

we'll need to include more information

So, the information that you added was the account for which you need to verify validity, right?

That's an interesting thing to point out. With ecrecover the signature and the signed message get you back the account. But there's no way to do that when the signer is a contract, so you need to know what account you will validate the signature against. IMO this doesn't affect this EIP at all, but it would be valuable to add a note about it in the document, because it will affect the design of APIs which do signature validation.

@PhABC
Copy link
Contributor Author

PhABC commented Aug 6, 2018

Regarding (action, proof) vs (data, signature)

@shrugs I do like both term pairs ; "proof" is probably more future proof, while "signature" will be more easily understood for a while given the current practices in the space. Action might not be general enough however, where it's not clear how the term "action" fits with the hash of an 0x order for example. Regardless, we can always change the terms in the future since argument naming changes should be backward compatible.


Regarding bytes vs bytes32

Both allow for passing extra data to the receiving contract, so it's really a question of efficiency. It does seem like both have tradeoffs ; bytes32 will be cheaper when people sign hashes, while bytes will be cheaper when receiver contracts need additional data. I am fairly convinced that in the next few years identity contracts will allow for fine grain action based permissions for different private keys (e.g. address A can sell any assets, while address B can only sell cryptokitties). However, identity contracts are still quite primitive and I agree that most data being signed will be hashes for a while.

In terms of converting bytes32 to bytes, you can use abi.encodePacked(), such as ;

contract C { 
  function toBytes() public pure returns (bytes) {
    bytes32 a;
    return abi.encodePacked(a);
  }
}

which costs about 210 gas. Not sure about bytes => bytes32 however.

Edit : Christian from the Solidity team told me the abi.decode() will be able to do bytes => bytes32 if the bytes array is only composed of a bytes32, which is true in the case of passing a hash. Hence, it seems to me like we should assume converting bytes <=> bytes32 should be relatively straightforward in the near future.


Regarding passing signer as argument

That's a good point @shrugs. As a side note, I think ISignatureDelegate(_signer).isValidAction(_signer, _hash, _sig); will not allow an EOA account to have use a signature validator contract, since _signer.supportsInterface(InterfaceId_SignatureDelegate) would always return false when signer is an EOA. Imo, if your EOA holds your fund and you want to use a specific signature scheme, something like what 0x v2 is doing might be more appropriate ;

isValid = IValidator(validatorAddress).isValidSignature(
                hash,
                signerAddress,
                signature
);

Code from here.

The above also works for contracts being signers. Also, why couldn't signer be also part of the signature() argument instead of an additional argument? @abandeali1 you guys probably thought about having signerAddress as part of signature vs as a separate argument.

@shrugs
Copy link

shrugs commented Aug 6, 2018

Although I'm realizing there still needs to be a way to recover the contract signer for a signature. Maybe it could be the highly compacted chain of contract addresses like [address] [address] [v] [r] [s]? Then you can remove the prefix and send it along to the next one while checking permissions.

@shrugs
Copy link

shrugs commented Aug 6, 2018

@phabcd if the account different doesn't support the interface, it's expected to be an EOA account that signed on behalf of itself so imo that still makes sense. Is that right, you think?

@PhABC
Copy link
Contributor Author

PhABC commented Aug 6, 2018

@phabcd if the account different support the interface, it's expected to be an EOA account that signed on behalf of itself so imo that still makes sense. Is that right, you think?

With respect to what? I'm not sure I get what this is specifically referring to.

@shrugs
Copy link

shrugs commented Aug 7, 2018

sorry, was on mobile;

basically, the logic to evaluate isValidSignature is

  1. does the _signerAddress have the permission in my security model?
  2. is that signature valid? in which case we either
    1. if supports interface, delegate to the _signerAddress.isValidSignature(hash, signature)
    2. otherwise, expect that this is an EOA account so check that _signerAddress = hash.toEthSignedMessage.recover(signature)

@PhABC
Copy link
Contributor Author

PhABC commented Aug 7, 2018

Ah, I understand. So the idea behind having an EOA account using a signature validator contract is for the scenario where the user wants to hold its funds in EOA but does not want to use ECDSA signature scheme, hence need to use a validator contract. It's a pretty niche case, but you could imagine things like BLS signatures or other aggregation signature methods to more efficiently verify many signatures in batch or even quantum secure signature schemes.

@shrugs
Copy link

shrugs commented Aug 7, 2018

I think we're misunderstood, again; I hadn't considered that case, actually.

Removing any preconditions, here is some proposed code.

// interface
function isValidSignature(bytes _action, bytes _sig)
  function isSignedBy(bytes _action, address _signer, bytes _sig)
    internal
    view
    returns (bool)
  {

    // if the signer address supports signature validation, ask for its permissions/validity
    // which means _sig can be anything
    if (_signer.supportsInterface(InterfaceId_SignatureDelegate)) {
      return ISignatureValidator(_signer).isValidSignature(_action, _sig);
    }

    // otherwise make sure the hash was personally signed by the EOA account
    // which means _sig should be highly compacted vrs
    bytes32 signedHash = toEthSignedMessageHash(BytesConverter.toBytes32(_action));
    return _signer == recover(signedHash, _sig);
  }
// "isValidSignature" implementation for identity contracts

  /**
   * @dev An action is valid iff the _sig of the _action is from an key with the ACTION purpose
   * @param _action
   * @param _sig [[address] [address] [...]] <address> <v> <r> <s>
   */
  function isValidSignature(bytes _action, bytes _sig)
    view
    public
    returns (bool)
  {
    // the fact that this method has been called means the caller knows/expects that
    //   this contract has permission to do a thing.

    (nextSigner, sig) = splitNextSignerAndSig(_sig);
    // permission
    bytes32 keyId = KeyUtils.idForAddress(nextSigner);
    bool hasPermission = keyHasPurpose(keyId, PURPOSE_ACTION);

    // validity
    bool isValid = _action.isSignedBy(nextSigner, _sig);

    return hasPermission && isValid;
  }
// now the 0x contracts can do something like

// https://github.com/0xProject/0x-monorepo/blob/development/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol#L188

BytesConverter.toBytes(hash).isSignedBy(signerAddress, signature);

// ...

and everything should cascade downwards.

So identity contracts can own identity contracts (i.e., multisigs can own multisigs) and we can encoded arbitrary validation logic as well (your identity contract would support BLS verification).

The recursive nature of validation is what I think we were missing. without it, we can only do single-level signature validation.

@frangio
Copy link
Contributor

frangio commented Aug 15, 2018

So identity contracts can own identity contracts (i.e., multisigs can own multisigs) and we can encoded arbitrary validation logic as well (your identity contract would support BLS verification).

Is there anything in the current EIP text that you think doesn't enable this?


I was going to suggest that isSignedBy should simply check if the signer is a contract, and not necessarily use supportsInterface, but I suppose this is a problem because of selector collisions, right? I mean, that a contract may reply to isValidSignature even if it doesn't really implement this method. This could be a real problem.

@shrugs
Copy link

shrugs commented Aug 15, 2018

presumably we're acting on the contract's behalf, so if it lied and said something was authorized when it wasn't, that'd be its own issue... but I'd have to take some time to think of attack scenarios.

@shrugs
Copy link

shrugs commented Aug 15, 2018

@frangio if a contract doesn't support a method, it might just return garbage data; that's why 165 does two calls to make doubly sure that it actually supports the supportInterface(bytes4) interface.

But the current interface definition in the spec is fine, since it claims bytes on both arguments, yeah. Once we get abi.decode, though, we'll be able to do this parsing of the array much easier. It was just merged in solidity's repo, but idk if it's worth waiting for it to land in order to change this spec to use it?

@alcueca
Copy link
Contributor

alcueca commented Jun 20, 2021

The previous attempt to get this EIP out of draft was not successful. Could someone summarize the status of this EIP, and what are the blockers? @PhABC?

@Amxx
Copy link
Contributor

Amxx commented Jun 25, 2021

I feel like a registry would be a significantly better solution to this problem as it would be backward compatible with all existing wallets (and all future wallets) that can do arbitrary execution but cannot modify their own code.

While it would import backward compatibility, the registry adds extra complexity and risks. While I understand these risks can be mitigated by deploying a single trusted registry per blockchain, which the same address on all networks, it would still increase the complexity of apps wanting to check signatures. An analogy would be 165 vs 1820. The only usecase I know where 1820 is used, is for 777 recipient ... and this is a major pain is the ***.

I'm also worried about the storage cost of the registry, and what would happen if a "renting model" is applied to storage.

@pedrouid
Copy link
Contributor

Honestly I'm still surprised that this ERC is taking so long to finalize... In my opinion it had a temporary shift in direction when it required the data as a parameter yet the complexity was dramatically higher with little benefit and the standard was reverted to use the hash instead again.

I think there is value to creating a new ERC where you can call a contract to hash and validate data to be signed but it definitely should not block this ERC from finalizing

ERC-1271 in my opinion should be minimal and focused on doing exactly what ecrecover would do off-chain but on-chain instead

@PhABC
Copy link
Contributor Author

PhABC commented Jun 25, 2021

@albertocuestacanada @pedrouid Updated the EIP based on @MicahZoltu's feeback from last year when attempted to push to final.

New attempt at Last Call ; #3628

@blmalone
Copy link
Contributor

blmalone commented Aug 10, 2021

Really like this EIP.

Just a small question, shouldn't this be an interface that makes use of ERC165?

pragma solidity ^0.5.0;

contract ERC1271 {

  // bytes4(keccak256("isValidSignature(bytes32,bytes)")
  bytes4 constant internal MAGICVALUE = 0x1626ba7e;

  /**
   * @dev Should return whether the signature provided is valid for the provided hash
   * @param _hash      Hash of the data to be signed
   * @param _signature Signature byte array associated with _hash
   *
   * MUST return the bytes4 magic value 0x1626ba7e when function passes.
   * MUST NOT modify state (using STATICCALL for solc < 0.5, view modifier for solc > 0.5)
   * MUST allow external calls
   */ 
  function isValidSignature(
    bytes32 _hash, 
    bytes memory _signature)
    public
    view 
    returns (bytes4 magicValue);
}

so...

interface IERC1271 is IERC165 {

  // bytes4(keccak256("isValidSignature(bytes32,bytes)")
  bytes4 private constant _INTERFACE_ID_ERC1271 = 0x1626ba7e;

  /**
   * @dev Should return whether the signature provided is valid for the provided hash
   * @param _hash      Hash of the data to be signed
   * @param _signature Signature byte array associated with _hash
   *
   * MUST return the bytes4 magic value 0x1626ba7e when function passes.
   * MUST NOT modify state (using STATICCALL for solc < 0.5, view modifier for solc > 0.5)
   * MUST allow external calls
   */ 
  function isValidSignature(
    bytes32 _hash, 
    bytes memory _signature)
    public
    view 
    returns (bytes4 magicValue);
    
    function supportsInterface(bytes4 interfaceID) external view returns (bool);
}

Could be completely misunderstanding this, but wouldn't you want to have ERC165 functionality in here? e.g.

(bool success) = IERC165(_contract).supportsInterface(_INTERFACE_ID_ERC1271);

@pedrouid
Copy link
Contributor

@MicahZoltu @PhABC @lightclient how is this still on Last Call since 25 Jun 2021??? Shouldn't this be Final already?

@PhABC
Copy link
Contributor Author

PhABC commented Jan 25, 2022

@pedrouid

Looks like we are synced :), made a PR yesterday to change it to Final ; #4717

@lightclient
Copy link
Member

@pedrouid unless an author submits a PR, we don't usually do it ourselves :)

@pedrouid
Copy link
Contributor

@pedrouid unless an author submits a PR, we don't usually do it ourselves :)

Yeah I didn't expect Editors to do it... I actually thought Final Calls were enforced by CI to merge after 2 weeks

@lightclient
Copy link
Member

That would be a really neat feature :)

@MicahZoltu
Copy link
Contributor

Closing this for housekeeping purposes. Feel free to continue using this issue for discussion about EIP-1271.

@lukasz-glen
Copy link

Nice work with this EIP.
I was learning about this and found some examples of calling contracts. This and this and another one I lost a link to. They apparently assume that signature must have the structure (v,r,s) or at least be 65 bits long. Is this valid? I would say not, the spec does not state that, it is up to an implementing contract to validate signature. Moreover, such assumption disallows some future signing schemes. For me this statement would work, for clarity.

   * A calling contract SHOULD NOT make any assumptions on the signature, 
   * in particular it can be other than ECDSA or BLS, it can be even zero length.

In my opinion, this is a proper implementation.

abstract contract NoSignatureWallet {
   mapping (bytes32 => bool) public approvedDigest;
   /// @dev an inheriting contract decides which digests are valid
   function _approveDigest(bytes32 digest, bool approval) internal {
      approvedDigest[digest] = approval;
   }
   function isValidSignature(bytes32 digest, bytes memory /*_signature*/) external view returns (bytes4) {
      return approvedDigest[digest] ? 0x1626ba7e : 0xffffffff;
   }
}

To approve a digest someone has to provide a valid signature anyway, so this is not any less secure. Another exemplary implementation could take signature as an array of signatures actually. Am I wrong?

@PhABC
Copy link
Contributor Author

PhABC commented Dec 19, 2022

@lukasz-glen excellent question and you are 100% correct. The signature can be anything, the EIP does not force any particular format or structure for the signature as it's signer contract specific.

@radeksvarz
Copy link
Contributor

Is there any reference implementation for the ERC20 based tokens with permit function?
I assume there should be a separate function / ERC besides ERC2612's permit as that adjustement is challenged as discussed here: https://forum.openzeppelin.com/t/implementation-erc20-permit-eip-2612-compatible-with-eip-1271/8830
Is there any 1271 corresponding ERC to complement 2612 / 3009?

@lukasz-glen
Copy link

Is there any reference implementation for the ERC20 based tokens with permit function?

What's wrong with this one?

Is there any 1271 corresponding ERC to complement 2612 / 3009?

What for actually? I was thinking about the same some time ago. I concluded that smart contract wallets (for instance) do not need permit, they should use regular approve. Even if gasless, it should be in ERC4337 style.

@radeksvarz
Copy link
Contributor

Is there any reference implementation for the ERC20 based tokens with permit function?

What's wrong with this one?

OZ permit does not have ERC1271 validation. Apologies, my question should contain that 1271 context.

Is there any 1271 corresponding ERC to complement 2612 / 3009?

What for actually? I was thinking about the same some time ago. I concluded that smart contract wallets (for instance) do not need permit, they should use regular approve. Even if gasless, it should be in ERC4337 style.

I am also in doubt, but would like to read opinions on this matter.

In fact the question is whether the new ERC20 implementations should include ERC1271?

@frangio
Copy link
Contributor

frangio commented Jul 28, 2023

ERC-2612 (permit) can't be made fully ERC-1271 compatible because it receives an r,s,v representation of a signature, whereas ERC-1271 uses arbitrary length signatures. Additionally ERC-2612 explicitly says that ecrecover should be used to recover the address, which would rule out ERC-1271 in general, but arguably this is a technicality.

@lukasz-glen
Copy link

I would change the perspective. What do we lose if 2612 and 3009 use r,s,v, not bytes signature and 1271?

3009 can be used to overcome approve/transferFrom problem. I assume that smartcontract wallets can execute approve and transferFrom within a single transaction (without 3009), still providing comparable gas savings.

2612 can be used in combination with 2771 to provide gasless tx. 3009 can be used to provide gasless tx also if I understand it correctly. But smartcontract wallets can provide gasless tx in a different way.

To be clear. I think that it would be better if 2612 and 3009 would have bytes signature and 1271 integration. But my point is that it would not be such gain as it seems.

@xinbenlv
Copy link
Contributor

xinbenlv commented Oct 6, 2023

Hi @frangio and authors, EIP-1271 is a great piece of standard. I have a clarification question that I like to ask:

What's the rationale in choosing a bytes4 magicValue as opposed to

  1. use a different dataType such as bool or bytes32. If not using bool to prevent collision attack where attacker compose a function that collide with function selector and return value, wouldn't bytes4 be also too small?
  2. return zero instead of a 0xffffffff when not match?

Thank you!

@PhABC
Copy link
Contributor Author

PhABC commented Oct 6, 2023

@xinbenlv

explicit bytes was chosen instead of a Boolean to decrease the risks of errors when validating, akin to type safety. It’s a common practice in case there were more than 2 types of messages, but also to better catch mock implementations. I believe it also makes it easier to distinguish and error from an invalid signature, but I forgot the nuances. Bytes32 is too big/ not worth it since we aren’t concerned with collisions here.

@alextnetto
Copy link

alextnetto commented Feb 16, 2024

@xinbenlv

explicit bytes was chosen instead of a Boolean to decrease the risks of errors when validating, akin to type safety. It’s a common practice in case there were more than 2 types of messages, but also to better catch mock implementations. I believe it also makes it easier to distinguish and error from an invalid signature, but I forgot the nuances. Bytes32 is too big/ not worth it since we aren’t concerned with collisions here.

Hey @PhABC, it is still not entirely clear for me what the difference between returning true or false and 0x1626ba7e or 0xffffffff (any other number would also mean false).

It's also more expensive to validate and deal with a bytes4 than a bool.

I appreciate the work put in here; remarkable EIP.

@Amxx
Copy link
Contributor

Amxx commented Feb 16, 2024

Hey @PhABC, it is still not entirely clear for me what the difference between returning true or false and 0x1626ba7e or 0xffffffff (any other number would also mean false).

A contract that doesnot implemente ERC-1271 may have a fallback function, and that fallback function may return data.
From an caller's point of view, there is a risk the data return by the fallback is interpreted as a true boolean. This would cause the the isValidSignature lookup to appear as it succedded, even though it did not.

The whole poinr of 0x1626ba7e is that its very specific, and its unlikelly that another function (a fallback) would return that.

@alextnetto
Copy link

Thanks a lot for the explanation @Amxx , make sense!

@novaknole
Copy link

novaknole commented Feb 27, 2024

I actually liked the use case presented in the very first message of this thread about decentralized exchanges.

If decentralized exchange implements EIP1271 which is basically for the reason that smart contracts can also hold funds, then DEX smart contract after "isValidSignature" succeeds, also needs to call: token.transferFrom(maker, taker, value). Note that maker is smart contract now. How can that transfer succeed ? The only choice I can think of is asset contract(i.e maker) must be able to first approve the tokens to be spendable by exchange smart contract. This can only be done if maker smart contract includes the code of ".call" that can call any function externally(including "approve") on token.

I can't think of any other use case - if the maker contract doesn't have the possibility of "calling" external functions on other smart contracts beforehand, then use case the first message in this thread talks about is doomed. Am I wrong or I'm not seeing an better use case or I am right and that's it ?

@PhABC
Copy link
Contributor Author

PhABC commented Feb 27, 2024

@novaknole Every single implementation of smart contract wallet (or account abstraction) uses ERC-1271(Gnosis safe, Sequence, Ambire, Pimlico, Immutable Passport, etc.). AA wallets don't have EOA signers like regular wallets, hence can only sign messages via ERC-1271 (and 6492)

The example outlined in ERC-1271 with exchanges was written well before AA became popular. If it were re-written today it would be centered around AA wallets.

@radeksvarz
Copy link
Contributor

Is there any reference implementation for the ERC20 based tokens with permit function?

What's wrong with this one?

OZ permit does not have ERC1271 validation. Apologies, my question should contain that 1271 context.

Is there any 1271 corresponding ERC to complement 2612 / 3009?

What for actually? I was thinking about the same some time ago. I concluded that smart contract wallets (for instance) do not need permit, they should use regular approve. Even if gasless, it should be in ERC4337 style.

I am also in doubt, but would like to read opinions on this matter.

In fact the question is whether the new ERC20 implementations should include ERC1271?

No answer here, yet USDC updated its contracts to reflect ERC1271.

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

No branches or pull requests