diff --git a/src/MultiOwnerLightAccount.sol b/src/MultiOwnerLightAccount.sol index 303e946..18300dd 100644 --- a/src/MultiOwnerLightAccount.sol +++ b/src/MultiOwnerLightAccount.sol @@ -31,21 +31,10 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { 0xaa296a366a62f6551d3ddfceae892d1791068a359a0d3461aab99dfc6c5fd700; // Signature types used for user operation validation and ERC-1271 signature validation. - // The enum value encodes the following bit layout: - // | is contract signature? | 0b_______1 - // | has address provided? | 0b______1_ - // | is transparent EIP-712? | 0b_____1__ - // | empty | 0b00000___ enum SignatureTypes { - EOA, // 0 - CONTRACT, // 1 - empty_1, // skip 2 to align bitmap - CONTRACT_WITH_ADDR, // 3 - TRANSPARENT_EOA, // 4 - TRANSPARENT_CONTRACT, // 5 - empty_2, // skip 6 to align bitmap - TRANSPARENT_CONTRACT_WITH_ADDR // 7 - + EOA, + CONTRACT, + CONTRACT_WITH_ADDR } struct LightAccountStorage { @@ -245,9 +234,25 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { override returns (bool) { - (address recovered, ECDSA.RecoverError error,) = derivedHash.tryRecover(trimmedSignature); - return (error == ECDSA.RecoverError.NoError && _getStorage().owners.contains(recovered.toSetValue())) - || _isValidContractOwnerSignatureNowLoop(derivedHash, trimmedSignature); + if (trimmedSignature.length < 1) { + return false; + } + uint8 signatureType = uint8(trimmedSignature[0]); + if (signatureType == uint8(SignatureTypes.EOA)) { + // EOA signature; + bytes memory signature = trimmedSignature[1:]; + return _isValidEOAOwnerSignature(derivedHash, signature); + } else if (signatureType == uint8(SignatureTypes.CONTRACT)) { + // Contract signature without address + bytes memory signature = trimmedSignature[1:]; + return _isValidContractOwnerSignatureNowLoop(derivedHash, signature); + } else if (signatureType == uint8(SignatureTypes.CONTRACT_WITH_ADDR)) { + // Contract signature with address + address contractOwner = address(bytes20(trimmedSignature[1:21])); + bytes memory signature = trimmedSignature[21:]; + return _isValidContractOwnerSignatureNowSingle(contractOwner, derivedHash, signature); + } + return false; } function _domainNameAndVersion() diff --git a/test/MultiOwnerLightAccount.t.sol b/test/MultiOwnerLightAccount.t.sol index d9cc370..26d6f2b 100644 --- a/test/MultiOwnerLightAccount.t.sol +++ b/test/MultiOwnerLightAccount.t.sol @@ -145,10 +145,7 @@ contract MultiOwnerLightAccountTest is Test { } function testFuzz_rejectsUserOpsWithInvalidSignatureType(uint8 signatureType) public { - // Valid values are 0,1,3 - if (signatureType != 2) { - signatureType = uint8(bound(signatureType, 4, type(uint8).max)); - } + signatureType = uint8(bound(signatureType, 3, type(uint8).max)); PackedUserOperation memory op = _getUnsignedOp( abi.encodeCall(BaseLightAccount.execute, (address(lightSwitch), 0, abi.encodeCall(LightSwitch.turnOn, ()))) @@ -410,7 +407,11 @@ contract MultiOwnerLightAccountTest is Test { function testIsValidSignatureForEoaOwner() public { bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); bytes memory signature = abi.encodePacked( - _sign(EOA_PRIVATE_KEY, _toERC1271Hash(child)), _PARENT_TYPEHASH, _domainSeparatorB(), child + MultiOwnerLightAccount.SignatureTypes.EOA, + _sign(EOA_PRIVATE_KEY, _toERC1271Hash(child)), + _PARENT_TYPEHASH, + _domainSeparatorB(), + child ); assertEq( account.isValidSignature(_toChildHash(child), signature), @@ -418,47 +419,124 @@ contract MultiOwnerLightAccountTest is Test { ); } - function testIsValidSignatureForContractOwner() public { + function testIsValidSignatureForContractOwnerUnspecified() public { _useContractOwner(); bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); - bytes memory signature = - abi.encodePacked(contractOwner.sign(_toERC1271Hash(child)), _PARENT_TYPEHASH, _domainSeparatorB(), child); + bytes memory signature = abi.encodePacked( + MultiOwnerLightAccount.SignatureTypes.CONTRACT, + contractOwner.sign(_toERC1271Hash(child)), + _PARENT_TYPEHASH, + _domainSeparatorB(), + child + ); assertEq( account.isValidSignature(_toChildHash(child), signature), bytes4(keccak256("isValidSignature(bytes32,bytes)")) ); } - function testIsValidSignatureRejectsInvalid() public { + function testIsValidSignatureForContractOwnerSpecified() public { + _useContractOwner(); bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); - bytes memory signature = - abi.encodePacked(_sign(123, _toERC1271Hash(child)), _PARENT_TYPEHASH, _domainSeparatorB(), child); + bytes memory signature = abi.encodePacked( + MultiOwnerLightAccount.SignatureTypes.CONTRACT_WITH_ADDR, + contractOwner, + contractOwner.sign(_toERC1271Hash(child)), + _PARENT_TYPEHASH, + _domainSeparatorB(), + child + ); + assertEq( + account.isValidSignature(_toChildHash(child), signature), + bytes4(keccak256("isValidSignature(bytes32,bytes)")) + ); + } + + function testIsValidSignatureRejectsInvalidEOA() public { + bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); + bytes memory signature = abi.encodePacked( + MultiOwnerLightAccount.SignatureTypes.EOA, + _sign(123, _toERC1271Hash(child)), + _PARENT_TYPEHASH, + _domainSeparatorB(), + child + ); assertEq(account.isValidSignature(_toChildHash(child), signature), bytes4(0xffffffff)); signature = abi.encodePacked( - _sign(EOA_PRIVATE_KEY, _toERC1271Hash(child)), _PARENT_TYPEHASH, _domainSeparatorA(), child + MultiOwnerLightAccount.SignatureTypes.EOA, + _sign(EOA_PRIVATE_KEY, _toERC1271Hash(child)), + _PARENT_TYPEHASH, + _domainSeparatorA(), + child ); assertEq(account.isValidSignature(_toChildHash(child), signature), bytes4(0xffffffff)); - assertEq(account.isValidSignature(_toChildHash(child), ""), bytes4(0xffffffff)); + assertEq( + account.isValidSignature(_toChildHash(child), abi.encodePacked(MultiOwnerLightAccount.SignatureTypes.EOA)), + bytes4(0xffffffff) + ); + } + + function testIsValidSignatureRejectsInvalidContractOwner() public { + // Signature should fail, because the contract owner is not an owner + bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); + bytes memory signature = abi.encodePacked( + MultiOwnerLightAccount.SignatureTypes.CONTRACT, + contractOwner.sign(_toERC1271Hash(child)), + _PARENT_TYPEHASH, + _domainSeparatorB(), + child + ); + assertEq(account.isValidSignature(_toChildHash(child), signature), bytes4(0xffffffff)); + } + + function testFuzz_IsValidSignatureRejectsInvalidSignatureType(uint8 signatureType) public { + signatureType = uint8(bound(signatureType, 3, type(uint8).max)); + + bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); + bytes memory signature = abi.encodePacked( + signatureType, _sign(EOA_PRIVATE_KEY, _toERC1271Hash(child)), _PARENT_TYPEHASH, _domainSeparatorB(), child + ); + assertEq(account.isValidSignature(_toChildHash(child), signature), bytes4(0xffffffff)); } function testIsValidSignaturePersonalSign() public { string memory message = "hello world"; bytes32 childHash = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n", bytes(message).length, message)); - bytes memory signature = - abi.encodePacked(_sign(EOA_PRIVATE_KEY, _toERC1271HashPersonalSign(childHash)), _PARENT_TYPEHASH); + bytes memory signature = abi.encodePacked( + MultiOwnerLightAccount.SignatureTypes.EOA, + _sign(EOA_PRIVATE_KEY, _toERC1271HashPersonalSign(childHash)), + _PARENT_TYPEHASH + ); + assertEq(account.isValidSignature(childHash, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)"))); + } + + function testIsValidSignaturePersonalSignForContractOwnerUnspecified() public { + _useContractOwner(); + string memory message = "hello world"; + bytes32 childHash = + keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n", bytes(message).length, message)); + bytes memory signature = abi.encodePacked( + MultiOwnerLightAccount.SignatureTypes.CONTRACT, + contractOwner.sign(_toERC1271HashPersonalSign(childHash)), + _PARENT_TYPEHASH + ); assertEq(account.isValidSignature(childHash, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)"))); } - function testIsValidSignaturePersonalSignForContractOwner() public { + function testIsValidSignaturePersonalSignForContractOwnerSpecified() public { _useContractOwner(); string memory message = "hello world"; bytes32 childHash = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n", bytes(message).length, message)); - bytes memory signature = - abi.encodePacked(contractOwner.sign(_toERC1271HashPersonalSign(childHash)), _PARENT_TYPEHASH); + bytes memory signature = abi.encodePacked( + MultiOwnerLightAccount.SignatureTypes.CONTRACT_WITH_ADDR, + contractOwner, + contractOwner.sign(_toERC1271HashPersonalSign(childHash)), + _PARENT_TYPEHASH + ); assertEq(account.isValidSignature(childHash, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)"))); } @@ -466,10 +544,15 @@ contract MultiOwnerLightAccountTest is Test { string memory message = "hello world"; bytes32 childHash = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n", bytes(message).length, message)); - bytes memory signature = abi.encodePacked(_sign(123, _toERC1271HashPersonalSign(childHash)), _PARENT_TYPEHASH); + bytes memory signature = abi.encodePacked( + MultiOwnerLightAccount.SignatureTypes.EOA, + _sign(123, _toERC1271HashPersonalSign(childHash)), + _PARENT_TYPEHASH + ); assertEq(account.isValidSignature(childHash, signature), bytes4(0xffffffff)); signature = abi.encodePacked( + MultiOwnerLightAccount.SignatureTypes.EOA, _sign(EOA_PRIVATE_KEY, _toERC1271HashPersonalSign(childHash)), _PARENT_TYPEHASH, _domainSeparatorB(), @@ -477,7 +560,10 @@ contract MultiOwnerLightAccountTest is Test { ); assertEq(account.isValidSignature(childHash, signature), bytes4(0xffffffff)); - assertEq(account.isValidSignature(childHash, ""), bytes4(0xffffffff)); + assertEq( + account.isValidSignature(childHash, abi.encodePacked(MultiOwnerLightAccount.SignatureTypes.EOA)), + bytes4(0xffffffff) + ); } function testOwnerCanUpgrade() public { @@ -579,7 +665,7 @@ contract MultiOwnerLightAccountTest is Test { bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032))) ) ), - 0x0a14a7d2e0fb6858a618de8e4a7cf52bd1cb8dad3f4adbf243078a4db5f13c92 + 0xead9408e740524880066371b7b1590829216c843719ab5a102236d1bb9d80ea1 ); }