ethereum / EIPs

The Ethereum Improvement Proposal repository
https://eips.ethereum.org/
Creative Commons Zero v1.0 Universal
12.83k stars 5.25k forks source link

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

Closed PhABC closed 2 years ago

PhABC commented 6 years ago

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 commented 5 years ago

@abandeali1 I've been pondering this question quite a bit and can't really make up my mind.

What use cases do you have to allow state modification? My fear is now that implementing isValidSignature() will require re-entrancy protection for all contracts calling it, among other things.

Allowing for state change is a lot more consistent with signatures allowing actions however, where the signer contract could update its condition based on what was signed.

It would be harder for signer contract to control actions (e.g. daily transfer limit per signer or async multi-sig), without this. I think the additional flexibility is worth allowing for state changes, so long as contract implementers are aware of the risk when calling isValidSignature() on another contract.

abandeali1 commented 5 years ago

I can think of a few use cases for allowing state changes. As you mentioned, transaction limits are a good example.

It's also not obvious if replay protection should be implemented in the signature verifier or the calling contract (the current assumption is that the calling contract will do this, but I could see verifiers wanting some extra level of protection).

Generally, having a signature that is valid only if some action is successfully taken also seems pretty valuable. In the context of 0x, it would be cool if you could create signatures that are only valid if you could atomically hedge/arbitrage your position via some other contract.

PhABC commented 5 years ago

I do start imagining a lot of scenarios where allowing state changes could be interesting. Perhaps it's also a good habit that everyone assumes re-entrancy risk instead of believing it to be not possible.

@izqui What do you think of not enforcing STATICALL in the standard, such that the isValidSignature() is allowed to update state as well?

dete commented 5 years ago

Thanks @PhABC for pushing this forward!

You’ll have to forgive me for jumping in late. However, I do think there’s a way to find a single entry-point that handles everyone.

I’m pretty sure that in all cases, we can think of the interaction we're trying to mediate as happening between three entities:

And our goal is to allow the human to be able to provide some "packet" to the service with the following properties:

As was laid out in the original proposal, there are two important parts to this "packet":

