Merge pull request #196 from bcnmy/feat/SMA-234-Aggregated-audit-reme…

…diations

Aggregated Audit Remediations

contracts/smart-account/base/FallbackManager.sol

@@ -1,15 +1,14 @@
 // SPDX-License-Identifier: LGPL-3.0-only
 pragma solidity ^0.8.23;
 
-import {SelfAuthorized} from "../common/SelfAuthorized.sol";
 import {IFallbackManager} from "../interfaces/base/IFallbackManager.sol";
 
 /**
  *   @title Fallback Manager - A contract that manages fallback calls made to the Smart Account
  *   @dev Fallback calls are handled by a `handler` contract that is stored at FALLBACK_HANDLER_STORAGE_SLOT
  *        fallback calls are not delegated to the `handler` so they can not directly change Smart Account storage
  */
-abstract contract FallbackManager is SelfAuthorized, IFallbackManager {
+abstract contract FallbackManager is IFallbackManager {
     // keccak-256 hash of "fallback_manager.handler.address" subtracted by 1
     bytes32 internal constant FALLBACK_HANDLER_STORAGE_SLOT =
         0x6c9a6c4a39284e37ed1cf53d337577d14212a4870fb976a4366c693b939918d4;

contracts/smart-account/base/ModuleManager.sol

@@ -1,15 +1,14 @@
 // SPDX-License-Identifier: LGPL-3.0-only
 pragma solidity ^0.8.23;
 
-import {SelfAuthorized} from "../common/SelfAuthorized.sol";
 import {Executor, Enum} from "./Executor.sol";
 import {IModuleManager} from "../interfaces/base/IModuleManager.sol";
 
 /**
  * @title Module Manager - A contract that manages modules that can execute transactions
  *        on behalf of the Smart Account via this contract.
  */
-abstract contract ModuleManager is SelfAuthorized, Executor, IModuleManager {
+abstract contract ModuleManager is Executor, IModuleManager {
     address internal constant SENTINEL_MODULES = address(0x1);
     mapping(address => address) internal _modules;
     uint256[24] private __gap;

contracts/smart-account/common/SignatureDecoder.sol

@@ -19,6 +19,7 @@ abstract contract SignatureDecoder {
         // The signature format is a compact form of:
         //   {bytes32 r}{bytes32 s}{uint8 v}
         // Compact means, uint8 is not padded to 32 bytes.
+        require(signature.length == 65, "Invalid signature length");
 
         assembly {
             r := mload(add(signature, 0x20))

contracts/smart-account/interfaces/IAddressResolver.sol

@@ -10,4 +10,43 @@ interface IAddressResolver {
         string factoryVersion;
         uint256 deploymentIndex;
     }
+
+    /**
+     * @dev Returns the addresses of all the smart accounts deployed by the EOA for any deployment index from 0 to _maxIndex.
+     * @param _eoa Address of the EOA.
+     * @param _maxIndex Maximum index to check.
+     * @notice This function is only for V1 Biconomy smart accounts.
+     */
+    function resolveAddressesV1(
+        address _eoa,
+        uint8 _maxIndex
+    ) external view returns (SmartAccountResult[] memory);
+
+    /**
+     * @dev Returns the addresses of all the smart accounts deployed by the EOA for any deployment index from 0 to _maxIndex.
+     * @param _eoa Address of the EOA.
+     * @param _maxIndex Maximum index to check.
+     * @notice This function is only for V1 and V2 Biconomy smart accounts.
+     * @notice For V2 smart accounts, the _moduleAddress and _moduleSetupData parameters are not used. It assumes ECDSA module.
+     */
+    function resolveAddresses(
+        address _eoa,
+        uint8 _maxIndex
+    ) external view returns (SmartAccountResult[] memory);
+
+    /**
+     * @dev Returns the addresses of all the smart accounts deployed by the EOA for any deployment index from 0 to _maxIndex.
+     * @param _eoa Address of the EOA.
+     * @param _maxIndex Maximum index to check.
+     * @param _moduleAddress Address of the auth module used to deploy the smart accounts.
+     * @param _moduleSetupData module setup data used to deploy the smart accounts.
+     * @notice This function is only for V1 and V2 Biconomy smart accounts.
+     * @notice For V2 smart accounts, the _moduleAddress and _moduleSetupData parameters are used. It can be any auth module (which uses ecda owner) used with the factory
+     */
+    function resolveAddressesFlexibleForV2(
+        address _eoa,
+        uint8 _maxIndex,
+        address _moduleAddress, // V2 factory could use any auth module
+        bytes memory _moduleSetupData
+    ) external view returns (SmartAccountResult[] memory);
 }

contracts/smart-account/interfaces/ISmartAccount.sol

@@ -26,17 +26,6 @@ interface ISmartAccount is IBaseSmartAccount, IModuleManager {
 
     error EntryPointCannotBeZero();
 
-    /**
-     * @notice Throws at mixedAuth when msg.sender is not an owner neither _self
-     * @param caller address that tried to call mixedAuth-protected method
-     */
-    error MixedAuthFail(address caller);
-
-    /**
-     * @notice Throws if trying to change an owner of a SmartAccount to the zero address
-     */
-    error OwnerCannotBeZero();
-
     /**
      * @notice Throws if zero address has been provided as Base Implementation address
      */

contracts/smart-account/interfaces/base/IModuleManager.sol

@@ -22,16 +22,6 @@ interface IModuleManager {
         Enum.Operation operation
     );
 
-    /**
-     * @notice Throws when trying to initialize module manager that already been initialized
-     */
-    error ModulesAlreadyInitialized();
-
-    /**
-     * @notice Throws when a delegatecall in course of module manager initialization has failed
-     */
-    error ModulesSetupExecutionFailed();
-
     /**
      * @notice Throws when address(0) or SENTINEL_MODULES constant has been provided as a module address
      * @param module Module address provided

contracts/smart-account/interfaces/modules/IAccountRecoveryModule.sol

@@ -109,7 +109,7 @@ interface IAccountRecoveryModule {
      */
     event SecurityDelayChanged(
         address indexed smartAccount,
-        uint48 securityDelay
+        uint24 securityDelay
     );
 
     /**
@@ -337,6 +337,42 @@ interface IAccountRecoveryModule {
      */
     function renounceRecoveryRequest() external;
 
+    /**
+     * @dev Resets the module for a Smart Account that calls the method
+     * Should be called by the Smart Account
+     * @param guardians the list of guardians that are enabled for the Smart Account
+     */
+    function resetModuleForCaller(bytes32[] memory guardians) external;
+
+    /**
+     * @dev Changes how many allowed recoveries left for a Smart Account (msg.sender)
+     * Should be called by the Smart Account
+     * @param allowedRecoveries new security delay
+     */
+    function setAllowedRecoveries(uint8 allowedRecoveries) external;
+
+    /**
+     * @dev Executes recovery request for a Smart Account (msg.sender)
+     * Should be called by the Smart Account
+     * SA.execute => AccRecovery.executeRecovery
+     * Decrements recoveries left, and if 0 left, no userOps will be validated by this module
+     * It forces user to perform an explicit action:
+     *      - If user wants same guardians to be able to recover the account again,
+     *      they have to call setAllowedRecoveries() => NOT RECOMMENDED
+     *     - If user wants to change guardians, they have to
+     *          -- remove/replace guardians + adjust threshold + setAllowedRecoveries()
+     *          or
+     *          -- clear all guardians + re-init the module => RECOMMENDED
+     * @param to destination address
+     * @param value value to send
+     * @param data callData to execute
+     */
+    function executeRecovery(
+        address to,
+        uint256 value,
+        bytes calldata data
+    ) external;
+
     /**
      * @dev Returns guardian validity timeframes for the Smart Account
      * @param guardian guardian to get params for

contracts/smart-account/interfaces/modules/IPasskeyRegistryModule.sol

@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.23;
 
+import {PassKeyId} from "../../modules/PasskeyValidationModules/Secp256r1.sol";
+
 /**
  * @title Passkey ownership Authorization module for Biconomy Smart Accounts.
  * @dev Compatible with Biconomy Modular Interface v 0.2
@@ -27,9 +29,40 @@ interface IPasskeyRegistryModule {
         string calldata _keyId
     ) external returns (address);
 
+    /**
+     * @dev Validates an EIP-1271 signature
+     * @dev Appends Smart Account address to the hash to avoid replay attacks
+     * @param signedDataHash hash of the data
+     * @param moduleSignature Signature to be validated.
+     * @param smartAccount expected signer Smart Account address.
+     * @return EIP1271_MAGIC_VALUE if signature is valid, 0xffffffff otherwise.
+     */
     function isValidSignatureForAddress(
         bytes32 signedDataHash,
         bytes memory moduleSignature,
         address smartAccount
     ) external view returns (bytes4);
+
+    /**
+     * @dev Same as isValidSignatureForAddress but does not append Smart Account address to the hash
+     * @dev Expects the data Hash to already include smart account address information
+     * @param signedDataHash hash of the data which includes smart account address
+     * @param moduleSignature Signature to be validated.
+     * @param smartAccount expected signer Smart Account address.
+     * @return EIP1271_MAGIC_VALUE if signature is valid, 0xffffffff otherwise.
+     */
+    function isValidSignatureForAddressUnsafe(
+        bytes32 signedDataHash,
+        bytes memory moduleSignature,
+        address smartAccount
+    ) external view returns (bytes4);
+
+    /**
+     * @dev Returns the owner of the Smart Account.
+     * @param smartAccount Smart Account address.
+     * @return PassKeyId The owner key of the Smart Account.
+     */
+    function getOwner(
+        address smartAccount
+    ) external view returns (PassKeyId memory);
 }

contracts/smart-account/libs/LibAddress.sol

@@ -8,12 +8,9 @@ library LibAddress {
      * @dev This contract will return false if called within the constructor of
      *      a contract's deployment, as the code is not yet stored on-chain.
      */
-    function isContract(address account) internal view returns (bool) {
-        uint256 csize;
-
+    function isContract(address account) internal view returns (bool res) {
         assembly {
-            csize := extcodesize(account)
+            res := gt(extcodesize(account), 0)
         }
-        return csize != 0;
     }
 }

contracts/smart-account/modules/AccountRecoveryModule.sol

@@ -323,6 +323,7 @@ contract AccountRecoveryModule is
         );
     }
 
+    //@inheritdoc IAccountRecoveryModule
     function resetModuleForCaller(bytes32[] memory guardians) external {
         uint256 length = guardians.length;
         for (uint256 i; i < length; ) {
@@ -370,11 +371,7 @@ contract AccountRecoveryModule is
         emit SecurityDelayChanged(msg.sender, newSecurityDelay);
     }
 
-    /**
-     * @dev Changes how many allowed recoveries left for a Smart Account (msg.sender)
-     * Should be called by the Smart Account
-     * @param allowedRecoveries new security delay
-     */
+    //@inheritdoc IAccountRecoveryModule
     function setAllowedRecoveries(uint8 allowedRecoveries) external {
         _smartAccountSettings[msg.sender].recoveriesLeft = allowedRecoveries;
         emit RecoveriesLeft(msg.sender, allowedRecoveries);

contracts/smart-account/modules/EcdsaOwnershipRegistryModule.sol

@@ -10,6 +10,7 @@ import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
 import {IEcdsaOwnershipRegistryModule} from "../interfaces/modules/IEcdsaOwnershipRegistryModule.sol";
 import {IAuthorizationModule} from "../interfaces/IAuthorizationModule.sol";
 import {ISignatureValidator} from "../interfaces/ISignatureValidator.sol";
+import {LibAddress} from "../libs/LibAddress.sol";
 
 /**
  * @title ECDSA ownership Authorization module for Biconomy Smart Accounts.
@@ -29,6 +30,7 @@ contract EcdsaOwnershipRegistryModule is
     IEcdsaOwnershipRegistryModule
 {
     using ECDSA for bytes32;
+    using LibAddress for address;
 
     string public constant NAME = "ECDSA Ownership Registry Module";
     string public constant VERSION = "1.1.0";
@@ -40,6 +42,10 @@ contract EcdsaOwnershipRegistryModule is
     function initForSmartAccount(
         address eoaOwner
     ) external override returns (address) {
+        // if (eoaOwner.isContract()) revert NotEOA(eoaOwner);
+        // This check is not possible because of [OP-041]
+        // See https://github.com/eth-infinitism/account-abstraction/blob/develop/erc/ERCS/erc-7562.md
+        // And https://github.com/eth-infinitism/bundler/issues/137 for details
         if (_smartAccountOwners[msg.sender] != address(0)) {
             revert AlreadyInitedForSmartAccount(msg.sender);
         }
@@ -50,7 +56,7 @@ contract EcdsaOwnershipRegistryModule is
 
     /// @inheritdoc IEcdsaOwnershipRegistryModule
     function transferOwnership(address owner) external override {
-        if (_isSmartContract(owner)) revert NotEOA(owner);
+        if (owner.isContract()) revert NotEOA(owner);
         if (owner == address(0)) revert ZeroAddressNotAllowedAsOwner();
         _transferOwnership(msg.sender, owner);
     }
@@ -201,16 +207,4 @@ contract EcdsaOwnershipRegistryModule is
         }
         return false;
     }
-
-    /**
-     * @dev Checks if the address provided is a smart contract.
-     * @param account Address to be checked.
-     */
-    function _isSmartContract(address account) internal view returns (bool) {
-        uint256 size;
-        assembly {
-            size := extcodesize(account)
-        }
-        return size > 0;
-    }
 }

contracts/smart-account/modules/MultiOwnedECDSAModule.sol

@@ -10,6 +10,7 @@ import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
 import {IMultiOwnedECDSAModule} from "../interfaces/modules/IMultiOwnedECDSAModule.sol";
 import {IAuthorizationModule} from "../interfaces/IAuthorizationModule.sol";
 import {ISignatureValidator} from "../interfaces/ISignatureValidator.sol";
+import {LibAddress} from "../libs/LibAddress.sol";
 
 /**
  * @title ECDSA Multi Ownership Authorization Module for Biconomy Smart Accounts.
@@ -28,6 +29,7 @@ contract MultiOwnedECDSAModule is
     IMultiOwnedECDSAModule
 {
     using ECDSA for bytes32;
+    using LibAddress for address;
 
     string public constant NAME = "Multiowned ECDSA Ownership Module";
     string public constant VERSION = "0.2.0";
@@ -46,6 +48,10 @@ contract MultiOwnedECDSAModule is
         uint256 ownersToAdd = eoaOwners.length;
         if (ownersToAdd == 0) revert NoOwnersToAdd();
         for (uint256 i; i < ownersToAdd; ++i) {
+            // if (eoaOwners[i].isContract()) revert NotEOA(eoaOwner);
+            // This check is not possible because of [OP-041]
+            // See https://github.com/eth-infinitism/account-abstraction/blob/develop/erc/ERCS/erc-7562.md
+            // And https://github.com/eth-infinitism/bundler/issues/137 for details
             if (eoaOwners[i] == address(0))
                 revert ZeroAddressNotAllowedAsOwner();
             if (_smartAccountOwners[eoaOwners[i]][msg.sender])
@@ -66,7 +72,7 @@ contract MultiOwnedECDSAModule is
         address owner,
         address newOwner
     ) external override {
-        if (_isSmartContract(newOwner)) revert NotEOA(newOwner);
+        if (newOwner.isContract()) revert NotEOA(newOwner);
         if (newOwner == address(0)) revert ZeroAddressNotAllowedAsOwner();
         if (owner == newOwner)
             revert OwnerAlreadyUsedForSmartAccount(newOwner, msg.sender);
@@ -80,7 +86,7 @@ contract MultiOwnedECDSAModule is
 
     /// @inheritdoc IMultiOwnedECDSAModule
     function addOwner(address owner) external override {
-        if (_isSmartContract(owner)) revert NotEOA(owner);
+        if (owner.isContract()) revert NotEOA(owner);
         if (owner == address(0)) revert ZeroAddressNotAllowedAsOwner();
         if (_smartAccountOwners[owner][msg.sender])
             revert OwnerAlreadyUsedForSmartAccount(owner, msg.sender);
@@ -247,16 +253,4 @@ contract MultiOwnedECDSAModule is
         }
         return false;
     }
-
-    /**
-     * @dev Checks if the address provided is a smart contract.
-     * @param account Address to be checked.
-     */
-    function _isSmartContract(address account) internal view returns (bool) {
-        uint256 size;
-        assembly {
-            size := extcodesize(account)
-        }
-        return size > 0;
-    }
 }