diff --git a/src/LightAccount.sol b/src/LightAccount.sol index 27731cd..bdfb326 100644 --- a/src/LightAccount.sol +++ b/src/LightAccount.sol @@ -1,10 +1,6 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.23; -/* solhint-disable avoid-low-level-calls */ -/* solhint-disable no-inline-assembly */ -/* solhint-disable reason-string */ - 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"; @@ -124,6 +120,9 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable { override returns (uint256 validationData) { + if (userOp.signature.length < 1) { + revert InvalidSignatureType(); + } uint8 signatureType = uint8(userOp.signature[0]); if (signatureType == uint8(SignatureType.EOA)) { // EOA signature @@ -135,18 +134,17 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable { bytes memory signature = userOp.signature[1:]; return _successToValidationData(_isValidContractOwnerSignatureNow(userOpHash, signature)); } - revert InvalidSignatureType(); } /// @notice Check if the signature is a valid by the EOA owner for the given digest. - /// @dev Only supports 65-byte signatures, and uses the digest directly. + /// @dev Only supports 65-byte signatures, and uses the digest directly. Reverts if the signature is malformed. /// @param digest The digest to be checked. /// @param signature The signature to be checked. /// @return True if the signature is valid and by the 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 && recovered == owner(); + address recovered = digest.recover(signature); + return recovered == owner(); } /// @notice Check if the signature is a valid ERC-1271 signature by a contract owner for the given digest. @@ -158,7 +156,7 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable { } /// @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). + /// valid ERC-1271 signature from the owner (if the owner is a contract). Reverts if the signature is malformed. function _isValidSignature(bytes32 derivedHash, bytes calldata trimmedSignature) internal view @@ -167,7 +165,7 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable { returns (bool) { if (trimmedSignature.length < 1) { - return false; + revert InvalidSignatureType(); } uint8 signatureType = uint8(trimmedSignature[0]); if (signatureType == uint8(SignatureType.EOA)) { @@ -179,7 +177,7 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable { bytes memory signature = trimmedSignature[1:]; return _isValidContractOwnerSignatureNow(derivedHash, signature); } - return false; + revert InvalidSignatureType(); } function _domainNameAndVersion() diff --git a/src/MultiOwnerLightAccount.sol b/src/MultiOwnerLightAccount.sol index 67949dc..b884f26 100644 --- a/src/MultiOwnerLightAccount.sol +++ b/src/MultiOwnerLightAccount.sol @@ -137,6 +137,9 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { override returns (uint256 validationData) { + if (userOp.signature.length < 1) { + revert InvalidSignatureType(); + } uint8 signatureType = uint8(userOp.signature[0]); if (signatureType == uint8(SignatureType.EOA)) { // EOA signature @@ -154,18 +157,17 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { return _successToValidationData(_isValidContractOwnerSignatureNowSingle(contractOwner, userOpHash, signature)); } - 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. + /// @dev Only supports 65-byte signatures, and uses the digest directly. Reverts if the signature is malformed. /// @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()); + address recovered = digest.recover(signature); + return _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 @@ -207,7 +209,7 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { } /// @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). + /// valid ERC-1271 signature from the owner (if the owner is a contract). Reverts if the signature is malformed. function _isValidSignature(bytes32 derivedHash, bytes calldata trimmedSignature) internal view @@ -216,7 +218,7 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { returns (bool) { if (trimmedSignature.length < 1) { - return false; + revert InvalidSignatureType(); } uint8 signatureType = uint8(trimmedSignature[0]); if (signatureType == uint8(SignatureType.EOA)) { @@ -233,7 +235,7 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { bytes memory signature = trimmedSignature[21:]; return _isValidContractOwnerSignatureNowSingle(contractOwner, derivedHash, signature); } - return false; + revert InvalidSignatureType(); } function _domainNameAndVersion() diff --git a/test/CustomSlotInitializable.t.sol b/test/CustomSlotInitializable.t.sol index f7165c9..6df24ef 100644 --- a/test/CustomSlotInitializable.t.sol +++ b/test/CustomSlotInitializable.t.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.23; import "forge-std/Test.sol"; diff --git a/test/LightAccount.t.sol b/test/LightAccount.t.sol index ce39117..516803b 100644 --- a/test/LightAccount.t.sol +++ b/test/LightAccount.t.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.23; import "forge-std/Test.sol"; @@ -6,7 +6,6 @@ import "forge-std/Test.sol"; import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; - import {EntryPoint} from "account-abstraction/core/EntryPoint.sol"; import {IEntryPoint} from "account-abstraction/interfaces/IEntryPoint.sol"; import {PackedUserOperation} from "account-abstraction/interfaces/PackedUserOperation.sol"; @@ -113,6 +112,47 @@ contract LightAccountTest is Test { entryPoint.handleOps(ops, BENEFICIARY); } + function testRevertsUserOpsWithMalformedSignature() public { + PackedUserOperation memory op = _getSignedOp( + abi.encodeCall(BaseLightAccount.execute, (address(lightSwitch), 0, abi.encodeCall(LightSwitch.turnOn, ()))), + 1234 + ); + op.signature = abi.encodePacked(BaseLightAccount.SignatureType.EOA, hex"aaaa"); + PackedUserOperation[] memory ops = new PackedUserOperation[](1); + ops[0] = op; + vm.expectRevert( + abi.encodeWithSelector( + IEntryPoint.FailedOpWithRevert.selector, + 0, + "AA23 reverted", + abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, (op.signature.length - 1)) + ) + ); + entryPoint.handleOps(ops, BENEFICIARY); + + op.signature = abi.encodePacked(uint8(3)); + vm.expectRevert( + abi.encodeWithSelector( + IEntryPoint.FailedOpWithRevert.selector, + 0, + "AA23 reverted", + abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector) + ) + ); + entryPoint.handleOps(ops, BENEFICIARY); + + op.signature = hex""; + vm.expectRevert( + abi.encodeWithSelector( + IEntryPoint.FailedOpWithRevert.selector, + 0, + "AA23 reverted", + abi.encodeWithSelector(BaseLightAccount.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, ())); @@ -334,16 +374,30 @@ contract LightAccountTest is Test { function testIsValidSignatureRejectsInvalid() public { 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( + BaseLightAccount.SignatureType.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 + BaseLightAccount.SignatureType.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)); + // ERC1271.isValidSignature only truncates 32 bytes (since the wrong domain separator was used) before passing it on to _isValidSignature + // _isValidSignature truncates the SignatureType byte before ecrecover + vm.expectRevert(abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, (signature.length - 32 - 1))); + account.isValidSignature(_toChildHash(child), signature); + + vm.expectRevert(abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector)); + account.isValidSignature(_toChildHash(child), abi.encodePacked(BaseLightAccount.SignatureType.EOA)); } function testIsValidSignaturePersonalSign() public { @@ -375,18 +429,26 @@ contract LightAccountTest 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( + BaseLightAccount.SignatureType.EOA, _sign(123, _toERC1271HashPersonalSign(childHash)), _PARENT_TYPEHASH + ); assertEq(account.isValidSignature(childHash, signature), bytes4(0xffffffff)); signature = abi.encodePacked( + BaseLightAccount.SignatureType.EOA, _sign(EOA_PRIVATE_KEY, _toERC1271HashPersonalSign(childHash)), _PARENT_TYPEHASH, _domainSeparatorB(), childHash ); - assertEq(account.isValidSignature(childHash, signature), bytes4(0xffffffff)); - assertEq(account.isValidSignature(childHash, ""), bytes4(0xffffffff)); + // ERC1271.isValidSignature only truncates 32 bytes for personal_sign before passing it on to _isValidSignature + // _isValidSignature truncates the SignatureType byte before ecrecover + vm.expectRevert(abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, (signature.length - 32 - 1))); + account.isValidSignature(childHash, signature); + + vm.expectRevert(abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector)); + account.isValidSignature(childHash, ""); } function testOwnerCanUpgrade() public { @@ -487,7 +549,7 @@ contract LightAccountTest is Test { bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032))) ) ), - 0x10f16a2b2ab8a4a6c680a35494382da995eea00c40bed1fc5391774da7de7fc9 + 0x9861f47fa11c7176759734bb81b2c2c764ce9219f757a6aa77774550dfe37f33 ); } diff --git a/test/LightAccountFactory.t.sol b/test/LightAccountFactory.t.sol index 487d4fc..efba7ed 100644 --- a/test/LightAccountFactory.t.sol +++ b/test/LightAccountFactory.t.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.23; import "forge-std/Test.sol"; diff --git a/test/MultiOwnerLightAccount.t.sol b/test/MultiOwnerLightAccount.t.sol index 42146e3..479600b 100644 --- a/test/MultiOwnerLightAccount.t.sol +++ b/test/MultiOwnerLightAccount.t.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.23; import "forge-std/Test.sol"; @@ -163,6 +163,47 @@ contract MultiOwnerLightAccountTest is Test { entryPoint.handleOps(ops, BENEFICIARY); } + function testRevertsUserOpsWithMalformedSignature() public { + PackedUserOperation memory op = _getSignedOp( + abi.encodeCall(BaseLightAccount.execute, (address(lightSwitch), 0, abi.encodeCall(LightSwitch.turnOn, ()))), + 1234 + ); + op.signature = abi.encodePacked(BaseLightAccount.SignatureType.EOA, hex"aaaa"); + PackedUserOperation[] memory ops = new PackedUserOperation[](1); + ops[0] = op; + vm.expectRevert( + abi.encodeWithSelector( + IEntryPoint.FailedOpWithRevert.selector, + 0, + "AA23 reverted", + abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, (op.signature.length - 1)) + ) + ); + entryPoint.handleOps(ops, BENEFICIARY); + + op.signature = abi.encodePacked(uint8(3)); + vm.expectRevert( + abi.encodeWithSelector( + IEntryPoint.FailedOpWithRevert.selector, + 0, + "AA23 reverted", + abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector) + ) + ); + entryPoint.handleOps(ops, BENEFICIARY); + + op.signature = hex""; + vm.expectRevert( + abi.encodeWithSelector( + IEntryPoint.FailedOpWithRevert.selector, + 0, + "AA23 reverted", + abi.encodeWithSelector(BaseLightAccount.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, ())); @@ -469,12 +510,14 @@ contract MultiOwnerLightAccountTest is Test { _domainSeparatorA(), child ); - assertEq(account.isValidSignature(_toChildHash(child), signature), bytes4(0xffffffff)); - assertEq( - account.isValidSignature(_toChildHash(child), abi.encodePacked(BaseLightAccount.SignatureType.EOA)), - bytes4(0xffffffff) - ); + // ERC1271.isValidSignature only truncates 32 bytes (since the wrong domain separator was used) before passing it on to _isValidSignature + // _isValidSignature truncates the SignatureType byte before ecrecover + vm.expectRevert(abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, (signature.length - 32 - 1))); + account.isValidSignature(_toChildHash(child), signature); + + vm.expectRevert(abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector)); + account.isValidSignature(_toChildHash(child), abi.encodePacked(BaseLightAccount.SignatureType.EOA)); } function testIsValidSignatureRejectsInvalidContractOwner() public { @@ -490,14 +533,15 @@ contract MultiOwnerLightAccountTest is Test { assertEq(account.isValidSignature(_toChildHash(child), signature), bytes4(0xffffffff)); } - function testFuzz_IsValidSignatureRejectsInvalidSignatureType(uint8 signatureType) public { + 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)); + vm.expectRevert(abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector)); + account.isValidSignature(_toChildHash(child), signature); } function testIsValidSignaturePersonalSign() public { @@ -555,12 +599,14 @@ contract MultiOwnerLightAccountTest is Test { _domainSeparatorB(), childHash ); - assertEq(account.isValidSignature(childHash, signature), bytes4(0xffffffff)); - assertEq( - account.isValidSignature(childHash, abi.encodePacked(BaseLightAccount.SignatureType.EOA)), - bytes4(0xffffffff) - ); + // ERC1271.isValidSignature only truncates 32 bytes for personal_sign before passing it on to _isValidSignature + // _isValidSignature truncates the SignatureType byte before ecrecover + vm.expectRevert(abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, (signature.length - 32 - 1))); + account.isValidSignature(childHash, signature); + + vm.expectRevert(abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector)); + account.isValidSignature(childHash, abi.encodePacked(BaseLightAccount.SignatureType.EOA)); } function testOwnerCanUpgrade() public { @@ -662,7 +708,7 @@ contract MultiOwnerLightAccountTest is Test { bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032))) ) ), - 0x04ef3a9310d1a2d867807831aa0361b20aad25421de58cf37457fcf7f9567b6a + 0xff5809a60394ff57666c1f7b34605ab8a7fe83c99b833b484ed65f4fdd479c4e ); } diff --git a/test/MultiOwnerLightAccountFactory.t.sol b/test/MultiOwnerLightAccountFactory.t.sol index ce0ca47..60a3417 100644 --- a/test/MultiOwnerLightAccountFactory.t.sol +++ b/test/MultiOwnerLightAccountFactory.t.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.23; import "forge-std/Test.sol";