diff --git a/src/LightAccount.sol b/src/LightAccount.sol index c06366c..fa78170 100644 --- a/src/LightAccount.sol +++ b/src/LightAccount.sol @@ -16,36 +16,28 @@ import {PackedUserOperation} from "account-abstraction/interfaces/PackedUserOper import {BaseLightAccount} from "./common/BaseLightAccount.sol"; import {CustomSlotInitializable} from "./common/CustomSlotInitializable.sol"; -/** - * @title A simple ERC-4337 compatible smart contract account with a designated owner account - * @dev Like eth-infinitism's `SimpleAccount`, but with the following changes: - * - * 1. Instead of the default storage slots, uses namespaced storage to avoid - * clashes when switching implementations. - * - * 2. Ownership can be transferred via `transferOwnership`, similar to the - * behavior of an `Ownable` contract. This is a simple single-step operation, - * so care must be taken to ensure that the ownership is being transferred to - * the correct address. - * - * 3. Supports [ERC-1271](https://eips.ethereum.org/EIPS/eip-1271) signature - * validation for both validating the signature on user operations and in - * exposing its own `isValidSignature` method. This only works when the owner of - * `LightAccount` also support ERC-1271. - * - * ERC-4337's bundler validation rules limit the types of contracts that can be - * used as owners to validate user operation signatures. For example, the - * contract's `isValidSignature` function may not use any forbidden opcodes - * such as `TIMESTAMP` or `NUMBER`, and the contract may not be an ERC-1967 - * proxy as it accesses a constant implementation slot not associated with - * the account, violating storage access rules. This also means that the - * owner of a `LightAccount` may not be another `LightAccount` if you want to - * send user operations through a bundler. - * - * 4. Event `SimpleAccountInitialized` renamed to `LightAccountInitialized`. - * - * 5. Uses custom errors. - */ +/// @title A simple ERC-4337 compatible smart contract account with a designated owner account. +/// @dev Like eth-infinitism's SimpleAccount, but with the following changes: +/// +/// 1. Instead of the default storage slots, uses namespaced storage to avoid clashes when switching implementations. +/// +/// 2. Ownership can be transferred via `transferOwnership`, similar to the behavior of an `Ownable` contract. This is +/// a simple single-step operation, so care must be taken to ensure that the ownership is being transferred to the +/// correct address. +/// +/// 3. Supports [ERC-1271](https://eips.ethereum.org/EIPS/eip-1271) signature validation for both validating the +/// signature on user operations and in exposing its own `isValidSignature` method. This only works when the owner of +/// LightAccount also support ERC-1271. +/// +/// ERC-4337's bundler validation rules limit the types of contracts that can be used as owners to validate user +/// operation signatures. For example, the contract's `isValidSignature` function may not use any forbidden opcodes +/// such as `TIMESTAMP` or `NUMBER`, and the contract may not be an ERC-1967 proxy as it accesses a constant +/// implementation slot not associated with the account, violating storage access rules. This also means that the +/// owner of a LightAccount may not be another LightAccount if you want to send user operations through a bundler. +/// +/// 4. Event `SimpleAccountInitialized` renamed to `LightAccountInitialized`. +/// +/// 5. Uses custom errors. contract LightAccount is BaseLightAccount, CustomSlotInitializable { using ECDSA for bytes32; using MessageHashUtils for bytes32; @@ -65,50 +57,38 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable { address owner; } - /** - * @notice Emitted when this account is first initialized - * @param entryPoint The entry point - * @param owner The initial owner - */ + /// @notice Emitted when this account is first initialized. + /// @param entryPoint The entry point. + /// @param owner The initial owner. event LightAccountInitialized(IEntryPoint indexed entryPoint, address indexed owner); - /** - * @notice Emitted when this account's owner changes. Also emitted once at - * initialization, with a `previousOwner` of 0. - * @param previousOwner The previous owner - * @param newOwner The new owner - */ + /// @notice Emitted when this account's owner changes. Also emitted once at initialization, with a + /// `previousOwner` of 0. + /// @param previousOwner The previous owner. + /// @param newOwner The new owner. event OwnershipTransferred(address indexed previousOwner, address indexed newOwner); - /** - * @dev The new owner is not a valid owner (e.g., `address(0)`, the - * account itself, or the current owner). - */ + /// @dev The new owner is not a valid owner (e.g., `address(0)`, the account itself, or the current owner). error InvalidOwner(address owner); - constructor(IEntryPoint anEntryPoint) CustomSlotInitializable(_INITIALIZABLE_STORAGE_POSITION) { - _ENTRY_POINT = anEntryPoint; + constructor(IEntryPoint entryPoint_) CustomSlotInitializable(_INITIALIZABLE_STORAGE_POSITION) { + _ENTRY_POINT = entryPoint_; _disableInitializers(); } - /** - * @notice Called once as part of initialization, either during initial deployment or when first upgrading to - * this contract. - * @dev The _ENTRY_POINT member is immutable, to reduce gas consumption. To upgrade EntryPoint, - * a new implementation of LightAccount must be deployed with the new EntryPoint address, then upgrading - * the implementation by calling `upgradeTo()` - * @param anOwner The initial owner of the account - */ - function initialize(address anOwner) public virtual initializer { - _initialize(anOwner); + /// @notice Called once as part of initialization, either during initial deployment or when first upgrading to + /// this contract. + /// @dev The `_ENTRY_POINT` member is immutable, to reduce gas consumption. To update the entry point address, a new + /// implementation of LightAccount must be deployed with the new entry point address, and then `upgradeToAndCall` + /// must be called to upgrade the implementation. + /// @param owner_ The initial owner of the account. + function initialize(address owner_) public virtual initializer { + _initialize(owner_); } - /** - * @notice Transfers ownership of the contract to a new account (`newOwner`). - * Can only be called by the current owner or from the entry point via a - * user operation signed by the current owner. - * @param newOwner The new owner - */ + /// @notice Transfers ownership of the contract to a new account (`newOwner`). Can only be called by the current + /// owner or from the entry point via a user operation signed by the current owner. + /// @param newOwner The new owner. function transferOwnership(address newOwner) external virtual onlyOwner { if (newOwner == address(0) || newOwner == address(this)) { revert InvalidOwner(newOwner); @@ -116,18 +96,14 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable { _transferOwnership(newOwner); } - /** - * @notice Return the current owner of this account - * @return The current owner - */ + /// @notice Return the current owner of this account. + /// @return The current owner. function owner() public view returns (address) { return _getStorage().owner; } - /** - * @notice Returns the domain separator for this contract, as defined in the EIP-712 standard. - * @return bytes32 The domain separator hash. - */ + /// @notice Returns the domain separator for this contract, as defined in the EIP-712 standard. + /// @return bytes32 The domain separator hash. function domainSeparator() public view returns (bytes32) { return keccak256( abi.encode( @@ -140,34 +116,26 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable { ); } - /** - * @notice Returns the pre-image of the message hash - * @param message Message that should be encoded. - * @return Encoded message. - */ + /// @notice Returns the pre-image of the message hash. + /// @param message Message that should be encoded. + /// @return Encoded message. function encodeMessageData(bytes memory message) public view returns (bytes memory) { bytes32 messageHash = keccak256(abi.encode(_LA_MSG_TYPEHASH, keccak256(message))); return abi.encodePacked("\x19\x01", domainSeparator(), messageHash); } - /** - * @notice Returns hash of a message that can be signed by owners. - * @param message Message that should be hashed. - * @return Message hash. - */ + /// @notice Returns hash of a message that can be signed by owners. + /// @param message Message that should be hashed. + /// @return Message hash. function getMessageHash(bytes memory message) public view returns (bytes32) { return keccak256(encodeMessageData(message)); } - /** - * @inheritdoc IERC1271 - * @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). Note that unlike the signature - * validation used in `validateUserOp`, this does **not** wrap the digest in - * an "Ethereum Signed Message" envelope before checking the signature in - * the EOA-owner case. - */ + /// @inheritdoc IERC1271 + /// @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). Note that unlike the signature validation + /// used in `validateUserOp`, this does **not** wrap the digest in an "Ethereum Signed Message" envelope before + /// checking the signature in the EOA-owner case. function isValidSignature(bytes32 hash, bytes memory signature) public view override returns (bytes4) { bytes32 messageHash = getMessageHash(abi.encode(hash)); if (SignatureChecker.isValidSignatureNow(owner(), messageHash, signature)) { @@ -176,19 +144,15 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable { return 0xffffffff; } - function _initialize(address anOwner) internal virtual { - if (anOwner == address(0)) { + function _initialize(address owner_) internal virtual { + if (owner_ == address(0)) { revert InvalidOwner(address(0)); } - _getStorage().owner = anOwner; - emit LightAccountInitialized(_ENTRY_POINT, anOwner); - emit OwnershipTransferred(address(0), anOwner); + _getStorage().owner = owner_; + emit LightAccountInitialized(_ENTRY_POINT, owner_); + emit OwnershipTransferred(address(0), owner_); } - /** - * @dev Transfers ownership of the contract to a new account (`newOwner`). - * Internal function without access restriction. - */ function _transferOwnership(address newOwner) internal virtual { LightAccountStorage storage _storage = _getStorage(); address oldOwner = _storage.owner; @@ -199,13 +163,9 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable { emit OwnershipTransferred(oldOwner, newOwner); } - /* - * Implement template method of BaseAccount. - * - * Uses a modified version of `SignatureChecker.isValidSignatureNow` in - * which the digest is wrapped with an "Ethereum Signed Message" envelope - * for the EOA-owner case but not in the ERC-1271 contract-owner case. - */ + /// @dev Implement template method of BaseAccount. + /// Uses a modified version of `SignatureChecker.isValidSignatureNow` in which the digest is wrapped with an + /// "Ethereum Signed Message" envelope for the EOA-owner case but not in the ERC-1271 contract-owner case. function _validateSignature(PackedUserOperation calldata userOp, bytes32 userOpHash) internal virtual diff --git a/src/LightAccountFactory.sol b/src/LightAccountFactory.sol index 91149a5..a766a1a 100644 --- a/src/LightAccountFactory.sol +++ b/src/LightAccountFactory.sol @@ -8,12 +8,10 @@ import {IEntryPoint} from "account-abstraction/interfaces/IEntryPoint.sol"; import {LightAccount} from "./LightAccount.sol"; -/** - * @title A factory contract for LightAccount - * @dev A UserOperations "initCode" holds the address of the factory, and a method call (to createAccount, in this sample factory). - * The factory's createAccount returns the target account address even if it is already installed. - * This way, the entryPoint.getSenderAddress() can be called either before or after the account is created. - */ +/// @title A factory contract for LightAccount. +/// @dev A UserOperations "initCode" holds the address of the factory, and a method call (`createAccount`). The +/// factory's `createAccount` returns the target account address even if it is already installed. This way, +/// `entryPoint.getSenderAddress()` can be called either before or after the account is created. contract LightAccountFactory { LightAccount public immutable accountImplementation; @@ -21,15 +19,13 @@ contract LightAccountFactory { accountImplementation = new LightAccount(_entryPoint); } - /** - * @notice Create an account, and return its address. - * Returns the address even if the account is already deployed. - * @dev During UserOperation execution, this method is called only if the account is not deployed. - * This method returns an existing account address so that entryPoint.getSenderAddress() would work even after account creation. - * @param owner The owner of the account to be created - * @param salt A salt, which can be changed to create multiple accounts with the same owner - * @return ret The address of either the newly deployed account or an existing account with this owner and salt - */ + /// @notice Create an account, and return its address. Returns the address even if the account is already deployed. + /// @dev During UserOperation execution, this method is called only if the account is not deployed. This method + /// returns an existing account address so that entryPoint.getSenderAddress() would work even after account + /// creation. + /// @param owner The owner of the account to be created. + /// @param salt A salt, which can be changed to create multiple accounts with the same owner. + /// @return ret The address of either the newly deployed account or an existing account with this owner and salt. function createAccount(address owner, uint256 salt) public returns (LightAccount ret) { address addr = getAddress(owner, salt); uint256 codeSize = addr.code.length; @@ -45,12 +41,10 @@ contract LightAccountFactory { ); } - /** - * @notice Calculate the counterfactual address of this account as it would be returned by createAccount() - * @param owner The owner of the account to be created - * @param salt A salt, which can be changed to create multiple accounts with the same owner - * @return The address of the account that would be created with createAccount() - */ + /// @notice Calculate the counterfactual address of this account as it would be returned by `createAccount`. + /// @param owner The owner of the account to be created. + /// @param salt A salt, which can be changed to create multiple accounts with the same owner. + /// @return The address of the account that would be created with `createAccount`. function getAddress(address owner, uint256 salt) public view returns (address) { return Create2.computeAddress( bytes32(salt), diff --git a/src/common/CustomSlotInitializable.sol b/src/common/CustomSlotInitializable.sol index 56760f0..d793027 100644 --- a/src/common/CustomSlotInitializable.sol +++ b/src/common/CustomSlotInitializable.sol @@ -3,102 +3,88 @@ pragma solidity ^0.8.23; -/** - * @dev Identical to OpenZeppelin's `Initializable`, except that its state variables are kept at a custom storage slot - * instead of at the start of storage. - * - * This is a base contract to aid in writing upgradeable contracts, or any kind of contract that will be deployed - * behind a proxy. Since proxied contracts do not make use of a constructor, it's common to move constructor logic to an - * external initializer function, usually called `initialize`. It then becomes necessary to protect this initializer - * function so it can only be called once. The {initializer} modifier provided by this contract will have this effect. - * - * The initialization functions use a version number. Once a version number is used, it is consumed and cannot be - * reused. This mechanism prevents re-execution of each "step" but allows the creation of new initialization steps in - * case an upgrade adds a module that needs to be initialized. - * - * For example: - * - * [.hljs-theme-light.nopadding] - * ```solidity - * contract MyToken is ERC20Upgradeable { - * function initialize() initializer public { - * __ERC20_init("MyToken", "MTK"); - * } - * } - * - * contract MyTokenV2 is MyToken, ERC20PermitUpgradeable { - * function initializeV2() reinitializer(2) public { - * __ERC20Permit_init("MyToken"); - * } - * } - * ``` - * - * TIP: To avoid leaving the proxy in an uninitialized state, the initializer function should be called as early as - * possible by providing the encoded function call as the `_data` argument to {ERC1967Proxy-constructor}. - * - * CAUTION: When used with inheritance, manual care must be taken to not invoke a parent initializer twice, or to ensure - * that all initializers are idempotent. This is not verified automatically as constructors are by Solidity. - * - * [CAUTION] - * ==== - * Avoid leaving a contract uninitialized. - * - * An uninitialized contract can be taken over by an attacker. This applies to both a proxy and its implementation - * contract, which may impact the proxy. To prevent the implementation contract from being used, you should invoke - * the {_disableInitializers} function in the constructor to automatically lock it when it is deployed: - * - * [.hljs-theme-light.nopadding] - * ``` - * /// @custom:oz-upgrades-unsafe-allow constructor - * constructor() { - * _disableInitializers(); - * } - * ``` - * ==== - */ +/// @dev Identical to OpenZeppelin's `Initializable`, except that its state variables are kept at a custom storage slot +/// instead of at the start of storage. +/// +/// This is a base contract to aid in writing upgradeable contracts, or any kind of contract that will be deployed +/// behind a proxy. Since proxied contracts do not make use of a constructor, it's common to move constructor logic to an +/// external initializer function, usually called `initialize`. It then becomes necessary to protect this initializer +/// function so it can only be called once. The {initializer} modifier provided by this contract will have this effect. +/// +/// The initialization functions use a version number. Once a version number is used, it is consumed and cannot be +/// reused. This mechanism prevents re-execution of each "step" but allows the creation of new initialization steps in +/// case an upgrade adds a module that needs to be initialized. +/// +/// For example: +/// +/// [.hljs-theme-light.nopadding] +/// ```solidity +/// contract MyToken is ERC20Upgradeable { +/// function initialize() initializer public { +/// __ERC20_init("MyToken", "MTK"); +/// } +/// } +/// +/// contract MyTokenV2 is MyToken, ERC20PermitUpgradeable { +/// function initializeV2() reinitializer(2) public { +/// __ERC20Permit_init("MyToken"); +/// } +/// } +/// ``` +/// +/// TIP: To avoid leaving the proxy in an uninitialized state, the initializer function should be called as early as +/// possible by providing the encoded function call as the `_data` argument to {ERC1967Proxy-constructor}. +/// +/// CAUTION: When used with inheritance, manual care must be taken to not invoke a parent initializer twice, or to ensure +/// that all initializers are idempotent. This is not verified automatically as constructors are by Solidity. +/// +/// [CAUTION] +/// ==== +/// Avoid leaving a contract uninitialized. +/// +/// An uninitialized contract can be taken over by an attacker. This applies to both a proxy and its implementation +/// contract, which may impact the proxy. To prevent the implementation contract from being used, you should invoke +/// the {_disableInitializers} function in the constructor to automatically lock it when it is deployed: +/// +/// [.hljs-theme-light.nopadding] +/// ``` +/// /// @custom:oz-upgrades-unsafe-allow constructor +/// constructor() { +/// _disableInitializers(); +/// } +/// ``` +/// ==== abstract contract CustomSlotInitializable { bytes32 internal immutable _storagePosition; struct CustomSlotInitializableStorage { - /** - * @dev Indicates that the contract has been initialized. - * @custom:oz-retyped-from bool - */ + /// @dev Indicates that the contract has been initialized. + /// @custom:oz-retyped-from bool uint64 initialized; - /** - * @dev Indicates that the contract is in the process of being initialized. - */ + /// @dev Indicates that the contract is in the process of being initialized. bool initializing; } - /** - * @dev The contract is already initialized. - */ + /// @dev The contract is already initialized. error InvalidInitialization(); - /** - * @dev The contract is not initializing. - */ + /// @dev The contract is not initializing. error NotInitializing(); - /** - * @dev Triggered when the contract has been initialized or reinitialized. - */ + /// @dev Triggered when the contract has been initialized or reinitialized. event Initialized(uint64 version); constructor(bytes32 storagePosition) { _storagePosition = storagePosition; } - /** - * @dev A modifier that defines a protected initializer function that can be invoked at most once. In its scope, - * `onlyInitializing` functions can be used to initialize parent contracts. - * - * Similar to `reinitializer(1)`, except that functions marked with `initializer` can be nested in the context of a - * constructor. - * - * Emits an {Initialized} event. - */ + /// @dev A modifier that defines a protected initializer function that can be invoked at most once. In its scope, + /// `onlyInitializing` functions can be used to initialize parent contracts. + /// + /// Similar to `reinitializer(1)`, except that functions marked with `initializer` can be nested in the context of a + /// constructor. + /// + /// Emits an {Initialized} event. modifier initializer() { CustomSlotInitializableStorage storage _storage = _getInitializableStorage(); @@ -128,24 +114,22 @@ abstract contract CustomSlotInitializable { } } - /** - * @dev A modifier that defines a protected reinitializer function that can be invoked at most once, and only if the - * contract hasn't been initialized to a greater version before. In its scope, `onlyInitializing` functions can be - * used to initialize parent contracts. - * - * A reinitializer may be used after the original initialization step. This is essential to configure modules that - * are added through upgrades and that require initialization. - * - * When `version` is 1, this modifier is similar to `initializer`, except that functions marked with `reinitializer` - * cannot be nested. If one is invoked in the context of another, execution will revert. - * - * Note that versions can jump in increments greater than 1; this implies that if multiple reinitializers coexist in - * a contract, executing them in the right order is up to the developer or operator. - * - * WARNING: setting the version to type(uint64).max will prevent any future reinitialization. - * - * Emits an {Initialized} event. - */ + /// @dev A modifier that defines a protected reinitializer function that can be invoked at most once, and only if the + /// contract hasn't been initialized to a greater version before. In its scope, `onlyInitializing` functions can be + /// used to initialize parent contracts. + /// + /// A reinitializer may be used after the original initialization step. This is essential to configure modules that + /// are added through upgrades and that require initialization. + /// + /// When `version` is 1, this modifier is similar to `initializer`, except that functions marked with `reinitializer` + /// cannot be nested. If one is invoked in the context of another, execution will revert. + /// + /// Note that versions can jump in increments greater than 1; this implies that if multiple reinitializers coexist in + /// a contract, executing them in the right order is up to the developer or operator. + /// + /// WARNING: setting the version to type(uint64).max will prevent any future reinitialization. + /// + /// Emits an {Initialized} event. modifier reinitializer(uint64 version) { CustomSlotInitializableStorage storage _storage = _getInitializableStorage(); @@ -159,10 +143,8 @@ abstract contract CustomSlotInitializable { emit Initialized(version); } - /** - * @dev Modifier to protect an initialization function so that it can only be invoked by functions with the - * {initializer} and {reinitializer} modifiers, directly or indirectly. - */ + /// @dev Modifier to protect an initialization function so that it can only be invoked by functions with the + /// {initializer} and {reinitializer} modifiers, directly or indirectly. modifier onlyInitializing() { if (!_isInitializing()) { revert NotInitializing(); @@ -170,14 +152,12 @@ abstract contract CustomSlotInitializable { _; } - /** - * @dev Locks the contract, preventing any future reinitialization. This cannot be part of an initializer call. - * Calling this in the constructor of a contract will prevent that contract from being initialized or reinitialized - * to any version. It is recommended to use this to lock implementation contracts that are designed to be called - * through proxies. - * - * Emits an {Initialized} event the first time it is successfully executed. - */ + /// @dev Locks the contract, preventing any future reinitialization. This cannot be part of an initializer call. + /// Calling this in the constructor of a contract will prevent that contract from being initialized or reinitialized + /// to any version. It is recommended to use this to lock implementation contracts that are designed to be called + /// through proxies. + /// + /// Emits an {Initialized} event the first time it is successfully executed. function _disableInitializers() internal virtual { CustomSlotInitializableStorage storage _storage = _getInitializableStorage(); @@ -190,16 +170,12 @@ abstract contract CustomSlotInitializable { } } - /** - * @dev Returns the highest version that has been initialized. See {reinitializer}. - */ + /// @dev Returns the highest version that has been initialized. See {reinitializer}. function _getInitializedVersion() internal view returns (uint64) { return _getInitializableStorage().initialized; } - /** - * @dev Returns `true` if the contract is currently initializing. See {onlyInitializing}. - */ + /// @dev Returns `true` if the contract is currently initializing. See {onlyInitializing}. function _isInitializing() internal view returns (bool) { return _getInitializableStorage().initializing; } diff --git a/test/LightAccount.t.sol b/test/LightAccount.t.sol index 981e2aa..53b752b 100644 --- a/test/LightAccount.t.sol +++ b/test/LightAccount.t.sol @@ -288,7 +288,7 @@ contract LightAccountTest is Test { bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032))) ) ), - 0x366526c31a9f3ae63f66515d13f8e6888fe6edac9bc95296cb53e68006ad9888 + 0x9eee97e8649714c9c53e786aa29791f862bb415d04ca7ed402f18eaea04d5a14 ); } diff --git a/test/MultiOwnerLightAccount.t.sol b/test/MultiOwnerLightAccount.t.sol index 8f5572c..2dbc494 100644 --- a/test/MultiOwnerLightAccount.t.sol +++ b/test/MultiOwnerLightAccount.t.sol @@ -330,7 +330,7 @@ contract MultiOwnerLightAccountTest is Test { bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032))) ) ), - 0x67239bd017c8365ae0798c8cc56c5b0e63b931855b19fd3433433ab6a49b5b8d + 0x55161f60f68c75fcefbc14161c9d81bf40af63e3c1defcdf9cf6f2366487d3ca ); }