Audit fixes for v5.3 (#99)

Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Arr00 <[email protected]>

Author: 3 people

contracts/account/extensions/AccountERC7579.sol

@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: MIT
 
-pragma solidity ^0.8.24;
+pragma solidity ^0.8.27;
 
 import {PackedUserOperation} from "@openzeppelin/contracts/interfaces/draft-IERC4337.sol";
 import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol";
@@ -19,7 +19,7 @@ import {Account} from "../Account.sol";
  * To comply with the ERC-1271 support requirement, this contract defers signature validation to
  * installed validator modules by calling {IERC7579Validator-isValidSignatureWithSender}.
  *
- * This contract does not implement validation logic for user operations since these functionality
+ * This contract does not implement validation logic for user operations since this functionality
  * is often delegated to self-contained validation modules. Developers must install a validator module
  * upon initialization (or any other mechanism to enable execution from the account):
  *
@@ -40,7 +40,12 @@ import {Account} from "../Account.sol";
  *   internal virtual functions {_extractUserOpValidator} and {_extractSignatureValidator}. Both are implemented
  *   following common practices. However, this part is not standardized in ERC-7579 (or in any follow-up ERC). Some
  *   accounts may want to override these internal functions.
+ * * When combined with {ERC7739}, resolution ordering of {isValidSignature} may have an impact ({ERC7739} does not
+ *   call super). Manual resolution might be necessary.
+ * * Static calls (using callType `0xfe`) are currently NOT supported.
  * ====
+ *
+ * WARNING: Removing all validator modules will render the account inoperable, as no user operations can be validated thereafter.
  */
 abstract contract AccountERC7579 is Account, IERC1271, IERC7579Execution, IERC7579AccountConfig, IERC7579ModuleConfig {
     using Bytes for *;
@@ -163,8 +168,8 @@ abstract contract AccountERC7579 is Account, IERC1271, IERC7579Execution, IERC75
      * @dev Implement ERC-1271 through IERC7579Validator modules. If module based validation fails, fallback to
      * "native" validation by the abstract signer.
      *
-     * NOTE: when combined with {ERC7739} (for example through {Account}), resolution ordering may have an impact
-     * ({ERC7739} does not call super). Manual resolution might be necessary.
+     * NOTE: when combined with {ERC7739}, resolution ordering may have an impact ({ERC7739} does not call super).
+     * Manual resolution might be necessary.
      */
     function isValidSignature(bytes32 hash, bytes calldata signature) public view virtual returns (bytes4) {
         // check signature length is enough for extraction
@@ -173,10 +178,10 @@ abstract contract AccountERC7579 is Account, IERC1271, IERC7579Execution, IERC75
             // if module is not installed, skip
             if (isModuleInstalled(MODULE_TYPE_VALIDATOR, module, Calldata.emptyBytes())) {
                 // try validation, skip any revert
-                try IERC7579Validator(module).isValidSignatureWithSender(address(this), hash, innerSignature) returns (
+                try IERC7579Validator(module).isValidSignatureWithSender(msg.sender, hash, innerSignature) returns (
                     bytes4 magic
                 ) {
-                    if (magic == IERC1271.isValidSignature.selector) return magic;
+                    return magic;
                 } catch {}
             }
         }
@@ -220,8 +225,8 @@ abstract contract AccountERC7579 is Account, IERC1271, IERC7579Execution, IERC75
     /**
      * @dev Installs a module of the given type with the given initialization data.
      *
-     * For the fallback module type, the `initData` is expected to be a tuple of a 4-byte selector and the
-     * rest of the data to be sent to the handler when calling {IERC7579Module-onInstall}.
+     * For the fallback module type, the `initData` is expected to be the (packed) concatenation of a 4-byte
+     * selector and the rest of the data to be sent to the handler when calling {IERC7579Module-onInstall}.
      *
      * Requirements:
      *
@@ -259,14 +264,16 @@ abstract contract AccountERC7579 is Account, IERC1271, IERC7579Execution, IERC75
     /**
      * @dev Uninstalls a module of the given type with the given de-initialization data.
      *
-     * For the fallback module type, the `deInitData` is expected to be a tuple of a 4-byte selector and the
-     * rest of the data to be sent to the handler when calling {IERC7579Module-onUninstall}.
+     * For the fallback module type, the `deInitData` is expected to be the (packed) concatenation of a 4-byte
+     * selector and the rest of the data to be sent to the handler when calling {IERC7579Module-onUninstall}.
      *
      * Requirements:
      *
      * * Module must be already installed. Reverts with {ERC7579UninstalledModule} otherwise.
      */
     function _uninstallModule(uint256 moduleTypeId, address module, bytes memory deInitData) internal virtual {
+        require(supportsModule(moduleTypeId), ERC7579Utils.ERC7579UnsupportedModuleType(moduleTypeId));
+
         if (moduleTypeId == MODULE_TYPE_VALIDATOR) {
             require(_validators.remove(module), ERC7579Utils.ERC7579UninstalledModule(moduleTypeId, module));
         } else if (moduleTypeId == MODULE_TYPE_EXECUTOR) {
@@ -336,9 +343,10 @@ abstract contract AccountERC7579 is Account, IERC1271, IERC7579Execution, IERC75
      * ```
      * <module address (20 bytes)> | <key (4 bytes)> | <nonce (8 bytes)>
      * ```
-     * NOTE: The default behavior of this function replicated the behavior of
-     * https://github.com/rhinestonewtf/safe7579/blob/bb29e8b1a66658790c4169e72608e27d220f79be/src/Safe7579.sol#L266[Safe adapter] and
-     * https://github.com/etherspot/etherspot-prime-contracts/blob/cfcdb48c4172cea0d66038324c0bae3288aa8caa/src/modular-etherspot-wallet/wallet/ModularEtherspotWallet.sol#L227[Etherspot's Prime Account].
+     * NOTE: The default behavior of this function replicates the behavior of
+     * https://github.com/rhinestonewtf/safe7579/blob/bb29e8b1a66658790c4169e72608e27d220f79be/src/Safe7579.sol#L266[Safe adapter],
+     * https://github.com/etherspot/etherspot-prime-contracts/blob/cfcdb48c4172cea0d66038324c0bae3288aa8caa/src/modular-etherspot-wallet/wallet/ModularEtherspotWallet.sol#L227[Etherspot's Prime Account], and
+     * https://github.com/erc7579/erc7579-implementation/blob/16138d1afd4e9711f6c1425133538837bd7787b5/src/MSAAdvanced.sol#L247[ERC7579 reference implementation].
      *
      * This is not standardized in ERC-7579 (or in any follow-up ERC). Some accounts may want to override these internal functions.
      *
@@ -359,10 +367,11 @@ abstract contract AccountERC7579 is Account, IERC1271, IERC7579Execution, IERC75
      * <module address (20 bytes)> | <signature data>
      * ```
      *
-     * NOTE: The default behavior of this function replicated the behavior of
+     * NOTE: The default behavior of this function replicates the behavior of
      * https://github.com/rhinestonewtf/safe7579/blob/bb29e8b1a66658790c4169e72608e27d220f79be/src/Safe7579.sol#L350[Safe adapter],
-     * https://github.com/bcnmy/nexus/blob/54f4e19baaff96081a8843672977caf712ef19f4/contracts/Nexus.sol#L239[Biconomy's Nexus] and
-     * https://github.com/etherspot/etherspot-prime-contracts/blob/cfcdb48c4172cea0d66038324c0bae3288aa8caa/src/modular-etherspot-wallet/wallet/ModularEtherspotWallet.sol#L252[Etherspot's Prime Account]
+     * https://github.com/bcnmy/nexus/blob/54f4e19baaff96081a8843672977caf712ef19f4/contracts/Nexus.sol#L239[Biconomy's Nexus],
+     * https://github.com/etherspot/etherspot-prime-contracts/blob/cfcdb48c4172cea0d66038324c0bae3288aa8caa/src/modular-etherspot-wallet/wallet/ModularEtherspotWallet.sol#L252[Etherspot's Prime Account], and
+     * https://github.com/erc7579/erc7579-implementation/blob/16138d1afd4e9711f6c1425133538837bd7787b5/src/MSAAdvanced.sol#L296[ERC7579 reference implementation].
      *
      * This is not standardized in ERC-7579 (or in any follow-up ERC). Some accounts may want to override these internal functions.
      */
@@ -375,10 +384,10 @@ abstract contract AccountERC7579 is Account, IERC1271, IERC7579Execution, IERC75
     /**
      * @dev Extract the function selector from initData/deInitData for MODULE_TYPE_FALLBACK
      *
-     * NOTE: If we had calldata here, we would could use calldata slice which are cheaper to manipulate and don't
-     * require actual copy. However, this would require `_installModule` to get a calldata bytes object instead of a
-     * memory bytes object. This would prevent calling `_installModule` from a contract constructor and would force
-     * the use of external initializers. That may change in the future, as most accounts will probably be deployed as
+     * NOTE: If we had calldata here, we could use calldata slice which are cheaper to manipulate and don't require
+     * actual copy. However, this would require `_installModule` to get a calldata bytes object instead of a memory
+     * bytes object. This would prevent calling `_installModule` from a contract constructor and would force the use
+     * of external initializers. That may change in the future, as most accounts will probably be deployed as
      * clones/proxy/ERC-7702 delegates and therefore rely on initializers anyway.
      */
     function _decodeFallbackData(

contracts/account/extensions/AccountERC7579Hooked.sol

@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: MIT
 
-pragma solidity ^0.8.24;
+pragma solidity ^0.8.27;
 
 import {IERC7579Hook, MODULE_TYPE_HOOK} from "@openzeppelin/contracts/interfaces/draft-IERC7579.sol";
 import {ERC7579Utils, Mode} from "@openzeppelin/contracts/account/utils/draft-ERC7579Utils.sol";
@@ -21,6 +21,9 @@ import {AccountERC7579} from "./AccountERC7579.sol";
 abstract contract AccountERC7579Hooked is AccountERC7579 {
     address private _hook;
 
+    /// @dev A hook module is already present. This contract only supports one hook module.
+    error ERC7579HookModuleAlreadyPresent(address hook);
+
     /**
      * @dev Calls {IERC7579Hook-preCheck} before executing the modified function and {IERC7579Hook-postCheck}
      * thereafter.
@@ -35,6 +38,12 @@ abstract contract AccountERC7579Hooked is AccountERC7579 {
         if (hook_ != address(0)) IERC7579Hook(hook_).postCheck(hookData);
     }
 
+    /// @inheritdoc AccountERC7579
+    function accountId() public view virtual override returns (string memory) {
+        // vendorname.accountname.semver
+        return "@openzeppelin/community-contracts.AccountERC7579Hooked.v0.0.0";
+    }
+
     /// @dev Returns the hook module address if installed, or `address(0)` otherwise.
     function hook() public view virtual returns (address) {
         return _hook;
@@ -63,7 +72,7 @@ abstract contract AccountERC7579Hooked is AccountERC7579 {
         bytes memory initData
     ) internal virtual override withHook {
         if (moduleTypeId == MODULE_TYPE_HOOK) {
-            require(_hook == address(0), ERC7579Utils.ERC7579AlreadyInstalledModule(moduleTypeId, module));
+            require(_hook == address(0), ERC7579HookModuleAlreadyPresent(_hook));
             _hook = module;
         }
         super._installModule(moduleTypeId, module, initData);

contracts/account/extensions/ERC7821.sol

@@ -7,7 +7,9 @@ import {IERC7821} from "../../interfaces/IERC7821.sol";
 import {Account} from "../Account.sol";
 
 /**
- * @dev Minimal batch executor following ERC-7821. Only supports basic mode (no optional "opData").
+ * @dev Minimal batch executor following ERC-7821.
+ *
+ * Only supports supports single batch mode (`0x01000000000000000000`). Does not support optional "opData".
  */
 abstract contract ERC7821 is IERC7821 {
     using ERC7579Utils for *;
@@ -18,7 +20,7 @@ abstract contract ERC7821 is IERC7821 {
      * @dev Executes the calls in `executionData` with no optional `opData` support.
      *
      * NOTE: Access to this function is controlled by {_erc7821AuthorizedExecutor}. Changing access permissions, for
-     * example to approve calls by the ERC-4337 entrypoint, should be implement by overriding it.
+     * example to approve calls by the ERC-4337 entrypoint, should be implemented by overriding it.
      *
      * Reverts and bubbles up error if any call fails.
      */

contracts/mocks/account/AccountERC7579Mock.sol

@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: MIT
 
-pragma solidity ^0.8.24;
+pragma solidity ^0.8.27;
 
 import {MODULE_TYPE_VALIDATOR} from "@openzeppelin/contracts/interfaces/draft-IERC7579.sol";
 import {AccountERC7579} from "../../account/extensions/AccountERC7579.sol";

contracts/mocks/account/AccountERC7702WithModulesMock.sol

@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: MIT
 
-pragma solidity ^0.8.24;
+pragma solidity ^0.8.27;
 
 import {AbstractSigner} from "../../utils/cryptography/AbstractSigner.sol";
 import {Account} from "../../account/Account.sol";

contracts/mocks/account/modules/ERC7579ValidatorMock.sol

@@ -33,12 +33,12 @@ abstract contract ERC7579ValidatorMock is ERC7579ModuleMock(MODULE_TYPE_VALIDATO
     }
 
     function isValidSignatureWithSender(
-        address sender,
+        address /*sender*/,
         bytes32 hash,
         bytes calldata signature
     ) public view virtual returns (bytes4) {
         return
-            SignatureChecker.isValidSignatureNow(_associatedSigners[sender], hash, signature)
+            SignatureChecker.isValidSignatureNow(_associatedSigners[msg.sender], hash, signature)
                 ? IERC1271.isValidSignature.selector
                 : bytes4(0xffffffff);
     }

contracts/token/ERC20/extensions/ERC20Bridgeable.sol

@@ -16,6 +16,8 @@ import {IERC7802} from "../../../interfaces/IERC7802.sol";
 abstract contract ERC20Bridgeable is ERC20, ERC165, IERC7802 {
     /// @dev Modifier to restrict access to the token bridge.
     modifier onlyTokenBridge() {
+        // Token bridge should never be impersonated using a relayer/forwarder. Using msg.sender is preferable to
+        // _msgSender() here, both for cost and security reasons.
         _checkTokenBridge(msg.sender);
         _;
     }
@@ -30,15 +32,15 @@ abstract contract ERC20Bridgeable is ERC20, ERC165, IERC7802 {
      */
     function crosschainMint(address to, uint256 value) public virtual override onlyTokenBridge {
         _mint(to, value);
-        emit CrosschainMint(to, value, msg.sender);
+        emit CrosschainMint(to, value, _msgSender());
     }
 
     /**
      * @dev See {IERC7802-crosschainBurn}. Emits a {CrosschainBurn} event.
      */
     function crosschainBurn(address from, uint256 value) public virtual override onlyTokenBridge {
         _burn(from, value);
-        emit CrosschainBurn(from, value, msg.sender);
+        emit CrosschainBurn(from, value, _msgSender());
     }
 
     /**

contracts/utils/cryptography/ERC7739.sol

@@ -38,8 +38,8 @@ abstract contract ERC7739 is AbstractSigner, EIP712, IERC1271 {
      */
     function isValidSignature(bytes32 hash, bytes calldata signature) public view virtual returns (bytes4 result) {
         // For the hash `0x7739773977397739773977397739773977397739773977397739773977397739` and an empty signature,
-        // we return the magic value too as it's assumed impossible to find a preimage for it that can be used maliciously.
-        // Useful for simulation purposes and to validate whether the contract supports ERC-7739.
+        // we return the magic value `0x77390001` as it's assumed impossible to find a preimage for it that can be used
+        // maliciously. Useful for simulation purposes and to validate whether the contract supports ERC-7739.
         return
             (_isValidNestedTypedDataSignature(hash, signature) || _isValidNestedPersonalSignSignature(hash, signature))
                 ? IERC1271.isValidSignature.selector