Merge pull request #238 from bcnmy/feat/pre-issued-1271-sigs-handling

Pre issued 1271 sigs handling

contracts/Nexus.sol

@@ -326,13 +326,30 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra
         // else proceed with normal signature verification
         // First 20 bytes of data will be validator address and rest of the bytes is complete signature.
         address validator = address(bytes20(signature[0:20]));
-        require(_isValidatorInstalled(validator), ValidatorNotInstalled(validator));
-        bytes memory signature_;
-        (hash, signature_) = _withPreValidationHook(hash, signature[20:]);
-        try IValidator(validator).isValidSignatureWithSender(msg.sender, hash, signature_) returns (bytes4 res) {
-            return res;
-        } catch {
-            return bytes4(0xffffffff);
+        if (_isValidatorInstalled(validator)) {
+            bytes memory signature_;
+            (hash, signature_) = _withPreValidationHook(hash, signature[20:]);
+            try IValidator(validator).isValidSignatureWithSender(msg.sender, hash, signature_) returns (bytes4 res) {
+                return res;
+            } catch {
+                return bytes4(0xffffffff);
+            }
+        } else {
+            // try to check the signature against the account
+            if (_checkSelfSignature(signature, hash)) {
+                // if it was signed by address(this),
+                // we still revert on this, to protect from the following attack vector:
+                // 1. This 7702 account (being an eoa as well) owns some other Smart Account (Smart Account B)
+                // 2. It signs some unsafe hash: the one that doesn't have Smart Account B address hashed in
+                // 3. In this case, if we just allow signatures by address(this), this above sig
+                //    over unsafe hash could be replayed here
+                // Thus, we revert here, but we revert with informational message, that
+                // lets know that the sig is ok, it is just potentially unsafe.
+                revert PotentiallyUnsafeSignature();
+            } else {
+                // othwerwise revert normally
+                revert ValidatorNotInstalled(validator);
+            }
         }
     }
 

contracts/interfaces/INexusEventsAndErrors.sol

@@ -51,4 +51,10 @@ interface INexusEventsAndErrors {
 
     /// @notice Error thrown when attempted to emergency-uninstall a hook
     error EmergencyTimeLockNotExpired();
+
+    /// @notice Error thrown when a valid, though potentially unsafe signature is detected
+    error PotentiallyUnsafeSignature();
+
+    /// @notice Error thrown when an invalid signature is detected
+    error InvalidSignature();
 }

test/hardhat/smart-account/Nexus.Basics.specs.ts

@@ -411,12 +411,12 @@ describe("Nexus Basic Specs", function () {
 
       const signatureData = ethers.solidityPacked(
         ["address", "bytes"],
-        [ZeroAddress, signature],
+        [await deployer.getAddress(), signature], // use some random address instead of installed validator
       );
 
       await expect(
         smartAccount.isValidSignature(hash, signatureData),
-      ).to.be.revertedWithCustomError(smartAccount, "ValidatorNotInstalled");
+      ).to.be.revertedWithCustomError(smartAccount, "InvalidSignature"); // sig with address appended will be treated as invalid by the ECDSA library
     });
   });