Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(v2): Add signature enum to user operation signature validation #41

Merged
merged 3 commits into from
Mar 15, 2024
Merged

feat(v2): Add signature enum to user operation signature validation #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
94d450b
Add signature enum to user operation signature validation
adamegyed Mar 15, 2024
465233c
remove unnecessary assembly
adamegyed Mar 15, 2024
b4d7b1a
Correct enum assignments to match bitmap
adamegyed Mar 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 86 additions & 12 deletions src/MultiOwnerLightAccount.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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

}
Copy link
Collaborator

@jaypaik jaypaik Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we going to be using the bits for checks later?

If not, maybe it's better to just go with regularly incrementing enums.

If we are, maintaining enums might get messy as we add on new signature types. So maybe just maintain these flags in a library and introduce helper functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was planning on using the bits during the 1271 signature checking, but I'll take a look and see if it makes sense to just drop this down to an enum.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! Edited my earlier comment to suggest a library instead if we're using the bits.


struct LightAccountStorage {
LinkedListSet owners;
}
Expand All @@ -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();
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I previously had this as just iszero in assembly, but I figured it might make sense to keep this simple and readable. The gas difference is about 20, so not very significant, but also OK to switch back.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was wondering the same thing. I think this reads better!

}

/// @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)
Expand All @@ -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()
Expand Down
91 changes: 87 additions & 4 deletions test/MultiOwnerLightAccount.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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, ()));
Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -499,7 +579,7 @@ contract MultiOwnerLightAccountTest is Test {
bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032)))
)
),
0x3765ef442bfa3b5ef7a30073b39949186919dd2344bb5aa0736a0e0be66ebfe1
0x0a14a7d2e0fb6858a618de8e4a7cf52bd1cb8dad3f4adbf243078a4db5f13c92
);
}

Expand Down Expand Up @@ -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;
}

Expand Down
Loading