From fb71c43da267527fd34adcdc32c9162a9f0f4d1a Mon Sep 17 00:00:00 2001 From: zer0dot Date: Tue, 30 Jul 2024 21:51:46 -0400 Subject: [PATCH 01/34] feat: implement single signer fallback validation --- src/account/AccountFactory.sol | 21 +++++- src/account/AccountStorage.sol | 2 + src/account/UpgradeableModularAccount.sol | 87 +++++++++++++++++++++-- test/account/FallbackValidationTest.t.sol | 77 ++++++++++++++++++++ test/mocks/SingleSignerFactoryFixture.sol | 16 +++++ 5 files changed, 197 insertions(+), 6 deletions(-) create mode 100644 test/account/FallbackValidationTest.t.sol diff --git a/src/account/AccountFactory.sol b/src/account/AccountFactory.sol index e768dee4..554f74fe 100644 --- a/src/account/AccountFactory.sol +++ b/src/account/AccountFactory.sol @@ -63,8 +63,25 @@ contract AccountFactory is Ownable { return UpgradeableModularAccount(payable(addr)); } - function addStake(uint32 unstakeDelay) external payable onlyOwner { - ENTRY_POINT.addStake{value: msg.value}(unstakeDelay); + function createAccountWithFallbackValidation(address owner, uint256 salt) + public + returns (UpgradeableModularAccount) + { + // The entityId for fallback validations is hardcoded at the maximum value. + address addr = Create2.computeAddress(getSalt(owner, salt, type(uint32).max), _PROXY_BYTECODE_HASH); + + // short circuit if exists + if (addr.code.length == 0) { + // not necessary to check return addr since next call will fail if so + new ERC1967Proxy{salt: getSalt(owner, salt, type(uint32).max)}(address(ACCOUNT_IMPL), ""); + UpgradeableModularAccount(payable(addr)).initialize(owner); + } + + return UpgradeableModularAccount(payable(addr)); + } + + function addStake() external payable onlyOwner { + ENTRY_POINT.addStake{value: msg.value}(UNSTAKE_DELAY); } function unlockStake() external onlyOwner { diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index f488e325..fb48a9df 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -41,6 +41,8 @@ struct AccountStorage { // AccountStorageInitializable variables uint8 initialized; bool initializing; + // Address for fallback single signer validation + address fallbackSigner; // Execution functions and their associated functions mapping(bytes4 selector => ExecutionData) executionData; mapping(ModuleEntity validationFunction => ValidationData) validationData; diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 2904e1e6..4e1303b9 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -34,6 +34,10 @@ import {AccountStorage, getAccountStorage, toHookConfig, toSetValue} from "./Acc import {AccountStorageInitializable} from "./AccountStorageInitializable.sol"; import {ModuleManagerInternals} from "./ModuleManagerInternals.sol"; +import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; +import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; +import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; + contract UpgradeableModularAccount is AccountExecutor, AccountLoupe, @@ -51,6 +55,8 @@ contract UpgradeableModularAccount is using ValidationConfigLib for ValidationConfig; using HookConfigLib for HookConfig; using SparseCalldataSegmentLib for bytes; + using MessageHashUtils for bytes32; + using ECDSA for bytes32; struct PostExecToRun { bytes preExecHookReturnData; @@ -67,6 +73,14 @@ contract UpgradeableModularAccount is bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e; bytes4 internal constant _1271_INVALID = 0xffffffff; + uint256 internal constant _SIG_VALIDATION_PASSED = 0; + uint256 internal constant _SIG_VALIDATION_FAILED = 1; + + ModuleEntity internal constant _FALLBACK_VALIDATION = ModuleEntity.wrap(bytes24(type(uint192).max)); + + event ModularAccountInitialized(IEntryPoint indexed entryPoint); + event FallbackSignerSet(address indexed previousFallbackSigner, address indexed newFallbackSigner); + error NonCanonicalEncoding(); error NotEntryPoint(); error PostExecHookReverted(address module, uint32 entityId, bytes revertReason); @@ -250,6 +264,19 @@ contract UpgradeableModularAccount is _installValidation(validationConfig, selectors, installData, hooks); } + function initialize(address fallbackSigner) external initializer { + getAccountStorage().fallbackSigner = fallbackSigner; + emit ModularAccountInitialized(_ENTRY_POINT); + emit FallbackSignerSet(address(0), fallbackSigner); + } + + function updateFallbackSigner(address fallbackSigner) external wrapNativeFunction { + AccountStorage storage _storage = getAccountStorage(); + + emit FallbackSignerSet(_storage.fallbackSigner, fallbackSigner); + _storage.fallbackSigner = fallbackSigner; + } + /// @inheritdoc IModuleManager /// @notice May be validated by a global validation. function installValidation( @@ -303,6 +330,11 @@ contract UpgradeableModularAccount is ModuleEntity sigValidation = ModuleEntity.wrap(bytes24(signature)); + if (sigValidation.eq(_FALLBACK_VALIDATION)) { + // do sig validation + return _fallbackSignatureValidation(hash, signature[24:]); + } + (address module, uint32 entityId) = sigValidation.unpack(); if (!_storage.validationData[sigValidation].isSignatureValidation) { revert SignatureValidationInvalid(module, entityId); @@ -413,8 +445,15 @@ contract UpgradeableModularAccount is userOp.signature = signatureSegment.getBody(); - (address module, uint32 entityId) = userOpValidationFunction.unpack(); - uint256 currentValidationRes = IValidationModule(module).validateUserOp(entityId, userOp, userOpHash); + uint256 currentValidationRes; + if (userOpValidationFunction.eq(_FALLBACK_VALIDATION)) { + // fallback userop validation + currentValidationRes = _fallbackUserOpValidation(userOp, userOpHash); + } else { + (address module, uint32 entityId) = userOpValidationFunction.unpack(); + + currentValidationRes = IValidation(module).validateUserOp(entityId, userOp, userOpHash); + } if (preUserOpValidationHooks.length != 0) { // If we have other validation data we need to coalesce with @@ -467,6 +506,11 @@ contract UpgradeableModularAccount is revert ValidationSignatureSegmentMissing(); } + if (runtimeValidationFunction.eq(_FALLBACK_VALIDATION)) { + _fallbackRuntimeValidation(); + return; + } + (address module, uint32 entityId) = runtimeValidationFunction.unpack(); try IValidationModule(module).validateRuntime( @@ -701,9 +745,16 @@ contract UpgradeableModularAccount is // Check that the provided validation function is applicable to the selector if (isGlobal) { - if (!_globalValidationAllowed(selector) || !_storage.validationData[validationFunction].isGlobal) { - revert ValidationFunctionMissing(selector); + if ( + _globalValidationAllowed(selector) + && ( + _storage.validationData[validationFunction].isGlobal + || validationFunction.eq(_FALLBACK_VALIDATION) + ) + ) { + return; } + revert ValidationFunctionMissing(selector); } else { // Not global validation, but per-selector if (!getAccountStorage().validationData[validationFunction].selectors.contains(toSetValue(selector))) { @@ -711,4 +762,32 @@ contract UpgradeableModularAccount is } } } + + function _fallbackRuntimeValidation() internal view { + require(msg.sender == getAccountStorage().fallbackSigner, "temp"); + } + + function _fallbackUserOpValidation(PackedUserOperation memory userOp, bytes32 userOpHash) + internal + view + returns (uint256) + { + // Validate the user op signature against the owner. + (address sigSigner,,) = (userOpHash.toEthSignedMessageHash()).tryRecover(userOp.signature); + if (sigSigner == address(0) || sigSigner != getAccountStorage().fallbackSigner) { + return _SIG_VALIDATION_FAILED; + } + return _SIG_VALIDATION_PASSED; + } + + function _fallbackSignatureValidation(bytes32 digest, bytes calldata signature) + internal + view + returns (bytes4) + { + if (SignatureChecker.isValidSignatureNow(getAccountStorage().fallbackSigner, digest, signature)) { + return _1271_MAGIC_VALUE; + } + return _1271_INVALID; + } } diff --git a/test/account/FallbackValidationTest.t.sol b/test/account/FallbackValidationTest.t.sol new file mode 100644 index 00000000..9dc8572e --- /dev/null +++ b/test/account/FallbackValidationTest.t.sol @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.25; + +import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; +import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; + +import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; + +import {ModuleEntityLib} from "../../src/helpers/ModuleEntityLib.sol"; +import {ModuleEntity} from "../../src/interfaces/IModuleManager.sol"; + +import {AccountTestBase} from "../utils/AccountTestBase.sol"; + +contract FallbackValidationTest is AccountTestBase { + using MessageHashUtils for bytes32; + + address public ethRecipient; + + // A separate account and owner that isn't deployed yet, used to test initcode + address public owner2; + uint256 public owner2Key; + UpgradeableModularAccount public account2; + + ModuleEntity constant FALLBACK_VALIDATION = ModuleEntity.wrap(bytes24(type(uint192).max)); + + function setUp() public { + (owner2, owner2Key) = makeAddrAndKey("owner2"); + + // Compute counterfactual address + account2 = UpgradeableModularAccount(payable(factory.getAddress(owner2, 0))); + vm.deal(address(account2), 100 ether); + + ethRecipient = makeAddr("ethRecipient"); + vm.deal(ethRecipient, 1 wei); + } + + function test_fallbackValidation_userOp_simple() public { + PackedUserOperation memory userOp = PackedUserOperation({ + sender: address(account2), + nonce: 0, + initCode: abi.encodePacked( + address(factory), abi.encodeCall(factory.createAccountWithFallbackValidation, (owner2, 0)) + ), + callData: abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, "")), + accountGasLimits: _encodeGas(VERIFICATION_GAS_LIMIT, CALL_GAS_LIMIT), + preVerificationGas: 0, + gasFees: _encodeGas(1, 1), + paymasterAndData: "", + signature: "" + }); + + // Generate signature + bytes32 userOpHash = entryPoint.getUserOpHash(userOp); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner2Key, userOpHash.toEthSignedMessageHash()); + userOp.signature = _encodeSignature(FALLBACK_VALIDATION, GLOBAL_VALIDATION, abi.encodePacked(r, s, v)); + + PackedUserOperation[] memory userOps = new PackedUserOperation[](1); + userOps[0] = userOp; + + entryPoint.handleOps(userOps, beneficiary); + + assertEq(ethRecipient.balance, 2 wei); + } + + function test_fallbackValidation_runtime_simple() public { + // Deploy the account first + factory.createAccountWithFallbackValidation(owner2, 0); + + vm.prank(owner2); + account2.executeWithAuthorization( + abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, "")), + _encodeSignature(FALLBACK_VALIDATION, GLOBAL_VALIDATION, "") + ); + + assertEq(ethRecipient.balance, 2 wei); + } +} diff --git a/test/mocks/SingleSignerFactoryFixture.sol b/test/mocks/SingleSignerFactoryFixture.sol index 38453951..6d939697 100644 --- a/test/mocks/SingleSignerFactoryFixture.sol +++ b/test/mocks/SingleSignerFactoryFixture.sol @@ -63,6 +63,22 @@ contract SingleSignerFactoryFixture is OptimizedTest { return UpgradeableModularAccount(payable(addr)); } + function createAccountWithFallbackValidation(address owner, uint256 salt) + public + returns (UpgradeableModularAccount) + { + address addr = Create2.computeAddress(getSalt(owner, salt), _PROXY_BYTECODE_HASH); + + // short circuit if exists + if (addr.code.length == 0) { + // not necessary to check return addr since next call will fail if so + new ERC1967Proxy{salt: getSalt(owner, salt)}(address(accountImplementation), ""); + UpgradeableModularAccount(payable(addr)).initialize(owner); + } + + return UpgradeableModularAccount(payable(addr)); + } + /** * calculate the counterfactual address of this account as it would be returned by createAccount() */ From 1977bd7d7b005ee95910578f72306dfcfe82d57f Mon Sep 17 00:00:00 2001 From: zer0dot Date: Tue, 30 Jul 2024 22:18:24 -0400 Subject: [PATCH 02/34] chore: use custom error for fallback signer mismatch --- src/account/UpgradeableModularAccount.sol | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 4e1303b9..fa7a5a50 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -95,6 +95,7 @@ contract UpgradeableModularAccount is error ValidationFunctionMissing(bytes4 selector); error ValidationSignatureSegmentMissing(); error SignatureSegmentOutOfOrder(); + error FallbackSignerMismatch(); // Wraps execution of a native function with runtime validation and hooks // Used for upgradeTo, upgradeToAndCall, execute, executeBatch, installExecution, uninstallExecution @@ -764,7 +765,9 @@ contract UpgradeableModularAccount is } function _fallbackRuntimeValidation() internal view { - require(msg.sender == getAccountStorage().fallbackSigner, "temp"); + if (msg.sender != getAccountStorage().fallbackSigner) { + revert FallbackSignerMismatch(); + } } function _fallbackUserOpValidation(PackedUserOperation memory userOp, bytes32 userOpHash) From 76e0478b333e20043516d06caf6287823531e1ca Mon Sep 17 00:00:00 2001 From: zer0dot Date: Tue, 30 Jul 2024 22:19:01 -0400 Subject: [PATCH 03/34] chore: modify create function visibility --- src/account/AccountFactory.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/account/AccountFactory.sol b/src/account/AccountFactory.sol index 554f74fe..b1a06315 100644 --- a/src/account/AccountFactory.sol +++ b/src/account/AccountFactory.sol @@ -64,7 +64,7 @@ contract AccountFactory is Ownable { } function createAccountWithFallbackValidation(address owner, uint256 salt) - public + external returns (UpgradeableModularAccount) { // The entityId for fallback validations is hardcoded at the maximum value. From 587c9a91de3c358cb26263db2575b171ec8423bb Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 31 Jul 2024 16:40:04 -0400 Subject: [PATCH 04/34] forge install solady --- .gitmodules | 3 +++ lib/solady | 1 + 2 files changed, 4 insertions(+) create mode 160000 lib/solady diff --git a/.gitmodules b/.gitmodules index 05bd137f..229aff1b 100644 --- a/.gitmodules +++ b/.gitmodules @@ -10,3 +10,6 @@ [submodule "lib/modular-account-libs"] path = lib/modular-account-libs url = https://github.com/erc6900/modular-account-libs +[submodule "lib/solady"] + path = lib/solady + url = https://github.com/vectorized/solady diff --git a/lib/solady b/lib/solady new file mode 160000 index 00000000..a1f9be98 --- /dev/null +++ b/lib/solady @@ -0,0 +1 @@ +Subproject commit a1f9be988d3c12655692cb8cdfc6864cc393cff6 From 1c0adfde5915a4879767ab390a2b5d0ebfbd6ec1 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 31 Jul 2024 18:12:27 -0400 Subject: [PATCH 05/34] feat: use an appended bytecode address as an initial fallback signer --- remappings.txt | 3 +- src/account/AccountFactory.sol | 29 ++++++++++++++---- src/account/AccountStorage.sol | 3 ++ src/account/UpgradeableModularAccount.sol | 36 ++++++++++++++++------- test/account/FallbackValidationTest.t.sol | 2 +- test/mocks/SingleSignerFactoryFixture.sol | 31 +++++++++++++++++-- 6 files changed, 84 insertions(+), 20 deletions(-) diff --git a/remappings.txt b/remappings.txt index bc2ce0be..8d9639cf 100644 --- a/remappings.txt +++ b/remappings.txt @@ -2,4 +2,5 @@ ds-test/=lib/forge-std/lib/ds-test/src/ forge-std/=lib/forge-std/src/ @eth-infinitism/account-abstraction/=lib/account-abstraction/contracts/ @openzeppelin/=lib/openzeppelin-contracts/ -@modular-account-libs/=lib/modular-account-libs/src/ \ No newline at end of file +@modular-account-libs/=lib/modular-account-libs/src/ +solady=lib/solady/src/ diff --git a/src/account/AccountFactory.sol b/src/account/AccountFactory.sol index b1a06315..6af31484 100644 --- a/src/account/AccountFactory.sol +++ b/src/account/AccountFactory.sol @@ -10,6 +10,8 @@ import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; import {UpgradeableModularAccount} from "../account/UpgradeableModularAccount.sol"; import {ValidationConfigLib} from "../helpers/ValidationConfigLib.sol"; +import {LibClone} from "solady/utils/LibClone.sol"; + contract AccountFactory is Ownable { UpgradeableModularAccount public immutable ACCOUNT_IMPL; bytes32 private immutable _PROXY_BYTECODE_HASH; @@ -64,17 +66,20 @@ contract AccountFactory is Ownable { } function createAccountWithFallbackValidation(address owner, uint256 salt) - external + external returns (UpgradeableModularAccount) { - // The entityId for fallback validations is hardcoded at the maximum value. - address addr = Create2.computeAddress(getSalt(owner, salt, type(uint32).max), _PROXY_BYTECODE_HASH); + // both module address and entityId for fallback validations are hardcoded at the maximum value. + bytes32 fullSalt = getSalt(owner, salt, type(uint32).max); + + bytes memory immutables = _getImmutableArgs(owner); + + address addr = _getAddressFallbackSigner(immutables, fullSalt); // short circuit if exists if (addr.code.length == 0) { // not necessary to check return addr since next call will fail if so - new ERC1967Proxy{salt: getSalt(owner, salt, type(uint32).max)}(address(ACCOUNT_IMPL), ""); - UpgradeableModularAccount(payable(addr)).initialize(owner); + LibClone.createDeterministicERC1967(address(ACCOUNT_IMPL), immutables, fullSalt); } return UpgradeableModularAccount(payable(addr)); @@ -99,7 +104,21 @@ contract AccountFactory is Ownable { return Create2.computeAddress(getSalt(owner, salt, entityId), _PROXY_BYTECODE_HASH); } + function getAddressFallbackSigner(address owner, uint256 salt) public view returns (address) { + bytes32 fullSalt = getSalt(owner, salt, type(uint32).max); + bytes memory immutables = _getImmutableArgs(owner); + return _getAddressFallbackSigner(immutables, fullSalt); + } + function getSalt(address owner, uint256 salt, uint32 entityId) public pure returns (bytes32) { return keccak256(abi.encodePacked(owner, salt, entityId)); } + + function _getAddressFallbackSigner(bytes memory immutables, bytes32 salt) public view returns (address) { + return LibClone.predictDeterministicAddressERC1967(address(ACCOUNT_IMPL), immutables, salt, address(this)); + } + + function _getImmutableArgs(address owner) private pure returns (bytes memory) { + return abi.encodePacked(owner); + } } diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index fb48a9df..f5f4e49d 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -43,6 +43,9 @@ struct AccountStorage { bool initializing; // Address for fallback single signer validation address fallbackSigner; + // Whether or not the fallback signer is enabled, we can't use a zero fallbackSigner for this since it defaults + // to reading the bytecode-appended signer. + bool fallbackSignerDisabled; // Execution functions and their associated functions mapping(bytes4 selector => ExecutionData) executionData; mapping(ModuleEntity validationFunction => ValidationData) validationData; diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index fa7a5a50..df21246d 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -38,6 +38,8 @@ import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; +import {LibClone} from "solady/utils/LibClone.sol"; + contract UpgradeableModularAccount is AccountExecutor, AccountLoupe, @@ -96,6 +98,7 @@ contract UpgradeableModularAccount is error ValidationSignatureSegmentMissing(); error SignatureSegmentOutOfOrder(); error FallbackSignerMismatch(); + error FallbackSignerDisabled(); // Wraps execution of a native function with runtime validation and hooks // Used for upgradeTo, upgradeToAndCall, execute, executeBatch, installExecution, uninstallExecution @@ -252,8 +255,10 @@ contract UpgradeableModularAccount is _uninstallExecution(module, manifest, moduleUninstallData); } + // TODO: Remove this function for SMA /// @notice Initializes the account with a validation function added to the global pool. - /// TODO: remove and merge with regular initialization, after we figure out a better install/uninstall workflow + /// TODO: remove and merge with regular initialization, after we figure out a better install/uninstall + // workflow /// with user install configs. /// @dev This function is only callable once. function initializeWithValidation( @@ -265,12 +270,6 @@ contract UpgradeableModularAccount is _installValidation(validationConfig, selectors, installData, hooks); } - function initialize(address fallbackSigner) external initializer { - getAccountStorage().fallbackSigner = fallbackSigner; - emit ModularAccountInitialized(_ENTRY_POINT); - emit FallbackSignerSet(address(0), fallbackSigner); - } - function updateFallbackSigner(address fallbackSigner) external wrapNativeFunction { AccountStorage storage _storage = getAccountStorage(); @@ -765,7 +764,7 @@ contract UpgradeableModularAccount is } function _fallbackRuntimeValidation() internal view { - if (msg.sender != getAccountStorage().fallbackSigner) { + if (msg.sender != _getFallbackSigner()) { revert FallbackSignerMismatch(); } } @@ -777,7 +776,7 @@ contract UpgradeableModularAccount is { // Validate the user op signature against the owner. (address sigSigner,,) = (userOpHash.toEthSignedMessageHash()).tryRecover(userOp.signature); - if (sigSigner == address(0) || sigSigner != getAccountStorage().fallbackSigner) { + if (sigSigner == address(0) || sigSigner != _getFallbackSigner()) { return _SIG_VALIDATION_FAILED; } return _SIG_VALIDATION_PASSED; @@ -788,9 +787,26 @@ contract UpgradeableModularAccount is view returns (bytes4) { - if (SignatureChecker.isValidSignatureNow(getAccountStorage().fallbackSigner, digest, signature)) { + if (SignatureChecker.isValidSignatureNow(_getFallbackSigner(), digest, signature)) { return _1271_MAGIC_VALUE; } return _1271_INVALID; } + + function _getFallbackSigner() internal view returns (address) { + AccountStorage storage _storage = getAccountStorage(); + + if (_storage.fallbackSignerDisabled) { + revert FallbackSignerDisabled(); + } + + address storageFallbackSigner = _storage.fallbackSigner; + if (storageFallbackSigner != address(0)) { + return storageFallbackSigner; + } + + bytes memory appendedData = LibClone.argsOnERC1967(address(this), 0, 20); + + return address(uint160(bytes20(appendedData))); + } } diff --git a/test/account/FallbackValidationTest.t.sol b/test/account/FallbackValidationTest.t.sol index 9dc8572e..f4efc4b2 100644 --- a/test/account/FallbackValidationTest.t.sol +++ b/test/account/FallbackValidationTest.t.sol @@ -27,7 +27,7 @@ contract FallbackValidationTest is AccountTestBase { (owner2, owner2Key) = makeAddrAndKey("owner2"); // Compute counterfactual address - account2 = UpgradeableModularAccount(payable(factory.getAddress(owner2, 0))); + account2 = UpgradeableModularAccount(payable(factory.getAddressFallbackSigner(owner2, 0))); vm.deal(address(account2), 100 ether); ethRecipient = makeAddr("ethRecipient"); diff --git a/test/mocks/SingleSignerFactoryFixture.sol b/test/mocks/SingleSignerFactoryFixture.sol index 6d939697..c0dfbe64 100644 --- a/test/mocks/SingleSignerFactoryFixture.sol +++ b/test/mocks/SingleSignerFactoryFixture.sol @@ -12,6 +12,8 @@ import {SingleSignerValidationModule} from "../../src/modules/validation/SingleS import {OptimizedTest} from "../utils/OptimizedTest.sol"; import {TEST_DEFAULT_VALIDATION_ENTITY_ID} from "../utils/TestConstants.sol"; +import {LibClone} from "solady/utils/LibClone.sol"; + contract SingleSignerFactoryFixture is OptimizedTest { UpgradeableModularAccount public accountImplementation; SingleSignerValidationModule public singleSignerValidationModule; @@ -33,6 +35,8 @@ contract SingleSignerFactoryFixture is OptimizedTest { self = address(this); } + // TODO: Make createAccount use createAccountFallbackSigner for testing + /** * create an account, and return its address. * returns the address even if the account is already deployed. @@ -67,13 +71,18 @@ contract SingleSignerFactoryFixture is OptimizedTest { public returns (UpgradeableModularAccount) { - address addr = Create2.computeAddress(getSalt(owner, salt), _PROXY_BYTECODE_HASH); + bytes32 fullSalt = getSalt(owner, salt); + + bytes memory immutables = _getImmutableArgs(owner); + + address addr = _getAddressFallbackSigner(immutables, fullSalt); // short circuit if exists if (addr.code.length == 0) { // not necessary to check return addr since next call will fail if so - new ERC1967Proxy{salt: getSalt(owner, salt)}(address(accountImplementation), ""); - UpgradeableModularAccount(payable(addr)).initialize(owner); + // new ERC1967Proxy{salt: getSalt(owner, salt, type(uint32).max)}(address(ACCOUNT_IMPL), ""); + // UpgradeableModularAccount(payable(addr)).initialize(owner); + LibClone.createDeterministicERC1967(address(accountImplementation), immutables, fullSalt); } return UpgradeableModularAccount(payable(addr)); @@ -86,6 +95,12 @@ contract SingleSignerFactoryFixture is OptimizedTest { return Create2.computeAddress(getSalt(owner, salt), _PROXY_BYTECODE_HASH); } + function getAddressFallbackSigner(address owner, uint256 salt) public view returns (address) { + bytes32 fullSalt = getSalt(owner, salt); + bytes memory immutables = _getImmutableArgs(owner); + return _getAddressFallbackSigner(immutables, fullSalt); + } + function addStake() external payable { entryPoint.addStake{value: msg.value}(UNSTAKE_DELAY); } @@ -93,4 +108,14 @@ contract SingleSignerFactoryFixture is OptimizedTest { function getSalt(address owner, uint256 salt) public pure returns (bytes32) { return keccak256(abi.encodePacked(owner, salt)); } + + function _getAddressFallbackSigner(bytes memory immutables, bytes32 salt) internal view returns (address) { + return LibClone.predictDeterministicAddressERC1967( + address(accountImplementation), immutables, salt, address(this) + ); + } + + function _getImmutableArgs(address owner) private pure returns (bytes memory) { + return abi.encodePacked(owner); + } } From a1eb3a9b525102d2c801a3aeaa7ead914081934b Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 31 Jul 2024 20:55:34 -0400 Subject: [PATCH 06/34] test: modify tests to use fallback signer rather than single signer validation --- src/account/UpgradeableModularAccount.sol | 2 +- test/account/AccountReturnData.t.sol | 12 +---- test/account/ImmutableAppend.t.sol | 36 +++++++++++++++ test/account/MultiValidation.t.sol | 3 +- test/account/PerHookData.t.sol | 5 ++- test/account/UpgradeableModularAccount.t.sol | 29 +++++------- test/module/AllowlistModule.t.sol | 5 ++- test/utils/AccountTestBase.sol | 47 +++----------------- 8 files changed, 67 insertions(+), 72 deletions(-) create mode 100644 test/account/ImmutableAppend.t.sol diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index df21246d..ed512b5e 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -729,7 +729,7 @@ contract UpgradeableModularAccount is selector == this.execute.selector || selector == this.executeBatch.selector || selector == this.installExecution.selector || selector == this.uninstallExecution.selector || selector == this.installValidation.selector || selector == this.uninstallValidation.selector - || selector == this.upgradeToAndCall.selector + || selector == this.upgradeToAndCall.selector || selector == this.updateFallbackSigner.selector ) { return true; } diff --git a/test/account/AccountReturnData.t.sol b/test/account/AccountReturnData.t.sol index 5f4cb98d..4e5647cd 100644 --- a/test/account/AccountReturnData.t.sol +++ b/test/account/AccountReturnData.t.sol @@ -67,11 +67,7 @@ contract AccountReturnDataTest is AccountTestBase { account1.execute, (address(regularResultContract), 0, abi.encodeCall(RegularResultContract.foo, ())) ), - _encodeSignature( - ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID), - GLOBAL_VALIDATION, - "" - ) + _encodeSignature(_signerValidation, GLOBAL_VALIDATION, "") ); bytes32 result = abi.decode(abi.decode(returnData, (bytes)), (bytes32)); @@ -95,11 +91,7 @@ contract AccountReturnDataTest is AccountTestBase { bytes memory retData = account1.executeWithAuthorization( abi.encodeCall(account1.executeBatch, (calls)), - _encodeSignature( - ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID), - GLOBAL_VALIDATION, - "" - ) + _encodeSignature(_signerValidation, GLOBAL_VALIDATION, "") ); bytes[] memory returnDatas = abi.decode(retData, (bytes[])); diff --git a/test/account/ImmutableAppend.t.sol b/test/account/ImmutableAppend.t.sol new file mode 100644 index 00000000..e92fb688 --- /dev/null +++ b/test/account/ImmutableAppend.t.sol @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.19; + +import {IEntryPoint, UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; +import {ModuleEntity, ModuleEntityLib} from "../../src/helpers/ModuleEntityLib.sol"; +import {ValidationConfig, ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol"; +import {ExecutionHook} from "../../src/interfaces/IAccountLoupe.sol"; +import {Call, IStandardExecutor} from "../../src/interfaces/IStandardExecutor.sol"; +import {DirectCallModule} from "../mocks/modules/DirectCallModule.sol"; + +import {AccountTestBase} from "../utils/AccountTestBase.sol"; + +import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import {LibClone} from "solady/utils/LibClone.sol"; + +contract ImmutableAppendTest is AccountTestBase { + using ValidationConfigLib for ValidationConfig; + + /* -------------------------------------------------------------------------- */ + /* Negatives */ + /* -------------------------------------------------------------------------- */ + + /* -------------------------------------------------------------------------- */ + /* Positives */ + /* -------------------------------------------------------------------------- */ + + function test_success_getData() public { + bytes memory expectedArgs = abi.encodePacked(owner1); + + assertEq(keccak256(LibClone.argsOnERC1967(address(account1))), keccak256(expectedArgs)); + } + + /* -------------------------------------------------------------------------- */ + /* Internals */ + /* -------------------------------------------------------------------------- */ +} diff --git a/test/account/MultiValidation.t.sol b/test/account/MultiValidation.t.sol index 298f8e87..f24a2094 100644 --- a/test/account/MultiValidation.t.sol +++ b/test/account/MultiValidation.t.sol @@ -43,8 +43,7 @@ contract MultiValidationTest is AccountTestBase { ); ModuleEntity[] memory validations = new ModuleEntity[](2); - validations[0] = - ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID); + validations[0] = _signerValidation; validations[1] = ModuleEntityLib.pack(address(validator2), TEST_DEFAULT_VALIDATION_ENTITY_ID); bytes4[] memory selectors0 = account1.getValidationData(validations[0]).selectors; diff --git a/test/account/PerHookData.t.sol b/test/account/PerHookData.t.sol index cc3f3415..65691a01 100644 --- a/test/account/PerHookData.t.sol +++ b/test/account/PerHookData.t.sol @@ -8,7 +8,7 @@ import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/Messa import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {HookConfigLib} from "../../src/helpers/HookConfigLib.sol"; -import {ModuleEntity} from "../../src/helpers/ModuleEntityLib.sol"; +import {ModuleEntity, ModuleEntityLib} from "../../src/helpers/ModuleEntityLib.sol"; import {Counter} from "../mocks/Counter.sol"; import {MockAccessControlHookModule} from "../mocks/modules/MockAccessControlHookModule.sol"; @@ -22,6 +22,9 @@ contract PerHookDataTest is CustomValidationTestBase { Counter internal _counter; function setUp() public { + _signerValidation = + ModuleEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID); + _counter = new Counter(); _accessControlHookModule = new MockAccessControlHookModule(); diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index e4394f34..de2e1841 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -52,7 +52,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { (owner2, owner2Key) = makeAddrAndKey("owner2"); // Compute counterfactual address - account2 = UpgradeableModularAccount(payable(factory.getAddress(owner2, 0))); + account2 = UpgradeableModularAccount(payable(factory.getAddressFallbackSigner(owner2, 0))); vm.deal(address(account2), 100 ether); ethRecipient = makeAddr("ethRecipient"); @@ -95,17 +95,10 @@ contract UpgradeableModularAccountTest is AccountTestBase { PackedUserOperation memory userOp = PackedUserOperation({ sender: address(account2), nonce: 0, - initCode: abi.encodePacked(address(factory), abi.encodeCall(factory.createAccount, (owner2, 0))), - callData: abi.encodeCall( - UpgradeableModularAccount.execute, - ( - address(singleSignerValidationModule), - 0, - abi.encodeCall( - SingleSignerValidationModule.transferSigner, (TEST_DEFAULT_VALIDATION_ENTITY_ID, owner2) - ) - ) + initCode: abi.encodePacked( + address(factory), abi.encodeCall(factory.createAccountWithFallbackValidation, (owner2, 0)) ), + callData: abi.encodeCall(UpgradeableModularAccount.updateFallbackSigner, (owner2)), accountGasLimits: _encodeGas(VERIFICATION_GAS_LIMIT, CALL_GAS_LIMIT), preVerificationGas: 0, gasFees: _encodeGas(1, 2), @@ -130,7 +123,9 @@ contract UpgradeableModularAccountTest is AccountTestBase { PackedUserOperation memory userOp = PackedUserOperation({ sender: address(account2), nonce: 0, - initCode: abi.encodePacked(address(factory), abi.encodeCall(factory.createAccount, (owner2, 0))), + initCode: abi.encodePacked( + address(factory), abi.encodeCall(factory.createAccountWithFallbackValidation, (owner2, 0)) + ), callData: abi.encodeCall(UpgradeableModularAccount.execute, (recipient, 1 wei, "")), accountGasLimits: _encodeGas(VERIFICATION_GAS_LIMIT, CALL_GAS_LIMIT), preVerificationGas: 0, @@ -355,10 +350,11 @@ contract UpgradeableModularAccountTest is AccountTestBase { assertEq(address(account3), address(uint160(uint256(vm.load(address(account1), slot))))); } + // TODO: Consider if this test belongs here or in the tests specific to the SingleSignerValidation function test_transferOwnership() public { - assertEq( - singleSignerValidationModule.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), owner1 - ); + // Note: replaced "owner1" with address(0), this doesn't actually affect the account, but allows the test + // to pass by ensuring the signer can be set in the validation + assertEq(singleSignerValidation.signerOf(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), address(0)); vm.prank(address(entryPoint)); account1.execute( @@ -381,8 +377,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { // singleSignerValidationModule.ownerOf(address(account1)); - bytes memory signature = - abi.encodePacked(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID, r, s, v); + bytes memory signature = abi.encodePacked(_signerValidation, r, s, v); bytes4 validationResult = IERC1271(address(account1)).isValidSignature(message, signature); diff --git a/test/module/AllowlistModule.t.sol b/test/module/AllowlistModule.t.sol index 5f0f243c..c17a9bf9 100644 --- a/test/module/AllowlistModule.t.sol +++ b/test/module/AllowlistModule.t.sol @@ -6,7 +6,7 @@ import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntry import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {HookConfigLib} from "../../src/helpers/HookConfigLib.sol"; -import {ModuleEntity} from "../../src/helpers/ModuleEntityLib.sol"; +import {ModuleEntity, ModuleEntityLib} from "../../src/helpers/ModuleEntityLib.sol"; import {Call} from "../../src/interfaces/IStandardExecutor.sol"; import {AllowlistModule} from "../../src/modules/permissionhooks/AllowlistModule.sol"; @@ -34,6 +34,9 @@ contract AllowlistModuleTest is CustomValidationTestBase { ); function setUp() public { + _signerValidation = + ModuleEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID); + allowlistModule = new AllowlistModule(); counters = new Counter[](10); diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index 8641f79b..34824635 100644 --- a/test/utils/AccountTestBase.sol +++ b/test/utils/AccountTestBase.sol @@ -56,11 +56,10 @@ abstract contract AccountTestBase is OptimizedTest { singleSignerValidationModule = _deploySingleSignerValidationModule(); factory = new SingleSignerFactoryFixture(entryPoint, singleSignerValidationModule); - account1 = factory.createAccount(owner1, 0); + account1 = factory.createAccountWithFallbackValidation(owner1, 0); vm.deal(address(account1), 100 ether); - _signerValidation = - ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID); + _signerValidation = ModuleEntity.wrap(bytes24(type(uint192).max)); } function _runExecUserOp(address target, bytes memory callData) internal { @@ -102,11 +101,7 @@ abstract contract AccountTestBase is OptimizedTest { bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = _encodeSignature( - ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID), - GLOBAL_VALIDATION, - abi.encodePacked(r, s, v) - ); + userOp.signature = _encodeSignature(_signerValidation, GLOBAL_VALIDATION, abi.encodePacked(r, s, v)); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); userOps[0] = userOp; @@ -153,14 +148,7 @@ abstract contract AccountTestBase is OptimizedTest { } vm.prank(owner1); - account1.executeWithAuthorization( - callData, - _encodeSignature( - ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID), - GLOBAL_VALIDATION, - "" - ) - ); + account1.executeWithAuthorization(callData, _encodeSignature(_signerValidation, GLOBAL_VALIDATION, "")); } // Always expects a revert, even if the revert data is zero-length. @@ -168,36 +156,15 @@ abstract contract AccountTestBase is OptimizedTest { vm.expectRevert(expectedRevertData); vm.prank(owner1); - account1.executeWithAuthorization( - callData, - _encodeSignature( - ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID), - GLOBAL_VALIDATION, - "" - ) - ); + account1.executeWithAuthorization(callData, _encodeSignature(_signerValidation, GLOBAL_VALIDATION, "")); } function _transferOwnershipToTest() internal { // Transfer ownership to test contract for easier invocation. vm.prank(owner1); account1.executeWithAuthorization( - abi.encodeCall( - account1.execute, - ( - address(singleSignerValidationModule), - 0, - abi.encodeCall( - SingleSignerValidationModule.transferSigner, - (TEST_DEFAULT_VALIDATION_ENTITY_ID, address(this)) - ) - ) - ), - _encodeSignature( - ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID), - GLOBAL_VALIDATION, - "" - ) + abi.encodeCall(account1.updateFallbackSigner, (address(this))), + _encodeSignature(_signerValidation, GLOBAL_VALIDATION, "") ); } From e6e69de68a8310386b1600435296eaa3e9df84d2 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 31 Jul 2024 21:03:58 -0400 Subject: [PATCH 07/34] feat: setter for enabling or disabling fallback signer --- src/account/UpgradeableModularAccount.sol | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index ed512b5e..3d795b70 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -277,6 +277,12 @@ contract UpgradeableModularAccount is _storage.fallbackSigner = fallbackSigner; } + function setFallbackSignerEnabled(bool enabled) external wrapNativeFunction { + AccountStorage storage _storage = getAccountStorage(); + _storage.fallbackSignerDisabled = !enabled; + // TODO: event + } + /// @inheritdoc IModuleManager /// @notice May be validated by a global validation. function installValidation( From 10074f85916e8af99632ec29ede22ab137e0813c Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 7 Aug 2024 14:29:15 -0400 Subject: [PATCH 08/34] fix: fix rebase inconsistencies --- src/account/AccountFactory.sol | 4 ++-- test/account/ImmutableAppend.t.sol | 1 - test/account/UpgradeableModularAccount.t.sol | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/account/AccountFactory.sol b/src/account/AccountFactory.sol index 6af31484..8dbea6bc 100644 --- a/src/account/AccountFactory.sol +++ b/src/account/AccountFactory.sol @@ -85,8 +85,8 @@ contract AccountFactory is Ownable { return UpgradeableModularAccount(payable(addr)); } - function addStake() external payable onlyOwner { - ENTRY_POINT.addStake{value: msg.value}(UNSTAKE_DELAY); + function addStake(uint32 unstakeDelay) external payable onlyOwner { + ENTRY_POINT.addStake{value: msg.value}(unstakeDelay); } function unlockStake() external onlyOwner { diff --git a/test/account/ImmutableAppend.t.sol b/test/account/ImmutableAppend.t.sol index e92fb688..a63f0408 100644 --- a/test/account/ImmutableAppend.t.sol +++ b/test/account/ImmutableAppend.t.sol @@ -4,7 +4,6 @@ pragma solidity ^0.8.19; import {IEntryPoint, UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {ModuleEntity, ModuleEntityLib} from "../../src/helpers/ModuleEntityLib.sol"; import {ValidationConfig, ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol"; -import {ExecutionHook} from "../../src/interfaces/IAccountLoupe.sol"; import {Call, IStandardExecutor} from "../../src/interfaces/IStandardExecutor.sol"; import {DirectCallModule} from "../mocks/modules/DirectCallModule.sol"; diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index de2e1841..66e30bb7 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -354,7 +354,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { function test_transferOwnership() public { // Note: replaced "owner1" with address(0), this doesn't actually affect the account, but allows the test // to pass by ensuring the signer can be set in the validation - assertEq(singleSignerValidation.signerOf(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), address(0)); + assertEq(singleSignerValidation.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), address(0)); vm.prank(address(entryPoint)); account1.execute( From 0ab4399a1e20bd81ff74eb9e34d2b4459c3cc67e Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 7 Aug 2024 15:50:55 -0400 Subject: [PATCH 09/34] fix: fix tests broken due to rebase --- test/account/ReplaceModule.t.sol | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/test/account/ReplaceModule.t.sol b/test/account/ReplaceModule.t.sol index 32d4c7d0..c70c20ac 100644 --- a/test/account/ReplaceModule.t.sol +++ b/test/account/ReplaceModule.t.sol @@ -86,11 +86,7 @@ contract UpgradeModuleTest is AccountTestBase { }); account1.executeWithAuthorization( abi.encodeCall(account1.executeBatch, (calls)), - _encodeSignature( - ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID), - GLOBAL_VALIDATION, - "" - ) + _encodeSignature(_signerValidation, GLOBAL_VALIDATION, "") ); // test installed, test if old module still installed @@ -189,11 +185,7 @@ contract UpgradeModuleTest is AccountTestBase { }); account1.executeWithAuthorization( abi.encodeCall(account1.executeBatch, (calls)), - _encodeSignature( - ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID), - GLOBAL_VALIDATION, - "" - ) + _encodeSignature(_signerValidation, GLOBAL_VALIDATION, "") ); // Test if old validation still works, expect fail From 61ccf2f606cb7afc8527555f966c8e0e62c9cbf6 Mon Sep 17 00:00:00 2001 From: Zer0dot Date: Mon, 12 Aug 2024 13:34:34 -0400 Subject: [PATCH 10/34] [DRAFT] refactor/SMA: Inheritable Account Refactor (#133) --- .env.example | 4 +- .github/workflows/test.yml | 10 +- src/account/AccountFactory.sol | 2 +- src/account/AccountStorage.sol | 5 - src/account/SemiModularAccount.sol | 162 ++++++++++++++ src/account/UpgradeableModularAccount.sol | 208 ++++++------------ .../SingleSignerValidationModule.sol | 9 +- test/account/AccountReturnData.t.sol | 2 - test/account/FallbackValidationTest.t.sol | 77 ------- test/account/ImmutableAppend.t.sol | 15 +- test/account/MultiValidation.t.sol | 2 +- test/account/PerHookData.t.sol | 6 +- test/account/ReplaceModule.t.sol | 1 - test/account/UpgradeableModularAccount.t.sol | 37 +++- test/mocks/SingleSignerFactoryFixture.sol | 12 +- test/module/AllowlistModule.t.sol | 6 +- .../module/SingleSignerValidationModule.t.sol | 2 +- test/utils/AccountTestBase.sol | 34 ++- test/utils/CustomValidationTestBase.sol | 25 ++- test/utils/OptimizedTest.sol | 11 + test/utils/TestConstants.sol | 2 +- 21 files changed, 360 insertions(+), 272 deletions(-) create mode 100644 src/account/SemiModularAccount.sol delete mode 100644 test/account/FallbackValidationTest.t.sol diff --git a/.env.example b/.env.example index 612c3436..a7bd543e 100644 --- a/.env.example +++ b/.env.example @@ -1,4 +1,3 @@ - # Factory owner capable only of managing stake OWNER= # EP 0.7 address @@ -22,3 +21,6 @@ UNSTAKE_DELAY= # Allowlist Module ALLOWLIST_MODULE= ALLOWLIST_MODULE_SALT= + +# Whether to test the semi-modular or full modular account +SMA_TEST=false diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index dfa5cf41..3187d269 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -66,7 +66,10 @@ jobs: run: FOUNDRY_PROFILE=optimized-build forge build - name: Run tests - run: FOUNDRY_PROFILE=optimized-test-deep forge test -vvv + run: FOUNDRY_PROFILE=optimized-test-deep SMA_TEST=false forge test -vvv + + - name: Run SMA tests + run: FOUNDRY_PROFILE=optimized-test-deep SMA_TEST=true forge test -vvv test-default: name: Run Forge Tests (default) @@ -88,4 +91,7 @@ jobs: run: forge build - name: Run tests - run: forge test -vvv + run: SMA_TEST=false forge test -vvv + + - name: Run SMA tests + run: SMA_TEST=true forge test -vvv diff --git a/src/account/AccountFactory.sol b/src/account/AccountFactory.sol index 8dbea6bc..285771f0 100644 --- a/src/account/AccountFactory.sol +++ b/src/account/AccountFactory.sol @@ -114,7 +114,7 @@ contract AccountFactory is Ownable { return keccak256(abi.encodePacked(owner, salt, entityId)); } - function _getAddressFallbackSigner(bytes memory immutables, bytes32 salt) public view returns (address) { + function _getAddressFallbackSigner(bytes memory immutables, bytes32 salt) internal view returns (address) { return LibClone.predictDeterministicAddressERC1967(address(ACCOUNT_IMPL), immutables, salt, address(this)); } diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index f5f4e49d..f488e325 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -41,11 +41,6 @@ struct AccountStorage { // AccountStorageInitializable variables uint8 initialized; bool initializing; - // Address for fallback single signer validation - address fallbackSigner; - // Whether or not the fallback signer is enabled, we can't use a zero fallbackSigner for this since it defaults - // to reading the bytecode-appended signer. - bool fallbackSignerDisabled; // Execution functions and their associated functions mapping(bytes4 selector => ExecutionData) executionData; mapping(ModuleEntity validationFunction => ValidationData) validationData; diff --git a/src/account/SemiModularAccount.sol b/src/account/SemiModularAccount.sol new file mode 100644 index 00000000..2c4e0b6d --- /dev/null +++ b/src/account/SemiModularAccount.sol @@ -0,0 +1,162 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.25; + +import {UpgradeableModularAccount} from "./UpgradeableModularAccount.sol"; +import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; +import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; + +import {ModuleEntityLib} from "../helpers/ModuleEntityLib.sol"; + +import {ModuleEntity, ValidationConfig} from "../interfaces/IModuleManager.sol"; + +import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; +import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; +import {LibClone} from "solady/utils/LibClone.sol"; + +contract SemiModularAccount is UpgradeableModularAccount { + using MessageHashUtils for bytes32; + using ModuleEntityLib for ModuleEntity; + + struct SemiModularAccountStorage { + address fallbackSigner; + bool fallbackSignerDisabled; + } + + // keccak256("ERC6900.SemiModularAccount.Storage") + uint256 internal constant _SEMI_MODULAR_ACCOUNT_STORAGE_SLOT = + 0x5b9dc9aa943f8fa2653ceceda5e3798f0686455280432166ba472eca0bc17a32; + + ModuleEntity internal constant _FALLBACK_VALIDATION = ModuleEntity.wrap(bytes24(type(uint192).max)); + + event FallbackSignerSet(address indexed previousFallbackSigner, address indexed newFallbackSigner); + event FallbackSignerEnabledSet(bool prevEnabled, bool newEnabled); + + error FallbackSignerMismatch(); + error FallbackSignerDisabled(); + error InitializerDisabled(); + + constructor(IEntryPoint anEntryPoint) UpgradeableModularAccount(anEntryPoint) {} + + /// Override reverts on initialization, effectively disabling the initializer. + function initializeWithValidation(ValidationConfig, bytes4[] calldata, bytes calldata, bytes[] calldata) + external + override + initializer + { + revert InitializerDisabled(); + } + + /// @notice Updates the fallback signer address in storage. + /// @dev This function causes the fallback signer getter to ignore the bytecode signer if it is nonzero. It can + /// also be used to revert back to the bytecode signer by setting to zero. + /// @param fallbackSigner The new signer to set. + function updateFallbackSigner(address fallbackSigner) external wrapNativeFunction { + SemiModularAccountStorage storage _storage = _getSemiModularAccountStorage(); + emit FallbackSignerSet(_storage.fallbackSigner, fallbackSigner); + + _storage.fallbackSigner = fallbackSigner; + } + + /// @notice Sets whether the fallback signer validation should be enabled or disabled. + function setFallbackSignerEnabled(bool enabled) external wrapNativeFunction { + SemiModularAccountStorage storage _storage = _getSemiModularAccountStorage(); + emit FallbackSignerEnabledSet(!_storage.fallbackSignerDisabled, enabled); + + _storage.fallbackSignerDisabled = !enabled; + } + + function isFallbackSignerEnabled() external view returns (bool) { + return !_getSemiModularAccountStorage().fallbackSignerDisabled; + } + + function getFallbackSigner() external view returns (address) { + return _getFallbackSigner(); + } + + function _execUserOpValidation( + ModuleEntity userOpValidationFunction, + PackedUserOperation memory userOp, + bytes32 userOpHash + ) internal override returns (uint256) { + if (userOpValidationFunction.eq(_FALLBACK_VALIDATION)) { + address fallbackSigner = _getFallbackSigner(); + + if ( + SignatureChecker.isValidSignatureNow( + fallbackSigner, userOpHash.toEthSignedMessageHash(), userOp.signature + ) + ) { + return _SIG_VALIDATION_PASSED; + } + return _SIG_VALIDATION_FAILED; + } + + return super._execUserOpValidation(userOpValidationFunction, userOp, userOpHash); + } + + function _execRuntimeValidation( + ModuleEntity runtimeValidationFunction, + bytes calldata callData, + bytes calldata authorization + ) internal override { + if (runtimeValidationFunction.eq(_FALLBACK_VALIDATION)) { + address fallbackSigner = _getFallbackSigner(); + + if (msg.sender != fallbackSigner) { + revert FallbackSignerMismatch(); + } + return; + } + super._execRuntimeValidation(runtimeValidationFunction, callData, authorization); + } + + function _exec1271Validation(ModuleEntity sigValidation, bytes32 hash, bytes calldata signature) + internal + view + override + returns (bytes4) + { + if (sigValidation.eq(_FALLBACK_VALIDATION)) { + address fallbackSigner = _getFallbackSigner(); + + if (SignatureChecker.isValidSignatureNow(fallbackSigner, hash, signature)) { + return _1271_MAGIC_VALUE; + } + return _1271_INVALID; + } + return super._exec1271Validation(sigValidation, hash, signature); + } + + function _globalValidationAllowed(bytes4 selector) internal view override returns (bool) { + return selector == this.updateFallbackSigner.selector || super._globalValidationAllowed(selector); + } + + function _isValidationGlobal(ModuleEntity validationFunction) internal view override returns (bool) { + return validationFunction.eq(_FALLBACK_VALIDATION) || super._isValidationGlobal(validationFunction); + } + + function _getFallbackSigner() internal view returns (address) { + SemiModularAccountStorage storage _storage = _getSemiModularAccountStorage(); + + if (_storage.fallbackSignerDisabled) { + revert FallbackSignerDisabled(); + } + + address storageFallbackSigner = _storage.fallbackSigner; + if (storageFallbackSigner != address(0)) { + return storageFallbackSigner; + } + + bytes memory appendedData = LibClone.argsOnERC1967(address(this), 0, 20); + + return address(uint160(bytes20(appendedData))); + } + + function _getSemiModularAccountStorage() internal pure returns (SemiModularAccountStorage storage) { + SemiModularAccountStorage storage _storage; + assembly ("memory-safe") { + _storage.slot := _SEMI_MODULAR_ACCOUNT_STORAGE_SLOT + } + return _storage; + } +} diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 3d795b70..083a4b32 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -36,9 +36,6 @@ import {ModuleManagerInternals} from "./ModuleManagerInternals.sol"; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; -import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; - -import {LibClone} from "solady/utils/LibClone.sol"; contract UpgradeableModularAccount is AccountExecutor, @@ -78,10 +75,7 @@ contract UpgradeableModularAccount is uint256 internal constant _SIG_VALIDATION_PASSED = 0; uint256 internal constant _SIG_VALIDATION_FAILED = 1; - ModuleEntity internal constant _FALLBACK_VALIDATION = ModuleEntity.wrap(bytes24(type(uint192).max)); - event ModularAccountInitialized(IEntryPoint indexed entryPoint); - event FallbackSignerSet(address indexed previousFallbackSigner, address indexed newFallbackSigner); error NonCanonicalEncoding(); error NotEntryPoint(); @@ -97,8 +91,6 @@ contract UpgradeableModularAccount is error ValidationFunctionMissing(bytes4 selector); error ValidationSignatureSegmentMissing(); error SignatureSegmentOutOfOrder(); - error FallbackSignerMismatch(); - error FallbackSignerDisabled(); // Wraps execution of a native function with runtime validation and hooks // Used for upgradeTo, upgradeToAndCall, execute, executeBatch, installExecution, uninstallExecution @@ -266,23 +258,10 @@ contract UpgradeableModularAccount is bytes4[] calldata selectors, bytes calldata installData, bytes[] calldata hooks - ) external initializer { + ) external virtual initializer { _installValidation(validationConfig, selectors, installData, hooks); } - function updateFallbackSigner(address fallbackSigner) external wrapNativeFunction { - AccountStorage storage _storage = getAccountStorage(); - - emit FallbackSignerSet(_storage.fallbackSigner, fallbackSigner); - _storage.fallbackSigner = fallbackSigner; - } - - function setFallbackSignerEnabled(bool enabled) external wrapNativeFunction { - AccountStorage storage _storage = getAccountStorage(); - _storage.fallbackSignerDisabled = !enabled; - // TODO: event - } - /// @inheritdoc IModuleManager /// @notice May be validated by a global validation. function installValidation( @@ -332,27 +311,9 @@ contract UpgradeableModularAccount is } function isValidSignature(bytes32 hash, bytes calldata signature) public view override returns (bytes4) { - AccountStorage storage _storage = getAccountStorage(); - ModuleEntity sigValidation = ModuleEntity.wrap(bytes24(signature)); - if (sigValidation.eq(_FALLBACK_VALIDATION)) { - // do sig validation - return _fallbackSignatureValidation(hash, signature[24:]); - } - - (address module, uint32 entityId) = sigValidation.unpack(); - if (!_storage.validationData[sigValidation].isSignatureValidation) { - revert SignatureValidationInvalid(module, entityId); - } - - if ( - IValidationModule(module).validateSignature(address(this), entityId, msg.sender, hash, signature[24:]) - == _1271_MAGIC_VALUE - ) { - return _1271_MAGIC_VALUE; - } - return _1271_INVALID; + return _exec1271Validation(sigValidation, hash, signature[24:]); } /// @notice Gets the entry point for this account @@ -366,7 +327,6 @@ contract UpgradeableModularAccount is // Parent function validateUserOp enforces that this call can only be made by the EntryPoint function _validateSignature(PackedUserOperation calldata userOp, bytes32 userOpHash) internal - virtual override returns (uint256 validationData) { @@ -451,15 +411,7 @@ contract UpgradeableModularAccount is userOp.signature = signatureSegment.getBody(); - uint256 currentValidationRes; - if (userOpValidationFunction.eq(_FALLBACK_VALIDATION)) { - // fallback userop validation - currentValidationRes = _fallbackUserOpValidation(userOp, userOpHash); - } else { - (address module, uint32 entityId) = userOpValidationFunction.unpack(); - - currentValidationRes = IValidation(module).validateUserOp(entityId, userOp, userOpHash); - } + uint256 currentValidationRes = _execUserOpValidation(userOpValidationFunction, userOp, userOpHash); if (preUserOpValidationHooks.length != 0) { // If we have other validation data we need to coalesce with @@ -512,22 +464,7 @@ contract UpgradeableModularAccount is revert ValidationSignatureSegmentMissing(); } - if (runtimeValidationFunction.eq(_FALLBACK_VALIDATION)) { - _fallbackRuntimeValidation(); - return; - } - - (address module, uint32 entityId) = runtimeValidationFunction.unpack(); - - try IValidationModule(module).validateRuntime( - address(this), entityId, msg.sender, msg.value, callData, authSegment.getBody() - ) - // forgefmt: disable-start - // solhint-disable-next-line no-empty-blocks - {} catch (bytes memory revertReason){ - // forgefmt: disable-end - revert RuntimeValidationFunctionReverted(module, entityId, revertReason); - } + _execRuntimeValidation(runtimeValidationFunction, callData, authSegment.getBody()); } function _doPreHooks(EnumerableSet.Bytes32Set storage executionHooks, bytes memory data) @@ -677,6 +614,73 @@ contract UpgradeableModularAccount is return (postPermissionHooks, postExecutionHooks); } + function _execUserOpValidation( + ModuleEntity userOpValidationFunction, + PackedUserOperation memory userOp, + bytes32 userOpHash + ) internal virtual returns (uint256) { + (address module, uint32 entityId) = userOpValidationFunction.unpack(); + + return IValidation(module).validateUserOp(entityId, userOp, userOpHash); + } + + function _execRuntimeValidation( + ModuleEntity runtimeValidationFunction, + bytes calldata callData, + bytes calldata authorization + ) internal virtual { + (address module, uint32 entityId) = runtimeValidationFunction.unpack(); + + try IValidation(module).validateRuntime( + address(this), entityId, msg.sender, msg.value, callData, authorization + ) + // forgefmt: disable-start + // solhint-disable-next-line no-empty-blocks + {} catch (bytes memory revertReason){ + // forgefmt: disable-end + revert RuntimeValidationFunctionReverted(module, entityId, revertReason); + } + } + + function _exec1271Validation(ModuleEntity sigValidation, bytes32 hash, bytes calldata signature) + internal + view + virtual + returns (bytes4) + { + AccountStorage storage _storage = getAccountStorage(); + + (address module, uint32 entityId) = sigValidation.unpack(); + if (!_storage.validationData[sigValidation].isSignatureValidation) { + revert SignatureValidationInvalid(module, entityId); + } + + if ( + IValidation(module).validateSignature(address(this), entityId, msg.sender, hash, signature) + == _1271_MAGIC_VALUE + ) { + return _1271_MAGIC_VALUE; + } + return _1271_INVALID; + } + + function _globalValidationAllowed(bytes4 selector) internal view virtual returns (bool) { + if ( + selector == this.execute.selector || selector == this.executeBatch.selector + || selector == this.installExecution.selector || selector == this.uninstallExecution.selector + || selector == this.installValidation.selector || selector == this.uninstallValidation.selector + || selector == this.upgradeToAndCall.selector + ) { + return true; + } + + return getAccountStorage().executionData[selector].allowGlobalValidation; + } + + function _isValidationGlobal(ModuleEntity validationFunction) internal view virtual returns (bool) { + return getAccountStorage().validationData[validationFunction].isGlobal; + } + function _checkIfValidationAppliesCallData( bytes calldata callData, ModuleEntity validationFunction, @@ -730,34 +734,13 @@ contract UpgradeableModularAccount is } } - function _globalValidationAllowed(bytes4 selector) internal view returns (bool) { - if ( - selector == this.execute.selector || selector == this.executeBatch.selector - || selector == this.installExecution.selector || selector == this.uninstallExecution.selector - || selector == this.installValidation.selector || selector == this.uninstallValidation.selector - || selector == this.upgradeToAndCall.selector || selector == this.updateFallbackSigner.selector - ) { - return true; - } - - return getAccountStorage().executionData[selector].allowGlobalValidation; - } - function _checkIfValidationAppliesSelector(bytes4 selector, ModuleEntity validationFunction, bool isGlobal) internal view { - AccountStorage storage _storage = getAccountStorage(); - // Check that the provided validation function is applicable to the selector if (isGlobal) { - if ( - _globalValidationAllowed(selector) - && ( - _storage.validationData[validationFunction].isGlobal - || validationFunction.eq(_FALLBACK_VALIDATION) - ) - ) { + if (_globalValidationAllowed(selector) && _isValidationGlobal(validationFunction)) { return; } revert ValidationFunctionMissing(selector); @@ -768,51 +751,4 @@ contract UpgradeableModularAccount is } } } - - function _fallbackRuntimeValidation() internal view { - if (msg.sender != _getFallbackSigner()) { - revert FallbackSignerMismatch(); - } - } - - function _fallbackUserOpValidation(PackedUserOperation memory userOp, bytes32 userOpHash) - internal - view - returns (uint256) - { - // Validate the user op signature against the owner. - (address sigSigner,,) = (userOpHash.toEthSignedMessageHash()).tryRecover(userOp.signature); - if (sigSigner == address(0) || sigSigner != _getFallbackSigner()) { - return _SIG_VALIDATION_FAILED; - } - return _SIG_VALIDATION_PASSED; - } - - function _fallbackSignatureValidation(bytes32 digest, bytes calldata signature) - internal - view - returns (bytes4) - { - if (SignatureChecker.isValidSignatureNow(_getFallbackSigner(), digest, signature)) { - return _1271_MAGIC_VALUE; - } - return _1271_INVALID; - } - - function _getFallbackSigner() internal view returns (address) { - AccountStorage storage _storage = getAccountStorage(); - - if (_storage.fallbackSignerDisabled) { - revert FallbackSignerDisabled(); - } - - address storageFallbackSigner = _storage.fallbackSigner; - if (storageFallbackSigner != address(0)) { - return storageFallbackSigner; - } - - bytes memory appendedData = LibClone.argsOnERC1967(address(this), 0, 20); - - return address(uint160(bytes20(appendedData))); - } } diff --git a/src/modules/validation/SingleSignerValidationModule.sol b/src/modules/validation/SingleSignerValidationModule.sol index 7ad1c768..8281f1b8 100644 --- a/src/modules/validation/SingleSignerValidationModule.sol +++ b/src/modules/validation/SingleSignerValidationModule.sol @@ -1,14 +1,13 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.25; -import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; -import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; -import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; - import {IModule, ModuleMetadata} from "../../interfaces/IModule.sol"; import {IValidationModule} from "../../interfaces/IValidationModule.sol"; import {BaseModule} from "../BaseModule.sol"; -import {ISingleSignerValidationModule} from "./ISingleSignerValidationModule.sol"; +import {ISingleSignerValidation} from "./ISingleSignerValidation.sol"; +import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; +import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; +import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; /// @title ECSDA Validation /// @author ERC-6900 Authors diff --git a/test/account/AccountReturnData.t.sol b/test/account/AccountReturnData.t.sol index 4e5647cd..3542f3db 100644 --- a/test/account/AccountReturnData.t.sol +++ b/test/account/AccountReturnData.t.sol @@ -2,7 +2,6 @@ pragma solidity ^0.8.19; import {DIRECT_CALL_VALIDATION_ENTITYID} from "../../src/helpers/Constants.sol"; -import {ModuleEntityLib} from "../../src/helpers/ModuleEntityLib.sol"; import {ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol"; import {Call} from "../../src/interfaces/IStandardExecutor.sol"; import {IStandardExecutor} from "../../src/interfaces/IStandardExecutor.sol"; @@ -13,7 +12,6 @@ import { ResultCreatorModule } from "../mocks/modules/ReturnDataModuleMocks.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; -import {TEST_DEFAULT_VALIDATION_ENTITY_ID} from "../utils/TestConstants.sol"; // Tests all the different ways that return data can be read from modules through an account contract AccountReturnDataTest is AccountTestBase { diff --git a/test/account/FallbackValidationTest.t.sol b/test/account/FallbackValidationTest.t.sol deleted file mode 100644 index f4efc4b2..00000000 --- a/test/account/FallbackValidationTest.t.sol +++ /dev/null @@ -1,77 +0,0 @@ -// SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.25; - -import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; -import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; - -import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; - -import {ModuleEntityLib} from "../../src/helpers/ModuleEntityLib.sol"; -import {ModuleEntity} from "../../src/interfaces/IModuleManager.sol"; - -import {AccountTestBase} from "../utils/AccountTestBase.sol"; - -contract FallbackValidationTest is AccountTestBase { - using MessageHashUtils for bytes32; - - address public ethRecipient; - - // A separate account and owner that isn't deployed yet, used to test initcode - address public owner2; - uint256 public owner2Key; - UpgradeableModularAccount public account2; - - ModuleEntity constant FALLBACK_VALIDATION = ModuleEntity.wrap(bytes24(type(uint192).max)); - - function setUp() public { - (owner2, owner2Key) = makeAddrAndKey("owner2"); - - // Compute counterfactual address - account2 = UpgradeableModularAccount(payable(factory.getAddressFallbackSigner(owner2, 0))); - vm.deal(address(account2), 100 ether); - - ethRecipient = makeAddr("ethRecipient"); - vm.deal(ethRecipient, 1 wei); - } - - function test_fallbackValidation_userOp_simple() public { - PackedUserOperation memory userOp = PackedUserOperation({ - sender: address(account2), - nonce: 0, - initCode: abi.encodePacked( - address(factory), abi.encodeCall(factory.createAccountWithFallbackValidation, (owner2, 0)) - ), - callData: abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, "")), - accountGasLimits: _encodeGas(VERIFICATION_GAS_LIMIT, CALL_GAS_LIMIT), - preVerificationGas: 0, - gasFees: _encodeGas(1, 1), - paymasterAndData: "", - signature: "" - }); - - // Generate signature - bytes32 userOpHash = entryPoint.getUserOpHash(userOp); - (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner2Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = _encodeSignature(FALLBACK_VALIDATION, GLOBAL_VALIDATION, abi.encodePacked(r, s, v)); - - PackedUserOperation[] memory userOps = new PackedUserOperation[](1); - userOps[0] = userOp; - - entryPoint.handleOps(userOps, beneficiary); - - assertEq(ethRecipient.balance, 2 wei); - } - - function test_fallbackValidation_runtime_simple() public { - // Deploy the account first - factory.createAccountWithFallbackValidation(owner2, 0); - - vm.prank(owner2); - account2.executeWithAuthorization( - abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, "")), - _encodeSignature(FALLBACK_VALIDATION, GLOBAL_VALIDATION, "") - ); - - assertEq(ethRecipient.balance, 2 wei); - } -} diff --git a/test/account/ImmutableAppend.t.sol b/test/account/ImmutableAppend.t.sol index a63f0408..dd75f4a5 100644 --- a/test/account/ImmutableAppend.t.sol +++ b/test/account/ImmutableAppend.t.sol @@ -1,20 +1,10 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.19; -import {IEntryPoint, UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; -import {ModuleEntity, ModuleEntityLib} from "../../src/helpers/ModuleEntityLib.sol"; -import {ValidationConfig, ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol"; -import {Call, IStandardExecutor} from "../../src/interfaces/IStandardExecutor.sol"; -import {DirectCallModule} from "../mocks/modules/DirectCallModule.sol"; - import {AccountTestBase} from "../utils/AccountTestBase.sol"; - -import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; import {LibClone} from "solady/utils/LibClone.sol"; contract ImmutableAppendTest is AccountTestBase { - using ValidationConfigLib for ValidationConfig; - /* -------------------------------------------------------------------------- */ /* Negatives */ /* -------------------------------------------------------------------------- */ @@ -24,6 +14,11 @@ contract ImmutableAppendTest is AccountTestBase { /* -------------------------------------------------------------------------- */ function test_success_getData() public { + if (!vm.envBool("SMA_TEST")) { + // this test isn't relevant at all for non-SMA, and is temporary. + return; + } + bytes memory expectedArgs = abi.encodePacked(owner1); assertEq(keccak256(LibClone.argsOnERC1967(address(account1))), keccak256(expectedArgs)); diff --git a/test/account/MultiValidation.t.sol b/test/account/MultiValidation.t.sol index f24a2094..78de0657 100644 --- a/test/account/MultiValidation.t.sol +++ b/test/account/MultiValidation.t.sol @@ -64,7 +64,7 @@ contract MultiValidationTest is AccountTestBase { abi.encodeWithSelector( UpgradeableModularAccount.RuntimeValidationFunctionReverted.selector, address(validator2), - 1, + TEST_DEFAULT_VALIDATION_ENTITY_ID, abi.encodeWithSignature("NotAuthorized()") ) ); diff --git a/test/account/PerHookData.t.sol b/test/account/PerHookData.t.sol index 65691a01..f9bfb823 100644 --- a/test/account/PerHookData.t.sol +++ b/test/account/PerHookData.t.sol @@ -344,13 +344,15 @@ contract PerHookDataTest is CustomValidationTestBase { ), abi.encode(_counter) ); - + // patched to also work during SMA tests by differentiating the validation + _signerValidation = + ModuleEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID - 1); return ( _signerValidation, true, true, new bytes4[](0), - abi.encode(TEST_DEFAULT_VALIDATION_ENTITY_ID, owner1), + abi.encode(TEST_DEFAULT_VALIDATION_ENTITY_ID - 1, owner1), hooks ); } diff --git a/test/account/ReplaceModule.t.sol b/test/account/ReplaceModule.t.sol index c70c20ac..9de8247b 100644 --- a/test/account/ReplaceModule.t.sol +++ b/test/account/ReplaceModule.t.sol @@ -21,7 +21,6 @@ import {IValidationHookModule} from "../../src/interfaces/IValidationHookModule. import {SingleSignerValidationModule} from "../../src/modules/validation/SingleSignerValidationModule.sol"; import {MockModule} from "../mocks/MockModule.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; -import {TEST_DEFAULT_VALIDATION_ENTITY_ID} from "../utils/TestConstants.sol"; interface TestModule { function testFunction() external; diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index 66e30bb7..29663e13 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -10,6 +10,8 @@ import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; import {ModuleManagerInternals} from "../../src/account/ModuleManagerInternals.sol"; + +import {SemiModularAccount} from "../../src/account/SemiModularAccount.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {ExecutionDataView, IAccountLoupe} from "../../src/interfaces/IAccountLoupe.sol"; @@ -52,7 +54,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { (owner2, owner2Key) = makeAddrAndKey("owner2"); // Compute counterfactual address - account2 = UpgradeableModularAccount(payable(factory.getAddressFallbackSigner(owner2, 0))); + account2 = UpgradeableModularAccount(payable(factory.getAddress(owner2, 0))); vm.deal(address(account2), 100 ether); ethRecipient = makeAddr("ethRecipient"); @@ -92,13 +94,22 @@ contract UpgradeableModularAccountTest is AccountTestBase { } function test_basicUserOp_withInitCode() public { + bytes memory callData = vm.envBool("SMA_TEST") + ? abi.encodeCall(SemiModularAccount(payable(account1)).updateFallbackSigner, (owner2)) + : abi.encodeCall( + UpgradeableModularAccount.execute, + ( + address(singleSignerValidation), + 0, + abi.encodeCall(SingleSignerValidation.transferSigner, (TEST_DEFAULT_VALIDATION_ENTITY_ID, owner2)) + ) + ); + PackedUserOperation memory userOp = PackedUserOperation({ sender: address(account2), nonce: 0, - initCode: abi.encodePacked( - address(factory), abi.encodeCall(factory.createAccountWithFallbackValidation, (owner2, 0)) - ), - callData: abi.encodeCall(UpgradeableModularAccount.updateFallbackSigner, (owner2)), + initCode: abi.encodePacked(address(factory), abi.encodeCall(factory.createAccount, (owner2, 0))), + callData: callData, accountGasLimits: _encodeGas(VERIFICATION_GAS_LIMIT, CALL_GAS_LIMIT), preVerificationGas: 0, gasFees: _encodeGas(1, 2), @@ -123,9 +134,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { PackedUserOperation memory userOp = PackedUserOperation({ sender: address(account2), nonce: 0, - initCode: abi.encodePacked( - address(factory), abi.encodeCall(factory.createAccountWithFallbackValidation, (owner2, 0)) - ), + initCode: abi.encodePacked(address(factory), abi.encodeCall(factory.createAccount, (owner2, 0))), callData: abi.encodeCall(UpgradeableModularAccount.execute, (recipient, 1 wei, "")), accountGasLimits: _encodeGas(VERIFICATION_GAS_LIMIT, CALL_GAS_LIMIT), preVerificationGas: 0, @@ -352,9 +361,15 @@ contract UpgradeableModularAccountTest is AccountTestBase { // TODO: Consider if this test belongs here or in the tests specific to the SingleSignerValidation function test_transferOwnership() public { - // Note: replaced "owner1" with address(0), this doesn't actually affect the account, but allows the test - // to pass by ensuring the signer can be set in the validation - assertEq(singleSignerValidation.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), address(0)); + if (vm.envBool("SMA_TEST")) { + // Note: replaced "owner1" with address(0), this doesn't actually affect the account, but allows the + // test to pass by ensuring the signer can be set in the validation. + assertEq( + singleSignerValidation.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), address(0) + ); + } else { + assertEq(singleSignerValidation.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), owner1); + } vm.prank(address(entryPoint)); account1.execute( diff --git a/test/mocks/SingleSignerFactoryFixture.sol b/test/mocks/SingleSignerFactoryFixture.sol index c0dfbe64..1214f9d1 100644 --- a/test/mocks/SingleSignerFactoryFixture.sol +++ b/test/mocks/SingleSignerFactoryFixture.sol @@ -27,7 +27,10 @@ contract SingleSignerFactoryFixture is OptimizedTest { constructor(IEntryPoint _entryPoint, SingleSignerValidationModule _singleSignerValidationModule) { entryPoint = _entryPoint; - accountImplementation = _deployUpgradeableModularAccount(_entryPoint); + + accountImplementation = vm.envBool("SMA_TEST") + ? _deploySemiModularAccount(_entryPoint) + : _deployUpgradeableModularAccount(_entryPoint); _PROXY_BYTECODE_HASH = keccak256( abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(address(accountImplementation), "")) ); @@ -45,6 +48,10 @@ contract SingleSignerFactoryFixture is OptimizedTest { * account creation */ function createAccount(address owner, uint256 salt) public returns (UpgradeableModularAccount) { + if (vm.envBool("SMA_TEST")) { + return createAccountWithFallbackValidation(owner, salt); + } + address addr = Create2.computeAddress(getSalt(owner, salt), _PROXY_BYTECODE_HASH); // short circuit if exists @@ -92,6 +99,9 @@ contract SingleSignerFactoryFixture is OptimizedTest { * calculate the counterfactual address of this account as it would be returned by createAccount() */ function getAddress(address owner, uint256 salt) public view returns (address) { + if (vm.envBool("SMA_TEST")) { + return getAddressFallbackSigner(owner, salt); + } return Create2.computeAddress(getSalt(owner, salt), _PROXY_BYTECODE_HASH); } diff --git a/test/module/AllowlistModule.t.sol b/test/module/AllowlistModule.t.sol index c17a9bf9..1edd422a 100644 --- a/test/module/AllowlistModule.t.sol +++ b/test/module/AllowlistModule.t.sol @@ -341,13 +341,15 @@ contract AllowlistModuleTest is CustomValidationTestBase { HookConfigLib.packValidationHook(address(allowlistModule), HOOK_ENTITY_ID), abi.encode(HOOK_ENTITY_ID, allowlistInit) ); - + // patched to also work during SMA tests by differentiating the validation + _signerValidation = + ModuleEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID - 1); return ( _signerValidation, true, true, new bytes4[](0), - abi.encode(TEST_DEFAULT_VALIDATION_ENTITY_ID, owner1), + abi.encode(TEST_DEFAULT_VALIDATION_ENTITY_ID - 1, owner1), hooks ); } diff --git a/test/module/SingleSignerValidationModule.t.sol b/test/module/SingleSignerValidationModule.t.sol index 7bddd5d8..24b97c2d 100644 --- a/test/module/SingleSignerValidationModule.t.sol +++ b/test/module/SingleSignerValidationModule.t.sol @@ -83,7 +83,7 @@ contract SingleSignerValidationModuleTest is AccountTestBase { } function test_runtime_with2SameValidationInstalled() public { - uint32 newEntityId = TEST_DEFAULT_VALIDATION_ENTITY_ID + 1; + uint32 newEntityId = TEST_DEFAULT_VALIDATION_ENTITY_ID - 1; vm.prank(address(entryPoint)); vm.expectEmit(address(singleSignerValidationModule)); diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index 34824635..297a0914 100644 --- a/test/utils/AccountTestBase.sol +++ b/test/utils/AccountTestBase.sol @@ -5,6 +5,7 @@ import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.so import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; +import {SemiModularAccount} from "../../src/account/SemiModularAccount.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {ModuleEntity, ModuleEntityLib} from "../../src/helpers/ModuleEntityLib.sol"; import {Call, IStandardExecutor} from "../../src/interfaces/IStandardExecutor.sol"; @@ -53,13 +54,20 @@ abstract contract AccountTestBase is OptimizedTest { (owner1, owner1Key) = makeAddrAndKey("owner1"); beneficiary = payable(makeAddr("beneficiary")); - singleSignerValidationModule = _deploySingleSignerValidationModule(); - factory = new SingleSignerFactoryFixture(entryPoint, singleSignerValidationModule); + address deployedSingleSignerValidation = address(_deploySingleSignerValidation()); - account1 = factory.createAccountWithFallbackValidation(owner1, 0); + // We etch the single signer validation to the max address, such that it coincides with the fallback + // validation module entity for semi modular account tests. + singleSignerValidation = SingleSignerValidation(address(type(uint160).max)); + vm.etch(address(singleSignerValidation), deployedSingleSignerValidation.code); + + factory = new SingleSignerFactoryFixture(entryPoint, singleSignerValidation); + + account1 = factory.createAccount(owner1, 0); vm.deal(address(account1), 100 ether); - _signerValidation = ModuleEntity.wrap(bytes24(type(uint192).max)); + _signerValidation = + ModuleEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID); } function _runExecUserOp(address target, bytes memory callData) internal { @@ -162,8 +170,24 @@ abstract contract AccountTestBase is OptimizedTest { function _transferOwnershipToTest() internal { // Transfer ownership to test contract for easier invocation. vm.prank(owner1); + if (vm.envBool("SMA_TEST")) { + account1.executeWithAuthorization( + abi.encodeCall(SemiModularAccount(payable(account1)).updateFallbackSigner, (address(this))), + _encodeSignature(_signerValidation, GLOBAL_VALIDATION, "") + ); + return; + } account1.executeWithAuthorization( - abi.encodeCall(account1.updateFallbackSigner, (address(this))), + abi.encodeCall( + account1.execute, + ( + address(singleSignerValidation), + 0, + abi.encodeCall( + SingleSignerValidation.transferSigner, (TEST_DEFAULT_VALIDATION_ENTITY_ID, address(this)) + ) + ) + ), _encodeSignature(_signerValidation, GLOBAL_VALIDATION, "") ); } diff --git a/test/utils/CustomValidationTestBase.sol b/test/utils/CustomValidationTestBase.sol index a7920623..ccac8da7 100644 --- a/test/utils/CustomValidationTestBase.sol +++ b/test/utils/CustomValidationTestBase.sol @@ -28,14 +28,23 @@ abstract contract CustomValidationTestBase is AccountTestBase { account1 = UpgradeableModularAccount(payable(new ERC1967Proxy{salt: 0}(accountImplementation, ""))); - _beforeInstallStep(address(account1)); - - account1.initializeWithValidation( - ValidationConfigLib.pack(validationFunction, isGlobal, isSignatureValidation), - selectors, - installData, - hooks - ); + if (vm.envBool("SMA_TEST")) { + vm.prank(address(entryPoint)); + // The initializer doesn't work on the SMA + account1.installValidation( + ValidationConfigLib.pack(validationFunction, isGlobal, isSignatureValidation), + selectors, + installData, + hooks + ); + } else { + account1.initializeWithValidation( + ValidationConfigLib.pack(validationFunction, isGlobal, isSignatureValidation), + selectors, + installData, + hooks + ); + } vm.deal(address(account1), 100 ether); } diff --git a/test/utils/OptimizedTest.sol b/test/utils/OptimizedTest.sol index 7792badb..a2fa3c22 100644 --- a/test/utils/OptimizedTest.sol +++ b/test/utils/OptimizedTest.sol @@ -5,6 +5,7 @@ import {Test} from "forge-std/Test.sol"; import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; +import {SemiModularAccount} from "../../src/account/SemiModularAccount.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {TokenReceiverModule} from "../../src/modules/TokenReceiverModule.sol"; @@ -45,6 +46,16 @@ abstract contract OptimizedTest is Test { : new UpgradeableModularAccount(entryPoint); } + function _deploySemiModularAccount(IEntryPoint entryPoint) internal returns (UpgradeableModularAccount) { + return _isOptimizedTest() + ? UpgradeableModularAccount( + payable( + deployCode("out-optimized/SemiModularAccount.sol/SemiModularAccount.json", abi.encode(entryPoint)) + ) + ) + : UpgradeableModularAccount(new SemiModularAccount(entryPoint)); + } + function _deployTokenReceiverModule() internal returns (TokenReceiverModule) { return _isOptimizedTest() ? TokenReceiverModule(deployCode("out-optimized/TokenReceiverModule.sol/TokenReceiverModule.json")) diff --git a/test/utils/TestConstants.sol b/test/utils/TestConstants.sol index c15b2dd3..119bcd0b 100644 --- a/test/utils/TestConstants.sol +++ b/test/utils/TestConstants.sol @@ -1,4 +1,4 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.25; -uint32 constant TEST_DEFAULT_VALIDATION_ENTITY_ID = 1; +uint32 constant TEST_DEFAULT_VALIDATION_ENTITY_ID = type(uint32).max; From e8d320f769204b6d656df90786c282e852809816 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 14 Aug 2024 19:20:40 -0400 Subject: [PATCH 11/34] chore: document SMA new functions --- src/account/SemiModularAccount.sol | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/account/SemiModularAccount.sol b/src/account/SemiModularAccount.sol index 2c4e0b6d..72345201 100644 --- a/src/account/SemiModularAccount.sol +++ b/src/account/SemiModularAccount.sol @@ -29,7 +29,7 @@ contract SemiModularAccount is UpgradeableModularAccount { ModuleEntity internal constant _FALLBACK_VALIDATION = ModuleEntity.wrap(bytes24(type(uint192).max)); event FallbackSignerSet(address indexed previousFallbackSigner, address indexed newFallbackSigner); - event FallbackSignerEnabledSet(bool prevEnabled, bool newEnabled); + event FallbackSignerDisabledSet(bool prevDisabled, bool newDisabled); error FallbackSignerMismatch(); error FallbackSignerDisabled(); @@ -58,17 +58,24 @@ contract SemiModularAccount is UpgradeableModularAccount { } /// @notice Sets whether the fallback signer validation should be enabled or disabled. - function setFallbackSignerEnabled(bool enabled) external wrapNativeFunction { + /// @dev Due to being initially zero, we need to store "disabled" rather than "enabled" in storage. + /// @param isDisabled True to disable fallback signer validation, false to enable it. + function setFallbackSignerDisabled(bool isDisabled) external wrapNativeFunction { SemiModularAccountStorage storage _storage = _getSemiModularAccountStorage(); - emit FallbackSignerEnabledSet(!_storage.fallbackSignerDisabled, enabled); + emit FallbackSignerDisabledSet(_storage.fallbackSignerDisabled, isDisabled); - _storage.fallbackSignerDisabled = !enabled; + _storage.fallbackSignerDisabled = isDisabled; } - function isFallbackSignerEnabled() external view returns (bool) { - return !_getSemiModularAccountStorage().fallbackSignerDisabled; + /// @notice Returns whether the fallback signer validation is disabled. + /// @return True if the fallback signer validation is disabled, false if it is enabled. + function isFallbackSignerDisabled() external view returns (bool) { + return _getSemiModularAccountStorage().fallbackSignerDisabled; } + /// @notice Returns the fallback signer associated with this account, regardless if the fallback signer + /// validation is enabled or not. + /// @return The fallback signer address, either overriden in storage, or read from bytecode. function getFallbackSigner() external view returns (address) { return _getFallbackSigner(); } @@ -142,6 +149,14 @@ contract SemiModularAccount is UpgradeableModularAccount { revert FallbackSignerDisabled(); } + return _retrieveFallbackSignerUnchecked(_storage); + } + + function _retrieveFallbackSignerUnchecked(SemiModularAccountStorage storage _storage) + internal + view + returns (address) + { address storageFallbackSigner = _storage.fallbackSigner; if (storageFallbackSigner != address(0)) { return storageFallbackSigner; From 0ec43dcf9dbd748919bd44c3558907d58b960278 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Thu, 15 Aug 2024 15:25:09 -0400 Subject: [PATCH 12/34] chore: reduce optimizer runs for SMA codesize --- foundry.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/foundry.toml b/foundry.toml index 516e1a1f..56131b8f 100644 --- a/foundry.toml +++ b/foundry.toml @@ -25,7 +25,7 @@ depth = 10 [profile.optimized-build] via_ir = true test = 'src' -optimizer_runs = 15000 +optimizer_runs = 10000 out = 'out-optimized' [profile.optimized-test] From 5f956cbfa353518f51d00f986c56c0c59b4c0b6d Mon Sep 17 00:00:00 2001 From: zer0dot Date: Thu, 15 Aug 2024 15:27:46 -0400 Subject: [PATCH 13/34] feat: add proper SMA support to factory --- script/Deploy.s.sol | 52 ++++++++++++++++++++--- src/account/AccountFactory.sol | 12 ++++-- test/account/AccountFactory.t.sol | 9 +++- test/mocks/SingleSignerFactoryFixture.sol | 4 +- test/script/Deploy.s.t.sol | 23 ++++++++-- 5 files changed, 84 insertions(+), 16 deletions(-) diff --git a/script/Deploy.s.sol b/script/Deploy.s.sol index 1ad11be5..95b46d9b 100644 --- a/script/Deploy.s.sol +++ b/script/Deploy.s.sol @@ -7,6 +7,8 @@ import {Script, console} from "forge-std/Script.sol"; import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; import {AccountFactory} from "../src/account/AccountFactory.sol"; + +import {SemiModularAccount} from "../src/account/SemiModularAccount.sol"; import {UpgradeableModularAccount} from "../src/account/UpgradeableModularAccount.sol"; import {SingleSignerValidationModule} from "../src/modules/validation/SingleSignerValidationModule.sol"; @@ -16,10 +18,12 @@ contract DeployScript is Script { address public owner = vm.envAddress("OWNER"); address public accountImpl = vm.envOr("ACCOUNT_IMPL", address(0)); + address public semiModularAccountImpl = vm.envOr("SMA_IMPL", address(0)); address public factory = vm.envOr("FACTORY", address(0)); address public singleSignerValidationModule = vm.envOr("SINGLE_SIGNER_VALIDATION_MODULE", address(0)); bytes32 public accountImplSalt = bytes32(vm.envOr("ACCOUNT_IMPL_SALT", uint256(0))); + bytes32 public semiModularAccountImplSalt = bytes32(vm.envOr("SMA_IMPL_SALT", uint256(0))); bytes32 public factorySalt = bytes32(vm.envOr("FACTORY_SALT", uint256(0))); bytes32 public singleSignerValidationModuleSalt = bytes32(vm.envOr("SINGLE_SIGNER_VALIDATION_MODULE_SALT", uint256(0))); @@ -35,7 +39,8 @@ contract DeployScript is Script { vm.startBroadcast(); _deployAccountImpl(accountImplSalt, accountImpl); - _deploySingleSignerValidationModule(singleSignerValidationModuleSalt, singleSignerValidationModule); + _deploySemiModularAccountImpl(semiModularAccountImplSalt, semiModularAccountImpl); + _deploySingleSignerValidation(singleSignerValidationSalt, singleSignerValidation); _deployAccountFactory(factorySalt, factory); _addStakeForFactory(uint32(requiredUnstakeDelay), requiredStakeAmount); vm.stopBroadcast(); @@ -73,8 +78,40 @@ contract DeployScript is Script { } } - function _deploySingleSignerValidationModule(bytes32 salt, address expected) internal { - console.log(string.concat("Deploying SingleSignerValidationModule with salt: ", vm.toString(salt))); + function _deploySemiModularAccountImpl(bytes32 salt, address expected) internal { + console.log(string.concat("Deploying SemiModularAccountImpl with salt: ", vm.toString(salt))); + + address addr = Create2.computeAddress( + salt, + keccak256(abi.encodePacked(type(SemiModularAccount).creationCode, abi.encode(entryPoint))), + CREATE2_FACTORY + ); + if (addr != expected) { + console.log("Expected address mismatch"); + console.log("Expected: ", expected); + console.log("Actual: ", addr); + revert(); + } + + if (addr.code.length == 0) { + console.log("No code found at expected address, deploying..."); + SemiModularAccount deployed = new SemiModularAccount{salt: salt}(entryPoint); + + if (address(deployed) != expected) { + console.log("Deployed address mismatch"); + console.log("Expected: ", expected); + console.log("Deployed: ", address(deployed)); + revert(); + } + + console.log("Deployed SemiModularAccount at: ", address(deployed)); + } else { + console.log("Code found at expected address, skipping deployment"); + } + } + + function _deploySingleSignerValidation(bytes32 salt, address expected) internal { + console.log(string.concat("Deploying SingleSignerValidation with salt: ", vm.toString(salt))); address addr = Create2.computeAddress( salt, keccak256(abi.encodePacked(type(SingleSignerValidationModule).creationCode)), CREATE2_FACTORY @@ -111,7 +148,7 @@ contract DeployScript is Script { keccak256( abi.encodePacked( type(AccountFactory).creationCode, - abi.encode(entryPoint, accountImpl, singleSignerValidationModule, owner) + abi.encode(entryPoint, accountImpl, semiModularAccountImpl, singleSignerValidation, owner) ) ), CREATE2_FACTORY @@ -125,8 +162,13 @@ contract DeployScript is Script { if (addr.code.length == 0) { console.log("No code found at expected address, deploying..."); + // Patched AccountFactory deployed = new AccountFactory{salt: salt}( - entryPoint, UpgradeableModularAccount(payable(accountImpl)), singleSignerValidationModule, owner + entryPoint, + UpgradeableModularAccount(payable(accountImpl)), + SemiModularAccount(payable(semiModularAccountImpl)), + singleSignerValidation, + owner ); if (address(deployed) != expected) { diff --git a/src/account/AccountFactory.sol b/src/account/AccountFactory.sol index 285771f0..8fb63d84 100644 --- a/src/account/AccountFactory.sol +++ b/src/account/AccountFactory.sol @@ -8,12 +8,14 @@ import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.s import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; import {UpgradeableModularAccount} from "../account/UpgradeableModularAccount.sol"; +import {SemiModularAccount} from "../account/SemiModularAccount.sol"; import {ValidationConfigLib} from "../helpers/ValidationConfigLib.sol"; import {LibClone} from "solady/utils/LibClone.sol"; contract AccountFactory is Ownable { UpgradeableModularAccount public immutable ACCOUNT_IMPL; + SemiModularAccount public immutable SEMI_MODULAR_ACCOUNT_IMPL; bytes32 private immutable _PROXY_BYTECODE_HASH; IEntryPoint public immutable ENTRY_POINT; address public immutable SINGLE_SIGNER_VALIDATION_MODULE; @@ -23,14 +25,16 @@ contract AccountFactory is Ownable { constructor( IEntryPoint _entryPoint, UpgradeableModularAccount _accountImpl, - address _singleSignerValidationModule, + SemiModularAccount _semiModularImpl, + address _singleSignerValidation, address owner ) Ownable(owner) { ENTRY_POINT = _entryPoint; _PROXY_BYTECODE_HASH = keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(address(_accountImpl), ""))); ACCOUNT_IMPL = _accountImpl; - SINGLE_SIGNER_VALIDATION_MODULE = _singleSignerValidationModule; + SEMI_MODULAR_ACCOUNT_IMPL = _semiModularImpl; + SINGLE_SIGNER_VALIDATION = _singleSignerValidation; } /** @@ -65,7 +69,7 @@ contract AccountFactory is Ownable { return UpgradeableModularAccount(payable(addr)); } - function createAccountWithFallbackValidation(address owner, uint256 salt) + function createSemiModularAccount(address owner, uint256 salt) external returns (UpgradeableModularAccount) { @@ -79,7 +83,7 @@ contract AccountFactory is Ownable { // short circuit if exists if (addr.code.length == 0) { // not necessary to check return addr since next call will fail if so - LibClone.createDeterministicERC1967(address(ACCOUNT_IMPL), immutables, fullSalt); + LibClone.createDeterministicERC1967(address(SEMI_MODULAR_ACCOUNT_IMPL), immutables, fullSalt); } return UpgradeableModularAccount(payable(addr)); diff --git a/test/account/AccountFactory.t.sol b/test/account/AccountFactory.t.sol index 25194f1d..bdcec368 100644 --- a/test/account/AccountFactory.t.sol +++ b/test/account/AccountFactory.t.sol @@ -2,16 +2,23 @@ pragma solidity ^0.8.19; import {AccountFactory} from "../../src/account/AccountFactory.sol"; + +import {SemiModularAccount} from "../../src/account/SemiModularAccount.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; contract AccountFactoryTest is AccountTestBase { AccountFactory internal _factory; UpgradeableModularAccount internal _account; + SemiModularAccount internal _semiModularAccount; function setUp() public { _account = new UpgradeableModularAccount(entryPoint); - _factory = new AccountFactory(entryPoint, _account, address(singleSignerValidationModule), address(this)); + _semiModularAccount = new SemiModularAccount(entryPoint); + + _factory = new AccountFactory( + entryPoint, _account, _semiModularAccount, address(singleSignerValidation), address(this) + ); } function test_createAccount() public { diff --git a/test/mocks/SingleSignerFactoryFixture.sol b/test/mocks/SingleSignerFactoryFixture.sol index 1214f9d1..76f6f079 100644 --- a/test/mocks/SingleSignerFactoryFixture.sol +++ b/test/mocks/SingleSignerFactoryFixture.sol @@ -49,7 +49,7 @@ contract SingleSignerFactoryFixture is OptimizedTest { */ function createAccount(address owner, uint256 salt) public returns (UpgradeableModularAccount) { if (vm.envBool("SMA_TEST")) { - return createAccountWithFallbackValidation(owner, salt); + return createSemiModularAccount(owner, salt); } address addr = Create2.computeAddress(getSalt(owner, salt), _PROXY_BYTECODE_HASH); @@ -74,7 +74,7 @@ contract SingleSignerFactoryFixture is OptimizedTest { return UpgradeableModularAccount(payable(addr)); } - function createAccountWithFallbackValidation(address owner, uint256 salt) + function createSemiModularAccount(address owner, uint256 salt) public returns (UpgradeableModularAccount) { diff --git a/test/script/Deploy.s.t.sol b/test/script/Deploy.s.t.sol index ae97ac0d..e5618c70 100644 --- a/test/script/Deploy.s.t.sol +++ b/test/script/Deploy.s.t.sol @@ -10,6 +10,8 @@ import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; import {DeployScript} from "../../script/Deploy.s.sol"; import {AccountFactory} from "../../src/account/AccountFactory.sol"; + +import {SemiModularAccount} from "../../src/account/SemiModularAccount.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {SingleSignerValidationModule} from "../../src/modules/validation/SingleSignerValidationModule.sol"; @@ -21,7 +23,8 @@ contract DeployTest is Test { address internal _owner; address internal _accountImpl; - address internal _singleSignerValidationModule; + address internal _smaImpl; + address internal _singleSignerValidation; address internal _factory; function setUp() public { @@ -42,28 +45,40 @@ contract DeployTest is Test { CREATE2_FACTORY ); - _singleSignerValidationModule = Create2.computeAddress( + _smaImpl = Create2.computeAddress( bytes32(0), - keccak256(abi.encodePacked(type(SingleSignerValidationModule).creationCode)), + keccak256(abi.encodePacked(type(SemiModularAccount).creationCode, abi.encode(address(_entryPoint)))), CREATE2_FACTORY ); + _singleSignerValidation = Create2.computeAddress( + bytes32(0), keccak256(abi.encodePacked(type(SingleSignerValidation).creationCode)), CREATE2_FACTORY + ); + _factory = Create2.computeAddress( bytes32(0), keccak256( abi.encodePacked( type(AccountFactory).creationCode, - abi.encode(address(_entryPoint), _accountImpl, _singleSignerValidationModule, _owner) + abi.encode( + address(_entryPoint), + _accountImpl, + _smaImpl, + _singleSignerValidation, + _owner + ) ) ), CREATE2_FACTORY ); vm.setEnv("ACCOUNT_IMPL", vm.toString(address(_accountImpl))); + vm.setEnv("SMA_IMPL", vm.toString(address(_smaImpl))); vm.setEnv("FACTORY", vm.toString(address(_factory))); vm.setEnv("SINGLE_SIGNER_VALIDATION_MODULE", vm.toString(_singleSignerValidationModule)); vm.setEnv("ACCOUNT_IMPL_SALT", vm.toString(uint256(0))); + vm.setEnv("SMA_IMPL_SALT", vm.toString(uint256(0))); vm.setEnv("FACTORY_SALT", vm.toString(uint256(0))); vm.setEnv("SINGLE_SIGNER_VALIDATION_MODULE_SALT", vm.toString(uint256(0))); From 9e752bccb2229936b3903bf5d79b972c8829559f Mon Sep 17 00:00:00 2001 From: zer0dot Date: Thu, 15 Aug 2024 15:47:22 -0400 Subject: [PATCH 14/34] chore: update to match new file naming --- script/Deploy.s.sol | 8 ++++---- src/account/AccountFactory.sol | 4 ++-- src/account/UpgradeableModularAccount.sol | 6 +++--- .../validation/SingleSignerValidationModule.sol | 2 +- test/account/AccountFactory.t.sol | 2 +- test/account/PerHookData.t.sol | 4 ++-- test/account/UpgradeableModularAccount.t.sol | 10 +++++----- test/module/AllowlistModule.t.sol | 4 ++-- test/script/Deploy.s.t.sol | 8 ++++---- test/utils/AccountTestBase.sol | 14 +++++++------- 10 files changed, 31 insertions(+), 31 deletions(-) diff --git a/script/Deploy.s.sol b/script/Deploy.s.sol index 95b46d9b..7d7ae1d7 100644 --- a/script/Deploy.s.sol +++ b/script/Deploy.s.sol @@ -40,7 +40,7 @@ contract DeployScript is Script { vm.startBroadcast(); _deployAccountImpl(accountImplSalt, accountImpl); _deploySemiModularAccountImpl(semiModularAccountImplSalt, semiModularAccountImpl); - _deploySingleSignerValidation(singleSignerValidationSalt, singleSignerValidation); + _deploySingleSignerValidation(singleSignerValidationModuleSalt, singleSignerValidationModule); _deployAccountFactory(factorySalt, factory); _addStakeForFactory(uint32(requiredUnstakeDelay), requiredStakeAmount); vm.stopBroadcast(); @@ -111,7 +111,7 @@ contract DeployScript is Script { } function _deploySingleSignerValidation(bytes32 salt, address expected) internal { - console.log(string.concat("Deploying SingleSignerValidation with salt: ", vm.toString(salt))); + console.log(string.concat("Deploying SingleSignerValidationModule with salt: ", vm.toString(salt))); address addr = Create2.computeAddress( salt, keccak256(abi.encodePacked(type(SingleSignerValidationModule).creationCode)), CREATE2_FACTORY @@ -148,7 +148,7 @@ contract DeployScript is Script { keccak256( abi.encodePacked( type(AccountFactory).creationCode, - abi.encode(entryPoint, accountImpl, semiModularAccountImpl, singleSignerValidation, owner) + abi.encode(entryPoint, accountImpl, semiModularAccountImpl, singleSignerValidationModule, owner) ) ), CREATE2_FACTORY @@ -167,7 +167,7 @@ contract DeployScript is Script { entryPoint, UpgradeableModularAccount(payable(accountImpl)), SemiModularAccount(payable(semiModularAccountImpl)), - singleSignerValidation, + singleSignerValidationModule, owner ); diff --git a/src/account/AccountFactory.sol b/src/account/AccountFactory.sol index 8fb63d84..e379c784 100644 --- a/src/account/AccountFactory.sol +++ b/src/account/AccountFactory.sol @@ -26,7 +26,7 @@ contract AccountFactory is Ownable { IEntryPoint _entryPoint, UpgradeableModularAccount _accountImpl, SemiModularAccount _semiModularImpl, - address _singleSignerValidation, + address _singleSignerValidationModule, address owner ) Ownable(owner) { ENTRY_POINT = _entryPoint; @@ -34,7 +34,7 @@ contract AccountFactory is Ownable { keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(address(_accountImpl), ""))); ACCOUNT_IMPL = _accountImpl; SEMI_MODULAR_ACCOUNT_IMPL = _semiModularImpl; - SINGLE_SIGNER_VALIDATION = _singleSignerValidation; + SINGLE_SIGNER_VALIDATION_MODULE = _singleSignerValidationModule; } /** diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 083a4b32..c069491f 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -621,7 +621,7 @@ contract UpgradeableModularAccount is ) internal virtual returns (uint256) { (address module, uint32 entityId) = userOpValidationFunction.unpack(); - return IValidation(module).validateUserOp(entityId, userOp, userOpHash); + return IValidationModule(module).validateUserOp(entityId, userOp, userOpHash); } function _execRuntimeValidation( @@ -631,7 +631,7 @@ contract UpgradeableModularAccount is ) internal virtual { (address module, uint32 entityId) = runtimeValidationFunction.unpack(); - try IValidation(module).validateRuntime( + try IValidationModule(module).validateRuntime( address(this), entityId, msg.sender, msg.value, callData, authorization ) // forgefmt: disable-start @@ -656,7 +656,7 @@ contract UpgradeableModularAccount is } if ( - IValidation(module).validateSignature(address(this), entityId, msg.sender, hash, signature) + IValidationModule(module).validateSignature(address(this), entityId, msg.sender, hash, signature) == _1271_MAGIC_VALUE ) { return _1271_MAGIC_VALUE; diff --git a/src/modules/validation/SingleSignerValidationModule.sol b/src/modules/validation/SingleSignerValidationModule.sol index 8281f1b8..c869af22 100644 --- a/src/modules/validation/SingleSignerValidationModule.sol +++ b/src/modules/validation/SingleSignerValidationModule.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.25; import {IModule, ModuleMetadata} from "../../interfaces/IModule.sol"; import {IValidationModule} from "../../interfaces/IValidationModule.sol"; import {BaseModule} from "../BaseModule.sol"; -import {ISingleSignerValidation} from "./ISingleSignerValidation.sol"; +import {ISingleSignerValidationModule} from "./ISingleSignerValidationModule.sol"; import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; diff --git a/test/account/AccountFactory.t.sol b/test/account/AccountFactory.t.sol index bdcec368..92d79752 100644 --- a/test/account/AccountFactory.t.sol +++ b/test/account/AccountFactory.t.sol @@ -17,7 +17,7 @@ contract AccountFactoryTest is AccountTestBase { _semiModularAccount = new SemiModularAccount(entryPoint); _factory = new AccountFactory( - entryPoint, _account, _semiModularAccount, address(singleSignerValidation), address(this) + entryPoint, _account, _semiModularAccount, address(singleSignerValidationModule), address(this) ); } diff --git a/test/account/PerHookData.t.sol b/test/account/PerHookData.t.sol index f9bfb823..85a59f10 100644 --- a/test/account/PerHookData.t.sol +++ b/test/account/PerHookData.t.sol @@ -23,7 +23,7 @@ contract PerHookDataTest is CustomValidationTestBase { function setUp() public { _signerValidation = - ModuleEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID); + ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID); _counter = new Counter(); @@ -346,7 +346,7 @@ contract PerHookDataTest is CustomValidationTestBase { ); // patched to also work during SMA tests by differentiating the validation _signerValidation = - ModuleEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID - 1); + ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID - 1); return ( _signerValidation, true, diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index 29663e13..9a76da77 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -99,9 +99,9 @@ contract UpgradeableModularAccountTest is AccountTestBase { : abi.encodeCall( UpgradeableModularAccount.execute, ( - address(singleSignerValidation), + address(singleSignerValidationModule), 0, - abi.encodeCall(SingleSignerValidation.transferSigner, (TEST_DEFAULT_VALIDATION_ENTITY_ID, owner2)) + abi.encodeCall(SingleSignerValidationModule.transferSigner, (TEST_DEFAULT_VALIDATION_ENTITY_ID, owner2)) ) ); @@ -359,16 +359,16 @@ contract UpgradeableModularAccountTest is AccountTestBase { assertEq(address(account3), address(uint160(uint256(vm.load(address(account1), slot))))); } - // TODO: Consider if this test belongs here or in the tests specific to the SingleSignerValidation + // TODO: Consider if this test belongs here or in the tests specific to the SingleSignerValidationModule function test_transferOwnership() public { if (vm.envBool("SMA_TEST")) { // Note: replaced "owner1" with address(0), this doesn't actually affect the account, but allows the // test to pass by ensuring the signer can be set in the validation. assertEq( - singleSignerValidation.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), address(0) + singleSignerValidationModule.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), address(0) ); } else { - assertEq(singleSignerValidation.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), owner1); + assertEq(singleSignerValidationModule.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), owner1); } vm.prank(address(entryPoint)); diff --git a/test/module/AllowlistModule.t.sol b/test/module/AllowlistModule.t.sol index 1edd422a..b03c080d 100644 --- a/test/module/AllowlistModule.t.sol +++ b/test/module/AllowlistModule.t.sol @@ -35,7 +35,7 @@ contract AllowlistModuleTest is CustomValidationTestBase { function setUp() public { _signerValidation = - ModuleEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID); + ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID); allowlistModule = new AllowlistModule(); @@ -343,7 +343,7 @@ contract AllowlistModuleTest is CustomValidationTestBase { ); // patched to also work during SMA tests by differentiating the validation _signerValidation = - ModuleEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID - 1); + ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID - 1); return ( _signerValidation, true, diff --git a/test/script/Deploy.s.t.sol b/test/script/Deploy.s.t.sol index e5618c70..a1d69c74 100644 --- a/test/script/Deploy.s.t.sol +++ b/test/script/Deploy.s.t.sol @@ -24,7 +24,7 @@ contract DeployTest is Test { address internal _accountImpl; address internal _smaImpl; - address internal _singleSignerValidation; + address internal _singleSignerValidationModule; address internal _factory; function setUp() public { @@ -51,8 +51,8 @@ contract DeployTest is Test { CREATE2_FACTORY ); - _singleSignerValidation = Create2.computeAddress( - bytes32(0), keccak256(abi.encodePacked(type(SingleSignerValidation).creationCode)), CREATE2_FACTORY + _singleSignerValidationModule = Create2.computeAddress( + bytes32(0), keccak256(abi.encodePacked(type(SingleSignerValidationModule).creationCode)), CREATE2_FACTORY ); _factory = Create2.computeAddress( @@ -64,7 +64,7 @@ contract DeployTest is Test { address(_entryPoint), _accountImpl, _smaImpl, - _singleSignerValidation, + _singleSignerValidationModule, _owner ) ) diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index 297a0914..675437da 100644 --- a/test/utils/AccountTestBase.sol +++ b/test/utils/AccountTestBase.sol @@ -54,20 +54,20 @@ abstract contract AccountTestBase is OptimizedTest { (owner1, owner1Key) = makeAddrAndKey("owner1"); beneficiary = payable(makeAddr("beneficiary")); - address deployedSingleSignerValidation = address(_deploySingleSignerValidation()); + address deployedSingleSignerValidation = address(_deploySingleSignerValidationModule()); // We etch the single signer validation to the max address, such that it coincides with the fallback // validation module entity for semi modular account tests. - singleSignerValidation = SingleSignerValidation(address(type(uint160).max)); - vm.etch(address(singleSignerValidation), deployedSingleSignerValidation.code); + singleSignerValidationModule = SingleSignerValidationModule(address(type(uint160).max)); + vm.etch(address(singleSignerValidationModule), deployedSingleSignerValidation.code); - factory = new SingleSignerFactoryFixture(entryPoint, singleSignerValidation); + factory = new SingleSignerFactoryFixture(entryPoint, singleSignerValidationModule); account1 = factory.createAccount(owner1, 0); vm.deal(address(account1), 100 ether); _signerValidation = - ModuleEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID); + ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID); } function _runExecUserOp(address target, bytes memory callData) internal { @@ -181,10 +181,10 @@ abstract contract AccountTestBase is OptimizedTest { abi.encodeCall( account1.execute, ( - address(singleSignerValidation), + address(singleSignerValidationModule), 0, abi.encodeCall( - SingleSignerValidation.transferSigner, (TEST_DEFAULT_VALIDATION_ENTITY_ID, address(this)) + SingleSignerValidationModule.transferSigner, (TEST_DEFAULT_VALIDATION_ENTITY_ID, address(this)) ) ) ), From 748032160060304f7957dcd6ad43dc592240c8fb Mon Sep 17 00:00:00 2001 From: zer0dot Date: Thu, 15 Aug 2024 15:50:04 -0400 Subject: [PATCH 15/34] chore: formatting --- test/account/UpgradeableModularAccount.t.sol | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index 9a76da77..7259c64f 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -101,7 +101,9 @@ contract UpgradeableModularAccountTest is AccountTestBase { ( address(singleSignerValidationModule), 0, - abi.encodeCall(SingleSignerValidationModule.transferSigner, (TEST_DEFAULT_VALIDATION_ENTITY_ID, owner2)) + abi.encodeCall( + SingleSignerValidationModule.transferSigner, (TEST_DEFAULT_VALIDATION_ENTITY_ID, owner2) + ) ) ); @@ -365,10 +367,13 @@ contract UpgradeableModularAccountTest is AccountTestBase { // Note: replaced "owner1" with address(0), this doesn't actually affect the account, but allows the // test to pass by ensuring the signer can be set in the validation. assertEq( - singleSignerValidationModule.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), address(0) + singleSignerValidationModule.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), + address(0) ); } else { - assertEq(singleSignerValidationModule.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), owner1); + assertEq( + singleSignerValidationModule.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), owner1 + ); } vm.prank(address(entryPoint)); From b01f8f33bc0c6fdfc882af64364e0f911afd06f0 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Thu, 15 Aug 2024 15:53:08 -0400 Subject: [PATCH 16/34] chore: formatting --- script/Deploy.s.sol | 4 +++- src/account/AccountFactory.sol | 7 ++----- test/mocks/SingleSignerFactoryFixture.sol | 5 +---- test/script/Deploy.s.t.sol | 12 ++++-------- test/utils/AccountTestBase.sol | 3 ++- 5 files changed, 12 insertions(+), 19 deletions(-) diff --git a/script/Deploy.s.sol b/script/Deploy.s.sol index 7d7ae1d7..f8c3771e 100644 --- a/script/Deploy.s.sol +++ b/script/Deploy.s.sol @@ -148,7 +148,9 @@ contract DeployScript is Script { keccak256( abi.encodePacked( type(AccountFactory).creationCode, - abi.encode(entryPoint, accountImpl, semiModularAccountImpl, singleSignerValidationModule, owner) + abi.encode( + entryPoint, accountImpl, semiModularAccountImpl, singleSignerValidationModule, owner + ) ) ), CREATE2_FACTORY diff --git a/src/account/AccountFactory.sol b/src/account/AccountFactory.sol index e379c784..5b4bab04 100644 --- a/src/account/AccountFactory.sol +++ b/src/account/AccountFactory.sol @@ -7,8 +7,8 @@ import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; -import {UpgradeableModularAccount} from "../account/UpgradeableModularAccount.sol"; import {SemiModularAccount} from "../account/SemiModularAccount.sol"; +import {UpgradeableModularAccount} from "../account/UpgradeableModularAccount.sol"; import {ValidationConfigLib} from "../helpers/ValidationConfigLib.sol"; import {LibClone} from "solady/utils/LibClone.sol"; @@ -69,10 +69,7 @@ contract AccountFactory is Ownable { return UpgradeableModularAccount(payable(addr)); } - function createSemiModularAccount(address owner, uint256 salt) - external - returns (UpgradeableModularAccount) - { + function createSemiModularAccount(address owner, uint256 salt) external returns (UpgradeableModularAccount) { // both module address and entityId for fallback validations are hardcoded at the maximum value. bytes32 fullSalt = getSalt(owner, salt, type(uint32).max); diff --git a/test/mocks/SingleSignerFactoryFixture.sol b/test/mocks/SingleSignerFactoryFixture.sol index 76f6f079..4a48314e 100644 --- a/test/mocks/SingleSignerFactoryFixture.sol +++ b/test/mocks/SingleSignerFactoryFixture.sol @@ -74,10 +74,7 @@ contract SingleSignerFactoryFixture is OptimizedTest { return UpgradeableModularAccount(payable(addr)); } - function createSemiModularAccount(address owner, uint256 salt) - public - returns (UpgradeableModularAccount) - { + function createSemiModularAccount(address owner, uint256 salt) public returns (UpgradeableModularAccount) { bytes32 fullSalt = getSalt(owner, salt); bytes memory immutables = _getImmutableArgs(owner); diff --git a/test/script/Deploy.s.t.sol b/test/script/Deploy.s.t.sol index a1d69c74..c8c1d6a2 100644 --- a/test/script/Deploy.s.t.sol +++ b/test/script/Deploy.s.t.sol @@ -52,7 +52,9 @@ contract DeployTest is Test { ); _singleSignerValidationModule = Create2.computeAddress( - bytes32(0), keccak256(abi.encodePacked(type(SingleSignerValidationModule).creationCode)), CREATE2_FACTORY + bytes32(0), + keccak256(abi.encodePacked(type(SingleSignerValidationModule).creationCode)), + CREATE2_FACTORY ); _factory = Create2.computeAddress( @@ -60,13 +62,7 @@ contract DeployTest is Test { keccak256( abi.encodePacked( type(AccountFactory).creationCode, - abi.encode( - address(_entryPoint), - _accountImpl, - _smaImpl, - _singleSignerValidationModule, - _owner - ) + abi.encode(address(_entryPoint), _accountImpl, _smaImpl, _singleSignerValidationModule, _owner) ) ), CREATE2_FACTORY diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index 675437da..c9d3328d 100644 --- a/test/utils/AccountTestBase.sol +++ b/test/utils/AccountTestBase.sol @@ -184,7 +184,8 @@ abstract contract AccountTestBase is OptimizedTest { address(singleSignerValidationModule), 0, abi.encodeCall( - SingleSignerValidationModule.transferSigner, (TEST_DEFAULT_VALIDATION_ENTITY_ID, address(this)) + SingleSignerValidationModule.transferSigner, + (TEST_DEFAULT_VALIDATION_ENTITY_ID, address(this)) ) ) ), From 07a54c39ab3916cea2183797405ba89e5ea79001 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Thu, 15 Aug 2024 15:55:03 -0400 Subject: [PATCH 17/34] chore: remove obsolete comment --- src/account/UpgradeableModularAccount.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index c069491f..cca80401 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -247,7 +247,6 @@ contract UpgradeableModularAccount is _uninstallExecution(module, manifest, moduleUninstallData); } - // TODO: Remove this function for SMA /// @notice Initializes the account with a validation function added to the global pool. /// TODO: remove and merge with regular initialization, after we figure out a better install/uninstall // workflow From 99d2b336a5916f06b37cc33edd6c116f26d2e315 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Thu, 15 Aug 2024 17:22:16 -0400 Subject: [PATCH 18/34] refactor: remove redundant factory check and correct return type for SMA --- src/account/AccountFactory.sol | 14 +++++++++----- test/mocks/SingleSignerFactoryFixture.sol | 12 +++++++----- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/account/AccountFactory.sol b/src/account/AccountFactory.sol index 5b4bab04..850e5fba 100644 --- a/src/account/AccountFactory.sol +++ b/src/account/AccountFactory.sol @@ -21,6 +21,8 @@ contract AccountFactory is Ownable { address public immutable SINGLE_SIGNER_VALIDATION_MODULE; event ModularAccountDeployed(address indexed account, address indexed owner, uint256 salt); + + error SemiModularAccountAddressMismatch(address expected, address returned); constructor( IEntryPoint _entryPoint, @@ -69,7 +71,7 @@ contract AccountFactory is Ownable { return UpgradeableModularAccount(payable(addr)); } - function createSemiModularAccount(address owner, uint256 salt) external returns (UpgradeableModularAccount) { + function createSemiModularAccount(address owner, uint256 salt) external returns (SemiModularAccount) { // both module address and entityId for fallback validations are hardcoded at the maximum value. bytes32 fullSalt = getSalt(owner, salt, type(uint32).max); @@ -77,13 +79,15 @@ contract AccountFactory is Ownable { address addr = _getAddressFallbackSigner(immutables, fullSalt); - // short circuit if exists - if (addr.code.length == 0) { - // not necessary to check return addr since next call will fail if so + // LibClone short-circuits if it's already deployed. + (, address instance) = LibClone.createDeterministicERC1967(address(SEMI_MODULAR_ACCOUNT_IMPL), immutables, fullSalt); + + if (instance != addr) { + revert SemiModularAccountAddressMismatch(addr, instance); } - return UpgradeableModularAccount(payable(addr)); + return SemiModularAccount(payable(addr)); } function addStake(uint32 unstakeDelay) external payable onlyOwner { diff --git a/test/mocks/SingleSignerFactoryFixture.sol b/test/mocks/SingleSignerFactoryFixture.sol index 4a48314e..859bc481 100644 --- a/test/mocks/SingleSignerFactoryFixture.sol +++ b/test/mocks/SingleSignerFactoryFixture.sol @@ -25,6 +25,8 @@ contract SingleSignerFactoryFixture is OptimizedTest { address public self; + error SemiModularAccountAddressMismatch(address expected, address returned); + constructor(IEntryPoint _entryPoint, SingleSignerValidationModule _singleSignerValidationModule) { entryPoint = _entryPoint; @@ -81,12 +83,12 @@ contract SingleSignerFactoryFixture is OptimizedTest { address addr = _getAddressFallbackSigner(immutables, fullSalt); - // short circuit if exists - if (addr.code.length == 0) { - // not necessary to check return addr since next call will fail if so - // new ERC1967Proxy{salt: getSalt(owner, salt, type(uint32).max)}(address(ACCOUNT_IMPL), ""); - // UpgradeableModularAccount(payable(addr)).initialize(owner); + // LibClone short-circuits if it's already deployed. + (, address instance) = LibClone.createDeterministicERC1967(address(accountImplementation), immutables, fullSalt); + + if (instance != addr) { + revert SemiModularAccountAddressMismatch(addr, instance); } return UpgradeableModularAccount(payable(addr)); From 47c407b501c54d6602757e90de1c64c1bd8e0662 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Thu, 15 Aug 2024 17:27:06 -0400 Subject: [PATCH 19/34] chore: small comment cleanup --- test/mocks/SingleSignerFactoryFixture.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/mocks/SingleSignerFactoryFixture.sol b/test/mocks/SingleSignerFactoryFixture.sol index 859bc481..3b9808ad 100644 --- a/test/mocks/SingleSignerFactoryFixture.sol +++ b/test/mocks/SingleSignerFactoryFixture.sol @@ -40,8 +40,6 @@ contract SingleSignerFactoryFixture is OptimizedTest { self = address(this); } - // TODO: Make createAccount use createAccountFallbackSigner for testing - /** * create an account, and return its address. * returns the address even if the account is already deployed. @@ -50,6 +48,8 @@ contract SingleSignerFactoryFixture is OptimizedTest { * account creation */ function createAccount(address owner, uint256 salt) public returns (UpgradeableModularAccount) { + // We cast the SemiModularAccount to an UpgradeableModularAccount to facilitate equivalence testing. + // However, we don't do this in the actual factory. if (vm.envBool("SMA_TEST")) { return createSemiModularAccount(owner, salt); } From b2f9257a6990e34a95fb64a53d6e4a0ad83d86e9 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Fri, 16 Aug 2024 17:28:33 -0400 Subject: [PATCH 20/34] feat: add SMA deploy event --- src/account/AccountFactory.sol | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/account/AccountFactory.sol b/src/account/AccountFactory.sol index 850e5fba..ff68dd1d 100644 --- a/src/account/AccountFactory.sol +++ b/src/account/AccountFactory.sol @@ -21,7 +21,8 @@ contract AccountFactory is Ownable { address public immutable SINGLE_SIGNER_VALIDATION_MODULE; event ModularAccountDeployed(address indexed account, address indexed owner, uint256 salt); - + event SemiModularAccountDeployed(address indexed account, address indexed owner, uint256 salt); + error SemiModularAccountAddressMismatch(address expected, address returned); constructor( @@ -80,13 +81,17 @@ contract AccountFactory is Ownable { address addr = _getAddressFallbackSigner(immutables, fullSalt); // LibClone short-circuits if it's already deployed. - (, address instance) = + (bool alreadyDeployed, address instance) = LibClone.createDeterministicERC1967(address(SEMI_MODULAR_ACCOUNT_IMPL), immutables, fullSalt); if (instance != addr) { revert SemiModularAccountAddressMismatch(addr, instance); } + if (!alreadyDeployed) { + emit SemiModularAccountDeployed(addr, owner, salt); + } + return SemiModularAccount(payable(addr)); } From b999e5b7d803bc36cf5b7c2f89bfeaf84d0450c2 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Fri, 16 Aug 2024 19:28:16 -0400 Subject: [PATCH 21/34] refactor: remove unused imports and extract variables to SMA --- src/account/SemiModularAccount.sol | 3 +++ src/account/UpgradeableModularAccount.sol | 8 -------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/account/SemiModularAccount.sol b/src/account/SemiModularAccount.sol index 72345201..19518901 100644 --- a/src/account/SemiModularAccount.sol +++ b/src/account/SemiModularAccount.sol @@ -28,6 +28,9 @@ contract SemiModularAccount is UpgradeableModularAccount { ModuleEntity internal constant _FALLBACK_VALIDATION = ModuleEntity.wrap(bytes24(type(uint192).max)); + uint256 internal constant _SIG_VALIDATION_PASSED = 0; + uint256 internal constant _SIG_VALIDATION_FAILED = 1; + event FallbackSignerSet(address indexed previousFallbackSigner, address indexed newFallbackSigner); event FallbackSignerDisabledSet(bool prevDisabled, bool newDisabled); diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index cca80401..4a5a8018 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -34,9 +34,6 @@ import {AccountStorage, getAccountStorage, toHookConfig, toSetValue} from "./Acc import {AccountStorageInitializable} from "./AccountStorageInitializable.sol"; import {ModuleManagerInternals} from "./ModuleManagerInternals.sol"; -import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; -import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; - contract UpgradeableModularAccount is AccountExecutor, AccountLoupe, @@ -54,8 +51,6 @@ contract UpgradeableModularAccount is using ValidationConfigLib for ValidationConfig; using HookConfigLib for HookConfig; using SparseCalldataSegmentLib for bytes; - using MessageHashUtils for bytes32; - using ECDSA for bytes32; struct PostExecToRun { bytes preExecHookReturnData; @@ -72,9 +67,6 @@ contract UpgradeableModularAccount is bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e; bytes4 internal constant _1271_INVALID = 0xffffffff; - uint256 internal constant _SIG_VALIDATION_PASSED = 0; - uint256 internal constant _SIG_VALIDATION_FAILED = 1; - event ModularAccountInitialized(IEntryPoint indexed entryPoint); error NonCanonicalEncoding(); From edce07b9fe5f6cd64309c6f08bb706ccb8e99cf0 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Mon, 19 Aug 2024 18:05:27 -0400 Subject: [PATCH 22/34] refactor: use vm.envOr instead of envBool --- test/account/ImmutableAppend.t.sol | 2 +- test/account/UpgradeableModularAccount.t.sol | 4 ++-- test/mocks/SingleSignerFactoryFixture.sol | 8 ++++---- test/utils/AccountTestBase.sol | 2 +- test/utils/CustomValidationTestBase.sol | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/test/account/ImmutableAppend.t.sol b/test/account/ImmutableAppend.t.sol index dd75f4a5..15f77c12 100644 --- a/test/account/ImmutableAppend.t.sol +++ b/test/account/ImmutableAppend.t.sol @@ -14,7 +14,7 @@ contract ImmutableAppendTest is AccountTestBase { /* -------------------------------------------------------------------------- */ function test_success_getData() public { - if (!vm.envBool("SMA_TEST")) { + if (!vm.envOr("SMA_TEST", false)) { // this test isn't relevant at all for non-SMA, and is temporary. return; } diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index 7259c64f..e6b9d28a 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -94,7 +94,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { } function test_basicUserOp_withInitCode() public { - bytes memory callData = vm.envBool("SMA_TEST") + bytes memory callData = vm.envOr("SMA_TEST", false) ? abi.encodeCall(SemiModularAccount(payable(account1)).updateFallbackSigner, (owner2)) : abi.encodeCall( UpgradeableModularAccount.execute, @@ -363,7 +363,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { // TODO: Consider if this test belongs here or in the tests specific to the SingleSignerValidationModule function test_transferOwnership() public { - if (vm.envBool("SMA_TEST")) { + if (vm.envOr("SMA_TEST", false)) { // Note: replaced "owner1" with address(0), this doesn't actually affect the account, but allows the // test to pass by ensuring the signer can be set in the validation. assertEq( diff --git a/test/mocks/SingleSignerFactoryFixture.sol b/test/mocks/SingleSignerFactoryFixture.sol index 3b9808ad..6faa1416 100644 --- a/test/mocks/SingleSignerFactoryFixture.sol +++ b/test/mocks/SingleSignerFactoryFixture.sol @@ -30,7 +30,7 @@ contract SingleSignerFactoryFixture is OptimizedTest { constructor(IEntryPoint _entryPoint, SingleSignerValidationModule _singleSignerValidationModule) { entryPoint = _entryPoint; - accountImplementation = vm.envBool("SMA_TEST") + accountImplementation = vm.envOr("SMA_TEST", false) ? _deploySemiModularAccount(_entryPoint) : _deployUpgradeableModularAccount(_entryPoint); _PROXY_BYTECODE_HASH = keccak256( @@ -50,7 +50,7 @@ contract SingleSignerFactoryFixture is OptimizedTest { function createAccount(address owner, uint256 salt) public returns (UpgradeableModularAccount) { // We cast the SemiModularAccount to an UpgradeableModularAccount to facilitate equivalence testing. // However, we don't do this in the actual factory. - if (vm.envBool("SMA_TEST")) { + if (vm.envOr("SMA_TEST", false)) { return createSemiModularAccount(owner, salt); } @@ -97,8 +97,8 @@ contract SingleSignerFactoryFixture is OptimizedTest { /** * calculate the counterfactual address of this account as it would be returned by createAccount() */ - function getAddress(address owner, uint256 salt) public view returns (address) { - if (vm.envBool("SMA_TEST")) { + function getAddress(address owner, uint256 salt) public returns (address) { + if (vm.envOr("SMA_TEST", false)) { return getAddressFallbackSigner(owner, salt); } return Create2.computeAddress(getSalt(owner, salt), _PROXY_BYTECODE_HASH); diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index c9d3328d..705690a8 100644 --- a/test/utils/AccountTestBase.sol +++ b/test/utils/AccountTestBase.sol @@ -170,7 +170,7 @@ abstract contract AccountTestBase is OptimizedTest { function _transferOwnershipToTest() internal { // Transfer ownership to test contract for easier invocation. vm.prank(owner1); - if (vm.envBool("SMA_TEST")) { + if (vm.envOr("SMA_TEST", false)) { account1.executeWithAuthorization( abi.encodeCall(SemiModularAccount(payable(account1)).updateFallbackSigner, (address(this))), _encodeSignature(_signerValidation, GLOBAL_VALIDATION, "") diff --git a/test/utils/CustomValidationTestBase.sol b/test/utils/CustomValidationTestBase.sol index ccac8da7..57643a1d 100644 --- a/test/utils/CustomValidationTestBase.sol +++ b/test/utils/CustomValidationTestBase.sol @@ -28,7 +28,7 @@ abstract contract CustomValidationTestBase is AccountTestBase { account1 = UpgradeableModularAccount(payable(new ERC1967Proxy{salt: 0}(accountImplementation, ""))); - if (vm.envBool("SMA_TEST")) { + if (vm.envOr("SMA_TEST", false)) { vm.prank(address(entryPoint)); // The initializer doesn't work on the SMA account1.installValidation( From 6c7ca4f33a5195b523a121b72522eed0311ed75e Mon Sep 17 00:00:00 2001 From: zer0dot Date: Mon, 19 Aug 2024 18:08:05 -0400 Subject: [PATCH 23/34] chore: remove obsolete comment --- script/Deploy.s.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/script/Deploy.s.sol b/script/Deploy.s.sol index f8c3771e..efbf0f05 100644 --- a/script/Deploy.s.sol +++ b/script/Deploy.s.sol @@ -164,7 +164,6 @@ contract DeployScript is Script { if (addr.code.length == 0) { console.log("No code found at expected address, deploying..."); - // Patched AccountFactory deployed = new AccountFactory{salt: salt}( entryPoint, UpgradeableModularAccount(payable(accountImpl)), From 9d43bde5a7c52f9e9e2238b55edd9d65c3664c7b Mon Sep 17 00:00:00 2001 From: zer0dot Date: Mon, 19 Aug 2024 18:15:15 -0400 Subject: [PATCH 24/34] chore: rename sma address getter --- src/account/AccountFactory.sol | 8 ++++---- test/mocks/SingleSignerFactoryFixture.sol | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/account/AccountFactory.sol b/src/account/AccountFactory.sol index ff68dd1d..9c63286f 100644 --- a/src/account/AccountFactory.sol +++ b/src/account/AccountFactory.sol @@ -78,7 +78,7 @@ contract AccountFactory is Ownable { bytes memory immutables = _getImmutableArgs(owner); - address addr = _getAddressFallbackSigner(immutables, fullSalt); + address addr = _getAddressSemiModular(immutables, fullSalt); // LibClone short-circuits if it's already deployed. (bool alreadyDeployed, address instance) = @@ -114,17 +114,17 @@ contract AccountFactory is Ownable { return Create2.computeAddress(getSalt(owner, salt, entityId), _PROXY_BYTECODE_HASH); } - function getAddressFallbackSigner(address owner, uint256 salt) public view returns (address) { + function getAddressSemiModular(address owner, uint256 salt) public view returns (address) { bytes32 fullSalt = getSalt(owner, salt, type(uint32).max); bytes memory immutables = _getImmutableArgs(owner); - return _getAddressFallbackSigner(immutables, fullSalt); + return _getAddressSemiModular(immutables, fullSalt); } function getSalt(address owner, uint256 salt, uint32 entityId) public pure returns (bytes32) { return keccak256(abi.encodePacked(owner, salt, entityId)); } - function _getAddressFallbackSigner(bytes memory immutables, bytes32 salt) internal view returns (address) { + function _getAddressSemiModular(bytes memory immutables, bytes32 salt) internal view returns (address) { return LibClone.predictDeterministicAddressERC1967(address(ACCOUNT_IMPL), immutables, salt, address(this)); } diff --git a/test/mocks/SingleSignerFactoryFixture.sol b/test/mocks/SingleSignerFactoryFixture.sol index 6faa1416..1594d29e 100644 --- a/test/mocks/SingleSignerFactoryFixture.sol +++ b/test/mocks/SingleSignerFactoryFixture.sol @@ -81,7 +81,7 @@ contract SingleSignerFactoryFixture is OptimizedTest { bytes memory immutables = _getImmutableArgs(owner); - address addr = _getAddressFallbackSigner(immutables, fullSalt); + address addr = _getAddressSemiModular(immutables, fullSalt); // LibClone short-circuits if it's already deployed. (, address instance) = @@ -99,15 +99,15 @@ contract SingleSignerFactoryFixture is OptimizedTest { */ function getAddress(address owner, uint256 salt) public returns (address) { if (vm.envOr("SMA_TEST", false)) { - return getAddressFallbackSigner(owner, salt); + return getAddressSemiModular(owner, salt); } return Create2.computeAddress(getSalt(owner, salt), _PROXY_BYTECODE_HASH); } - function getAddressFallbackSigner(address owner, uint256 salt) public view returns (address) { + function getAddressSemiModular(address owner, uint256 salt) public view returns (address) { bytes32 fullSalt = getSalt(owner, salt); bytes memory immutables = _getImmutableArgs(owner); - return _getAddressFallbackSigner(immutables, fullSalt); + return _getAddressSemiModular(immutables, fullSalt); } function addStake() external payable { @@ -118,7 +118,7 @@ contract SingleSignerFactoryFixture is OptimizedTest { return keccak256(abi.encodePacked(owner, salt)); } - function _getAddressFallbackSigner(bytes memory immutables, bytes32 salt) internal view returns (address) { + function _getAddressSemiModular(bytes memory immutables, bytes32 salt) internal view returns (address) { return LibClone.predictDeterministicAddressERC1967( address(accountImplementation), immutables, salt, address(this) ); From a8eb22026f3a7fb1a38b1a4007a7b2a2b229a88d Mon Sep 17 00:00:00 2001 From: zer0dot Date: Mon, 19 Aug 2024 18:21:10 -0400 Subject: [PATCH 25/34] refactor: remove dependency on default validation id for specific derived ids --- test/account/PerHookData.t.sol | 4 ++-- test/module/AllowlistModule.t.sol | 4 ++-- test/module/SingleSignerValidationModule.t.sol | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/test/account/PerHookData.t.sol b/test/account/PerHookData.t.sol index 85a59f10..219183fc 100644 --- a/test/account/PerHookData.t.sol +++ b/test/account/PerHookData.t.sol @@ -346,13 +346,13 @@ contract PerHookDataTest is CustomValidationTestBase { ); // patched to also work during SMA tests by differentiating the validation _signerValidation = - ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID - 1); + ModuleEntityLib.pack(address(singleSignerValidationModule), type(uint32).max - 1); return ( _signerValidation, true, true, new bytes4[](0), - abi.encode(TEST_DEFAULT_VALIDATION_ENTITY_ID - 1, owner1), + abi.encode(type(uint32).max - 1, owner1), hooks ); } diff --git a/test/module/AllowlistModule.t.sol b/test/module/AllowlistModule.t.sol index b03c080d..c1655009 100644 --- a/test/module/AllowlistModule.t.sol +++ b/test/module/AllowlistModule.t.sol @@ -343,13 +343,13 @@ contract AllowlistModuleTest is CustomValidationTestBase { ); // patched to also work during SMA tests by differentiating the validation _signerValidation = - ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID - 1); + ModuleEntityLib.pack(address(singleSignerValidationModule), type(uint32).max - 1); return ( _signerValidation, true, true, new bytes4[](0), - abi.encode(TEST_DEFAULT_VALIDATION_ENTITY_ID - 1, owner1), + abi.encode(type(uint32).max - 1, owner1), hooks ); } diff --git a/test/module/SingleSignerValidationModule.t.sol b/test/module/SingleSignerValidationModule.t.sol index 24b97c2d..5235ad87 100644 --- a/test/module/SingleSignerValidationModule.t.sol +++ b/test/module/SingleSignerValidationModule.t.sol @@ -83,7 +83,7 @@ contract SingleSignerValidationModuleTest is AccountTestBase { } function test_runtime_with2SameValidationInstalled() public { - uint32 newEntityId = TEST_DEFAULT_VALIDATION_ENTITY_ID - 1; + uint32 newEntityId = type(uint32).max - 1; vm.prank(address(entryPoint)); vm.expectEmit(address(singleSignerValidationModule)); From b7ab9e8a763303b2c5f5a9a3456b20bf6910c4a8 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Mon, 19 Aug 2024 18:29:50 -0400 Subject: [PATCH 26/34] test: add codesize check for sma impl --- test/script/Deploy.s.t.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/test/script/Deploy.s.t.sol b/test/script/Deploy.s.t.sol index c8c1d6a2..329d2501 100644 --- a/test/script/Deploy.s.t.sol +++ b/test/script/Deploy.s.t.sol @@ -87,6 +87,7 @@ contract DeployTest is Test { _deployScript.run(); assertTrue(_accountImpl.code.length > 0); + assertTrue(_smaImpl.code.length > 0); assertTrue(_factory.code.length > 0); assertTrue(_singleSignerValidationModule.code.length > 0); From 12fc8ad8169778a419ac730ac48d6271c94e5ecd Mon Sep 17 00:00:00 2001 From: zer0dot Date: Mon, 19 Aug 2024 18:37:08 -0400 Subject: [PATCH 27/34] refactor: revert early rather than explicitly returning --- src/account/UpgradeableModularAccount.sol | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 4a5a8018..2c7162e1 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -731,10 +731,9 @@ contract UpgradeableModularAccount is { // Check that the provided validation function is applicable to the selector if (isGlobal) { - if (_globalValidationAllowed(selector) && _isValidationGlobal(validationFunction)) { - return; + if (!_globalValidationAllowed(selector) || !_isValidationGlobal(validationFunction)) { + revert ValidationFunctionMissing(selector); } - revert ValidationFunctionMissing(selector); } else { // Not global validation, but per-selector if (!getAccountStorage().validationData[validationFunction].selectors.contains(toSetValue(selector))) { From 57577f0b7a8a3b473d99fbde3ccb3b5d8d1e6069 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Mon, 19 Aug 2024 18:39:57 -0400 Subject: [PATCH 28/34] chore: formatting --- test/account/PerHookData.t.sol | 12 ++---------- test/module/AllowlistModule.t.sol | 12 ++---------- 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/test/account/PerHookData.t.sol b/test/account/PerHookData.t.sol index 219183fc..2be42f86 100644 --- a/test/account/PerHookData.t.sol +++ b/test/account/PerHookData.t.sol @@ -345,15 +345,7 @@ contract PerHookDataTest is CustomValidationTestBase { abi.encode(_counter) ); // patched to also work during SMA tests by differentiating the validation - _signerValidation = - ModuleEntityLib.pack(address(singleSignerValidationModule), type(uint32).max - 1); - return ( - _signerValidation, - true, - true, - new bytes4[](0), - abi.encode(type(uint32).max - 1, owner1), - hooks - ); + _signerValidation = ModuleEntityLib.pack(address(singleSignerValidationModule), type(uint32).max - 1); + return (_signerValidation, true, true, new bytes4[](0), abi.encode(type(uint32).max - 1, owner1), hooks); } } diff --git a/test/module/AllowlistModule.t.sol b/test/module/AllowlistModule.t.sol index c1655009..0d66ed85 100644 --- a/test/module/AllowlistModule.t.sol +++ b/test/module/AllowlistModule.t.sol @@ -342,16 +342,8 @@ contract AllowlistModuleTest is CustomValidationTestBase { abi.encode(HOOK_ENTITY_ID, allowlistInit) ); // patched to also work during SMA tests by differentiating the validation - _signerValidation = - ModuleEntityLib.pack(address(singleSignerValidationModule), type(uint32).max - 1); - return ( - _signerValidation, - true, - true, - new bytes4[](0), - abi.encode(type(uint32).max - 1, owner1), - hooks - ); + _signerValidation = ModuleEntityLib.pack(address(singleSignerValidationModule), type(uint32).max - 1); + return (_signerValidation, true, true, new bytes4[](0), abi.encode(type(uint32).max - 1, owner1), hooks); } // Unfortunately, this is a feature that solidity has only implemented in via-ir, so we need to do it manually From 4ab5b75c3a64860510a53221ded7dd94564d8ee1 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Mon, 19 Aug 2024 18:52:35 -0400 Subject: [PATCH 29/34] refactor: removed obsolete address comparison from sma creation --- src/account/AccountFactory.sol | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/account/AccountFactory.sol b/src/account/AccountFactory.sol index 9c63286f..11ad5684 100644 --- a/src/account/AccountFactory.sol +++ b/src/account/AccountFactory.sol @@ -23,8 +23,6 @@ contract AccountFactory is Ownable { event ModularAccountDeployed(address indexed account, address indexed owner, uint256 salt); event SemiModularAccountDeployed(address indexed account, address indexed owner, uint256 salt); - error SemiModularAccountAddressMismatch(address expected, address returned); - constructor( IEntryPoint _entryPoint, UpgradeableModularAccount _accountImpl, @@ -78,21 +76,15 @@ contract AccountFactory is Ownable { bytes memory immutables = _getImmutableArgs(owner); - address addr = _getAddressSemiModular(immutables, fullSalt); - // LibClone short-circuits if it's already deployed. (bool alreadyDeployed, address instance) = LibClone.createDeterministicERC1967(address(SEMI_MODULAR_ACCOUNT_IMPL), immutables, fullSalt); - if (instance != addr) { - revert SemiModularAccountAddressMismatch(addr, instance); - } - if (!alreadyDeployed) { - emit SemiModularAccountDeployed(addr, owner, salt); + emit SemiModularAccountDeployed(instance, owner, salt); } - return SemiModularAccount(payable(addr)); + return SemiModularAccount(payable(instance)); } function addStake(uint32 unstakeDelay) external payable onlyOwner { From 1d6b1eda70c3489dae4386e209943ed3548e139f Mon Sep 17 00:00:00 2001 From: zer0dot Date: Mon, 19 Aug 2024 18:56:31 -0400 Subject: [PATCH 30/34] fix: use unchecked fallback signer getter for external getter --- src/account/SemiModularAccount.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/account/SemiModularAccount.sol b/src/account/SemiModularAccount.sol index 19518901..3566e26e 100644 --- a/src/account/SemiModularAccount.sol +++ b/src/account/SemiModularAccount.sol @@ -80,7 +80,7 @@ contract SemiModularAccount is UpgradeableModularAccount { /// validation is enabled or not. /// @return The fallback signer address, either overriden in storage, or read from bytecode. function getFallbackSigner() external view returns (address) { - return _getFallbackSigner(); + return _retrieveFallbackSignerUnchecked(_getSemiModularAccountStorage()); } function _execUserOpValidation( From 44a42115f4ecfb3dc703239bdae9108cadf8ae76 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Mon, 19 Aug 2024 20:32:27 -0400 Subject: [PATCH 31/34] chore: remove obsolete test --- test/account/ImmutableAppend.t.sol | 30 ------------------------------ 1 file changed, 30 deletions(-) delete mode 100644 test/account/ImmutableAppend.t.sol diff --git a/test/account/ImmutableAppend.t.sol b/test/account/ImmutableAppend.t.sol deleted file mode 100644 index 15f77c12..00000000 --- a/test/account/ImmutableAppend.t.sol +++ /dev/null @@ -1,30 +0,0 @@ -// SPDX-License-Identifier: GPL-3.0 -pragma solidity ^0.8.19; - -import {AccountTestBase} from "../utils/AccountTestBase.sol"; -import {LibClone} from "solady/utils/LibClone.sol"; - -contract ImmutableAppendTest is AccountTestBase { - /* -------------------------------------------------------------------------- */ - /* Negatives */ - /* -------------------------------------------------------------------------- */ - - /* -------------------------------------------------------------------------- */ - /* Positives */ - /* -------------------------------------------------------------------------- */ - - function test_success_getData() public { - if (!vm.envOr("SMA_TEST", false)) { - // this test isn't relevant at all for non-SMA, and is temporary. - return; - } - - bytes memory expectedArgs = abi.encodePacked(owner1); - - assertEq(keccak256(LibClone.argsOnERC1967(address(account1))), keccak256(expectedArgs)); - } - - /* -------------------------------------------------------------------------- */ - /* Internals */ - /* -------------------------------------------------------------------------- */ -} From 37ab830e353e8ac420a29d0c48d4455f7b333b9a Mon Sep 17 00:00:00 2001 From: zer0dot Date: Mon, 19 Aug 2024 20:35:20 -0400 Subject: [PATCH 32/34] chore: remove leftover event --- src/account/UpgradeableModularAccount.sol | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 2c7162e1..410c8610 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -67,8 +67,6 @@ contract UpgradeableModularAccount is bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e; bytes4 internal constant _1271_INVALID = 0xffffffff; - event ModularAccountInitialized(IEntryPoint indexed entryPoint); - error NonCanonicalEncoding(); error NotEntryPoint(); error PostExecHookReverted(address module, uint32 entityId, bytes revertReason); @@ -240,9 +238,6 @@ contract UpgradeableModularAccount is } /// @notice Initializes the account with a validation function added to the global pool. - /// TODO: remove and merge with regular initialization, after we figure out a better install/uninstall - // workflow - /// with user install configs. /// @dev This function is only callable once. function initializeWithValidation( ValidationConfig validationConfig, From dbbca2a5ab59d52da93c29f912c01389d38d2b3e Mon Sep 17 00:00:00 2001 From: zer0dot Date: Tue, 20 Aug 2024 15:16:05 -0400 Subject: [PATCH 33/34] fix: add missing SMA function selector to global validation check, slight renaming --- src/account/SemiModularAccount.sol | 3 ++- test/account/DirectCallsFromModule.t.sol | 19 ++++++++++--------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/account/SemiModularAccount.sol b/src/account/SemiModularAccount.sol index 3566e26e..4a6e918d 100644 --- a/src/account/SemiModularAccount.sol +++ b/src/account/SemiModularAccount.sol @@ -138,7 +138,8 @@ contract SemiModularAccount is UpgradeableModularAccount { } function _globalValidationAllowed(bytes4 selector) internal view override returns (bool) { - return selector == this.updateFallbackSigner.selector || super._globalValidationAllowed(selector); + return selector == this.setFallbackSignerDisabled.selector + || selector == this.updateFallbackSigner.selector || super._globalValidationAllowed(selector); } function _isValidationGlobal(ModuleEntity validationFunction) internal view override returns (bool) { diff --git a/test/account/DirectCallsFromModule.t.sol b/test/account/DirectCallsFromModule.t.sol index 5ffeecf3..eff6ba0c 100644 --- a/test/account/DirectCallsFromModule.t.sol +++ b/test/account/DirectCallsFromModule.t.sol @@ -20,6 +20,7 @@ contract DirectCallsFromModuleTest is AccountTestBase { function setUp() public { _module = new DirectCallModule(); + assertNotEq(_module, address(type(uint160).max)); assertFalse(_module.preHookRan()); assertFalse(_module.postHookRan()); _moduleEntity = ModuleEntityLib.pack(address(_module), type(uint32).max); @@ -36,9 +37,9 @@ contract DirectCallsFromModuleTest is AccountTestBase { } function test_Fail_DirectCallModuleUninstalled() external { - _installExecution(); + _installValidation(); - _uninstallExecution(); + _uninstallValidation(); vm.prank(address(_module)); vm.expectRevert(_buildDirectCallDisallowedError(IStandardExecutor.execute.selector)); @@ -46,7 +47,7 @@ contract DirectCallsFromModuleTest is AccountTestBase { } function test_Fail_DirectCallModuleCallOtherSelector() external { - _installExecution(); + _installValidation(); Call[] memory calls = new Call[](0); @@ -60,7 +61,7 @@ contract DirectCallsFromModuleTest is AccountTestBase { /* -------------------------------------------------------------------------- */ function test_Pass_DirectCallFromModulePrank() external { - _installExecution(); + _installValidation(); vm.prank(address(_module)); account1.execute(address(0), 0, ""); @@ -70,7 +71,7 @@ contract DirectCallsFromModuleTest is AccountTestBase { } function test_Pass_DirectCallFromModuleCallback() external { - _installExecution(); + _installValidation(); bytes memory encodedCall = abi.encodeCall(DirectCallModule.directCall, ()); @@ -88,7 +89,7 @@ contract DirectCallsFromModuleTest is AccountTestBase { function test_Flow_DirectCallFromModuleSequence() external { // Install => Succeesfully call => uninstall => fail to call - _installExecution(); + _installValidation(); vm.prank(address(_module)); account1.execute(address(0), 0, ""); @@ -96,7 +97,7 @@ contract DirectCallsFromModuleTest is AccountTestBase { assertTrue(_module.preHookRan()); assertTrue(_module.postHookRan()); - _uninstallExecution(); + _uninstallValidation(); vm.prank(address(_module)); vm.expectRevert(_buildDirectCallDisallowedError(IStandardExecutor.execute.selector)); @@ -107,7 +108,7 @@ contract DirectCallsFromModuleTest is AccountTestBase { /* Internals */ /* -------------------------------------------------------------------------- */ - function _installExecution() internal { + function _installValidation() internal { bytes4[] memory selectors = new bytes4[](1); selectors[0] = IStandardExecutor.execute.selector; @@ -124,7 +125,7 @@ contract DirectCallsFromModuleTest is AccountTestBase { account1.installValidation(validationConfig, selectors, "", hooks); } - function _uninstallExecution() internal { + function _uninstallValidation() internal { (address module, uint32 entityId) = ModuleEntityLib.unpack(_moduleEntity); vm.prank(address(entryPoint)); vm.expectEmit(true, true, true, true); From bc829fc08bfa69d9fa5f84ab93b7361bfd0c391a Mon Sep 17 00:00:00 2001 From: zer0dot Date: Tue, 20 Aug 2024 15:22:22 -0400 Subject: [PATCH 34/34] fix: remove obsolete check --- test/account/DirectCallsFromModule.t.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/test/account/DirectCallsFromModule.t.sol b/test/account/DirectCallsFromModule.t.sol index eff6ba0c..5f013055 100644 --- a/test/account/DirectCallsFromModule.t.sol +++ b/test/account/DirectCallsFromModule.t.sol @@ -20,7 +20,6 @@ contract DirectCallsFromModuleTest is AccountTestBase { function setUp() public { _module = new DirectCallModule(); - assertNotEq(_module, address(type(uint160).max)); assertFalse(_module.preHookRan()); assertFalse(_module.postHookRan()); _moduleEntity = ModuleEntityLib.pack(address(_module), type(uint32).max);