diff --git a/src/plugins/owner/MultiOwnerPlugin.sol b/src/plugins/owner/MultiOwnerPlugin.sol index c9404a58..aad4de6e 100644 --- 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 index f29530ac..89bf4964 100644 --- a/test/mocks/ContractOwner.sol +++ b/test/mocks/ContractOwner.sol @@ -1,18 +1,30 @@ // 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))) { return _1271_MAGIC_VALUE; + } else { + (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 index e1d507de..ee16fee4 100644 --- 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,19 @@ 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 + // bytes memory signature = owner3.sign(messageDigest); + (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 +226,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);