feature: clean up safe owner verification in launchpad

rhinestonewtf · kopy-kat · Jul 8, 2024 · Jun 27, 2024 · Jun 27, 2024 · Jun 27, 2024
commit 7a66b3bc7c4f52cfe739c81b11d596339fbd001f
diff --git a/src/ISafe7579.sol b/src/ISafe7579.sol
@@ -240,7 +240,9 @@ interface ISafe7579 is IERC7579Account {
     /**
      * Domain Separator for EIP-712.
      */
-    function domainSeparator() external view returns (bytes32);
+    // TODO: is this required?
+    // function domainSeparator() external view returns (bytes32);
+
     /**
      * Safe7579 is using validator selection encoding in the userop nonce.
      * to make it easier for SDKs / devs to integrate, this function can be

diff --git a/src/Safe7579.sol b/src/Safe7579.sol
@@ -24,7 +24,7 @@ import {
 import { ModuleInstallUtil } from "./utils/DCUtil.sol";
 import { AccessControl } from "./core/AccessControl.sol";
 import { Initializer } from "./core/Initializer.sol";
-import { ISafeOp, SAFE_OP_TYPEHASH } from "./interfaces/ISafeOp.sol";
+import { SafeOp } from "./core/SafeOp.sol";
 import { ISafe } from "./interfaces/ISafe.sol";
 import { ISafe7579 } from "./ISafe7579.sol";
 import {
@@ -50,7 +50,7 @@ uint256 constant MULTITYPE_MODULE = 0;
  * event emissions to be done via the SafeProxy as msg.sender using Safe's
  * "executeTransactionFromModule" features.
  */
-contract Safe7579 is ISafe7579, ISafeOp, AccessControl, Initializer {
+contract Safe7579 is ISafe7579, SafeOp, AccessControl, Initializer {
     using UserOperationLib for PackedUserOperation;
     using ExecutionLib for bytes;
 
@@ -306,7 +306,7 @@ contract Safe7579 is ISafe7579, ISafeOp, AccessControl, Initializer {
             uint48 validAfter,
             uint48 validUntil,
             bytes calldata signatures
-        ) = _getSafeOp(userOp);
+        ) = _getSafeOp(userOp, entryPoint());
         try ISafe((msg.sender)).checkSignatures(keccak256(operationData), operationData, signatures)
         {
             // The timestamps are validated by the entry point,
@@ -530,91 +530,6 @@ contract Safe7579 is ISafe7579, ISafeOp, AccessControl, Initializer {
         return string(abi.encodePacked("safe-", safeVersion, ".erc7579.v0.0.1"));
     }
 
-    /**
-     * @dev Decodes an ERC-4337 user operation into a Safe operation.
-     * @param userOp The ERC-4337 user operation.
-     * @return operationData Encoded EIP-712 Safe operation data bytes used for signature
-     * verification.
-     * @return validAfter The timestamp the user operation is valid from.
-     * @return validUntil The timestamp the user operation is valid until.
-     * @return signatures The Safe owner signatures extracted from the user operation.
-     */
-    function _getSafeOp(PackedUserOperation calldata userOp)
-        internal
-        view
-        returns (
-            bytes memory operationData,
-            uint48 validAfter,
-            uint48 validUntil,
-            bytes calldata signatures
-        )
-    {
-        // Extract additional Safe operation fields from the user operation signature which is
-        // encoded as:
-        // `abi.encodePacked(validAfter, validUntil, signatures)`
-        {
-            bytes calldata sig = userOp.signature;
-            validAfter = uint48(bytes6(sig[0:6]));
-            validUntil = uint48(bytes6(sig[6:12]));
-            signatures = sig[12:];
-        }
-
-        // It is important that **all** user operation fields are represented in the `SafeOp` data
-        // somehow, to prevent
-        // user operations from being submitted that do not fully respect the user preferences. The
-        // only exception is
-        // the `signature` bytes. Note that even `initCode` needs to be represented in the operation
-        // data, otherwise
-        // it can be replaced with a more expensive initialization that would charge the user
-        // additional fees.
-        {
-            // In order to work around Solidity "stack too deep" errors related to too many stack
-            // variables, manually
-            // encode the `SafeOp` fields into a memory `struct` for computing the EIP-712
-            // struct-hash. This works
-            // because the `EncodedSafeOpStruct` struct has no "dynamic" fields so its memory layout
-            // is identical to the
-            // result of `abi.encode`-ing the individual fields.
-            EncodedSafeOpStruct memory encodedSafeOp = EncodedSafeOpStruct({
-                typeHash: SAFE_OP_TYPEHASH,
-                safe: userOp.sender,
-                nonce: userOp.nonce,
-                initCodeHash: keccak256(userOp.initCode),
-                callDataHash: keccak256(userOp.callData),
-                verificationGasLimit: uint128(userOp.unpackVerificationGasLimit()),
-                callGasLimit: uint128(userOp.unpackCallGasLimit()),
-                preVerificationGas: userOp.preVerificationGas,
-                maxPriorityFeePerGas: uint128(userOp.unpackMaxPriorityFeePerGas()),
-                maxFeePerGas: uint128(userOp.unpackMaxFeePerGas()),
-                paymasterAndDataHash: keccak256(userOp.paymasterAndData),
-                validAfter: validAfter,
-                validUntil: validUntil,
-                entryPoint: entryPoint()
-            });
-
-            bytes32 safeOpStructHash;
-            // solhint-disable-next-line no-inline-assembly
-            assembly ("memory-safe") {
-                // Since the `encodedSafeOp` value's memory layout is identical to the result of
-                // `abi.encode`-ing the
-                // individual `SafeOp` fields, we can pass it directly to `keccak256`. Additionally,
-                // there are 14
-                // 32-byte fields to hash, for a length of `14 * 32 = 448` bytes.
-                safeOpStructHash := keccak256(encodedSafeOp, 448)
-            }
-
-            operationData =
-                abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), safeOpStructHash);
-        }
-    }
-
-    /**
-     * @inheritdoc ISafe7579
-     */
-    function domainSeparator() public view returns (bytes32) {
-        return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, block.chainid, this));
-    }
-
     /**
      * @inheritdoc ISafe7579
      */

