fix: [spearbit-38] ERC-1271 signature validation with EOA owners of c…

…ontract owners of MSCA (#15)
alchemyplatform · Jan 25, 2024 · 5f1a29a · 5f1a29a
1 parent 482c268
commit 5f1a29a
Show file tree

Hide file tree

Showing 3 changed files with 72 additions and 22 deletions.
diff --git a/src/plugins/owner/MultiOwnerPlugin.sol b/src/plugins/owner/MultiOwnerPlugin.sol
@@ -136,18 +136,15 @@ contract MultiOwnerPlugin is BasePlugin, IMultiOwnerPlugin, IERC1271, EIP712 {
     function isValidSignature(bytes32 digest, bytes memory signature) public view override returns (bytes4) {
         bytes memory messageData = encodeMessageData(msg.sender, abi.encode(digest));
         bytes32 messageHash = keccak256(messageData);
+
         // try to recover through ECDSA
         (address signer, ECDSA.RecoverError error) = ECDSA.tryRecover(messageHash, signature);
-        if (error == ECDSA.RecoverError.NoError) {
-            if (_owners.contains(msg.sender, CastLib.toSetValue(signer))) {
-                return _1271_MAGIC_VALUE;
-            } else {
-                return _1271_MAGIC_VALUE_FAILURE;
-            }
-        } else {
-            if (_isValidERC1271OwnerTypeSignature(msg.sender, messageHash, signature)) {
-                return _1271_MAGIC_VALUE;
-            }
+        if (error == ECDSA.RecoverError.NoError && _owners.contains(msg.sender, CastLib.toSetValue(signer))) {
+            return _1271_MAGIC_VALUE;
+        }
+
+        if (_isValidERC1271OwnerTypeSignature(msg.sender, messageHash, signature)) {
+            return _1271_MAGIC_VALUE;
         }
 
         return _1271_MAGIC_VALUE_FAILURE;
@@ -208,20 +205,20 @@ contract MultiOwnerPlugin is BasePlugin, IMultiOwnerPlugin, IERC1271, EIP712 {
         override
         returns (uint256)
     {
-        (address signer, ECDSA.RecoverError error) =
-            (userOpHash.toEthSignedMessageHash()).tryRecover(userOp.signature);
         if (functionId == uint8(FunctionId.USER_OP_VALIDATION_OWNER)) {
-            if (error == ECDSA.RecoverError.NoError) {
-                if (isOwnerOf(msg.sender, signer)) {
-                    return _SIG_VALIDATION_PASSED;
-                }
-            } else {
-                if (_isValidERC1271OwnerTypeSignature(msg.sender, userOpHash, userOp.signature)) {
-                    return _SIG_VALIDATION_PASSED;
-                }
+            (address signer, ECDSA.RecoverError error) =
+                userOpHash.toEthSignedMessageHash().tryRecover(userOp.signature);
+            if (error == ECDSA.RecoverError.NoError && isOwnerOf(msg.sender, signer)) {
+                return _SIG_VALIDATION_PASSED;
+            }
+
+            if (_isValidERC1271OwnerTypeSignature(msg.sender, userOpHash, userOp.signature)) {
+                return _SIG_VALIDATION_PASSED;
             }
+
             return _SIG_VALIDATION_FAILED;
         }
+
         revert NotImplemented();
     }
 

diff --git a/test/mocks/ContractOwner.sol b/test/mocks/ContractOwner.sol
@@ -1,18 +1,32 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity ^0.8.21;
 
+import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
+
 import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol";
 
 contract ContractOwner is IERC1271 {
     bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e;
+    address public owner;
+
+    constructor(address _owner) {
+        owner = _owner;
+    }
 
     function sign(bytes32 digest) public pure returns (bytes memory) {
         return abi.encodePacked("Signed: ", digest);
     }
 
-    function isValidSignature(bytes32 digest, bytes memory signature) public pure override returns (bytes4) {
+    function isValidSignature(bytes32 digest, bytes memory signature) public view override returns (bytes4) {
         if (keccak256(signature) == keccak256(sign(digest))) {
+            // simple owner sig validation path
             return _1271_MAGIC_VALUE;
+        } else {
+            // EOA owner of this contractOwner path
+            (address signer,) = ECDSA.tryRecover(digest, signature);
+            if (signer == owner) {
+                return _1271_MAGIC_VALUE;
+            }
         }
         return 0xffffffff;
     }

diff --git a/test/plugin/owner/MultiOwnerPlugin.t.sol b/test/plugin/owner/MultiOwnerPlugin.t.sol
@@ -29,6 +29,8 @@ contract MultiOwnerPluginTest is Test {
     address public owner1;
     address public owner2;
     address public owner3;
+    address public ownerofContractOwner;
+    uint256 public ownerofContractOwnerKey;
     ContractOwner public contractOwner;
     address[] public ownerArray;
 
@@ -41,7 +43,8 @@ contract MultiOwnerPluginTest is Test {
         owner1 = makeAddr("owner1");
         owner2 = makeAddr("owner2");
         owner3 = makeAddr("owner3");
-        contractOwner = new ContractOwner();
+        (ownerofContractOwner, ownerofContractOwnerKey) = makeAddrAndKey("ownerofContractOwner");
+        contractOwner = new ContractOwner(ownerofContractOwner);
 
         // set up owners for accountA
         ownerArray = new address[](3);
@@ -172,6 +175,18 @@ contract MultiOwnerPluginTest is Test {
         assertEq(_1271_MAGIC_VALUE, plugin.isValidSignature(digest, signature));
     }
 
+    function testFuzz_isValidSignature_ContractOwnerWithEOAOwner(bytes32 digest) public {
+        address[] memory ownersToAdd = new address[](1);
+        ownersToAdd[0] = address(contractOwner);
+        plugin.updateOwners(ownersToAdd, new address[](0));
+
+        bytes32 messageDigest = plugin.getMessageHash(address(accountA), abi.encode(digest));
+        // owner3 is the EOA Owner of the contractOwner
+        (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerofContractOwnerKey, messageDigest);
+        bytes memory signature = abi.encodePacked(r, s, v);
+        assertEq(_1271_MAGIC_VALUE, plugin.isValidSignature(digest, signature));
+    }
+
     function test_runtimeValidationFunction_OwnerOrSelf() public {
         // should pass with owner as sender
         plugin.runtimeValidationFunction(
@@ -210,6 +225,30 @@ contract MultiOwnerPluginTest is Test {
         assertEq(resSuccess, 0);
     }
 
+    function testFuzz_userOpValidationFunction_ContractOwnerWithEOAOwner(UserOperation memory userOp) public {
+        bytes32 userOpHash = entryPoint.getUserOpHash(userOp);
+        (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerofContractOwnerKey, userOpHash);
+
+        // sig cannot cover the whole userop struct since userop struct has sig field
+        userOp.signature = abi.encodePacked(r, s, v);
+
+        // should fail without owner access
+        uint256 resFail = plugin.userOpValidationFunction(
+            uint8(IMultiOwnerPlugin.FunctionId.USER_OP_VALIDATION_OWNER), userOp, userOpHash
+        );
+        assertEq(resFail, 1);
+
+        address[] memory ownersToAdd = new address[](1);
+        ownersToAdd[0] = address(contractOwner);
+        plugin.updateOwners(ownersToAdd, new address[](0));
+
+        // should pass with owner access
+        uint256 resSuccess = plugin.userOpValidationFunction(
+            uint8(IMultiOwnerPlugin.FunctionId.USER_OP_VALIDATION_OWNER), userOp, userOpHash
+        );
+        assertEq(resSuccess, 0);
+    }
+
     function testFuzz_userOpValidationFunction_EOAOwner(string memory salt, UserOperation memory userOp) public {
         // range bound the possible set of priv keys
         (address signer, uint256 privateKey) = makeAddrAndKey(salt);