Merge 369d39b into 511b7fc

ERCS/erc-6492.md

@@ -102,7 +102,8 @@ interface IERC1271Wallet {
 }
 
 error ERC1271Revert(bytes error);
-error ERC6492DeployFailed(bytes error);
+error ERC6492CallFailed(bytes error);
+error ERC6492DeploySilentlyFailed();
 
 contract UniversalSigValidator {
   bytes32 private constant ERC6492_DETECTION_SUFFIX = 0x6492649264926492649264926492649264926492649264926492649264926492;
@@ -115,21 +116,34 @@ contract UniversalSigValidator {
     bool allowSideEffects,
     bool tryPrepare
   ) public returns (bool) {
-    uint contractCodeLen = address(_signer).code.length;
+    uint contractCodeLen = _signer.code.length;
     bytes memory sigToValidate;
     // The order here is strictly 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; if the contract is deployed we will check the sig against the deployed version, this allows 6492 signatures to still be validated while taking into account potential key rotation
     // - ERC-1271 verification if there's contract code
     // - finally, ecrecover
-    bool isCounterfactual = bytes32(_signature[_signature.length-32:_signature.length]) == ERC6492_DETECTION_SUFFIX;
+    bool isCounterfactual = _signature.length >= 32
+      && bytes32(_signature[_signature.length-32:_signature.length]) == ERC6492_DETECTION_SUFFIX;
+    bool shouldTryPrepareNext = isCounterfactual && !tryPrepare && contractCodeLen > 0;
+    bool makingACall = isCounterfactual && (contractCodeLen == 0 || tryPrepare);
+
+    // Store these for error reporting later
+    bytes memory callErr;
+    bool callSuccess;
+
+    if (tryPrepare) require(isCounterfactual, 'SignatureValidator: tryPrepare should be used with counterfactual wrapped sigs');
     if (isCounterfactual) {
       address create2Factory;
       bytes memory factoryCalldata;
       (create2Factory, factoryCalldata, sigToValidate) = abi.decode(_signature[0:_signature.length-32], (address, bytes, bytes));
 
-      if (contractCodeLen == 0 || tryPrepare) {
-        (bool success, bytes memory err) = create2Factory.call(factoryCalldata);
-        if (!success) revert ERC6492DeployFailed(err);
+      if (makingACall) {
+        (callSuccess, callErr) = create2Factory.call(factoryCalldata);
+      }
+
+      if (_signer.code.length == 0) {
+        if (!callSuccess) revert ERC6492CallFailed(callErr);
+        else revert ERC6492DeploySilentlyFailed();
       }
     } else {
       sigToValidate = _signature;
@@ -140,12 +154,17 @@ contract UniversalSigValidator {
       try IERC1271Wallet(_signer).isValidSignature(_hash, sigToValidate) returns (bytes4 magicValue) {
         bool isValid = magicValue == ERC1271_SUCCESS;
 
-        // retry, but this time assume the prefix is a prepare call
-        if (!isValid && !tryPrepare && contractCodeLen > 0) {
-          return isValidSigImpl(_signer, _hash, _signature, allowSideEffects, true);
+        if (!isValid) {
+          // retry, but this time assume the prefix is a prepare call
+          if (shouldTryPrepareNext) {
+            return isValidSigImpl(_signer, _hash, _signature, allowSideEffects, true);
+          }
+          // we already tried prepare but we have an actual callErr while doing it
+          if (tryPrepare && !callSuccess) revert ERC6492CallFailed(callErr);
         }
 
-        if (contractCodeLen == 0 && isCounterfactual && !allowSideEffects) {
+        // only reverting in case a call was made and we DONT want side effects
+        if (makingACall && !allowSideEffects) {
           // if the call had side effects we need to return the
           // result using a `revert` (to undo the state changes)
           assembly {
@@ -157,10 +176,10 @@ contract UniversalSigValidator {
         return isValid;
       } catch (bytes memory err) {
         // retry, but this time assume the prefix is a prepare call
-        if (!tryPrepare && contractCodeLen > 0) {
+        if (shouldTryPrepareNext) {
           return isValidSigImpl(_signer, _hash, _signature, allowSideEffects, true);
         }
-
+        if (tryPrepare && !callSuccess) revert ERC6492CallFailed(callErr);
         revert ERC1271Revert(err);
       }
     }
@@ -191,7 +210,7 @@ contract UniversalSigValidator {
       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) }
+      else assembly { revert(add(error, 0x20), len) }
     }
   }
 }
@@ -243,6 +262,8 @@ However, deploying a contract requires a `CALL` rather than a `STATICCALL`, whic
 
 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.
 
+**The reference implementation in this ERC has been audited:** [report 1](../assets/erc-6492/ERC6492-Hunter-Security-Audit-Report-v1.0.pdf).
+
 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, checking against the current live version of the contract.