fix: address best practices

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

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

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) {

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(

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

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

test/LightAccount.t.sol

@@ -556,7 +556,7 @@ contract LightAccountTest is Test {
                     bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032)))
                 )
             ),
-            0x8199f8205e3258d7c2f19a108ae7d87beba2ee39a352779987b9bb16c13d0763
+            0xefe804f2c00bf5ade462dc2ba6c9c59fe5a69a24914737fbd7613bdb2dfcf600
         );
     }
 

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

test/MultiOwnerLightAccount.t.sol

@@ -715,7 +715,7 @@ contract MultiOwnerLightAccountTest is Test {
                     bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032)))
                 )
             ),
-            0xbb599752b3425b84e8ae3cb828517ab8257400426f76cdb9d522785032b80568
+            0x38d4efa1969cecb0d91391970b4a410bfc1f26e6a41a29bebfda173a9a739ea0
         );
     }
 

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