From 94d450b2359f8e053aada3cb95ee8c5c5e67144c Mon Sep 17 00:00:00 2001 From: adam Date: Thu, 14 Mar 2024 21:37:01 -0400 Subject: [PATCH 1/3] Add signature enum to user operation signature validation --- src/MultiOwnerLightAccount.sol | 99 +++++++++++++++++++++++++++---- test/MultiOwnerLightAccount.t.sol | 88 +++++++++++++++++++++++++-- 2 files changed, 171 insertions(+), 16 deletions(-) diff --git a/src/MultiOwnerLightAccount.sol b/src/MultiOwnerLightAccount.sol index e10bfde..2e7528a 100644 --- a/src/MultiOwnerLightAccount.sol +++ b/src/MultiOwnerLightAccount.sol @@ -4,7 +4,6 @@ pragma solidity ^0.8.23; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; -import {SIG_VALIDATION_FAILED} from "account-abstraction/core/Helpers.sol"; import {IEntryPoint} from "account-abstraction/interfaces/IEntryPoint.sol"; import {PackedUserOperation} from "account-abstraction/interfaces/PackedUserOperation.sol"; import {CastLib} from "modular-account/helpers/CastLib.sol"; @@ -30,6 +29,24 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { bytes32 internal constant _INITIALIZABLE_STORAGE_POSITION = 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 + CONTRACT_WITH_ADDR, // 2 + empty_1, // skip 3 to align bitmap + TRANSPARENT_EOA, // 4 + TRANSPARENT_CONTRACT, // 5 + empty_2, // skip 6 to align bitmap + TRANSPARENT_CONTRACT_WITH_ADDR // 7 + + } + struct LightAccountStorage { LinkedListSet owners; } @@ -54,6 +71,9 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { /// @dev The owner to be removed does not exist. error OwnerDoesNotExist(address owner); + /// @dev The signature type provided is not valid. + error InvalidSignatureType(); + constructor(IEntryPoint entryPoint_) CustomSlotInitializable(_INITIALIZABLE_STORAGE_POSITION) { _ENTRY_POINT = entryPoint_; _disableInitializers(); @@ -138,19 +158,64 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { override returns (uint256 validationData) { - bytes32 signedHash = userOpHash.toEthSignedMessageHash(); - bytes memory signature = userOp.signature; - (address recovered, ECDSA.RecoverError error,) = signedHash.tryRecover(signature); - if (error == ECDSA.RecoverError.NoError && _getStorage().owners.contains(recovered.toSetValue())) { - return 0; - } - if (_isValidERC1271SignatureNow(userOpHash, signature)) { - return 0; + uint8 signatureType = uint8(userOp.signature[0]); + if (signatureType == uint8(SignatureTypes.EOA)) { + // EOA signature + bytes32 signedHash = userOpHash.toEthSignedMessageHash(); + bytes memory signature = userOp.signature[1:]; + return _successToValidationData(_isValidEOAOwnerSignature(signedHash, signature)); + } else if (signatureType == uint8(SignatureTypes.CONTRACT)) { + // Contract signature without address + bytes memory signature = userOp.signature[1:]; + return _successToValidationData(_isValidContractOwnerSignatureNowLoop(userOpHash, signature)); + } else if (signatureType == uint8(SignatureTypes.CONTRACT_WITH_ADDR)) { + // Contract signature with address + address contractOwner = address(bytes20(userOp.signature[1:21])); + bytes memory signature = userOp.signature[21:]; + return + _successToValidationData(_isValidContractOwnerSignatureNowSingle(contractOwner, userOpHash, signature)); } - return SIG_VALIDATION_FAILED; + + revert InvalidSignatureType(); + } + + /// @notice Check if the signature is a valid by an EOA owner for the given digest. + /// @dev Only supports 65-byte signatures, and uses the digest directly. + /// @param digest The digest to be checked. + /// @param signature The signature to be checked. + /// @return True if the signature is valid and by an owner, false otherwise. + function _isValidEOAOwnerSignature(bytes32 digest, bytes memory signature) internal view returns (bool) { + (address recovered, ECDSA.RecoverError error,) = digest.tryRecover(signature); + return error == ECDSA.RecoverError.NoError && _getStorage().owners.contains(recovered.toSetValue()); + } + + /// @notice Check if the given verifier is a contract owner, and if the signature is a valid ERC-1271 signature by + /// a contract owner for the given digest. + /// @param contractOwner The address of the contract owner. + /// @param digest The digest to be checked. + /// @param signature The signature to be checked. + /// @return True if the signature is valid and by an owner, false otherwise. + function _isValidContractOwnerSignatureNowSingle(address contractOwner, bytes32 digest, bytes memory signature) + internal + view + returns (bool) + { + return SignatureChecker.isValidERC1271SignatureNow(contractOwner, digest, signature) + && _getStorage().owners.contains(contractOwner.toSetValue()); } - function _isValidERC1271SignatureNow(bytes32 digest, bytes memory signature) internal view returns (bool) { + /// @notice Check if the signature is a valid ERC-1271 signature by a contract owner for the given digest by + /// checking all owners in a loop. + /// @dev Susceptible to denial-of-service by a malicious owner contract. To avoid this, use a signature type that + /// includes the owner address. + /// @param digest The digest to be checked. + /// @param signature The signature to be checked. + /// @return True if the signature is valid and by an owner, false otherwise. + function _isValidContractOwnerSignatureNowLoop(bytes32 digest, bytes memory signature) + internal + view + returns (bool) + { LightAccountStorage storage _storage = _getStorage(); address[] memory owners_ = _storage.owners.getAll().toAddressArray(); uint256 length = owners_.length; @@ -162,6 +227,16 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { return false; } + /// @dev Convert a boolean success value to a validation data value. + /// @param success The success value to be converted. + /// @return validationData The validation data value. 0 if success is true, 1 (SIG_VALIDATION_FAILED) if + /// success is false. + function _successToValidationData(bool success) internal pure returns (uint256 validationData) { + assembly ("memory-safe") { + validationData := iszero(success) + } + } + /// @dev The signature is valid if it is signed by the owner's private key (if the owner is an EOA) or if it is a /// valid ERC-1271 signature from the owner (if the owner is a contract). function _isValidSignature(bytes32 derivedHash, bytes calldata trimmedSignature) @@ -173,7 +248,7 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { { (address recovered, ECDSA.RecoverError error,) = derivedHash.tryRecover(trimmedSignature); return (error == ECDSA.RecoverError.NoError && _getStorage().owners.contains(recovered.toSetValue())) - || _isValidERC1271SignatureNow(derivedHash, trimmedSignature); + || _isValidContractOwnerSignatureNowLoop(derivedHash, trimmedSignature); } function _domainNameAndVersion() diff --git a/test/MultiOwnerLightAccount.t.sol b/test/MultiOwnerLightAccount.t.sol index 3d5a4af..47f2578 100644 --- a/test/MultiOwnerLightAccount.t.sol +++ b/test/MultiOwnerLightAccount.t.sol @@ -72,12 +72,30 @@ contract MultiOwnerLightAccountTest is Test { assertTrue(lightSwitch.on()); } - function testExecutedCanBeCalledByEntryPointWithContractOwner() public { + function testExecuteCanBeCalledByEntryPointWithContractOwnerUnspecified() public { _useContractOwner(); PackedUserOperation memory op = _getUnsignedOp( abi.encodeCall(BaseLightAccount.execute, (address(lightSwitch), 0, abi.encodeCall(LightSwitch.turnOn, ()))) ); - op.signature = contractOwner.sign(entryPoint.getUserOpHash(op)); + op.signature = abi.encodePacked( + MultiOwnerLightAccount.SignatureTypes.CONTRACT, contractOwner.sign(entryPoint.getUserOpHash(op)) + ); + PackedUserOperation[] memory ops = new PackedUserOperation[](1); + ops[0] = op; + entryPoint.handleOps(ops, BENEFICIARY); + assertTrue(lightSwitch.on()); + } + + function testExecuteCanBeCalledByEntryPointWithContractOwnerSpecified() public { + _useContractOwner(); + PackedUserOperation memory op = _getUnsignedOp( + abi.encodeCall(BaseLightAccount.execute, (address(lightSwitch), 0, abi.encodeCall(LightSwitch.turnOn, ()))) + ); + op.signature = abi.encodePacked( + MultiOwnerLightAccount.SignatureTypes.CONTRACT_WITH_ADDR, + contractOwner, + contractOwner.sign(entryPoint.getUserOpHash(op)) + ); PackedUserOperation[] memory ops = new PackedUserOperation[](1); ops[0] = op; entryPoint.handleOps(ops, BENEFICIARY); @@ -95,6 +113,57 @@ contract MultiOwnerLightAccountTest is Test { entryPoint.handleOps(ops, BENEFICIARY); } + function testRejectsUserOpWithInvalidContractOwnerSpecified() public { + PackedUserOperation memory op = _getUnsignedOp( + abi.encodeCall(BaseLightAccount.execute, (address(lightSwitch), 0, abi.encodeCall(LightSwitch.turnOn, ()))) + ); + op.signature = abi.encodePacked( + MultiOwnerLightAccount.SignatureTypes.CONTRACT_WITH_ADDR, + contractOwner, + contractOwner.sign(entryPoint.getUserOpHash(op)) + ); + PackedUserOperation[] memory ops = new PackedUserOperation[](1); + ops[0] = op; + vm.expectRevert(abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error")); + entryPoint.handleOps(ops, BENEFICIARY); + assertFalse(lightSwitch.on()); + } + + function testRejectsUserOpWithPartialContractOwnerSpecified() public { + _useContractOwner(); + PackedUserOperation memory op = _getUnsignedOp( + abi.encodeCall(BaseLightAccount.execute, (address(lightSwitch), 0, abi.encodeCall(LightSwitch.turnOn, ()))) + ); + op.signature = abi.encodePacked( + MultiOwnerLightAccount.SignatureTypes.CONTRACT_WITH_ADDR, bytes10(bytes20(address(contractOwner))) + ); + PackedUserOperation[] memory ops = new PackedUserOperation[](1); + ops[0] = op; + vm.expectRevert(abi.encodeWithSelector(IEntryPoint.FailedOpWithRevert.selector, 0, "AA23 reverted", bytes(""))); + entryPoint.handleOps(ops, BENEFICIARY); + assertFalse(lightSwitch.on()); + } + + function testFuzz_rejectsUserOpsWithInvalidSignatureType(uint8 signatureType) public { + signatureType = uint8(bound(signatureType, 3, type(uint8).max)); + + PackedUserOperation memory op = _getUnsignedOp( + abi.encodeCall(BaseLightAccount.execute, (address(lightSwitch), 0, abi.encodeCall(LightSwitch.turnOn, ()))) + ); + op.signature = abi.encodePacked(signatureType); + PackedUserOperation[] memory ops = new PackedUserOperation[](1); + ops[0] = op; + vm.expectRevert( + abi.encodeWithSelector( + IEntryPoint.FailedOpWithRevert.selector, + 0, + "AA23 reverted", + abi.encodePacked(MultiOwnerLightAccount.InvalidSignatureType.selector) + ) + ); + entryPoint.handleOps(ops, BENEFICIARY); + } + function testExecuteCannotBeCalledByRandos() public { vm.expectRevert(abi.encodeWithSelector(BaseLightAccount.NotAuthorized.selector, (address(this)))); account.execute(address(lightSwitch), 0, abi.encodeCall(LightSwitch.turnOn, ())); @@ -323,6 +392,14 @@ contract MultiOwnerLightAccountTest is Test { assertEq(owners[0], eoaAddress); } + function testRemoveNonexistantOwner() public { + address[] memory ownersToRemove = new address[](1); + ownersToRemove[0] = address(0x100); + vm.prank(eoaAddress); + vm.expectRevert(abi.encodeWithSelector(MultiOwnerLightAccount.OwnerDoesNotExist.selector, (address(0x100)))); + account.updateOwners(new address[](0), ownersToRemove); + } + function testEntryPointGetter() public { assertEq(address(account.entryPoint()), address(entryPoint)); } @@ -499,7 +576,7 @@ contract MultiOwnerLightAccountTest is Test { bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032))) ) ), - 0x3765ef442bfa3b5ef7a30073b39949186919dd2344bb5aa0736a0e0be66ebfe1 + 0xc673ee55a4508417eef16886472aeca3661be5e66bf59521dee374cf0021fa13 ); } @@ -536,7 +613,10 @@ contract MultiOwnerLightAccountTest is Test { returns (PackedUserOperation memory) { PackedUserOperation memory op = _getUnsignedOp(callData); - op.signature = _sign(privateKey, entryPoint.getUserOpHash(op).toEthSignedMessageHash()); + op.signature = abi.encodePacked( + MultiOwnerLightAccount.SignatureTypes.EOA, + _sign(privateKey, entryPoint.getUserOpHash(op).toEthSignedMessageHash()) + ); return op; } From 465233c85c10aa67bde8d3a583bd426abe31fe61 Mon Sep 17 00:00:00 2001 From: adam Date: Fri, 15 Mar 2024 11:14:46 -0400 Subject: [PATCH 2/3] remove unnecessary assembly --- src/MultiOwnerLightAccount.sol | 5 ++--- test/MultiOwnerLightAccount.t.sol | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/MultiOwnerLightAccount.sol b/src/MultiOwnerLightAccount.sol index 2e7528a..2517253 100644 --- a/src/MultiOwnerLightAccount.sol +++ b/src/MultiOwnerLightAccount.sol @@ -6,6 +6,7 @@ import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/Messa import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; import {IEntryPoint} from "account-abstraction/interfaces/IEntryPoint.sol"; import {PackedUserOperation} from "account-abstraction/interfaces/PackedUserOperation.sol"; +import {SIG_VALIDATION_FAILED, SIG_VALIDATION_SUCCESS} from "account-abstraction/core/Helpers.sol"; import {CastLib} from "modular-account/helpers/CastLib.sol"; import {SetValue} from "modular-account/libraries/Constants.sol"; import {LinkedListSet, LinkedListSetLib} from "modular-account/libraries/LinkedListSetLib.sol"; @@ -232,9 +233,7 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { /// @return validationData The validation data value. 0 if success is true, 1 (SIG_VALIDATION_FAILED) if /// success is false. function _successToValidationData(bool success) internal pure returns (uint256 validationData) { - assembly ("memory-safe") { - validationData := iszero(success) - } + return success ? SIG_VALIDATION_SUCCESS : SIG_VALIDATION_FAILED; } /// @dev The signature is valid if it is signed by the owner's private key (if the owner is an EOA) or if it is a diff --git a/test/MultiOwnerLightAccount.t.sol b/test/MultiOwnerLightAccount.t.sol index 47f2578..e812b9f 100644 --- a/test/MultiOwnerLightAccount.t.sol +++ b/test/MultiOwnerLightAccount.t.sol @@ -576,7 +576,7 @@ contract MultiOwnerLightAccountTest is Test { bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032))) ) ), - 0xc673ee55a4508417eef16886472aeca3661be5e66bf59521dee374cf0021fa13 + 0x3b81b3fa5e00c31f57c12db971bb4fbf4d1eee382ae26b211b2165efbbe07437 ); } From b4d7b1a1e4afdaa42957a5a975c75381dbe53b1a Mon Sep 17 00:00:00 2001 From: adam Date: Fri, 15 Mar 2024 11:30:18 -0400 Subject: [PATCH 3/3] Correct enum assignments to match bitmap --- src/MultiOwnerLightAccount.sol | 4 ++-- test/MultiOwnerLightAccount.t.sol | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/MultiOwnerLightAccount.sol b/src/MultiOwnerLightAccount.sol index 2517253..303e946 100644 --- a/src/MultiOwnerLightAccount.sol +++ b/src/MultiOwnerLightAccount.sol @@ -39,8 +39,8 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { enum SignatureTypes { EOA, // 0 CONTRACT, // 1 - CONTRACT_WITH_ADDR, // 2 - empty_1, // skip 3 to align bitmap + empty_1, // skip 2 to align bitmap + CONTRACT_WITH_ADDR, // 3 TRANSPARENT_EOA, // 4 TRANSPARENT_CONTRACT, // 5 empty_2, // skip 6 to align bitmap diff --git a/test/MultiOwnerLightAccount.t.sol b/test/MultiOwnerLightAccount.t.sol index e812b9f..d9cc370 100644 --- a/test/MultiOwnerLightAccount.t.sol +++ b/test/MultiOwnerLightAccount.t.sol @@ -145,7 +145,10 @@ contract MultiOwnerLightAccountTest is Test { } function testFuzz_rejectsUserOpsWithInvalidSignatureType(uint8 signatureType) public { - signatureType = uint8(bound(signatureType, 3, type(uint8).max)); + // Valid values are 0,1,3 + if (signatureType != 2) { + signatureType = uint8(bound(signatureType, 4, type(uint8).max)); + } PackedUserOperation memory op = _getUnsignedOp( abi.encodeCall(BaseLightAccount.execute, (address(lightSwitch), 0, abi.encodeCall(LightSwitch.turnOn, ()))) @@ -576,7 +579,7 @@ contract MultiOwnerLightAccountTest is Test { bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032))) ) ), - 0x3b81b3fa5e00c31f57c12db971bb4fbf4d1eee382ae26b211b2165efbbe07437 + 0x0a14a7d2e0fb6858a618de8e4a7cf52bd1cb8dad3f4adbf243078a4db5f13c92 ); }