diff --git a/src/MultiOwnerLightAccount.sol b/src/MultiOwnerLightAccount.sol index e10bfde..303e946 100644 --- a/src/MultiOwnerLightAccount.sol +++ b/src/MultiOwnerLightAccount.sol @@ -4,9 +4,9 @@ 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 {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"; @@ -30,6 +30,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 + 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 + + } + struct LightAccountStorage { LinkedListSet owners; } @@ -54,6 +72,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 +159,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 +228,14 @@ 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) { + 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 /// valid ERC-1271 signature from the owner (if the owner is a contract). function _isValidSignature(bytes32 derivedHash, bytes calldata trimmedSignature) @@ -173,7 +247,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..d9cc370 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,60 @@ 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 { + // 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, ()))) + ); + 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 +395,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 +579,7 @@ contract MultiOwnerLightAccountTest is Test { bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032))) ) ), - 0x3765ef442bfa3b5ef7a30073b39949186919dd2344bb5aa0736a0e0be66ebfe1 + 0x0a14a7d2e0fb6858a618de8e4a7cf52bd1cb8dad3f4adbf243078a4db5f13c92 ); } @@ -536,7 +616,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; }