From 14546430160f6fec87fde14efb7ab8ae4e30ae4d Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Mon, 11 Dec 2023 16:13:03 -0800 Subject: [PATCH 1/2] fix: [spearbit-38] ERC-1271 signature validation with EOA owners of contract owners of MSCA --- src/plugins/owner/MultiOwnerPlugin.sol | 37 ++++++++++----------- test/mocks/ContractOwner.sol | 14 +++++++- test/plugin/owner/MultiOwnerPlugin.t.sol | 42 +++++++++++++++++++++++- 3 files changed, 71 insertions(+), 22 deletions(-) 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); From 0dff5bbe00036a4504c023c19e52358a9f6a3547 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Tue, 12 Dec 2023 13:08:23 -0800 Subject: [PATCH 2/2] clean up and add more comments in tests --- src/plugins/owner/MultiOwnerPlugin.sol | 2 +- test/mocks/ContractOwner.sol | 2 ++ test/plugin/owner/MultiOwnerPlugin.t.sol | 1 - 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/plugins/owner/MultiOwnerPlugin.sol b/src/plugins/owner/MultiOwnerPlugin.sol index aad4de6e..47588c2f 100644 --- a/src/plugins/owner/MultiOwnerPlugin.sol +++ b/src/plugins/owner/MultiOwnerPlugin.sol @@ -207,7 +207,7 @@ contract MultiOwnerPlugin is BasePlugin, IMultiOwnerPlugin, IERC1271, EIP712 { { if (functionId == uint8(FunctionId.USER_OP_VALIDATION_OWNER)) { (address signer, ECDSA.RecoverError error) = - (userOpHash.toEthSignedMessageHash()).tryRecover(userOp.signature); + userOpHash.toEthSignedMessageHash().tryRecover(userOp.signature); if (error == ECDSA.RecoverError.NoError && isOwnerOf(msg.sender, signer)) { return _SIG_VALIDATION_PASSED; } diff --git a/test/mocks/ContractOwner.sol b/test/mocks/ContractOwner.sol index 89bf4964..5ed0c3f9 100644 --- a/test/mocks/ContractOwner.sol +++ b/test/mocks/ContractOwner.sol @@ -19,8 +19,10 @@ contract ContractOwner is IERC1271 { 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; diff --git a/test/plugin/owner/MultiOwnerPlugin.t.sol b/test/plugin/owner/MultiOwnerPlugin.t.sol index ee16fee4..12f9bbd0 100644 --- a/test/plugin/owner/MultiOwnerPlugin.t.sol +++ b/test/plugin/owner/MultiOwnerPlugin.t.sol @@ -182,7 +182,6 @@ contract MultiOwnerPluginTest is Test { 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));