diff --git a/src/LightAccount.sol b/src/LightAccount.sol index bdfb326..1baced2 100644 --- a/src/LightAccount.sol +++ b/src/LightAccount.sol @@ -37,9 +37,11 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable { using ECDSA for bytes32; using MessageHashUtils for bytes32; - // keccak256(abi.encode(uint256(keccak256("light_account_v1.storage")) - 1)) & ~bytes32(uint256(0xff)); + /// @dev The version used for namespaced storage is not linked to the release version of the contract. Storage + /// versions will be updated only when storage layout changes are made. + /// keccak256(abi.encode(uint256(keccak256("light_account_v1.storage")) - 1)) & ~bytes32(uint256(0xff)); bytes32 internal constant _STORAGE_POSITION = 0x691ec1a18226d004c07c9f8e5c4a6ff15a7b38db267cf7e3c945aef8be512200; - // keccak256(abi.encode(uint256(keccak256("light_account_v1.initializable")) - 1)) & ~bytes32(uint256(0xff)); + /// @dev keccak256(abi.encode(uint256(keccak256("light_account_v1.initializable")) - 1)) & ~bytes32(uint256(0xff)); bytes32 internal constant _INITIALIZABLE_STORAGE_POSITION = 0x33e4b41198cc5b8053630ed667ea7c0c4c873f7fc8d9a478b5d7259cec0a4a00; @@ -72,7 +74,7 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable { /// 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 { + function initialize(address owner_) external virtual initializer { _initialize(owner_); } @@ -188,6 +190,7 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable { returns (string memory name, string memory version) { name = "LightAccount"; + // Set to the major version of the GitHub release at which the contract was last updated. version = "2"; } diff --git a/src/LightAccountFactory.sol b/src/LightAccountFactory.sol index 69cd602..f0348cd 100644 --- a/src/LightAccountFactory.sol +++ b/src/LightAccountFactory.sol @@ -16,6 +16,7 @@ contract LightAccountFactory is BaseLightAccountFactory { LightAccount public immutable ACCOUNT_IMPLEMENTATION; constructor(address owner, IEntryPoint entryPoint) Ownable(owner) { + _verifyEntryPointAddress(address(entryPoint)); ACCOUNT_IMPLEMENTATION = new LightAccount(entryPoint); ENTRY_POINT = entryPoint; } @@ -27,7 +28,7 @@ contract LightAccountFactory is BaseLightAccountFactory { /// @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 account 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 account) { + function createAccount(address owner, uint256 salt) external returns (LightAccount account) { (bool alreadyDeployed, address accountAddress) = LibClone.createDeterministicERC1967(address(ACCOUNT_IMPLEMENTATION), _getCombinedSalt(owner, salt)); @@ -42,7 +43,7 @@ contract LightAccountFactory is BaseLightAccountFactory { /// @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) { + function getAddress(address owner, uint256 salt) external view returns (address) { return LibClone.predictDeterministicAddressERC1967( address(ACCOUNT_IMPLEMENTATION), _getCombinedSalt(owner, salt), address(this) ); diff --git a/src/MultiOwnerLightAccount.sol b/src/MultiOwnerLightAccount.sol index b884f26..9fb3a7d 100644 --- a/src/MultiOwnerLightAccount.sol +++ b/src/MultiOwnerLightAccount.sol @@ -23,9 +23,11 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { using CastLib for address; using CastLib for SetValue[]; - // keccak256(abi.encode(uint256(keccak256("multi_owner_light_account_v1.storage")) - 1)) & ~bytes32(uint256(0xff)); + /// @dev The version used for namespaced storage is not linked to the release version of the contract. Storage + /// versions will be updated only when storage layout changes are made. + /// keccak256(abi.encode(uint256(keccak256("multi_owner_light_account_v1.storage")) - 1)) & ~bytes32(uint256(0xff)); bytes32 internal constant _STORAGE_POSITION = 0x0eb5184329babcda7203727c83eff940fb292fc735f61720a6182b755bf7f900; - // keccak256(abi.encode(uint256(keccak256("multi_owner_light_account_v1.initializable")) - 1)) & ~bytes32(uint256(0xff)); + /// @dev keccak256(abi.encode(uint256(keccak256("multi_owner_light_account_v1.initializable")) - 1)) & ~bytes32(uint256(0xff)); bytes32 internal constant _INITIALIZABLE_STORAGE_POSITION = 0xaa296a366a62f6551d3ddfceae892d1791068a359a0d3461aab99dfc6c5fd700; @@ -109,10 +111,7 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { uint256 length = ownersToAdd.length; for (uint256 i = 0; i < length; ++i) { address ownerToAdd = ownersToAdd[i]; - if ( - ownerToAdd == address(0) || ownerToAdd == address(this) - || !_storage.owners.tryAdd(ownerToAdd.toSetValue()) - ) { + if (ownerToAdd == address(this) || !_storage.owners.tryAdd(ownerToAdd.toSetValue())) { revert InvalidOwner(ownerToAdd); } } @@ -246,7 +245,8 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { returns (string memory name, string memory version) { name = "MultiOwnerLightAccount"; - version = "1"; + // Set to the major version of the GitHub release at which the contract was last updated. + version = "2"; } function _isFromOwner() internal view virtual override returns (bool) { diff --git a/src/MultiOwnerLightAccountFactory.sol b/src/MultiOwnerLightAccountFactory.sol index 7ea0dde..0e26e23 100644 --- a/src/MultiOwnerLightAccountFactory.sol +++ b/src/MultiOwnerLightAccountFactory.sol @@ -21,6 +21,7 @@ contract MultiOwnerLightAccountFactory is BaseLightAccountFactory { error OwnersLimitExceeded(); constructor(address owner, IEntryPoint entryPoint) Ownable(owner) { + _verifyEntryPointAddress(address(entryPoint)); ACCOUNT_IMPLEMENTATION = new MultiOwnerLightAccount(entryPoint); ENTRY_POINT = entryPoint; } @@ -32,7 +33,7 @@ contract MultiOwnerLightAccountFactory is BaseLightAccountFactory { /// @param owners The owners of the account to be created. /// @param salt A salt, which can be changed to create multiple accounts with the same owners. /// @return account The address of either the newly deployed account or an existing account with these owners and salt. - function createAccount(address[] calldata owners, uint256 salt) public returns (MultiOwnerLightAccount account) { + function createAccount(address[] calldata owners, uint256 salt) external returns (MultiOwnerLightAccount account) { _validateOwnersArray(owners); (bool alreadyDeployed, address accountAddress) = @@ -50,12 +51,14 @@ contract MultiOwnerLightAccountFactory is BaseLightAccountFactory { /// @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 account The address of either the newly deployed account or an existing account with this owner and salt. - function createAccountSingle(address owner, uint256 salt) public returns (MultiOwnerLightAccount account) { + function createAccountSingle(address owner, uint256 salt) external returns (MultiOwnerLightAccount account) { + if (owner == address(0)) { + revert InvalidOwners(); + } + address[] memory owners = new address[](1); owners[0] = owner; - _validateOwnersArray(owners); - (bool alreadyDeployed, address accountAddress) = LibClone.createDeterministicERC1967(address(ACCOUNT_IMPLEMENTATION), _getCombinedSalt(owners, salt)); @@ -70,7 +73,7 @@ contract MultiOwnerLightAccountFactory is BaseLightAccountFactory { /// @param owners The owners of the account to be created. /// @param salt A salt, which can be changed to create multiple accounts with the same owners. /// @return The address of the account that would be created with `createAccount`. - function getAddress(address[] memory owners, uint256 salt) public view returns (address) { + function getAddress(address[] memory owners, uint256 salt) external view returns (address) { _validateOwnersArray(owners); return LibClone.predictDeterministicAddressERC1967( diff --git a/src/common/BaseLightAccount.sol b/src/common/BaseLightAccount.sol index 78691d8..473f8e5 100644 --- a/src/common/BaseLightAccount.sol +++ b/src/common/BaseLightAccount.sol @@ -2,7 +2,6 @@ pragma solidity ^0.8.23; import {BaseAccount} from "account-abstraction/core/BaseAccount.sol"; -import {SIG_VALIDATION_FAILED} from "account-abstraction/core/Helpers.sol"; import {SIG_VALIDATION_FAILED, SIG_VALIDATION_SUCCESS} from "account-abstraction/core/Helpers.sol"; import {IEntryPoint} from "account-abstraction/interfaces/IEntryPoint.sol"; import {PackedUserOperation} from "account-abstraction/interfaces/PackedUserOperation.sol"; @@ -77,31 +76,31 @@ abstract contract BaseLightAccount is BaseAccount, TokenCallbackHandler, UUPSUpg } /// @notice Deposit more funds for this account in the entry point. - function addDeposit() public payable { + function addDeposit() external payable { entryPoint().depositTo{value: msg.value}(address(this)); } /// @notice Withdraw value from the account's deposit. /// @param withdrawAddress Target to send to. /// @param amount Amount to withdraw. - function withdrawDepositTo(address payable withdrawAddress, uint256 amount) public onlyAuthorized { + function withdrawDepositTo(address payable withdrawAddress, uint256 amount) external onlyAuthorized { if (withdrawAddress == address(0)) { revert ZeroAddressNotAllowed(); } entryPoint().withdrawTo(withdrawAddress, amount); } - /// @inheritdoc BaseAccount - function entryPoint() public view virtual override returns (IEntryPoint) { - return _ENTRY_POINT; - } - /// @notice Check current account deposit in the entry point. /// @return The current account deposit. - function getDeposit() public view returns (uint256) { + function getDeposit() external view returns (uint256) { return entryPoint().balanceOf(address(this)); } + /// @inheritdoc BaseAccount + function entryPoint() public view virtual override returns (IEntryPoint) { + return _ENTRY_POINT; + } + /// @dev Must override to allow calls to protected functions. function _isFromOwner() internal view virtual returns (bool); diff --git a/src/common/BaseLightAccountFactory.sol b/src/common/BaseLightAccountFactory.sol index dbc5b52..4399436 100644 --- a/src/common/BaseLightAccountFactory.sol +++ b/src/common/BaseLightAccountFactory.sol @@ -4,12 +4,14 @@ pragma solidity ^0.8.23; import {Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol"; import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; +import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol"; import {IEntryPoint} from "account-abstraction/interfaces/IEntryPoint.sol"; abstract contract BaseLightAccountFactory is Ownable2Step { IEntryPoint public immutable ENTRY_POINT; error InvalidAction(); + error InvalidEntryPoint(address entryPoint); error TransferFailed(); error ZeroAddressNotAllowed(); @@ -63,4 +65,12 @@ abstract contract BaseLightAccountFactory is Ownable2Step { function renounceOwnership() public view override onlyOwner { revert InvalidAction(); } + + /// @dev Verify that the entry point implements the IEntryPoint interface. + /// @param entryPointAddress The entry point address to verify. + function _verifyEntryPointAddress(address entryPointAddress) internal view { + if (!ERC165Checker.supportsInterface(address(entryPointAddress), type(IEntryPoint).interfaceId)) { + revert InvalidEntryPoint(address(entryPointAddress)); + } + } } diff --git a/test/LightAccount.t.sol b/test/LightAccount.t.sol index b7b59f4..7258a1a 100644 --- a/test/LightAccount.t.sol +++ b/test/LightAccount.t.sol @@ -556,7 +556,7 @@ contract LightAccountTest is Test { bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032))) ) ), - 0x8199f8205e3258d7c2f19a108ae7d87beba2ee39a352779987b9bb16c13d0763 + 0xefe804f2c00bf5ade462dc2ba6c9c59fe5a69a24914737fbd7613bdb2dfcf600 ); } diff --git a/test/LightAccountFactory.t.sol b/test/LightAccountFactory.t.sol index 27dcc8c..5ca141e 100644 --- a/test/LightAccountFactory.t.sol +++ b/test/LightAccountFactory.t.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.23; import "forge-std/Test.sol"; import {EntryPoint} from "account-abstraction/core/EntryPoint.sol"; +import {IEntryPoint} from "account-abstraction/interfaces/IEntryPoint.sol"; import {BaseLightAccountFactory} from "../src/common/BaseLightAccountFactory.sol"; import {LightAccount} from "../src/LightAccount.sol"; @@ -93,6 +94,14 @@ contract LightAccountFactoryTest is Test { factory.renounceOwnership(); } + function testRevertWithInvalidEntryPoint() public { + IEntryPoint invalidEntryPoint = IEntryPoint(address(123)); + vm.expectRevert( + abi.encodeWithSelector(BaseLightAccountFactory.InvalidEntryPoint.selector, (address(invalidEntryPoint))) + ); + new LightAccountFactory(address(this), invalidEntryPoint); + } + /// @dev Receive funds from withdraw. receive() external payable {} } diff --git a/test/MultiOwnerLightAccount.t.sol b/test/MultiOwnerLightAccount.t.sol index 558f4ac..966d2a0 100644 --- a/test/MultiOwnerLightAccount.t.sol +++ b/test/MultiOwnerLightAccount.t.sol @@ -715,7 +715,7 @@ contract MultiOwnerLightAccountTest is Test { bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032))) ) ), - 0xbb599752b3425b84e8ae3cb828517ab8257400426f76cdb9d522785032b80568 + 0x38d4efa1969cecb0d91391970b4a410bfc1f26e6a41a29bebfda173a9a739ea0 ); } diff --git a/test/MultiOwnerLightAccountFactory.t.sol b/test/MultiOwnerLightAccountFactory.t.sol index fcdcc90..ff2532e 100644 --- a/test/MultiOwnerLightAccountFactory.t.sol +++ b/test/MultiOwnerLightAccountFactory.t.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.23; import "forge-std/Test.sol"; import {EntryPoint} from "account-abstraction/core/EntryPoint.sol"; +import {IEntryPoint} from "account-abstraction/interfaces/IEntryPoint.sol"; import {BaseLightAccountFactory} from "../src/common/BaseLightAccountFactory.sol"; import {MultiOwnerLightAccount} from "../src/MultiOwnerLightAccount.sol"; @@ -169,6 +170,14 @@ contract MultiOwnerLightAccountFactoryTest is Test { factory.renounceOwnership(); } + function testRevertWithInvalidEntryPoint() public { + IEntryPoint invalidEntryPoint = IEntryPoint(address(123)); + vm.expectRevert( + abi.encodeWithSelector(BaseLightAccountFactory.InvalidEntryPoint.selector, (address(invalidEntryPoint))) + ); + new MultiOwnerLightAccountFactory(address(this), invalidEntryPoint); + } + /// @dev Receive funds from withdraw. receive() external payable {} }