diff --git a/src/Safe7579Launchpad.sol b/src/Safe7579Launchpad.sol
@@ -2,7 +2,7 @@
 pragma solidity ^0.8.20;
 
 import { _packValidationData } from "@ERC4337/account-abstraction/contracts/core/Helpers.sol";
-import "./interfaces/ISafeOp.sol";
+import { SafeOp } from "./core/SafeOp.sol";
 
 import {
     IAccount,
@@ -22,7 +22,6 @@ import { SafeStorage } from "@safe-global/safe-contracts/contracts/libraries/Saf
 import { MODULE_TYPE_VALIDATOR } from "erc7579/interfaces/IERC7579Module.sol";
 import { CheckSignatures } from "@rhinestone/checknsignatures/src/CheckNSignatures.sol";
 import { LibSort } from "solady/utils/LibSort.sol";
-import "forge-std/console2.sol";
 
 /**
  * Launchpad to deploy a Safe account and connect the Safe7579 adapter.
@@ -31,7 +30,7 @@ import "forge-std/console2.sol";
  * technique](https://github.com/safe-global/safe-modules/pull/184)
  * @author rhinestone | zeroknots.eth
  */
-contract Safe7579Launchpad is IAccount, SafeStorage, ISafeOp {
+contract Safe7579Launchpad is IAccount, SafeStorage, SafeOp {
     using UserOperationLib for PackedUserOperation;
     using LibSort for address[];
     using CheckSignatures for bytes32;
@@ -123,17 +122,21 @@ contract Safe7579Launchpad is IAccount, SafeStorage, ISafeOp {
     function addSafe7579(
         address safe7579,
         ModuleInit[] calldata validators,
+        ModuleInit[] calldata executors,
+        ModuleInit[] calldata fallbacks,
+        ModuleInit[] calldata hooks,
         address[] calldata attesters,
         uint8 threshold
     )
         external
     {
         ISafe(address(this)).enableModule(safe7579);
+        ISafe(address(this)).setFallbackHandler(safe7579);
         ISafe7579(payable(this)).initializeAccount({
             validators: validators,
-            executors: new ModuleInit[](0),
-            fallbacks: new ModuleInit[](0),
-            hooks: new ModuleInit[](0),
+            executors: executors,
+            fallbacks: fallbacks,
+            hooks: hooks,
             registryInit: RegistryInit({ registry: REGISTRY, attesters: attesters, threshold: threshold })
         });
     }
@@ -227,48 +230,36 @@ contract Safe7579Launchpad is IAccount, SafeStorage, ISafeOp {
         // ensure that the call was successful
         if (!success) revert InvalidUserOperationData();
 
-        // to support validation with the safe native signers, we allow to set the validator module
-        // to address(0)
-        if (validator == address(0)) {
+        // Call onInstall on each validator module to set up the validators.
+        // Since this function is delegatecalled by the SafeProxy, the Validator Module is
+        // called
+        // with msg.sender == SafeProxy.
+        bool userOpValidatorInstalled;
+        uint256 validatorsLength = initData.validators.length;
+        for (uint256 i; i < validatorsLength; i++) {
+            address validatorModule = initData.validators[i].module;
+            IValidator(validatorModule).onInstall(initData.validators[i].initData);
+            emit ModuleInstalled(MODULE_TYPE_VALIDATOR, validatorModule);
+
+            if (validatorModule == validator) userOpValidatorInstalled = true;
+        }
+
+        // Ensure that the validator module selected in the userOp was
+        // part of the validators in InitData
+        if (userOpValidatorInstalled) {
+            // validate userOp with selected validation module.
+            validationData = IValidator(validator).validateUserOp(userOp, userOpHash);
+        } else {
+            // otherwise we fall back to safe-style validation, like in the safe7579
             (bool validSig, uint48 validAfter, uint48 validUntil) =
                 _isValidSafeSigners(initData.safe7579, userOpHash, userOp);
 
-            if (missingAccountFunds > 0) {
-                // solhint-disable-next-line no-inline-assembly
-                assembly ("memory-safe") {
-                    pop(call(gas(), caller(), missingAccountFunds, 0, 0, 0, 0))
-                }
-            }
-            return _packValidationData({
+            validationData = _packValidationData({
                 sigFailed: !validSig,
                 validUntil: validUntil,
                 validAfter: validAfter
             });
         }
-        // if the validator module is non address(0), we validate the userOp with the validator
-        // module
-        else {
-            // Call onInstall on each validator module to set up the validators.
-            // Since this function is delegatecalled by the SafeProxy, the Validator Module is
-            // called
-            // with msg.sender == SafeProxy.
-            bool userOpValidatorInstalled;
-            uint256 validatorsLength = initData.validators.length;
-            for (uint256 i; i < validatorsLength; i++) {
-                address validatorModule = initData.validators[i].module;
-                IValidator(validatorModule).onInstall(initData.validators[i].initData);
-                emit ModuleInstalled(MODULE_TYPE_VALIDATOR, validatorModule);
-
-                if (validatorModule == validator) userOpValidatorInstalled = true;
-            }
-            // Ensure that the validator module selected in the userOp was
-            // part of the validators in InitData
-            if (!userOpValidatorInstalled) {
-                return _packValidationData({ sigFailed: true, validUntil: 0, validAfter: 0 });
-            }
-            // validate userOp with selected validation module.
-            validationData = IValidator(validator).validateUserOp(userOp, userOpHash);
-        }
 
         // pay back gas to EntryPoint
         if (missingAccountFunds > 0) {
@@ -286,13 +277,14 @@ contract Safe7579Launchpad is IAccount, SafeStorage, ISafeOp {
     )
         internal
         view
-        returns (bool validSig, uint48 validAfter, uint48 validUntil)
+        returns (bool, uint48, uint48)
     {
-        bytes memory operationData;
-        bytes calldata signatures;
-
-        (operationData, validAfter, validUntil, signatures) = _getSafeOp(userOp);
-
+        (
+            bytes memory operationData,
+            uint48 validAfter,
+            uint48 validUntil,
+            bytes calldata signatures
+        ) = _getSafeOp(userOp, SUPPORTED_ENTRYPOINT);
         bytes32 _hash = keccak256(operationData);
 
         InitData memory safeSetupCallData = abi.decode(userOp.callData[4:], (InitData));
@@ -303,17 +295,16 @@ contract Safe7579Launchpad is IAccount, SafeStorage, ISafeOp {
         owners.insertionSort();
         owners.uniquifySorted();
 
-        uint256 length = owners.length;
+        uint256 ownersLength = owners.length;
 
         uint256 validSigs;
-        for (uint256 i; i < length; i++) {
+        for (uint256 i; i < ownersLength; i++) {
             (bool found,) = signers.searchSorted(owners[i]);
             if (found) {
-                uint256 _validSigs = validSigs + 1;
-                if (_validSigs >= safeSetupCallData.threshold) {
+                validSigs++;
+                if (validSigs >= safeSetupCallData.threshold) {
                     return (true, validAfter, validUntil);
                 }
-                validSigs = _validSigs;
             }
         }
         return (false, validAfter, validUntil);
@@ -438,97 +429,4 @@ contract Safe7579Launchpad is IAccount, SafeStorage, ISafeOp {
             )
         );
     }
-
-    /**
-     * @dev Decodes an ERC-4337 user operation into a Safe operation.
-     * @param userOp The ERC-4337 user operation.
-     * @return operationData Encoded EIP-712 Safe operation data bytes used for signature
-     * verification.
-     * @return validAfter The timestamp the user operation is valid from.
-     * @return validUntil The timestamp the user operation is valid until.
-     * @return signatures The Safe owner signatures extracted from the user operation.
-     */
-    function _getSafeOp(PackedUserOperation calldata userOp)
-        public
-        view
-        returns (
-            bytes memory operationData,
-            uint48 validAfter,
-            uint48 validUntil,
-            bytes calldata signatures
-        )
-    {
-        // Extract additional Safe operation fields from the user operation signature which is
-        // encoded as:
-        // `abi.encodePacked(validAfter, validUntil, signatures)`
-        {
-            bytes calldata sig = userOp.signature;
-            validAfter = uint48(bytes6(sig[0:6]));
-            validUntil = uint48(bytes6(sig[6:12]));
-            signatures = sig[12:];
-        }
-
-        // It is important that **all** user operation fields are represented in the `SafeOp` data
-        // somehow, to prevent
-        // user operations from being submitted that do not fully respect the user preferences. The
-        // only exception is
-        // the `signature` bytes. Note that even `initCode` needs to be represented in the operation
-        // data, otherwise
-        // it can be replaced with a more expensive initialization that would charge the user
-        // additional fees.
-        {
-            // In order to work around Solidity "stack too deep" errors related to too many stack
-            // variables, manually
-            // encode the `SafeOp` fields into a memory `struct` for computing the EIP-712
-            // struct-hash. This works
-            // because the `EncodedSafeOpStruct` struct has no "dynamic" fields so its memory layout
-            // is identical to the
-            // result of `abi.encode`-ing the individual fields.
-            EncodedSafeOpStruct memory encodedSafeOp = EncodedSafeOpStruct({
-                typeHash: SAFE_OP_TYPEHASH,
-                safe: userOp.sender,
-                nonce: userOp.nonce,
-                initCodeHash: keccak256(userOp.initCode),
-                callDataHash: keccak256(userOp.callData),
-                verificationGasLimit: uint128(userOp.unpackVerificationGasLimit()),
-                callGasLimit: uint128(userOp.unpackCallGasLimit()),
-                preVerificationGas: userOp.preVerificationGas,
-                maxPriorityFeePerGas: uint128(userOp.unpackMaxPriorityFeePerGas()),
-                maxFeePerGas: uint128(userOp.unpackMaxFeePerGas()),
-                paymasterAndDataHash: keccak256(userOp.paymasterAndData),
-                validAfter: validAfter,
-                validUntil: validUntil,
-                entryPoint: SUPPORTED_ENTRYPOINT
-            });
-
-            bytes32 safeOpStructHash;
-            // solhint-disable-next-line no-inline-assembly
-            assembly ("memory-safe") {
-                // Since the `encodedSafeOp` value's memory layout is identical to the result of
-                // `abi.encode`-ing the
-                // individual `SafeOp` fields, we can pass it directly to `keccak256`. Additionally,
-                // there are 14
-                // 32-byte fields to hash, for a length of `14 * 32 = 448` bytes.
-                safeOpStructHash := keccak256(encodedSafeOp, 448)
-            }
-
-            operationData =
-                abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), safeOpStructHash);
-        }
-    }
-
-    function domainSeparator() public view returns (bytes32) {
-        bytes32 DOMAIN_SEPARATOR_TYPEHASH =
-            0x47e79534a245952e8b16893a336b85a3d9ea9fa8c573f3d803afb92a79469218;
-        return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, getChainId(), this));
-    }
-
-    function getChainId() public view returns (uint256) {
-        uint256 id;
-        // solhint-disable-next-line no-inline-assembly
-        assembly {
-            id := chainid()
-        }
-        return id;
-    }
 }