The human needs to know the format of both the "request" (so they know what they are agreeing to!) and the "authorization" (since it's their job to produce it).

The service needs to know the format of the "request". (Indeed, in many cases the service will provide the request.) This is critical for avoiding replay attacks. Even in a simple case – like logging in to a Dapp like CryptoKitties – the CK website needs to know that this authorization is specifically for CryptoKitties, and not just a replay of some other authorization that the user gave for some other request.

The service doesn't need to know the format of the "authorization", and should always treat it as a black box. (If the service makes any assumptions about the contents of the authorization, it will limit the ability of wallets to implement new authorization schemes.)

The wallet needs to know the format of the "authorization". Indeed, that's the whole point! However, it doesn't need to know the contents of the "request": the user will have reviewed the request before signing. I would go so far as to say that if a wallet did require access to the contents of the "request", it would severely limit the ability of the wallet to work with new kinds of services. This also allows the smart contract API to use bytes32 requestHash as the first argument, saving gas for on-chain queries.

So, why use the term "authorization"? Because there are mechanisms other than signatures that can be part of a valid authorization, such as an expiry time. The "authorization" bucket isn't just for signatures, it's for any wallet-defined data that is necessary to implement its own security regime. This name makes it clear that the field is intended to be used flexibly for any security scheme defined by a wallet.

sohkai commented 5 years ago

What do you think of not enforcing STATICALL in the standard, such that the isValidSignature() is allowed to update state as well?

If we're allowing the user/dapp to decide which implementation to use, it also seems like this could be something they choose as well? If they know verification doesn't need to modify state, they could call through a STATIC_CALL, or alternatively another interface whose name explicitly expresses state-modifiability could be deemed optional (perhaps this is better with 0.5's enforcement of STATIC_CALL for ease of use).

Given that, I do think guaranteeing a way to verify these "signatures" through a safe, non-state modifying call is important for ease of use since it's very likely that the majority of users won't use the state modifying behaviours (for now).

PhABC commented 5 years ago

Thanks for the comments @dete ! I agree on all your points, except the following:

The wallet [...] doesn't need to know the contents of the "request": the user will have reviewed the request before signing. I would go so far as to say that if a wallet did require access to the contents of the "request", it would severely limit the ability of the wallet to work with new kinds of services. This also allows the smart contract API to use bytes32 requestHash as the first argument, saving gas for on-chain queries.

It's unclear to me how only passing the hash of what is being signed as sufficient for the contract verifying the authorization. If the contract has a daily transaction limit for some users or if the contract verifying the authorization has a whitelist of actions/contracts it allows, how does the contract become aware of the information if it's only provided a hash? Shouldn't the contract be aware of what the request is in order to assess if it wants to authorize it? Signature from a user might not be sufficient for an action to be allowed.

@sohkai

If we're allowing the user/dapp to decide which implementation to use, it also seems like this could be something they choose as well? If they know verification doesn't need to modify state, they could call through a STATIC_CALL, or alternatively another interface whose name explicitly expresses state-modifiability could be deemed optional (perhaps this is better with 0.5's enforcement of STATIC_CALL for ease of use).

If we support state change, I would argue that ERC-1271 compliant contracts should always be assumed to be able to modify the state, such that dapp and other contracts know that if they use STATICCALL some signer contracts will not be able to provide a valid signature. It should be the responsibility of the calling contract to make sure that the signer contract can't do bad things considering it's able to modify the state.

catageek commented 5 years ago

Hi,

The wallet [...] doesn't need to know the contents of the "request": the user will have reviewed the request before signing. I would go so far as to say that if a wallet did require access to the contents of the "request", it would severely limit the ability of the wallet to work with new kinds of services. This also allows the smart contract API to use bytes32 requestHash as the first argument, saving gas for on-chain queries.

It's unclear to me how only passing the hash of what is being signed as sufficient for the contract verifying the authorization. If the contract has a daily transaction limit for some users or if the contract verifying the authorization has a whitelist of actions/contracts it allows, how does the contract become aware of the information if it's only provided a hash? Shouldn't the contract be aware of what the request is in order to assess if it wants to authorize it? Signature from a user might not be sufficient for an action to be allowed.

I agree with @PhABC as passing only the signature hash is sufficient to manage the signature life cycle but not any potential additional action.

For the ability to change the state, a warning about the possibility of reentrancy attack and the necessity to put some protection against it would not change present state of art. Alternatively, developpers can set "view" keyword on per use case, with the risk to break signature verification that modify state.

Should we also define in this standard a maximum gas comsumption for the caller, and the rest of the gas is provided by the signature contract if more gas i needed ? This would allow an implementation of isValidSignature() to call the signature contract with a standard amount of gas to avoid gas storage or gas exhaustion. A best practice is to limit the gas sent in a transaction, putting this limit in the standard would set a clear definition of the amount to send.

izqui commented 5 years ago

Long time without checking this thread, great progress!

izqui commented 5 years ago

@recmo EVM has non-analyzable control-flow which makes accurate static analysis impossible.

You don't need real static analysis from an arbitrary entry-point, you can just check that a contract doesn't have some opcodes that you blacklist (SSTORE + all the call types, can ensure you that a contract is pure). Opcode checking is a bit expensive, but it could be done pretty efficiently if checked optimistically only once for each code hash with Constantinople's EXTCODEHASH.

catageek commented 5 years ago
* I think that we should have an arbitrary gas limit for this call though, otherwise a `isValidSignature()` infinite loop can be created with a chain of validation contracts that could be used to grief users: [aragon/aragon-apps@abf1c91](https://github.com/aragon/aragon-apps/commit/abf1c91fa8fcfa67e44cc8a10f3b6fc9f2544d89). We have a 250k gas limit in our code for this, which allows performing a decent amount of compute for verifying signatures.

My point is to define a minimum amount of gas to send in the standard. Signature contract implementer must use less than the standard amount or provide the additional gas by itself (using abstract account when it is implemented). I think that it is the signature contract that must support the extra cost for an expensive verification, not the caller. My gut feeling is that 250k is too much for a decent signature verification, but I may be wrong.

izqui commented 5 years ago

Here's Aragon's ERC1271 implementation with some of the changes I propose above. We will likely be releasing the first beta of Aragon Agent v1 with ERC1271 to the mainnet later this week or early next week.

dete commented 5 years ago

It's unclear to me how only passing the hash of what is being signed as sufficient for the contract verifying the authorization.

Well, my impulse is always to err on the side as "as simple as possible", with enough functionality to cover 80% of use cases. The other 20% can always implement an extension to this mechanism when the added complexity warrants it.

Having said that, the difference between bytes and bytes32 is probably not that big, so using bytes doesn't seem like too much of a "burden" for those wallets that don't need it. I happily concede that bytes is the better choice.

I feel like this argument plays out in the opposite direction when it comes to enforcing the view modifier, however. The whole point of using off-chain authentication is that it doesn't cost any gas, and can happen instantly without waiting for a new block to be mined. If the standard allows state-updating implementations of isValidSignature, it more or less requires that every usage must be called on-chain: that's a massive burden for the many, many instances where the on-chain call isn't needed.

@izqui's suggestion of an optional extension to the base spec makes sense here. The spec could require that intermediaries that get a positive result for isValidSignature must call didUseSignature on-chain (if it's implemented). However, this change probably warrants an ERC-165 compliance, which balloons the spec from one method two three... 🤷‍♂️

PhABC commented 5 years ago

@izqui

I still feel quite strongly about having isValidSignature() be a view function (solc5 enforces STATIC_CALL) that cannot modify state.

Why do you feel strongly? What is the main benefit of enforcing this?

Replay protection seems like a problem that should be handled by the application and not the wallet itself.

Agreed.

A separate ERC could be implemented that calls a didUseSignature(bytes32) back to the wallet after performing whatever action to mark the signature as used.

I feel like creating another ERC makes things less convenient and more complicated, since signer contract has no way of enforcing that didUseSignature() was called after the signature was verified / used. Even if it did and if it enforced it, that means that all calling contracts would always need to also call didUseSignature() after calling isValidSignature(), otherwise isValidSignature() would not work for contracts implementing these two standards. Imo it's simpler to let both the wallet contract and application contract to do what they want and protect themselves against undesirables events. Some events are undesirable for some while not for others and I don't think it should be enforced in this ERC.

I think that we should have an arbitrary gas limit for this call though, otherwise a isValidSignature() infinite loop can be created with a chain of validation contracts that could be used to grief user.

Imo this is a corner case scenario, since a user can always simulate the execution of a contract call prior to sending the transaction. They can also verify that the wallet contract doesn't have something that leads to "infinite gas" before sending the transaction on-chain. All this can be done at the client level and does not require an on-chain limitation in most cases. Overall, the limit is itself pretty arbitrary and should be either user specified or left-out of this ERC imo (i.e. application specific). That being said, I would be willing to say that isValidSignature() SHOULD/MUST consume less than 250000 gas. If that's not appropriate, ERC-1271 can be upgraded to remove this limit for future implementations.

The main method of this standard should be isValidSignature(bytes32,bytes) and then optionally also support isValidSignature(bytes,bytes) which could do some validation on the data and then this.isValidSignature(H(data), sig) (can also be an internal call, added this for extra clarity), as otherwise it is impossible to chain validation to another validator that only supports isValidSignature(bytes,bytes) if the original check was done using isValidSignature(bytes32,bytes).

Why are you against both and prefer choosing one with some cascading logic? Supporting both adds about 2-3 lines of code in a contract (see here, but I fear like making one optional can add quite a bit of complexity for those who want to support the optional version (especially application side).

@catageek

My gut feeling is that 250k is too much for a decent signature verification, but I may be wrong.

As my comment to Luis, I would like a concrete example when imposing no limits could actually be used maliciously in a way that is not easily predictable / countered.

@dete

If the standard allows state-updating implementations of isValidSignature, it more or less requires that every usage must be called on-chain: that's a massive burden for the many, many instances where the on-chain call isn't needed.

Every transaction that goes on-chain can be simulated locally, so one can verify a signature off-chain even if view modifier is not imposed. In both the view or public cases, one need to run a node to know if a signature is valid, but neither require an on-chain transaction.

pazams commented 5 years ago

Why are you against both and prefer choosing one with some cascading logic? Supporting both adds about 2-3 lines of code in a contract

I'd like to join the isValidSignature(bytes32,bytes) vs isValidSignature(bytes,bytes) discussion. While isValidSignature(bytes,bytes) may provide more flexibility, the schema within bytes _data needs to be spelled out explicitly. Otherwise, how would dApps and contracts know in advance how to format this parameter? A spec is needed for that.

For this reason, and since there is no plans to spec how would a multi-purpose bytes _data work, I think the stricter bytes32 _data, i.e. isValidSignature(bytes32,bytes), should be preferred here.

PhABC commented 5 years ago

Otherwise, how would dApps and contracts know in advance how to format this parameter? A spec is needed for that.

Currently, bytes is just the data being signed, whatever it is, while bytes32 is the hash of that data. The main difference is one you pass isValidSignature(_data,_signature) while the other is isValidSignature(keccak256(_data), signature). The current contract should not need to care about how to pass the data to the signer contract and it's the job of the signer contract to handle what to do with the data. We can't force dapp to organize their data in the same way, but signature validation logic can be made application specific no problem.

How would you standardize the bytes version otherwise? Perhaps I'm missing something, but it seems to me that just passing the bytesarray that is being hashed is good a enough standard.

pazams commented 5 years ago

@PhABC I'll try to illustrate my point with an example. Consider data to be a structured to contain 4 bytes of a "claims", and 32 bytes for "expiration time".

If the signer contract receives a bytes32 hash of that structure, then the original structure has no meaning to the signer contract.

On the other hand, if the signer contract receives the raw data, it has to ability to parse it, and do something with the claims and expiration time. That can be great, but for that to happen, the signer contract would need to know in advance the structure format of the data, right? If we can't standardise this (making signer contracts compatible across different callers), then the advantage of bytes of bytes32 is moot, and therefore why I side with the stricter bytes32.

PhABC commented 5 years ago

That can be great, but for that to happen, the signer contract would need to know in advance the structure format of the data, right?

Yes, that's correct and that will be unique to each application, which I think is actually what we want. Most signer contracts could just do the following if they don't care about the data:

function isValidSignature(bytes _data, bytes _signature) public {
  bytes32 dataHash = keccak256(_data);
  isValidSignature(dataHash, _signature);
}

If they do care about the contract and the data, they can do something like:

function isValidSignature(bytes _data, bytes _signature) public {
  if (msg.sender == ZEROEX_CONTRACT) { 
    Validator(ZEROEX_CONTRACT).isValidData(_data, _signature); 
  } else if  (msg.sender == ARAGON_CONTRACT) {
    Validator(ARAGON_CONTRACT).isValidData(_data, _signature); 
  } else {
    revert();
  }
}

Obviously you wouldn't do a bunch of if-else statement and you would use a mapping (msg.sender address => data validation logic contract). This allows signer contract to validate the data however they want for all the applications. What we could standardize is specific data validation logic "modules" (e.g. ERC-20 transfer daily limit), but we can't standardize how data is handled imo and that's fine. This is already pretty much how Gnosis Safe's modules work for example.

dcarlcf commented 5 years ago

@PhABC , is there resolution on whether the function will return a bool or the magic value?

In your comment on Jan 17, you have this:

isValidSignature(bytes32 hash, bytes signature) returns (bool) {
  ...
}

isValidSignature(bytes data, bytes signature) returns (bool) {
  return isValidSignature(keccak256(data), signature);
}

But the EIP proposal returns the magic value 0x20c13b0b

Is there finalization on this?

The current implementation of 0x looks for a bool to be returned, however they told me that V3 will look for the magic value.

pazams commented 5 years ago

@PhABC I think we agree on the description of the issue of, but have different opinions regarding it.

Obviously you wouldn't do a bunch of if-else statement and you would use a mapping

Imagine if Chrome's source code had that type of mapping with google domains in it so it would deliver a special experience for those sites. That just leads to browser wars, app wars, etc'. In the same sense, I feel like there should be no special paths inside the contract, and all applications should be treated the same. I may be a purist :)


To @dcarlcf point, I've also been unsure on the resolution of the return value. Given from the discussion here that multiple method signatures will be supported, I'd like to suggest that each of these will return a magic value unique to the method's 4-byte identifier. For example, if a contract chooses to support only this form:

function isValidSignature(bytes32 hash, bytes _signature) returns(bytes4 magicValue);

then the return value should be 0x1626ba7e. This would allow the spec to evolve with more methods, each of them decoupled with it's own return value. It's not ideal, but could be a practical way to move forward.

wighawag commented 5 years ago

Hey, We are apparently seeing different implementation being published. Some using bool as return value, some using a bytes32 hash as input (as opposed to the bytes data mentioned in the current draft).

I personally do not mind too much between bytes32 hash vs bytes data but tend to agree with @pazams that bytes32 should be sufficient unless we can standardize on the structure of the data.

As mentioned we could have both, but both should be mandatory then.

As for the return value, I agree it should be a magic value to avoid accidently returning true.

@pazams , I am fine to have different return value for each method but I don't think this is necessary. On the other hand if we go down the route of having different optional method, we should also define an explicit rejection magic value. This would allow the calling contract to check each one of these optional methods.

PSEUDO CODE

(success, returnData) = _to.call("isValidSignature(bytes,bytes)", data, sig);
if(!success || (returnData != ACCEPT_MAGIC_VALUE && returnData != REJECTION_MAGIC_VALUE)){
(success, returnData) = _to.call("isValidSignature(bytes32,bytes)", keccak256(data), sig);
}
if(success && returnData == ACCEPT_MAGIC_VALUE) {
//ACCEPTED
} else if(success && returnData == REJECTION_MAGIC_VALUE) {
//REJECTED
} else {
 // REJECTED because not supported
}

I would prefer not having optional methods though.

pedrouid commented 5 years ago

Hey everyone, I was looking into how Dapps could validate messages using ERC-1271 but there isn't a standard way of detecting smart contract wallets so I added a proposal for Signature Type Recognition using the method used by 0x Protocol v2.0 stack.

Check out the EIP #2126. I would love to have more feedback. Thanks!

PhABC commented 5 years ago

Time to polish EIP-1271 for those interested!

What about using EIP-712 for the isValidSignature(bytes _data, bytes _signature)? @wighawag

wighawag commented 5 years ago

@PhABC I am not sure I understand, what would _data contains, the EIP-712 message itself ? + the type information so isValidSignature can extract and do its own validity checks ?

I think this would only make sense for application specific logic and that's why I think the bytes32 version is all we need, like @pazams mention.

Re-reading the threads I have few more comments :

b) looks to me the best way, as I take the stance, that it is the signer responsibility to have its validation contract to behave correctly. But I might miss some potential attack.

As for EIP-165 if we have only one function, it is unnecessary and It would complicate recursive isValidSignature as EIP-165 also require a minimal gas amount.

As for supporting 2 different functions (unless they both need to be implemented). This is not going to be easy with b), unless we have EIP-165 to check for the 2 different interfaces (they would be separated) . But as mentioned EIP-165 would prevent recursive isValidSignature

cwhinfrey commented 5 years ago

I made an npm package, is-valid-signature, for checking if a signature is valid for an address that may be an externally owned account or a contract that complies with ERC-1271. It checks the bytecode at the address and if it's 0, it recovers the signature in javascript, otherwise, it calls the ERC1271 method and checks the return value for the MAGICVALUE. https://github.com/authereum/is-valid-signature

PhABC commented 5 years ago

@wighawag The idea behind using bytes instead of bytes32 is not "application" specific but wallet specific, where wallet is the "signing contract" and the application is the contract the wallet wants to interact with.

The easiest example is that smart wallet X (contract wallet) is controlled by Bob with 2 private keys, where one private key has limited functionality (say stored on phone) while the other has no restriction (say a HW nano ledger). Say wallet X has daily transfer limit for the Phone key. What does it do in the case of a off-chain orderbook dex like 0x? The signer is the phone key, the application is 0x, but if we use bytes32 than all that Wallet X knows is that the phone key signed a 0x order, but it has no idea if that order exceeds the phone key daily transfer limit or not since it only sees the hash of the order.

Here the easiest solution is to basically prevent the phone key to sign 0x orders, but that seems quite restrictive in my opinion. You could imagine many applications where different keys have different "permissions" and using bytes32 prevent smart contract wallets from validating correctly these permissions.

wighawag commented 5 years ago

@PhABC that's interesting use case and something I would support but unless there is a standard for the bytes format, applications cannot know weather the bytes is meaningful to the wallet being asked to verify the signature.

Reversely, the wallet cannot know whether the application is simply passing the data in the wrong format (maybe because it was built before the standardisation took place), and might block the transfer by ignorance instead of security.

EIP-712 specify only the message format to be passed to the signer and how to encode that data in a hash to be signed. The smart contract can decide what data is being passed through the function call and in what order. So EIP-712 itself is not sufficient : it does not prescribe the bytes data format.

So while I think the idea is good since as you say, bytes32 prevent such use case completely, for bytes to be useful the data format need to be standardised and in that case, another EIP could do that. This will probably result in several extra EIP even, one for each kind of interaction wallets want to have knowledge of.

Why can't we postpone such standardization and focus on the simplest case : hash signature ?

Once we come up with bytes format standard for wallets, we could inroduce new functions, that application would be able to call meaningfully if they want their user to benefit from the wallet's features it provides

cwhinfrey commented 5 years ago

@PhABC This is a really interesting use case and we're actually implementing a wallet that has keys with different roles and spending limits. I think whether or not a 0x order (or any other transaction) will fail should be out of scope of this standard though. For one, the contract's state may change between the off-chain check and when the signature goes on chain so no guarantees are being provided anyway.

Also, I think the use case you're talking about is specific to the context of individual wallets rather than wallets in general. If an external application already knows it needs to check if a signer has specific roles and that certain limits won't be hit, then a standardized function shouldn't be necessary. The application could just call something like isValidSigned0xOrder because it already knows about the context. I think the point of a standard like this is to allow applications to test if a signature is valid for a given contract without having to know any specifics about the contract.

I think right now I agree with @pazams comment and don't see the advantage of using bytes but could be persuaded otherwise 😉

If we can't standardise this (making signer contracts compatible across different callers), then the advantage of bytes of bytes32 is moot, and therefore why I side with the stricter bytes32.

@marekkirejczyk I believe you're considering adopting this standard for Universal Login. Any thoughts on this?

marekkirejczyk commented 5 years ago

@cwhinfrey CC @Amxx

Quick thoughts:

cag commented 5 years ago

WRT interop with EIP-712, I see a few options:

  1. Use bytes data and put the EIP-712 digest as the data (so the hashed value that would normally get signed by an EOA in that EIP).
  2. Use bytes data and put the EIP-712 digest pre-image as the data (so 0x1901 . domainSeparator . structHash(message)). I happen to be using this form at the moment, but the contract I validate with ends up just hashing this data into the digest typically used for signatures.
  3. Use bytes32 data and put the EIP-712 digest as the data.

A separate thought: has anyone thought about validity permanence for contract signatures? EOA signatures are tied to an address and stay valid forever, but these contract signatures may change in validity if the contract state changes. I think it could be useful if signatures can be made permanently valid somehow.

pazams commented 5 years ago

Hi again!

I'd like explain some of crossover between this spec and ERC-1654. ERC-1654 specifies a similar interface and was adopted by a number of popular games, including:

Crypto Kitties - https://www.cryptokitties.co/ Cheeze Wizards - https://www.cheezewizards.com/ MyCryptoHeroes - https://www.mycryptoheroes.net/ Etheremon - https://www.etheremon.com/ Megacryptopolis - https://www.megacryptopolis.com/ Blockchain Cuties Universe - https://blockchaincuties.com/ Hyperdragons - https://hyperdragons.alfakingdom.com/

The goal of ERC-1654 was to spec the off-chain authentication process. The goal was NOT to specify an interface.

However, since at the launch-time of Dapper, ERC-1271 had some ambiguity on return values, and we had a stricter view with bytes32, we eventually did specify a contract interface.

This was NOT meant to compete with 1271. The return type in ERC-1654 was chosen to be free of merge-conflicts with ERC 1271 draft. This was done in the hopes that we can have a future where ERC-1271 is the canonical reference for all isValidSignature contract interface flavors. Having a different magic value per each flavor that is unique to the 4-byte identifier promotes this vision.

cwhinfrey commented 5 years ago

@marekkirejczyk

bytes signature could be bytes signatures as some off-chain actions on multisig might require more than one signature

Our use case will require two signatures to prove a valid signer in most cases so completely agree with this. 👍

wighawag commented 5 years ago

Hey all, @PhABC

Any update on this ? I would love to implement that for mainnet soon.

I think we all agree that signatures need to be bytes But it would be great if we could finalise on whether we want to fully support bytes _data and how.

On that note, we now need to consider that there are apps and wallets that follow #1654 which is basically the bytes32 _data version. So if we go with bytes _data we probably want to support the scenario where a contract want to support both version to be compatible with as many wallet as possible. it was your thinking right @pazams @dete ?

This is possible, but contrary to what I was mentioning in https://github.com/ethereum/EIPs/issues/1271#issuecomment-488648761 wallet contracts that implements 1271 (the bytes _data version) MUST revert when called with the 1654 bytes32 _data version. See below :

Assuming a no gas limit and that a revert is to be considered as a reason for the caller to revert (see https://github.com/ethereum/EIPs/issues/1271#issuecomment-504136144 for a comment on that) , the validation would work as follow:

  1. caller call 1271 isValidSignature(bytes...)
  2. if this return the magic value, consider the signature valid
  3. if this return something else, consider the signature invalid, (I don't see any reason to call the 1654 version in that case as we can assume that not reverting means the contract being called support isValidSignature and rejected the signature based on the bytes data provided. But we could also as 1654 did specify a specific rejection magic value (0x00000000 ) )
  4. if it reverts (this can be the result of lack of gas or non-implementation, we can't know), call 1654 isValidSignature(bytes32 ...)
  5. if this return magic value consider signature valid
  6. if this return 0x00000000 consider the signature invalid
  7. if this returns something else, we can also consider it an invalid signature (or @pazams did you think of something else?)
  8. if it reverts , revert

note that when the reply is that of an invalid signature, the caller would probably revert at this point so it can signal the tx signer that the intended action could not proceed, but is that always the case? Could there be cases where the calling contract want to record an invalid signature? if so it is necessary to revert when the call to isValidSignature revert, else it would record the signature as invalid even though that signature was not invalid (the isValidSignature revert was caused due to a lack of gas only)

By the way, the reason to call the 1271 bytes _data version first is that since it supports bytes it would have more capability and would provide a more accurate answer. This should not matter anyway if the rules above is respected : a contract wallet MUST revert on a call to 1654 version when implementing both 1271 and 1654.

Indeed, if it was implementing both, the transaction signer could potentially make sure the call to 1271 fails at 4) in purpose (indeed, assuming the 1271 isValidSignature(bytes... use significantly more gas, the tx signer can make sure there is not enough gas for the 1271 call to succeed but enough for the 1654 and the rest of the call to succeed (due to EIP-150 1/64 rules)) so that the 1654 version is called instead. And since with 1654 there is no extra information for validation, it will validate even if 1271 would not have (daily limit, etc...). This comes from the fact that we cannot know whether the revert resulting from the call to 1271 version is due to a lack of gas or simply because the wallet does not implement it. This is quite a particular edge-case and can only happen if the rest of the call is using minimum gas. It is a risk nevertheless.

But actually now since we already have #1654 with bytes32 _data we don't need to decide here whether we choose between the 2. The question are more like

For example, do we want 1271 to be a superset of 1654 where it specify the 2 types of call or do we want 1271 to be specified separately for bytes _data ?

I think it makes sense to have 1271 to be a superset to ensure that existing wallet that use 1654 are supported by apps that use 1271. Similary, this way we ensure app that call 1654 wallets will continue to work on wallet that support 1271.

We also need to decide on whether there is a maximum gas to be provided. I would argue for not having it as this would prevent recursive call unless we have what @catageek describe here : https://github.com/ethereum/EIPs/issues/1271#issuecomment-462812486 with "abstract account"

Plus a gas limit might cause compatibility problem with existing 1654 app and wallet.

dekz commented 4 years ago

FYI 0x is moving ahead with implementing EIP-1271 in v3.

With regards to SignatureTypes, we have an enum byte appended to the signature to indicate the type of signature. With 0x07 representing EIP1271Wallet. This implementation uses bytes and returns the magic value 0x20c13b0b.

Our EIP-1271 Interface declaration can be found here.

We have also chosen to prepend the bytes with a bytes4 indicating what the bytes represent. As there can be different bytes data in different context with no one-size-fits-all. As such, in 0x context, when it is an Order we append a bytes4 to the bytes data to indicate to the contract the context of what they're validating.

Two simple bytes4 "contexts" can be seen here. For example OrderWithHash(LibOrder.Order,bytes32) will indicate the contract is verifying the signature of an Order. ZeroExTransactionWithHash(LibZeroExTransaction.ZeroExTransaction transaction,bytes32) will indicate a 0x transaction (similar to a metatransaction).

3esmit commented 4 years ago

I think is important to clarify what should be the return value for a failed return. I see this mostly should not matter as we are testing against the magic value as true, and anything else besides would be a false. However I seen some implementations using 0x00000000 and other using 0xffffffff, and it's better be safe then sorry.

Why not standarize the return value for false aswell?

Seems like 0x00000000 would be better because of gas efficiency but 0xffffffff seems more clear that contract intended to say false.

frozeman commented 4 years ago

First of, I love this ERC. Very useful, especially for ERC725.

One question: Why returning magic value 0x20c13b0b , and not a boolean?

catageek commented 4 years ago

@frozeman It has been discussed a while. The magic value, from the point of view of the verifier, ensures that the reply from the function is explicit, being equal to a hardcoded value unique to this ERC.

On the contrary, a boolean could be returned by any function returning a boolean, with a doubt, from the point of view of the verifier, that the function actually executed is intended to verify a signature according to this ERC, or it is a function returning a boolean that is not related to ERC-1271.

This prevents deniability, i.e pretending that the reply was accidental or fraudulent. IRL, this is equivalent to write a full sentence to sign a document instead of a simple signature. You can't deny the content of the sentence and the subsequent agreement.

PhABC commented 4 years ago

To add to @catageek, if we used a boolean, the calling contract couldn't tell if the inner call failed because it was rightfully "rejected" by target contract or because it ran out of gas. Now if the inner call throws because of out of gas, it will not return the magic value and calling contract can act accordingly.

frozeman commented 4 years ago

Good point, and i understand the usefulness.

But to play the devils advocate here: We would ideally need to do that for every function on every standard. Thats why we should use ERC165 or ERC1820 to verify that a smart contract implements a certain interface...

If a smart contract wants to fool, he can always do that, and the magic return doesnt fix that.

I would go the standard route and say, this EIP should require ERC165 instead of a magic return value :)

frozeman commented 4 years ago

@PhABC thats a good point...

catageek commented 4 years ago

@PhABC A real response management framework at application level, similar to HTTP response 2XX,3XX,4XX, etc. would not require a magic value.

@frozeman Good point about using ERC165 or ERC820 instead

frozeman commented 4 years ago

0x implementation uses: bytes32 for the data, rather than bytes https://github.com/0xProject/0x-monorepo/blob/05b35c0fdcbca7980d4195e96ec791c1c2d13398/packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol#L85-L87

Why bytes, when it is more convenient to test against a hash? when its a hash then the format is also more flexible. Eg. using the \x19Ethereum Signed Message:\n+ length + data, or simple a plain has would work the same... As this function is purely about validating the signature and not executing anything it should be fine. Problem is if we determine how the data is hashed, then we end up with the most of a function that 0x wrote... which is not ideal for this simple signature verification purpose. Any thoughts to that?

btw creates also more anonymity, as the data is not submitted in transactions, just the hash.

And sorry if that was all discussed above :(

frozeman commented 4 years ago

with bytes32 the implementation for an ERC725 account, could be as simple as:

    function isValidSignature(bytes32 _hash, bytes memory _signature)
    override
    public
    view
    returns (bytes4 magicValue)
    {
        if (UtilsLib.isContract(owner)){
            return IERC1271(owner).isValidSignature(_hash, _signature);
        } else {
            return owner == ECDSA.recover(_hash, _signature) ? ERC1271MAGICVALUE : bytes4(0xffffffff);
        }
    }
pedrouid commented 4 years ago

I'm also in favor of using the hash instead since it would make it compatible with EIP-712 signing while bytes expect the data to be encoded as an eth_sign which is not as useful

Alternatively it should be possible to communicate to the validation method whether or not it should be encoded one way or the other.

But why do the latter when the hash is much simpler

PhABC commented 4 years ago

I also now prefer the hash, but we couldn't seem to converge on one of the two arguments. The main use case for bytes is to allow a contract to validate the content being signed, which is not possible when passing a hash, but this seems to be a use case that is non-existent in practice as of now.

Most projects I've seen now support both bytes and bytes32, like 0x (see here). I'm personally happy with applications using either or both EIP-1271 and EIP-1654, where ERC-165 can be used to know which is implemented. I would also be happy to support changes to ERC-1271 to put bytes32 version first.

pedrouid commented 4 years ago

That would be great because currently there is a divergence between Smart Wallet implementations which gets asked a lot by dapp developers integrating WalletConnect on how to properly validate signatures

Consolidating on this would allow dapps to easily support all Smart Wallets right away and I could write documentation for this and start pushing for adoption.

function isValidSignature(bytes32 hash, bytes _signature) returns(bytes4 magicValue);

Should we then consolidate on bytes32 hash as first argument and magicValue 0x1626ba7e??

PhABC commented 4 years ago

We could make so that apps need to implement the bytes32, but they can also add support for the bytes version, in which case they would need to call the bytes version when the bytes32 version is called, like so ;

function isValidSignature(bytes32 _hash, bytes memory _signature) override public view returns (bytes4 magicValue)
{
    bytes memory encoded_hash = abi.encode(_hash);
    bytes4 result = isValidSignature(encoded_hash, _signature); // <- that's the `bytes` version of the function.
    return result == 0x20c13b0b ? 0x1626ba7e : 0x00000000;
}

Importantly, apps could only implement the bytes32 version, which basically signals they don't care about validating the content signed, but they have to implement both if they also want to add support for bytes.

frozeman commented 4 years ago

Where is the 0x1626ba7e coming from? I personally think, if its about evaluating the content we probably would need to come up with standards for each validation type... The bytes version could then return a hash based on which type of content was validated.. E.g. it could return 0x00001654 if the return value was correctly validated as 1654.

But to be honest i think this results in two standards:

But for a simple signature verification i am a fan of simple boolean return, through i do share the concern of @PhABC

PhABC commented 4 years ago

All magic values are just the function signatures, i.e.

bytes4(keccak256('isValidSignature(bytes32,bytes)')) == 0x1626ba7e
frozeman commented 4 years ago

To give an example for an ERC725 account (smart contract based account). If you want that a dapp or website can login/authenticate a user, he simply needs to be able to sign any message, so a simple isValidSignature(bytes32 _hash, bytes memory _signature) works.

If its a wallet that wants to execute a tx, i assume this would be a custom function anyway to pertly decode the input data..

cwhinfrey commented 4 years ago

Another alternative would be to accept bytes as the first argument without the _type parameter like discussed previously

isValidSignature(bytes memory _data, bytes memory _signature)

but add to the standard that _data must be the fully encoded message. That way _data already includes any prefixes specific to personal_sign or EIP 712.

This solves for:

I believe Argent already implemented this standard this way. We would need to update the Authereum contracts to remove the logic that appends \x19Ethereum Signed Message:\n.