Update EIP-6492: add on-chain validation (ethereum#6667)

* Add EIP: Standard Signature Validation Method for Counterfactually Deployed Contracts (ethereum#6492)

* Update EIP-6492: Standard Signature Validation Method for Counterfactually Deployed Contracts

* Error fixes
* Readability improvements
* Better definitions

* Update EIP-6492: Signature Validation for Predeploy Contracts

* Moved counterfactual check to the beginning

* Update EIP-6492: Signature Validation for Predeploy Contracts

* Add a note about key invalidation

* Update EIP-6492: Signature Validation for Predeploy Contracts

* Propose a validation implementation that does not require pre-deploying singletons

* Update EIP-6492: Signature Validation for Predeploy Contracts

* Consistency in using verification/validation

* Suggest a library to use

* Update EIP-6492: Signature Validation for Predeploy Contracts

* Specify off-chain usage

* Change example contract to something a bit more compiler optimizable

* Update EIP-6492: Signature Validation for Predeploy Contracts

* Fix contract issue

* Update EIP-6492: Signature Validation for Predeploy Contracts

* Add on-chain validation singleton

* Update EIP-6492: Signature Validation for Predeploy Contracts

* Add on-chain validation singleton

EIPS/eip-6492.md

@@ -84,46 +84,63 @@ This ERC is backward compatible with previous work on signature validation, incl
 
 ## Reference Implementation
 
-Example implementation of a universal verification contract, intended to be used off-chain:
+Below you can find an implementation of a universal verification contract that can be used both on-chain and off-chain, intended to be deployed as a singleton. It can validate signatures signed with this ERC, [ERC-1271](./eip-1271.md) and traditional `ecrecover`. [EIP-712](./eip-712.md) is also supported by extension, as we validate the final digest (`_hash`).
 
 ```solidity
 // As per ERC-1271
 interface IERC1271Wallet {
   function isValidSignature(bytes32 hash, bytes calldata signature) external view returns (bytes4 magicValue);
 }
 
+error ERC1271Revert(bytes error);
+error ERC6492DeployFailed(bytes error);
+
 contract UniversalSigValidator {
   bytes32 private constant ERC6492_DETECTION_SUFFIX = 0x6492649264926492649264926492649264926492649264926492649264926492;
   bytes4 private constant ERC1271_SUCCESS = 0x1626ba7e;
 
-function isValidUniversalSig(
+  function isValidSigImpl(
     address _signer,
     bytes32 _hash,
-    bytes calldata _signature
+    bytes calldata _signature,
+    bool allowSideEffects
   ) public returns (bool) {
     uint contractCodeLen = address(_signer).code.length;
+    bytes memory sigToValidate;
     // The order here is striclty defined in https://eips.ethereum.org/EIPS/eip-6492
     // - ERC-6492 suffix check and verification first, while being permissive in case the contract is already deployed so as to not invalidate old sigs
     // - ERC-1271 verification if there's contract code
     // - finally, ecrecover
-    if (bytes32(_signature[_signature.length-32:_signature.length]) == ERC6492_DETECTION_SUFFIX) {
-      // resize the sig to remove the magic suffix
-      _signature = _signature[0:_signature.length-32];
-
+    bool isCounterfactual = bytes32(_signature[_signature.length-32:_signature.length]) == ERC6492_DETECTION_SUFFIX;
+    if (isCounterfactual) {
       address create2Factory;
       bytes memory factoryCalldata;
-      bytes memory originalSig;
-      (create2Factory, factoryCalldata, originalSig) = abi.decode(_signature, (address, bytes, bytes));
+      (create2Factory, factoryCalldata, sigToValidate) = abi.decode(_signature[0:_signature.length-32], (address, bytes, bytes));
 
       if (contractCodeLen == 0) {
-        (bool success, ) = create2Factory.call(factoryCalldata);
-        require(success, 'SignatureValidator: deployment');
+        (bool success, bytes memory err) = create2Factory.call(factoryCalldata);
+        if (!success) revert ERC6492DeployFailed(err);
       }
-      return IERC1271Wallet(_signer).isValidSignature(_hash, originalSig) == ERC1271_SUCCESS;
+    } else {
+      sigToValidate = _signature;
     }
 
-    if (contractCodeLen > 0) {
-      return IERC1271Wallet(_signer).isValidSignature(_hash, _signature) == ERC1271_SUCCESS;
+    // Try ERC-1271 verification
+    if (isCounterfactual || contractCodeLen > 0) {
+      try IERC1271Wallet(_signer).isValidSignature(_hash, sigToValidate) returns (bytes4 magicValue) {
+        bool isValid = magicValue == ERC1271_SUCCESS;
+
+        if (contractCodeLen == 0 && isCounterfactual && !allowSideEffects) {
+          // if the call had side effects we need to return the
+          // result using a `revert` (to undo the state changes)
+          assembly {
+           mstore(0, isValid)
+           revert(31, 1)
+          }
+        }
+
+        return isValid;
+      } catch (bytes memory err) { revert ERC1271Revert(err); }
     }
 
     // ecrecover verification
@@ -132,34 +149,63 @@ function isValidUniversalSig(
     bytes32 s = bytes32(_signature[32:64]);
     uint8 v = uint8(_signature[64]);
     if (v != 27 && v != 28) {
-      revert('SignatureValidator#recoverSigner: invalid signature v value');
+      revert('SignatureValidator: invalid signature v value');
     }
     return ecrecover(_hash, v, r, s) == _signer;
   }
+
+  function isValidSigWithSideEffects(address _signer, bytes32 _hash, bytes calldata _signature)
+    external returns (bool)
+  {
+    return this.isValidSigImpl(_signer, _hash, _signature, true);
+  }
+
+  function isValidSig(address _signer, bytes32 _hash, bytes calldata _signature)
+    external returns (bool)
+  {
+    try this.isValidSigImpl(_signer, _hash, _signature, false) returns (bool isValid) { return isValid; }
+    catch (bytes memory error) {
+      // in order to avoid side effects from the contract getting deployed, the entire call will revert with a single byte result
+      uint len = error.length;
+      if (len == 1) return error[0] == 0x01;
+      // all other errors are simply forwarded, but in custom formats so that nothing else can revert with a single byte in the call
+      else assembly { revert(error, len) }
+    }
+  }
 }
+
 // this is a helper so we can perform validation in a single eth_call without pre-deploying a singleton
-contract ValidateSig {
+contract ValidateSigOffchain {
   constructor (address _signer, bytes32 _hash, bytes memory _signature) {
-    // We deploy a new contract solely to take advantage of calldata array slices; otherwise we could implement the verification here too
     UniversalSigValidator validator = new UniversalSigValidator();
-    bool isValidSig = validator.isValidUniversalSig(_signer, _hash, _signature);
+    bool isValidSig = validator.isValidSigWithSideEffects(_signer, _hash, _signature);
     assembly {
       mstore(0, isValidSig)
       return(31, 1)
     }
   }
 }
-
 ```
 
-This validation contract does not need to be pre-deployed on-chain: on the contrary, it's designed to perform universal off-chain signature validation with support for this ERC, [ERC-1271](./eip-1271.md) and traditional `ecrecover` just by calling `eth_call` in a front-end verifier library.
+### On-chain validation
+
+For on-chain validation, you could use two separate methods:
+
+- `UniversalSigValidator.isValidSig(_signer, _hash, _signature)`: returns a bool of whether the signature is valid or not; this is reentrancy-safe
+- `UniversalSigValidator.isValidSigWithSideEffects(_signer, _hash, _signature)`: this is equivalent to the former - it is not reentrancy-safe but it is more gas-efficient in certain cases
+
+Both methods may revert if the underlying calls revert.
+
+### Off-chain validation
+
+The `ValidateSigOffchain` helper allows you to perform the universal validation in one `eth_call`, without any pre-deployed contracts.
 
 Here's example of how to do this with the `ethers` library:
 
 ```javascript
 const isValidSignature = '0x01' === await provider.call({
   data: ethers.utils.concat([
-    validateSigBytecode,
+    validateSigOffchainBytecode,
     (new ethers.utils.AbiCoder()).encode(['address', 'bytes32', 'bytes'], [signer, hash, signature])
   ])
 })
@@ -171,9 +217,11 @@ You may also use a library to perform the universal signature validation, such a
 
 The same considerations as [ERC-1271](./eip-1271.md) apply.
 
-However, deploying a contract requires a `CALL` rather than a `STATICCALL`, which introduces reentrancy concerns. As such, we recommend that this ERC is mainly used for off-chain signature validation. It is possible to use it for on-chain signature validation as well, but such a case is by definition redundant, as we are presuming that the contract will be deployed in that case.
+However, deploying a contract requires a `CALL` rather than a `STATICCALL`, which introduces reentrancy concerns. This is mitigated in the reference implementation by having the validation method always revert if there are side-effects, and capturing it's actual result from the revert data. For use cases where reentrancy is not a concern, we have provided the `isValidSigWithSideEffects` method.
+
+Furthermore, it is likely that this ERC will be more frequently used for off-chain validation, as in many cases, validating a signature on-chain presumes the wallet has been already deployed.
 
-Another out-of-scope security consideration worth mentioning is whether the contract is going to be set-up with the correct permissions at deploy time, in order to allow for meaningful signature verification. By design, this is up to the implementation, but it's worth noting that thanks to how CREATE2 works, changing the bytecode or contructor callcode in the signature will not allow you to escalate permissions as it will change the deploy address and therefore make verification fail.
+One out-of-scope security consideration worth mentioning is whether the contract is going to be set-up with the correct permissions at deploy time, in order to allow for meaningful signature verification. By design, this is up to the implementation, but it's worth noting that thanks to how CREATE2 works, changing the bytecode or contructor callcode in the signature will not allow you to escalate permissions as it will change the deploy address and therefore make verification fail.
 
 It must be noted that contract accounts can dynamically change their methods of authentication. This issue is mitigated by design in this EIP - even when validating counterfactual signatures, if the contract is already deployed, we will still call it.