From e41d52d43dc953c9b9b259449e9e9500e58c8280 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Tue, 6 Aug 2024 21:48:31 -0400 Subject: [PATCH 01/15] feat: refactor account to be inheritable, env variable toggle for SMA test --- .env.example | 4 +- src/account/SemiModularAccount.sol | 102 ++++++++++++++++++ src/account/UpgradeableModularAccount.sol | 67 +++++++----- .../validation/SingleSignerValidation.sol | 8 +- test/account/ImmutableAppend.t.sol | 8 +- test/account/MultiValidation.t.sol | 11 +- test/account/PerHookData.t.sol | 6 +- test/account/UpgradeableModularAccount.t.sol | 29 +++-- test/mocks/SingleSignerFactoryFixture.sol | 13 ++- test/module/AllowlistModule.t.sol | 6 +- test/module/SingleSignerValidation.t.sol | 2 +- test/utils/AccountTestBase.sol | 27 ++++- test/utils/CustomValidationTestBase.sol | 25 +++-- test/utils/OptimizedTest.sol | 9 ++ test/utils/TestConstants.sol | 2 +- 15 files changed, 263 insertions(+), 56 deletions(-) create mode 100644 src/account/SemiModularAccount.sol diff --git a/.env.example b/.env.example index 7412b0ad..01628425 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/src/account/SemiModularAccount.sol b/src/account/SemiModularAccount.sol new file mode 100644 index 00000000..b1ea8bd5 --- /dev/null +++ b/src/account/SemiModularAccount.sol @@ -0,0 +1,102 @@ +// 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 {IModuleManager, ModuleEntity, ValidationConfig} from "../interfaces/IModuleManager.sol"; +import {IValidation} from "../interfaces/IValidation.sol"; + +import {AccountStorage, getAccountStorage} from "./AccountStorage.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; + + error InitializerDisabled(); + + constructor(IEntryPoint anEntryPoint) UpgradeableModularAccount(anEntryPoint) {} + + function initializeWithValidation(ValidationConfig, bytes4[] calldata, bytes calldata, bytes[] calldata) + external + override + initializer + { + revert InitializerDisabled(); + } + + function _exec1271Validation(ModuleEntity sigValidation, bytes32 hash, bytes calldata signature) + internal + view + override + returns (bytes4) + { + if (sigValidation.eq(_FALLBACK_VALIDATION)) { + // do sig validation + if (SignatureChecker.isValidSignatureNow(_getFallbackSigner(), hash, signature)) { + return _1271_MAGIC_VALUE; + } + return _1271_INVALID; + } + return super._exec1271Validation(sigValidation, hash, signature); + } + + function _execRuntimeValidation( + ModuleEntity runtimeValidationFunction, + bytes calldata callData, + bytes calldata authorization + ) internal override { + if (runtimeValidationFunction.eq(_FALLBACK_VALIDATION)) { + if (msg.sender != _getFallbackSigner()) { + revert FallbackSignerMismatch(); + } + return; + } + super._execRuntimeValidation(runtimeValidationFunction, callData, authorization); + } + + function _execUserOpValidation( + ModuleEntity userOpValidationFunction, + PackedUserOperation memory userOp, + bytes32 userOpHash + ) internal override returns (uint256) { + if (userOpValidationFunction.eq(_FALLBACK_VALIDATION)) { + // fallback userop validation + // Validate the user op signature against the owner. + if ( + SignatureChecker.isValidSignatureNow( + _getFallbackSigner(), userOpHash.toEthSignedMessageHash(), userOp.signature + ) + ) { + return _SIG_VALIDATION_PASSED; + } + return _SIG_VALIDATION_FAILED; + } + + return super._execUserOpValidation(userOpValidationFunction, userOp, userOpHash); + } + + 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/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index e2982ae6..4d7b39ff 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -35,9 +35,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, @@ -270,7 +267,7 @@ contract UpgradeableModularAccount is bytes4[] calldata selectors, bytes calldata installData, bytes[] calldata hooks - ) external initializer { + ) external virtual initializer { _installValidation(validationConfig, selectors, installData, hooks); emit ModularAccountInitialized(_ENTRY_POINT); } @@ -337,14 +334,18 @@ 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:]); - } + return _exec1271Validation(sigValidation, hash, signature[24:]); + } + + 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) { @@ -352,7 +353,7 @@ contract UpgradeableModularAccount is } if ( - IValidation(module).validateSignature(address(this), entityId, msg.sender, hash, signature[24:]) + IValidation(module).validateSignature(address(this), entityId, msg.sender, hash, signature) == _1271_MAGIC_VALUE ) { return _1271_MAGIC_VALUE; @@ -456,15 +457,15 @@ 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(); + uint256 currentValidationRes = _execUserOpValidation(userOpValidationFunction, userOp, userOpHash); + // 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); - } + // currentValidationRes = IValidation(module).validateUserOp(entityId, userOp, userOpHash); + // } if (preUserOpValidationHooks.length != 0) { // If we have other validation data we need to coalesce with @@ -477,6 +478,16 @@ contract UpgradeableModularAccount is return validationRes; } + 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 _doRuntimeValidation( ModuleEntity runtimeValidationFunction, bytes calldata callData, @@ -517,15 +528,23 @@ contract UpgradeableModularAccount is revert ValidationSignatureSegmentMissing(); } - if (runtimeValidationFunction.eq(_FALLBACK_VALIDATION)) { - _fallbackRuntimeValidation(); - return; - } + _execRuntimeValidation(runtimeValidationFunction, callData, authSegment.getBody()); + } + + function _execRuntimeValidation( + ModuleEntity runtimeValidationFunction, + bytes calldata callData, + bytes calldata authorization + ) internal virtual { + // if (runtimeValidationFunction.eq(_FALLBACK_VALIDATION)) { + // _fallbackRuntimeValidation(); + // return; + // } (address module, uint32 entityId) = runtimeValidationFunction.unpack(); try IValidation(module).validateRuntime( - address(this), entityId, msg.sender, msg.value, callData, authSegment.getBody() + address(this), entityId, msg.sender, msg.value, callData, authorization ) // forgefmt: disable-start // solhint-disable-next-line no-empty-blocks diff --git a/src/modules/validation/SingleSignerValidation.sol b/src/modules/validation/SingleSignerValidation.sol index b9432009..e80cac1c 100644 --- a/src/modules/validation/SingleSignerValidation.sol +++ b/src/modules/validation/SingleSignerValidation.sol @@ -1,14 +1,14 @@ // 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 {IValidation} from "../../interfaces/IValidation.sol"; import {BaseModule} from "../BaseModule.sol"; import {ISingleSignerValidation} from "./ISingleSignerValidation.sol"; +import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.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"; /// @title ECSDA Validation /// @author ERC-6900 Authors diff --git a/test/account/ImmutableAppend.t.sol b/test/account/ImmutableAppend.t.sol index a63f0408..dfb2a39c 100644 --- a/test/account/ImmutableAppend.t.sol +++ b/test/account/ImmutableAppend.t.sol @@ -1,11 +1,12 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.19; -import {IEntryPoint, UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; +import {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 {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; @@ -24,6 +25,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 9693c5ac..e1f9e340 100644 --- a/test/account/MultiValidation.t.sol +++ b/test/account/MultiValidation.t.sol @@ -17,6 +17,7 @@ import {SingleSignerValidation} from "../../src/modules/validation/SingleSignerV import {AccountTestBase} from "../utils/AccountTestBase.sol"; import {TEST_DEFAULT_VALIDATION_ENTITY_ID} from "../utils/TestConstants.sol"; +import "forge-std/console.sol"; contract MultiValidationTest is AccountTestBase { using ECDSA for bytes32; @@ -64,7 +65,15 @@ contract MultiValidationTest is AccountTestBase { abi.encodeWithSelector( UpgradeableModularAccount.RuntimeValidationFunctionReverted.selector, address(validator2), - 1, + TEST_DEFAULT_VALIDATION_ENTITY_ID, + abi.encodeWithSignature("NotAuthorized()") + ) + ); + console.logBytes( + abi.encodeWithSelector( + UpgradeableModularAccount.RuntimeValidationFunctionReverted.selector, + address(validator2), + 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/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index 651e6f8c..52fe9fef 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -92,13 +92,22 @@ contract UpgradeableModularAccountTest is AccountTestBase { } function test_basicUserOp_withInitCode() public { + bytes memory callData = vm.envBool("SMA_TEST") + ? abi.encodeCall(UpgradeableModularAccount.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), @@ -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 c474308b..86a0506b 100644 --- a/test/mocks/SingleSignerFactoryFixture.sol +++ b/test/mocks/SingleSignerFactoryFixture.sol @@ -5,6 +5,7 @@ import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntry import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; +import {SemiModularAccount} from "../../src/account/SemiModularAccount.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol"; import {SingleSignerValidation} from "../../src/modules/validation/SingleSignerValidation.sol"; @@ -27,7 +28,10 @@ contract SingleSignerFactoryFixture is OptimizedTest { constructor(IEntryPoint _entryPoint, SingleSignerValidation _singleSignerValidation) { 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 +49,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 +100,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/SingleSignerValidation.t.sol b/test/module/SingleSignerValidation.t.sol index e3b4e4fa..8108f236 100644 --- a/test/module/SingleSignerValidation.t.sol +++ b/test/module/SingleSignerValidation.t.sol @@ -79,7 +79,7 @@ contract SingleSignerValidationTest 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(true, true, true, true); diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index b6d5fbbe..1dd7e0a2 100644 --- a/test/utils/AccountTestBase.sol +++ b/test/utils/AccountTestBase.sol @@ -53,13 +53,27 @@ abstract contract AccountTestBase is OptimizedTest { (owner1, owner1Key) = makeAddrAndKey("owner1"); beneficiary = payable(makeAddr("beneficiary")); - singleSignerValidation = _deploySingleSignerValidation(); + address deployedSingleSignerValidation = address(_deploySingleSignerValidation()); + + // 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.createAccountWithFallbackValidation(owner1, 0); + if (vm.envBool("SMA_TEST")) { + account1 = factory.createAccountWithFallbackValidation(owner1, 0); + } else { + account1 = factory.createAccount(owner1, 0); + } + // account1 = factory.createAccount(owner1, 0); vm.deal(address(account1), 100 ether); - _signerValidation = ModuleEntity.wrap(bytes24(type(uint192).max)); + //TODO: env variable for semi-modular test? + _signerValidation = + ModuleEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID); + // _signerValidation = ModuleEntity.wrap(bytes24(type(uint192).max)); } function _runExecUserOp(address target, bytes memory callData) internal { @@ -162,6 +176,13 @@ 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(account1.updateFallbackSigner, (address(this))), + _encodeSignature(_signerValidation, GLOBAL_VALIDATION, "") + ); + return; + } account1.executeWithAuthorization( abi.encodeCall(account1.updateFallbackSigner, (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 870d416a..cf1a2b9d 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,14 @@ 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")) + ) + : 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 5e7bd0f75d15d1394acfc9db52ba7e6a845db89a Mon Sep 17 00:00:00 2001 From: zer0dot Date: Tue, 6 Aug 2024 22:06:54 -0400 Subject: [PATCH 02/15] test: fix workflow --- .github/workflows/test.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index dfa5cf41..c23f229f 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -88,4 +88,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 From d5290d65fa7c995f427f2ce77fee24be25a2916f Mon Sep 17 00:00:00 2001 From: zer0dot Date: Tue, 6 Aug 2024 22:23:41 -0400 Subject: [PATCH 03/15] refactor: clean up unnecessary env variable check, remove obsolete comments --- src/account/UpgradeableModularAccount.sol | 13 ------------- test/utils/AccountTestBase.sol | 9 +-------- 2 files changed, 1 insertion(+), 21 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 4d7b39ff..717391fe 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -458,14 +458,6 @@ contract UpgradeableModularAccount is userOp.signature = signatureSegment.getBody(); uint256 currentValidationRes = _execUserOpValidation(userOpValidationFunction, userOp, userOpHash); - // 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 @@ -536,11 +528,6 @@ contract UpgradeableModularAccount is bytes calldata callData, bytes calldata authorization ) internal virtual { - // if (runtimeValidationFunction.eq(_FALLBACK_VALIDATION)) { - // _fallbackRuntimeValidation(); - // return; - // } - (address module, uint32 entityId) = runtimeValidationFunction.unpack(); try IValidation(module).validateRuntime( diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index 1dd7e0a2..909cb09f 100644 --- a/test/utils/AccountTestBase.sol +++ b/test/utils/AccountTestBase.sol @@ -62,18 +62,11 @@ abstract contract AccountTestBase is OptimizedTest { factory = new SingleSignerFactoryFixture(entryPoint, singleSignerValidation); - if (vm.envBool("SMA_TEST")) { - account1 = factory.createAccountWithFallbackValidation(owner1, 0); - } else { - account1 = factory.createAccount(owner1, 0); - } - // account1 = factory.createAccount(owner1, 0); + account1 = factory.createAccount(owner1, 0); vm.deal(address(account1), 100 ether); - //TODO: env variable for semi-modular test? _signerValidation = ModuleEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID); - // _signerValidation = ModuleEntity.wrap(bytes24(type(uint192).max)); } function _runExecUserOp(address target, bytes memory callData) internal { From b3bbeb46b04ff0299e9db8fc049d502db3c706b7 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 7 Aug 2024 15:53:47 -0400 Subject: [PATCH 04/15] chore: fix optimized-test workflow --- .github/workflows/test.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index c23f229f..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) From 0e8d36b530424fadfd14aa9bf45d64208c2f55cc Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 7 Aug 2024 16:26:15 -0400 Subject: [PATCH 05/15] feat: extract remaining semi-modular features from modular account --- src/account/SemiModularAccount.sol | 48 ++++++++++++-- src/account/UpgradeableModularAccount.sol | 76 +++-------------------- 2 files changed, 49 insertions(+), 75 deletions(-) diff --git a/src/account/SemiModularAccount.sol b/src/account/SemiModularAccount.sol index b1ea8bd5..3a8635b3 100644 --- a/src/account/SemiModularAccount.sol +++ b/src/account/SemiModularAccount.sol @@ -20,6 +20,12 @@ contract SemiModularAccount is UpgradeableModularAccount { using MessageHashUtils for bytes32; using ModuleEntityLib for ModuleEntity; + ModuleEntity internal constant _FALLBACK_VALIDATION = ModuleEntity.wrap(bytes24(type(uint192).max)); + + event FallbackSignerSet(address indexed previousFallbackSigner, address indexed newFallbackSigner); + + error FallbackSignerMismatch(); + error FallbackSignerDisabled(); error InitializerDisabled(); constructor(IEntryPoint anEntryPoint) UpgradeableModularAccount(anEntryPoint) {} @@ -32,6 +38,19 @@ contract SemiModularAccount is UpgradeableModularAccount { revert InitializerDisabled(); } + 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 + } + function _exec1271Validation(ModuleEntity sigValidation, bytes32 hash, bytes calldata signature) internal view @@ -39,8 +58,9 @@ contract SemiModularAccount is UpgradeableModularAccount { returns (bytes4) { if (sigValidation.eq(_FALLBACK_VALIDATION)) { - // do sig validation - if (SignatureChecker.isValidSignatureNow(_getFallbackSigner(), hash, signature)) { + address fallbackSigner = _getFallbackSigner(); + + if (SignatureChecker.isValidSignatureNow(fallbackSigner, hash, signature)) { return _1271_MAGIC_VALUE; } return _1271_INVALID; @@ -54,7 +74,9 @@ contract SemiModularAccount is UpgradeableModularAccount { bytes calldata authorization ) internal override { if (runtimeValidationFunction.eq(_FALLBACK_VALIDATION)) { - if (msg.sender != _getFallbackSigner()) { + address fallbackSigner = _getFallbackSigner(); + + if (msg.sender != fallbackSigner) { revert FallbackSignerMismatch(); } return; @@ -68,11 +90,11 @@ contract SemiModularAccount is UpgradeableModularAccount { bytes32 userOpHash ) internal override returns (uint256) { if (userOpValidationFunction.eq(_FALLBACK_VALIDATION)) { - // fallback userop validation - // Validate the user op signature against the owner. + address fallbackSigner = _getFallbackSigner(); + if ( SignatureChecker.isValidSignatureNow( - _getFallbackSigner(), userOpHash.toEthSignedMessageHash(), userOp.signature + fallbackSigner, userOpHash.toEthSignedMessageHash(), userOp.signature ) ) { return _SIG_VALIDATION_PASSED; @@ -99,4 +121,18 @@ contract SemiModularAccount is UpgradeableModularAccount { return address(uint160(bytes20(appendedData))); } + + function _globalValidationAllowed(bytes4 selector) internal view override returns (bool) { + return selector == this.updateFallbackSigner.selector || super._globalValidationAllowed(selector); + } + + function _isValidationGlobal(AccountStorage storage _storage, ModuleEntity validationFunction) + internal + view + override + returns (bool) + { + return + validationFunction.eq(_FALLBACK_VALIDATION) || super._isValidationGlobal(_storage, validationFunction); + } } diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 717391fe..c565bf0a 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -74,10 +74,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 AuthorizeUpgradeReverted(bytes revertReason); error ExecFromModuleNotPermitted(address module, bytes4 selector); @@ -99,8 +96,6 @@ contract UpgradeableModularAccount is error ValidationDoesNotApply(bytes4 selector, address module, uint32 entityId, bool isGlobal); 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 @@ -272,19 +267,6 @@ contract UpgradeableModularAccount is emit ModularAccountInitialized(_ENTRY_POINT); } - 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( @@ -741,12 +723,12 @@ contract UpgradeableModularAccount is } } - function _globalValidationAllowed(bytes4 selector) internal view returns (bool) { + 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 || selector == this.updateFallbackSigner.selector + || selector == this.upgradeToAndCall.selector ) { return true; } @@ -762,13 +744,7 @@ contract UpgradeableModularAccount is // 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(_storage, validationFunction)) { return; } revert ValidationFunctionMissing(selector); @@ -780,50 +756,12 @@ contract UpgradeableModularAccount is } } - function _fallbackRuntimeValidation() internal view { - if (msg.sender != _getFallbackSigner()) { - revert FallbackSignerMismatch(); - } - } - - function _fallbackUserOpValidation(PackedUserOperation memory userOp, bytes32 userOpHash) + function _isValidationGlobal(AccountStorage storage _storage, ModuleEntity validationFunction) 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) + virtual + returns (bool) { - 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))); + return _storage.validationData[validationFunction].isGlobal; } } From 9c19aec6bbbea48c916adebf51a1a6d9a0541e07 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 7 Aug 2024 16:26:25 -0400 Subject: [PATCH 06/15] chore: remove obsolete test --- test/account/FallbackValidationTest.t.sol | 77 ----------------------- 1 file changed, 77 deletions(-) delete mode 100644 test/account/FallbackValidationTest.t.sol 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); - } -} From f4a757c2363359bc474f8f3f17816ea42a52c4e2 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 7 Aug 2024 16:26:36 -0400 Subject: [PATCH 07/15] fix: fix tests post-rebase --- test/account/UpgradeableModularAccount.t.sol | 10 +++++----- test/utils/AccountTestBase.sol | 14 ++++++++++++-- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index 52fe9fef..b53cb33e 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"); @@ -93,7 +95,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { function test_basicUserOp_withInitCode() public { bytes memory callData = vm.envBool("SMA_TEST") - ? abi.encodeCall(UpgradeableModularAccount.updateFallbackSigner, (owner2)) + ? abi.encodeCall(SemiModularAccount(payable(account1)).updateFallbackSigner, (owner2)) : abi.encodeCall( UpgradeableModularAccount.execute, ( @@ -132,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, diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index 909cb09f..9aebbf65 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"; @@ -171,13 +172,22 @@ abstract contract AccountTestBase is OptimizedTest { vm.prank(owner1); if (vm.envBool("SMA_TEST")) { account1.executeWithAuthorization( - abi.encodeCall(account1.updateFallbackSigner, (address(this))), + 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, "") ); } From 540200e6ca8fa1d7ec38d5c4cf197a7bddc97238 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 7 Aug 2024 16:33:47 -0400 Subject: [PATCH 08/15] chore: organized overridable functions to the bottom of Modular Account --- src/account/SemiModularAccount.sol | 32 +++--- src/account/UpgradeableModularAccount.sol | 127 +++++++++++----------- 2 files changed, 79 insertions(+), 80 deletions(-) diff --git a/src/account/SemiModularAccount.sol b/src/account/SemiModularAccount.sol index 3a8635b3..fb3a1d46 100644 --- a/src/account/SemiModularAccount.sol +++ b/src/account/SemiModularAccount.sol @@ -68,22 +68,6 @@ contract SemiModularAccount is UpgradeableModularAccount { return super._exec1271Validation(sigValidation, hash, signature); } - 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 _execUserOpValidation( ModuleEntity userOpValidationFunction, PackedUserOperation memory userOp, @@ -105,6 +89,22 @@ contract SemiModularAccount is UpgradeableModularAccount { 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 _getFallbackSigner() internal view returns (address) { AccountStorage storage _storage = getAccountStorage(); diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index c565bf0a..1625d0b0 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -321,28 +321,6 @@ contract UpgradeableModularAccount is return _exec1271Validation(sigValidation, hash, signature[24:]); } - 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; - } - /// @notice Gets the entry point for this account /// @return entryPoint The entry point for this account function entryPoint() public view override returns (IEntryPoint) { @@ -354,7 +332,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) { @@ -452,16 +429,6 @@ contract UpgradeableModularAccount is return validationRes; } - 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 _doRuntimeValidation( ModuleEntity runtimeValidationFunction, bytes calldata callData, @@ -505,24 +472,6 @@ contract UpgradeableModularAccount is _execRuntimeValidation(runtimeValidationFunction, callData, authSegment.getBody()); } - 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 _doPreHooks(EnumerableSet.Bytes32Set storage executionHooks, bytes memory data) internal returns (PostExecToRun[] memory postHooksToRun) @@ -723,19 +672,6 @@ contract UpgradeableModularAccount is } } - 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 _checkIfValidationAppliesSelector(bytes4 selector, ModuleEntity validationFunction, bool isGlobal) internal view @@ -756,6 +692,69 @@ contract UpgradeableModularAccount is } } + 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 _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 _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(AccountStorage storage _storage, ModuleEntity validationFunction) internal view From a099bbd3bdf5454df23c0d0b1dc62c922c4f8e97 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Thu, 8 Aug 2024 14:42:07 -0400 Subject: [PATCH 09/15] refactor: extract SMA storage into SMA itself --- src/account/AccountStorage.sol | 5 ---- src/account/SemiModularAccount.sol | 30 +++++++++++++++++------ src/account/UpgradeableModularAccount.sol | 13 +++------- 3 files changed, 26 insertions(+), 22 deletions(-) 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 index fb3a1d46..89200431 100644 --- a/src/account/SemiModularAccount.sol +++ b/src/account/SemiModularAccount.sol @@ -10,8 +10,6 @@ import {ModuleEntityLib} from "../helpers/ModuleEntityLib.sol"; import {IModuleManager, ModuleEntity, ValidationConfig} from "../interfaces/IModuleManager.sol"; import {IValidation} from "../interfaces/IValidation.sol"; -import {AccountStorage, getAccountStorage} from "./AccountStorage.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"; @@ -20,6 +18,15 @@ contract SemiModularAccount is UpgradeableModularAccount { using MessageHashUtils for bytes32; using ModuleEntityLib for ModuleEntity; + struct SemiModularAccountStorage { + address fallbackSigner; + bool fallbackSignerDisabled; + } + + // keccak256("ERC6900.SemiModularAccount.Storage") + uint256 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); @@ -39,14 +46,14 @@ contract SemiModularAccount is UpgradeableModularAccount { } function updateFallbackSigner(address fallbackSigner) external wrapNativeFunction { - AccountStorage storage _storage = getAccountStorage(); + SemiModularAccountStorage storage _storage = _getSemiModularAccountStorage(); emit FallbackSignerSet(_storage.fallbackSigner, fallbackSigner); _storage.fallbackSigner = fallbackSigner; } function setFallbackSignerEnabled(bool enabled) external wrapNativeFunction { - AccountStorage storage _storage = getAccountStorage(); + SemiModularAccountStorage storage _storage = _getSemiModularAccountStorage(); _storage.fallbackSignerDisabled = !enabled; // TODO: event } @@ -106,7 +113,7 @@ contract SemiModularAccount is UpgradeableModularAccount { } function _getFallbackSigner() internal view returns (address) { - AccountStorage storage _storage = getAccountStorage(); + SemiModularAccountStorage storage _storage = _getSemiModularAccountStorage(); if (_storage.fallbackSignerDisabled) { revert FallbackSignerDisabled(); @@ -126,13 +133,22 @@ contract SemiModularAccount is UpgradeableModularAccount { return selector == this.updateFallbackSigner.selector || super._globalValidationAllowed(selector); } - function _isValidationGlobal(AccountStorage storage _storage, ModuleEntity validationFunction) + // todo: remove storage from input + function _isValidationGlobal(ModuleEntity validationFunction) internal view override returns (bool) { return - validationFunction.eq(_FALLBACK_VALIDATION) || super._isValidationGlobal(_storage, validationFunction); + validationFunction.eq(_FALLBACK_VALIDATION) || super._isValidationGlobal(validationFunction); + } + + 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 1625d0b0..ae709ebd 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -676,11 +676,9 @@ contract UpgradeableModularAccount is internal view { - AccountStorage storage _storage = getAccountStorage(); - // Check that the provided validation function is applicable to the selector if (isGlobal) { - if (_globalValidationAllowed(selector) && _isValidationGlobal(_storage, validationFunction)) { + if (_globalValidationAllowed(selector) && _isValidationGlobal(validationFunction)) { return; } revert ValidationFunctionMissing(selector); @@ -755,12 +753,7 @@ contract UpgradeableModularAccount is return getAccountStorage().executionData[selector].allowGlobalValidation; } - function _isValidationGlobal(AccountStorage storage _storage, ModuleEntity validationFunction) - internal - view - virtual - returns (bool) - { - return _storage.validationData[validationFunction].isGlobal; + function _isValidationGlobal(ModuleEntity validationFunction) internal view virtual returns (bool) { + return getAccountStorage().validationData[validationFunction].isGlobal; } } From 0cb353118acea857a04eec054123e1261e4452d2 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Thu, 8 Aug 2024 14:53:32 -0400 Subject: [PATCH 10/15] chore: remove unneeded todo --- src/account/SemiModularAccount.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/src/account/SemiModularAccount.sol b/src/account/SemiModularAccount.sol index 89200431..30ecc77e 100644 --- a/src/account/SemiModularAccount.sol +++ b/src/account/SemiModularAccount.sol @@ -133,7 +133,6 @@ contract SemiModularAccount is UpgradeableModularAccount { return selector == this.updateFallbackSigner.selector || super._globalValidationAllowed(selector); } - // todo: remove storage from input function _isValidationGlobal(ModuleEntity validationFunction) internal view From 29a8aa17020f84ead94ea5ffae35cf3372ec36cd Mon Sep 17 00:00:00 2001 From: zer0dot Date: Thu, 8 Aug 2024 15:22:53 -0400 Subject: [PATCH 11/15] feat: add missing getters and event to SMA --- src/account/SemiModularAccount.sol | 33 ++++++++++++++++-------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/src/account/SemiModularAccount.sol b/src/account/SemiModularAccount.sol index 30ecc77e..8b0bae08 100644 --- a/src/account/SemiModularAccount.sol +++ b/src/account/SemiModularAccount.sol @@ -30,6 +30,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); error FallbackSignerMismatch(); error FallbackSignerDisabled(); @@ -54,8 +55,16 @@ contract SemiModularAccount is UpgradeableModularAccount { function setFallbackSignerEnabled(bool enabled) external wrapNativeFunction { SemiModularAccountStorage storage _storage = _getSemiModularAccountStorage(); + emit FallbackSignerEnabledSet(!_storage.fallbackSignerDisabled, enabled); _storage.fallbackSignerDisabled = !enabled; - // TODO: event + } + + function isFallbackSignerEnabled() external view returns (bool) { + return !_getSemiModularAccountStorage().fallbackSignerDisabled; + } + + function getFallbackSigner() external view returns (address) { + return _getFallbackSigner(); } function _exec1271Validation(ModuleEntity sigValidation, bytes32 hash, bytes calldata signature) @@ -112,6 +121,14 @@ contract SemiModularAccount is UpgradeableModularAccount { super._execRuntimeValidation(runtimeValidationFunction, callData, authorization); } + 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(); @@ -129,20 +146,6 @@ contract SemiModularAccount is UpgradeableModularAccount { return address(uint160(bytes20(appendedData))); } - 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 _getSemiModularAccountStorage() internal pure returns (SemiModularAccountStorage storage) { SemiModularAccountStorage storage _storage; assembly ("memory-safe") { From 2c41a3cb974609563a006756d761f877f26ad6ef Mon Sep 17 00:00:00 2001 From: zer0dot Date: Fri, 9 Aug 2024 16:56:02 -0400 Subject: [PATCH 12/15] test: fix optimized test deploy failing for SMA due to missing constructor arg --- test/utils/OptimizedTest.sol | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/utils/OptimizedTest.sol b/test/utils/OptimizedTest.sol index cf1a2b9d..69a45996 100644 --- a/test/utils/OptimizedTest.sol +++ b/test/utils/OptimizedTest.sol @@ -49,7 +49,9 @@ abstract contract OptimizedTest is Test { function _deploySemiModularAccount(IEntryPoint entryPoint) internal returns (UpgradeableModularAccount) { return _isOptimizedTest() ? UpgradeableModularAccount( - payable(deployCode("out-optimized/SemiModularAccount.sol/SemiModularAccount.json")) + payable( + deployCode("out-optimized/SemiModularAccount.sol/SemiModularAccount.json", abi.encode(entryPoint)) + ) ) : UpgradeableModularAccount(new SemiModularAccount(entryPoint)); } From d09f41512ec9a9186f1ace9398f2f36db575e6ce Mon Sep 17 00:00:00 2001 From: zer0dot Date: Fri, 9 Aug 2024 18:27:10 -0400 Subject: [PATCH 13/15] chore: linting and cleanup --- src/account/AccountFactory.sol | 4 +- src/account/SemiModularAccount.sol | 48 ++++--- src/account/UpgradeableModularAccount.sol | 134 +++++++++--------- .../validation/SingleSignerValidation.sol | 1 - test/account/AccountReturnData.t.sol | 2 - test/account/ImmutableAppend.t.sol | 11 -- test/account/MultiValidation.t.sol | 9 -- test/account/ReplaceModule.t.sol | 1 - test/mocks/SingleSignerFactoryFixture.sol | 1 - 9 files changed, 96 insertions(+), 115 deletions(-) diff --git a/src/account/AccountFactory.sol b/src/account/AccountFactory.sol index 14d0b674..ae89756d 100644 --- a/src/account/AccountFactory.sol +++ b/src/account/AccountFactory.sol @@ -25,7 +25,7 @@ contract AccountFactory is Ownable { address owner ) Ownable(owner) { ENTRY_POINT = _entryPoint; - _PROXY_BYTECODE_HASH = + _PROXY_BYTECODE_HASH = keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(address(_accountImpl), ""))); ACCOUNT_IMPL = _accountImpl; SINGLE_SIGNER_VALIDATION = _singleSignerValidation; @@ -111,7 +111,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/SemiModularAccount.sol b/src/account/SemiModularAccount.sol index 8b0bae08..2c4e0b6d 100644 --- a/src/account/SemiModularAccount.sol +++ b/src/account/SemiModularAccount.sol @@ -7,8 +7,7 @@ import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interface import {ModuleEntityLib} from "../helpers/ModuleEntityLib.sol"; -import {IModuleManager, ModuleEntity, ValidationConfig} from "../interfaces/IModuleManager.sol"; -import {IValidation} from "../interfaces/IValidation.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"; @@ -24,7 +23,7 @@ contract SemiModularAccount is UpgradeableModularAccount { } // keccak256("ERC6900.SemiModularAccount.Storage") - uint256 constant _SEMI_MODULAR_ACCOUNT_STORAGE_SLOT = + uint256 internal constant _SEMI_MODULAR_ACCOUNT_STORAGE_SLOT = 0x5b9dc9aa943f8fa2653ceceda5e3798f0686455280432166ba472eca0bc17a32; ModuleEntity internal constant _FALLBACK_VALIDATION = ModuleEntity.wrap(bytes24(type(uint192).max)); @@ -38,6 +37,7 @@ contract SemiModularAccount is UpgradeableModularAccount { constructor(IEntryPoint anEntryPoint) UpgradeableModularAccount(anEntryPoint) {} + /// Override reverts on initialization, effectively disabling the initializer. function initializeWithValidation(ValidationConfig, bytes4[] calldata, bytes calldata, bytes[] calldata) external override @@ -46,16 +46,22 @@ contract SemiModularAccount is UpgradeableModularAccount { 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; } @@ -67,23 +73,6 @@ contract SemiModularAccount is UpgradeableModularAccount { return _getFallbackSigner(); } - 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 _execUserOpValidation( ModuleEntity userOpValidationFunction, PackedUserOperation memory userOp, @@ -121,6 +110,23 @@ contract SemiModularAccount is UpgradeableModularAccount { 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); } diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index ae709ebd..5597fa6b 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -619,6 +619,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, @@ -689,71 +756,4 @@ contract UpgradeableModularAccount is } } } - - 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 _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 _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; - } } diff --git a/src/modules/validation/SingleSignerValidation.sol b/src/modules/validation/SingleSignerValidation.sol index e80cac1c..67b8e77b 100644 --- a/src/modules/validation/SingleSignerValidation.sol +++ b/src/modules/validation/SingleSignerValidation.sol @@ -6,7 +6,6 @@ import {IValidation} from "../../interfaces/IValidation.sol"; import {BaseModule} from "../BaseModule.sol"; import {ISingleSignerValidation} from "./ISingleSignerValidation.sol"; import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.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"; 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/ImmutableAppend.t.sol b/test/account/ImmutableAppend.t.sol index dfb2a39c..dd75f4a5 100644 --- a/test/account/ImmutableAppend.t.sol +++ b/test/account/ImmutableAppend.t.sol @@ -1,21 +1,10 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.19; -import {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 {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.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 */ /* -------------------------------------------------------------------------- */ diff --git a/test/account/MultiValidation.t.sol b/test/account/MultiValidation.t.sol index e1f9e340..9d755a42 100644 --- a/test/account/MultiValidation.t.sol +++ b/test/account/MultiValidation.t.sol @@ -17,7 +17,6 @@ import {SingleSignerValidation} from "../../src/modules/validation/SingleSignerV import {AccountTestBase} from "../utils/AccountTestBase.sol"; import {TEST_DEFAULT_VALIDATION_ENTITY_ID} from "../utils/TestConstants.sol"; -import "forge-std/console.sol"; contract MultiValidationTest is AccountTestBase { using ECDSA for bytes32; @@ -69,14 +68,6 @@ contract MultiValidationTest is AccountTestBase { abi.encodeWithSignature("NotAuthorized()") ) ); - console.logBytes( - abi.encodeWithSelector( - UpgradeableModularAccount.RuntimeValidationFunctionReverted.selector, - address(validator2), - TEST_DEFAULT_VALIDATION_ENTITY_ID, - abi.encodeWithSignature("NotAuthorized()") - ) - ); account1.executeWithAuthorization( abi.encodeCall(IStandardExecutor.execute, (address(0), 0, "")), _encodeSignature( diff --git a/test/account/ReplaceModule.t.sol b/test/account/ReplaceModule.t.sol index 380d5a4a..79a30b01 100644 --- a/test/account/ReplaceModule.t.sol +++ b/test/account/ReplaceModule.t.sol @@ -21,7 +21,6 @@ import {IValidationHook} from "../../src/interfaces/IValidationHook.sol"; import {SingleSignerValidation} from "../../src/modules/validation/SingleSignerValidation.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/mocks/SingleSignerFactoryFixture.sol b/test/mocks/SingleSignerFactoryFixture.sol index 86a0506b..64f223c1 100644 --- a/test/mocks/SingleSignerFactoryFixture.sol +++ b/test/mocks/SingleSignerFactoryFixture.sol @@ -5,7 +5,6 @@ import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntry import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; -import {SemiModularAccount} from "../../src/account/SemiModularAccount.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol"; import {SingleSignerValidation} from "../../src/modules/validation/SingleSignerValidation.sol"; From e3eed1d4335e0f49dd65dd7d0df3b4857a817488 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Fri, 9 Aug 2024 18:35:15 -0400 Subject: [PATCH 14/15] chore: formatting --- 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 ae89756d..9fe12dac 100644 --- a/src/account/AccountFactory.sol +++ b/src/account/AccountFactory.sol @@ -25,7 +25,7 @@ contract AccountFactory is Ownable { address owner ) Ownable(owner) { ENTRY_POINT = _entryPoint; - _PROXY_BYTECODE_HASH = + _PROXY_BYTECODE_HASH = keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(address(_accountImpl), ""))); ACCOUNT_IMPL = _accountImpl; SINGLE_SIGNER_VALIDATION = _singleSignerValidation; From 25528091002cd77478025e15b93fb50e6630cb2e Mon Sep 17 00:00:00 2001 From: zer0dot Date: Mon, 12 Aug 2024 11:58:37 -0400 Subject: [PATCH 15/15] chore: update foundry and re-run formatting --- src/account/UpgradeableModularAccount.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 5597fa6b..ddcd2dbe 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -556,7 +556,7 @@ contract UpgradeableModularAccount is ) // forgefmt: disable-start // solhint-disable-next-line no-empty-blocks - {} catch (bytes memory revertReason) { + {} catch (bytes memory revertReason){ // forgefmt: disable-end revert PreRuntimeValidationHookFailed(hookModule, hookEntityId, revertReason); } @@ -641,7 +641,7 @@ contract UpgradeableModularAccount is ) // forgefmt: disable-start // solhint-disable-next-line no-empty-blocks - {} catch (bytes memory revertReason) { + {} catch (bytes memory revertReason){ // forgefmt: disable-end revert RuntimeValidationFunctionReverted(module, entityId, revertReason); }