Skip to content

Commit

Permalink
feat(v2): Add signature enum to user operation signature validation f…
Browse files Browse the repository at this point in the history 
…or multi owner (#41)
  • Loading branch information
@adam-alchemy
adam-alchemy authored Mar 15, 2024
1 parent dac748b commit 816168b
Show file tree
Hide file tree
Showing 2 changed files with 173 additions and 16 deletions.
98 changes: 86 additions & 12 deletions src/MultiOwnerLightAccount.sol
View file
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

}

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;
}

/// @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
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

0 comments on commit 816168b

Please sign in to comment.