From d22bec997a9f0b5c228016c5cdc156e53da8421d Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Fri, 12 Jul 2024 09:17:43 -0700 Subject: [PATCH 1/7] add ecdsa validation --- src/plugins/validation/EcdsaValidation.sol | 110 ++++++++++++++++++++ src/plugins/validation/IEcdsaValidation.sol | 28 +++++ test/mocks/EcdsaFactoryFixture.sol | 80 ++++++++++++++ test/utils/OptimizedTest.sol | 7 ++ test/utils/TestConstants.sol | 1 + test/validation/EcdsaValidation.t.sol | 78 ++++++++++++++ 6 files changed, 304 insertions(+) create mode 100644 src/plugins/validation/EcdsaValidation.sol create mode 100644 src/plugins/validation/IEcdsaValidation.sol create mode 100644 test/mocks/EcdsaFactoryFixture.sol create mode 100644 test/validation/EcdsaValidation.t.sol diff --git a/src/plugins/validation/EcdsaValidation.sol b/src/plugins/validation/EcdsaValidation.sol new file mode 100644 index 00000000..24bd4e71 --- /dev/null +++ b/src/plugins/validation/EcdsaValidation.sol @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.25; + +import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; +import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; +import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; + +import {IPlugin, PluginManifest, PluginMetadata} from "../../interfaces/IPlugin.sol"; +import {IValidation} from "../../interfaces/IValidation.sol"; +import {BasePlugin} from "../BasePlugin.sol"; +import {IEcdsaValidation} from "./IEcdsaValidation.sol"; + +contract EcdsaValidation is IEcdsaValidation, BasePlugin { + using ECDSA for bytes32; + using MessageHashUtils for bytes32; + + string public constant NAME = "Ecdsa Validation Plugin"; + string public constant VERSION = "1.0.0"; + string public constant AUTHOR = "ERC-6900 Authors"; + + uint256 internal constant _SIG_VALIDATION_PASSED = 0; + uint256 internal constant _SIG_VALIDATION_FAILED = 1; + + mapping(uint32 validationId => mapping(address account => address)) public signer; + + /// @inheritdoc IEcdsaValidation + function signerOf(uint32 validationId, address account) external view returns (address) { + return signer[validationId][account]; + } + + /// @inheritdoc IEcdsaValidation + function transferSigner(uint32 validationId, address newSigner) external { + _transferSigner(validationId, newSigner); + } + + // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ + // ┃ Plugin interface functions ┃ + // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + + /// @inheritdoc IPlugin + function pluginManifest() external pure override returns (PluginManifest memory) { + PluginManifest memory manifest; + return manifest; + } + + /// @inheritdoc IPlugin + function pluginMetadata() external pure virtual override returns (PluginMetadata memory) { + PluginMetadata memory metadata; + metadata.name = NAME; + metadata.version = VERSION; + metadata.author = AUTHOR; + return metadata; + } + + /// @inheritdoc IPlugin + function onInstall(bytes calldata data) external override { + (uint32 validationId, address newSigner) = abi.decode(data, (uint32, address)); + _transferSigner(validationId, newSigner); + } + + /// @inheritdoc IPlugin + function onUninstall(bytes calldata data) external override { + // ToDo: what does it mean in the world of composable validation world to uninstall one type of validation + // We can either get rid of all Ecdsa signers. What about the nested ones? + _transferSigner(abi.decode(data, (uint32)), address(0)); + } + + /// @inheritdoc IValidation + function validateUserOp(uint32 validationId, PackedUserOperation calldata userOp, bytes32 userOpHash) + external + view + override + returns (uint256) + { + // Validate the user op signature against the owner. + (address sigSigner,,) = (userOpHash.toEthSignedMessageHash()).tryRecover(userOp.signature); + if (sigSigner == address(0) || sigSigner != signer[validationId][msg.sender]) { + return _SIG_VALIDATION_FAILED; + } + return _SIG_VALIDATION_PASSED; + } + + /// @inheritdoc IValidation + function validateRuntime(uint32 validationId, address sender, uint256, bytes calldata, bytes calldata) + external + view + override + { + // Validate that the sender is the owner of the account or self. + if (sender != signer[validationId][msg.sender] && sender != msg.sender) { + revert NotAuthorized(); + } + return; + } + + /// @inheritdoc IValidation + function validateSignature(uint32, address, bytes32, bytes calldata) external pure override returns (bytes4) { + revert NotImplemented(); + } + + // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ + // ┃ Internal / Private functions ┃ + // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + + function _transferSigner(uint32 validationId, address newSigner) internal { + address previousSigner = signer[validationId][msg.sender]; + signer[validationId][msg.sender] = newSigner; + emit SignerTransferred(msg.sender, validationId, previousSigner, newSigner); + } +} diff --git a/src/plugins/validation/IEcdsaValidation.sol b/src/plugins/validation/IEcdsaValidation.sol new file mode 100644 index 00000000..61723a6b --- /dev/null +++ b/src/plugins/validation/IEcdsaValidation.sol @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.25; + +import {IValidation} from "../../interfaces/IValidation.sol"; + +interface IEcdsaValidation is IValidation { + /// @notice This event is emitted when Signer of the account's validation changes. + /// @param account The account whose validation Signer changed. + /// @param validationId The validationId for the account and the signer. + /// @param previousSigner The address of the previous signer. + /// @param newSigner The address of the new signer. + event SignerTransferred( + address indexed account, uint32 indexed validationId, address previousSigner, address newSigner + ); + + error NotAuthorized(); + + /// @notice Transfer Signer of the account's validation to `newSigner`. + /// @param validationId The validationId for the account and the signer. + /// @param newSigner The address of the new signer. + function transferSigner(uint32 validationId, address newSigner) external; + + /// @notice Get the signer of the `account`'s validation. + /// @param validationId The validationId for the account and the signer. + /// @param account The account to get the signer of. + /// @return The address of the signer. + function signerOf(uint32 validationId, address account) external view returns (address); +} diff --git a/test/mocks/EcdsaFactoryFixture.sol b/test/mocks/EcdsaFactoryFixture.sol new file mode 100644 index 00000000..a4fc6ce4 --- /dev/null +++ b/test/mocks/EcdsaFactoryFixture.sol @@ -0,0 +1,80 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.19; + +import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; +import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; + +import {ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol"; +import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; +import {PackedPluginEntityLib} from "../../src/helpers/PackedPluginEntityLib.sol"; +import {EcdsaValidation} from "../../src/plugins/validation/EcdsaValidation.sol"; + +import {OptimizedTest} from "../utils/OptimizedTest.sol"; +import {TEST_DEFAULT_VALIDATION_ID} from "../utils/TestConstants.sol"; + +contract EcdsaFactoryFixture is OptimizedTest { + UpgradeableModularAccount public accountImplementation; + EcdsaValidation public ecdsaValidation; + bytes32 private immutable _PROXY_BYTECODE_HASH; + + uint32 public constant UNSTAKE_DELAY = 1 weeks; + + IEntryPoint public entryPoint; + + address public self; + + constructor(IEntryPoint _entryPoint, EcdsaValidation _ecdsaValidation) { + entryPoint = _entryPoint; + accountImplementation = _deployUpgradeableModularAccount(_entryPoint); + _PROXY_BYTECODE_HASH = keccak256( + abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(address(accountImplementation), "")) + ); + ecdsaValidation = _ecdsaValidation; + self = address(this); + } + + /** + * create an account, and return its address. + * returns the address even if the account is already deployed. + * Note that during user operation execution, this method is called only if the account is not deployed. + * This method returns an existing account address so that entryPoint.getSenderAddress() would work even after + * account creation + */ + function createAccount(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) { + bytes memory pluginInstallData = abi.encode(TEST_DEFAULT_VALIDATION_ID, owner); + // not necessary to check return addr since next call will fail if so + new ERC1967Proxy{salt: getSalt(owner, salt)}(address(accountImplementation), ""); + + // point proxy to actual implementation and init plugins + UpgradeableModularAccount(payable(addr)).initializeWithValidation( + ValidationConfigLib.pack(address(ecdsaValidation), TEST_DEFAULT_VALIDATION_ID, true, true), + new bytes4[](0), + pluginInstallData, + "", + "" + ); + } + + return UpgradeableModularAccount(payable(addr)); + } + + /** + * calculate the counterfactual address of this account as it would be returned by createAccount() + */ + function getAddress(address owner, uint256 salt) public view returns (address) { + return Create2.computeAddress(getSalt(owner, salt), _PROXY_BYTECODE_HASH); + } + + function addStake() external payable { + entryPoint.addStake{value: msg.value}(UNSTAKE_DELAY); + } + + function getSalt(address owner, uint256 salt) public pure returns (bytes32) { + return keccak256(abi.encodePacked(owner, salt)); + } +} diff --git a/test/utils/OptimizedTest.sol b/test/utils/OptimizedTest.sol index f9431acc..c2e5f9d6 100644 --- a/test/utils/OptimizedTest.sol +++ b/test/utils/OptimizedTest.sol @@ -7,6 +7,7 @@ import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntry import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; +import {EcdsaValidation} from "../../src/plugins/validation/EcdsaValidation.sol"; import {TokenReceiverPlugin} from "../../src/plugins/TokenReceiverPlugin.sol"; /// @dev This contract provides functions to deploy optimized (via IR) precompiled contracts. By compiling just @@ -55,4 +56,10 @@ abstract contract OptimizedTest is Test { ? TokenReceiverPlugin(deployCode("out-optimized/TokenReceiverPlugin.sol/TokenReceiverPlugin.json")) : new TokenReceiverPlugin(); } + + function _deployEcdsaValidation() internal returns (EcdsaValidation) { + return _isOptimizedTest() + ? EcdsaValidation(deployCode("out-optimized/EcdsaValidation.sol/EcdsaValidation.json")) + : new EcdsaValidation(); + } } diff --git a/test/utils/TestConstants.sol b/test/utils/TestConstants.sol index f9cc8c90..c505a1a0 100644 --- a/test/utils/TestConstants.sol +++ b/test/utils/TestConstants.sol @@ -2,3 +2,4 @@ pragma solidity ^0.8.25; uint32 constant TEST_DEFAULT_OWNER_FUNCTION_ID = 0; +uint32 constant TEST_DEFAULT_VALIDATION_ID = 0; diff --git a/test/validation/EcdsaValidation.t.sol b/test/validation/EcdsaValidation.t.sol new file mode 100644 index 00000000..b983f142 --- /dev/null +++ b/test/validation/EcdsaValidation.t.sol @@ -0,0 +1,78 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.19; + +import {console} from "forge-std/Test.sol"; +import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; +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 {EcdsaValidation} from "../../src/plugins/validation/EcdsaValidation.sol"; + +import {OptimizedTest} from "../utils/OptimizedTest.sol"; +import {EcdsaFactoryFixture} from "../mocks/EcdsaFactoryFixture.sol"; +import {TEST_DEFAULT_VALIDATION_ID} from "../utils/TestConstants.sol"; + +contract EcdsaValidationTest is OptimizedTest { + using MessageHashUtils for bytes32; + + EntryPoint public entryPoint; + EcdsaValidation public ecdsaValidation; + address payable public beneficiary; + address public ethRecipient; + + address public owner1; + uint256 public owner1Key; + EcdsaFactoryFixture public factory; + UpgradeableModularAccount public account1; + bytes32 public validationId1; + + uint256 public constant CALL_GAS_LIMIT = 50000; + uint256 public constant VERIFICATION_GAS_LIMIT = 1200000; + uint8 public constant SELECTOR_ASSOCIATED_VALIDATION = 0; + uint8 public constant DEFAULT_VALIDATION = 1; + + function setUp() public { + entryPoint = new EntryPoint(); + (owner1, owner1Key) = makeAddrAndKey("owner1"); + beneficiary = payable(makeAddr("beneficiary")); + ethRecipient = makeAddr("ethRecipient"); + + ecdsaValidation = _deployEcdsaValidation(); + factory = new EcdsaFactoryFixture(entryPoint, ecdsaValidation); + + account1 = factory.createAccount(owner1, 0); + vm.deal(address(account1), 100 ether); + } + + function test_userOpValidation() public { + PackedUserOperation memory userOp = PackedUserOperation({ + sender: address(account1), + nonce: 0, + initCode: "", + 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(owner1Key, userOpHash.toEthSignedMessageHash()); + userOp.signature = abi.encodePacked(TEST_DEFAULT_VALIDATION_ID, DEFAULT_VALIDATION, r, s, v); + + PackedUserOperation[] memory userOps = new PackedUserOperation[](1); + userOps[0] = userOp; + + entryPoint.handleOps(userOps, beneficiary); + + assertEq(ethRecipient.balance, 1 wei); + } + + // helper function to compress 2 gas values into a single bytes32 + function _encodeGas(uint256 g1, uint256 g2) internal pure returns (bytes32) { + return bytes32(uint256((g1 << 128) + uint128(g2))); + } +} From 03f9cf385e5cadfc8ed49d5a24aaa5d5df86380a Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Fri, 12 Jul 2024 12:13:12 -0700 Subject: [PATCH 2/7] update ecdsa validation and add more test --- src/plugins/validation/EcdsaValidation.sol | 13 +++- test/mocks/EcdsaFactoryFixture.sol | 3 +- test/utils/AccountTestBase.sol | 7 ++ test/utils/TestConstants.sol | 2 +- test/validation/EcdsaValidation.t.sol | 80 +++++++++++++--------- 5 files changed, 65 insertions(+), 40 deletions(-) diff --git a/src/plugins/validation/EcdsaValidation.sol b/src/plugins/validation/EcdsaValidation.sol index 24bd4e71..6d50f698 100644 --- a/src/plugins/validation/EcdsaValidation.sol +++ b/src/plugins/validation/EcdsaValidation.sol @@ -10,11 +10,18 @@ import {IValidation} from "../../interfaces/IValidation.sol"; import {BasePlugin} from "../BasePlugin.sol"; import {IEcdsaValidation} from "./IEcdsaValidation.sol"; +/// @title ECSDA Validation +/// @author ERC-6900 Authors +/// @notice This validation enables any ECDSA (secp256k1 curve) signature validation. It handles installation by +/// each entity (validationId). Uninstallation will NOT disable all installed validation entities. None of the +/// functions are installed on the account. Account states are to be retrieved from this global singleton directly. +/// Note: This validation also support composition that other validation can relay on entities in this validation +/// to validate partially or fully. contract EcdsaValidation is IEcdsaValidation, BasePlugin { using ECDSA for bytes32; using MessageHashUtils for bytes32; - string public constant NAME = "Ecdsa Validation Plugin"; + string public constant NAME = "Ecdsa Validation"; string public constant VERSION = "1.0.0"; string public constant AUTHOR = "ERC-6900 Authors"; @@ -74,7 +81,7 @@ contract EcdsaValidation is IEcdsaValidation, BasePlugin { { // Validate the user op signature against the owner. (address sigSigner,,) = (userOpHash.toEthSignedMessageHash()).tryRecover(userOp.signature); - if (sigSigner == address(0) || sigSigner != signer[validationId][msg.sender]) { + if (sigSigner == address(0) || sigSigner != signer[validationId][userOp.sender]) { return _SIG_VALIDATION_FAILED; } return _SIG_VALIDATION_PASSED; @@ -87,7 +94,7 @@ contract EcdsaValidation is IEcdsaValidation, BasePlugin { override { // Validate that the sender is the owner of the account or self. - if (sender != signer[validationId][msg.sender] && sender != msg.sender) { + if (sender != signer[validationId][msg.sender]) { revert NotAuthorized(); } return; diff --git a/test/mocks/EcdsaFactoryFixture.sol b/test/mocks/EcdsaFactoryFixture.sol index a4fc6ce4..77073610 100644 --- a/test/mocks/EcdsaFactoryFixture.sol +++ b/test/mocks/EcdsaFactoryFixture.sol @@ -7,7 +7,6 @@ import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntry import {ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; -import {PackedPluginEntityLib} from "../../src/helpers/PackedPluginEntityLib.sol"; import {EcdsaValidation} from "../../src/plugins/validation/EcdsaValidation.sol"; import {OptimizedTest} from "../utils/OptimizedTest.sol"; @@ -52,7 +51,7 @@ contract EcdsaFactoryFixture is OptimizedTest { // point proxy to actual implementation and init plugins UpgradeableModularAccount(payable(addr)).initializeWithValidation( - ValidationConfigLib.pack(address(ecdsaValidation), TEST_DEFAULT_VALIDATION_ID, true, true), + ValidationConfigLib.pack(address(ecdsaValidation), TEST_DEFAULT_VALIDATION_ID, true, false), new bytes4[](0), pluginInstallData, "", diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index a8f84a21..42d020d0 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 {EcdsaValidation} from "../../src/plugins/validation/EcdsaValidation.sol"; import {PluginEntity, PluginEntityLib} from "../../src/helpers/PluginEntityLib.sol"; import {IStandardExecutor, Call} from "../../src/interfaces/IStandardExecutor.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; @@ -13,6 +14,7 @@ import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {OptimizedTest} from "./OptimizedTest.sol"; import {TEST_DEFAULT_OWNER_FUNCTION_ID as EXT_CONST_TEST_DEFAULT_OWNER_FUNCTION_ID} from "./TestConstants.sol"; +import {EcdsaFactoryFixture} from "../mocks/EcdsaFactoryFixture.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; /// @dev This contract handles common boilerplate setup for tests using UpgradeableModularAccount with @@ -26,6 +28,9 @@ abstract contract AccountTestBase is OptimizedTest { SingleOwnerPlugin public singleOwnerPlugin; MSCAFactoryFixture public factory; + EcdsaValidation public ecdsaValidation; + EcdsaFactoryFixture public ecdsaFactory; + address public owner1; uint256 public owner1Key; UpgradeableModularAccount public account1; @@ -53,6 +58,8 @@ abstract contract AccountTestBase is OptimizedTest { singleOwnerPlugin = _deploySingleOwnerPlugin(); factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); + ecdsaValidation = _deployEcdsaValidation(); + ecdsaFactory = new EcdsaFactoryFixture(entryPoint, ecdsaValidation); account1 = factory.createAccount(owner1, 0); vm.deal(address(account1), 100 ether); diff --git a/test/utils/TestConstants.sol b/test/utils/TestConstants.sol index c505a1a0..1bd467ad 100644 --- a/test/utils/TestConstants.sol +++ b/test/utils/TestConstants.sol @@ -2,4 +2,4 @@ pragma solidity ^0.8.25; uint32 constant TEST_DEFAULT_OWNER_FUNCTION_ID = 0; -uint32 constant TEST_DEFAULT_VALIDATION_ID = 0; +uint32 constant TEST_DEFAULT_VALIDATION_ID = 1; diff --git a/test/validation/EcdsaValidation.t.sol b/test/validation/EcdsaValidation.t.sol index b983f142..d3750b03 100644 --- a/test/validation/EcdsaValidation.t.sol +++ b/test/validation/EcdsaValidation.t.sol @@ -1,53 +1,34 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; -import {console} from "forge-std/Test.sol"; -import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; 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 {EcdsaValidation} from "../../src/plugins/validation/EcdsaValidation.sol"; +import {PluginEntityLib} from "../../src/helpers/PluginEntityLib.sol"; +import {ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol"; -import {OptimizedTest} from "../utils/OptimizedTest.sol"; -import {EcdsaFactoryFixture} from "../mocks/EcdsaFactoryFixture.sol"; +import {AccountTestBase} from "../utils/AccountTestBase.sol"; import {TEST_DEFAULT_VALIDATION_ID} from "../utils/TestConstants.sol"; -contract EcdsaValidationTest is OptimizedTest { +contract EcdsaValidationTest is AccountTestBase { using MessageHashUtils for bytes32; - EntryPoint public entryPoint; - EcdsaValidation public ecdsaValidation; - address payable public beneficiary; address public ethRecipient; - - address public owner1; - uint256 public owner1Key; - EcdsaFactoryFixture public factory; - UpgradeableModularAccount public account1; - bytes32 public validationId1; - - uint256 public constant CALL_GAS_LIMIT = 50000; - uint256 public constant VERIFICATION_GAS_LIMIT = 1200000; - uint8 public constant SELECTOR_ASSOCIATED_VALIDATION = 0; - uint8 public constant DEFAULT_VALIDATION = 1; + address public owner2; + uint256 public owner2Key; + UpgradeableModularAccount public account; function setUp() public { - entryPoint = new EntryPoint(); - (owner1, owner1Key) = makeAddrAndKey("owner1"); - beneficiary = payable(makeAddr("beneficiary")); ethRecipient = makeAddr("ethRecipient"); - - ecdsaValidation = _deployEcdsaValidation(); - factory = new EcdsaFactoryFixture(entryPoint, ecdsaValidation); - - account1 = factory.createAccount(owner1, 0); - vm.deal(address(account1), 100 ether); + (owner2, owner2Key) = makeAddrAndKey("owner2"); + account = ecdsaFactory.createAccount(owner1, 0); + vm.deal(address(account), 100 ether); } function test_userOpValidation() public { PackedUserOperation memory userOp = PackedUserOperation({ - sender: address(account1), + sender: address(account), nonce: 0, initCode: "", callData: abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, "")), @@ -61,7 +42,11 @@ contract EcdsaValidationTest is OptimizedTest { // Generate signature bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = abi.encodePacked(TEST_DEFAULT_VALIDATION_ID, DEFAULT_VALIDATION, r, s, v); + userOp.signature = _encodeSignature( + PluginEntityLib.pack(address(ecdsaValidation), TEST_DEFAULT_VALIDATION_ID), + GLOBAL_VALIDATION, + abi.encodePacked(r, s, v) + ); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); userOps[0] = userOp; @@ -71,8 +56,35 @@ contract EcdsaValidationTest is OptimizedTest { assertEq(ethRecipient.balance, 1 wei); } - // helper function to compress 2 gas values into a single bytes32 - function _encodeGas(uint256 g1, uint256 g2) internal pure returns (bytes32) { - return bytes32(uint256((g1 << 128) + uint128(g2))); + function test_runtime() public { + vm.prank(owner1); + account.executeWithAuthorization( + abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, "")), + _encodeSignature( + PluginEntityLib.pack(address(ecdsaValidation), TEST_DEFAULT_VALIDATION_ID), GLOBAL_VALIDATION, "" + ) + ); + assertEq(ethRecipient.balance, 1 wei); + } + + function test_runtime_with2ndValidation() public { + uint32 newValidationId = TEST_DEFAULT_OWNER_FUNCTION_ID + 1; + vm.prank(address(entryPoint)); + account.installValidation( + ValidationConfigLib.pack(address(ecdsaValidation), newValidationId, true, false), + new bytes4[](0), + abi.encode(newValidationId, owner2), + "", + "" + ); + + vm.prank(owner2); + account.executeWithAuthorization( + abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, "")), + _encodeSignature( + PluginEntityLib.pack(address(ecdsaValidation), newValidationId), GLOBAL_VALIDATION, "" + ) + ); + assertEq(ethRecipient.balance, 1 wei); } } From 7488ffd292af76d1575160797c1086c052caf284 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Mon, 15 Jul 2024 16:09:49 -0700 Subject: [PATCH 3/7] add account as passed in param in validate runtime --- src/account/UpgradeableModularAccount.sol | 4 +++- src/interfaces/IValidation.sol | 2 ++ src/plugins/owner/SingleOwnerPlugin.sol | 15 +++++++++------ src/plugins/validation/EcdsaValidation.sol | 15 +++++++++------ test/mocks/plugins/ComprehensivePlugin.sol | 2 +- test/mocks/plugins/ReturnDataPluginMocks.sol | 5 ++++- test/mocks/plugins/ValidationPluginMocks.sol | 6 +++++- test/plugin/SingleOwnerPlugin.t.sol | 4 ++-- 8 files changed, 35 insertions(+), 18 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index ec075e8b..7fe5b1f1 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -517,7 +517,9 @@ contract UpgradeableModularAccount is (address plugin, uint32 entityId) = runtimeValidationFunction.unpack(); - try IValidation(plugin).validateRuntime(entityId, msg.sender, msg.value, callData, authSegment.getBody()) + try IValidation(plugin).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) { diff --git a/src/interfaces/IValidation.sol b/src/interfaces/IValidation.sol index 03aed016..2a992849 100644 --- a/src/interfaces/IValidation.sol +++ b/src/interfaces/IValidation.sol @@ -18,6 +18,7 @@ interface IValidation is IPlugin { /// @notice Run the runtime validationFunction specified by the `entityId`. /// @dev To indicate the entire call should revert, the function MUST revert. + /// @param account the account to validate for. /// @param entityId An identifier that routes the call to different internal implementations, should there /// be more than one. /// @param sender The caller address. @@ -25,6 +26,7 @@ interface IValidation is IPlugin { /// @param data The calldata sent. /// @param authorization Additional data for the validation function to use. function validateRuntime( + address account, uint32 entityId, address sender, uint256 value, diff --git a/src/plugins/owner/SingleOwnerPlugin.sol b/src/plugins/owner/SingleOwnerPlugin.sol index d82a1824..1ef677d7 100644 --- a/src/plugins/owner/SingleOwnerPlugin.sol +++ b/src/plugins/owner/SingleOwnerPlugin.sol @@ -82,13 +82,16 @@ contract SingleOwnerPlugin is IValidation, BasePlugin { } /// @inheritdoc IValidation - function validateRuntime(uint32 entityId, address sender, uint256, bytes calldata, bytes calldata) - external - view - override - { + function validateRuntime( + address account, + uint32 entityId, + address sender, + uint256, + bytes calldata, + bytes calldata + ) external view override { // Validate that the sender is the owner of the account or self. - if (sender != owners[entityId][msg.sender]) { + if (sender != owners[entityId][account]) { revert NotAuthorized(); } return; diff --git a/src/plugins/validation/EcdsaValidation.sol b/src/plugins/validation/EcdsaValidation.sol index 6d50f698..a2fcde05 100644 --- a/src/plugins/validation/EcdsaValidation.sol +++ b/src/plugins/validation/EcdsaValidation.sol @@ -88,13 +88,16 @@ contract EcdsaValidation is IEcdsaValidation, BasePlugin { } /// @inheritdoc IValidation - function validateRuntime(uint32 validationId, address sender, uint256, bytes calldata, bytes calldata) - external - view - override - { + function validateRuntime( + address account, + uint32 validationId, + address sender, + uint256, + bytes calldata, + bytes calldata + ) external view override { // Validate that the sender is the owner of the account or self. - if (sender != signer[validationId][msg.sender]) { + if (sender != signer[validationId][account]) { revert NotAuthorized(); } return; diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index 9bff9237..87fb27f1 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -85,7 +85,7 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba revert NotImplemented(); } - function validateRuntime(uint32 entityId, address, uint256, bytes calldata, bytes calldata) + function validateRuntime(address, uint32 entityId, address, uint256, bytes calldata, bytes calldata) external pure override diff --git a/test/mocks/plugins/ReturnDataPluginMocks.sol b/test/mocks/plugins/ReturnDataPluginMocks.sol index 6bab7b0b..fad0485f 100644 --- a/test/mocks/plugins/ReturnDataPluginMocks.sol +++ b/test/mocks/plugins/ReturnDataPluginMocks.sol @@ -76,7 +76,10 @@ contract ResultConsumerPlugin is BasePlugin, IValidation { revert NotImplemented(); } - function validateRuntime(uint32, address sender, uint256, bytes calldata, bytes calldata) external view { + function validateRuntime(address, uint32, address sender, uint256, bytes calldata, bytes calldata) + external + view + { if (sender != address(this)) { revert NotAuthorized(); } diff --git a/test/mocks/plugins/ValidationPluginMocks.sol b/test/mocks/plugins/ValidationPluginMocks.sol index af57eaaa..5aa3719d 100644 --- a/test/mocks/plugins/ValidationPluginMocks.sol +++ b/test/mocks/plugins/ValidationPluginMocks.sol @@ -73,7 +73,11 @@ abstract contract MockBaseUserOpValidationPlugin is IValidation, IValidationHook revert NotImplemented(); } - function validateRuntime(uint32, address, uint256, bytes calldata, bytes calldata) external pure override { + function validateRuntime(address, uint32, address, uint256, bytes calldata, bytes calldata) + external + pure + override + { revert NotImplemented(); } } diff --git a/test/plugin/SingleOwnerPlugin.t.sol b/test/plugin/SingleOwnerPlugin.t.sol index ddfe4d41..366f6cf8 100644 --- a/test/plugin/SingleOwnerPlugin.t.sol +++ b/test/plugin/SingleOwnerPlugin.t.sol @@ -115,11 +115,11 @@ contract SingleOwnerPluginTest is OptimizedTest { assertEq(address(0), plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); plugin.transferOwnership(TEST_DEFAULT_OWNER_FUNCTION_ID, owner1); assertEq(owner1, plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); - plugin.validateRuntime(TEST_DEFAULT_OWNER_FUNCTION_ID, owner1, 0, "", ""); + plugin.validateRuntime(a, TEST_DEFAULT_OWNER_FUNCTION_ID, owner1, 0, "", ""); vm.startPrank(b); vm.expectRevert(SingleOwnerPlugin.NotAuthorized.selector); - plugin.validateRuntime(TEST_DEFAULT_OWNER_FUNCTION_ID, owner1, 0, "", ""); + plugin.validateRuntime(b, TEST_DEFAULT_OWNER_FUNCTION_ID, owner1, 0, "", ""); } function testFuzz_validateUserOpSig(string memory salt, PackedUserOperation memory userOp) public { From 6fb6dee906c28a63b69207cfc3e1fa19a91dd113 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Mon, 15 Jul 2024 16:26:50 -0700 Subject: [PATCH 4/7] add account as param in validateSignature method and add 1271 support in ecdsaValidation --- src/account/UpgradeableModularAccount.sol | 6 ++- src/interfaces/IValidation.sol | 12 +++-- src/plugins/owner/SingleOwnerPlugin.sol | 4 +- src/plugins/validation/EcdsaValidation.sol | 36 +++++++++++-- test/mocks/plugins/ComprehensivePlugin.sol | 6 ++- test/mocks/plugins/ReturnDataPluginMocks.sol | 2 +- test/mocks/plugins/ValidationPluginMocks.sol | 7 ++- test/plugin/SingleOwnerPlugin.t.sol | 6 +-- test/validation/EcdsaValidation.t.sol | 54 +++++++++++++++++++- 9 files changed, 112 insertions(+), 21 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 7fe5b1f1..acbd6de5 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -348,8 +348,10 @@ contract UpgradeableModularAccount is revert SignatureValidationInvalid(plugin, entityId); } - if (IValidation(plugin).validateSignature(entityId, msg.sender, hash, signature[24:]) == _1271_MAGIC_VALUE) - { + if ( + IValidation(plugin).validateSignature(address(this), entityId, msg.sender, hash, signature[24:]) + == _1271_MAGIC_VALUE + ) { return _1271_MAGIC_VALUE; } return _1271_INVALID; diff --git a/src/interfaces/IValidation.sol b/src/interfaces/IValidation.sol index 2a992849..471cd2c1 100644 --- a/src/interfaces/IValidation.sol +++ b/src/interfaces/IValidation.sol @@ -36,14 +36,18 @@ interface IValidation is IPlugin { /// @notice Validates a signature using ERC-1271. /// @dev To indicate the entire call should revert, the function MUST revert. + /// @param account the account to validate for. /// @param entityId An identifier that routes the call to different internal implementations, should there /// be more than one. /// @param sender the address that sent the ERC-1271 request to the smart account /// @param hash the hash of the ERC-1271 request /// @param signature the signature of the ERC-1271 request /// @return the ERC-1271 `MAGIC_VALUE` if the signature is valid, or 0xFFFFFFFF if invalid. - function validateSignature(uint32 entityId, address sender, bytes32 hash, bytes calldata signature) - external - view - returns (bytes4); + function validateSignature( + address account, + uint32 entityId, + address sender, + bytes32 hash, + bytes calldata signature + ) external view returns (bytes4); } diff --git a/src/plugins/owner/SingleOwnerPlugin.sol b/src/plugins/owner/SingleOwnerPlugin.sol index 1ef677d7..5cfa6592 100644 --- a/src/plugins/owner/SingleOwnerPlugin.sol +++ b/src/plugins/owner/SingleOwnerPlugin.sol @@ -126,13 +126,13 @@ contract SingleOwnerPlugin is IValidation, BasePlugin { /// validation used in `validateUserOp`, this does///*not** wrap the digest in /// an "Ethereum Signed Message" envelope before checking the signature in /// the EOA-owner case. - function validateSignature(uint32 entityId, address, bytes32 digest, bytes calldata signature) + function validateSignature(address account, uint32 entityId, address, bytes32 digest, bytes calldata signature) external view override returns (bytes4) { - if (SignatureChecker.isValidSignatureNow(owners[entityId][msg.sender], digest, signature)) { + if (SignatureChecker.isValidSignatureNow(owners[entityId][account], digest, signature)) { return _1271_MAGIC_VALUE; } return _1271_INVALID; diff --git a/src/plugins/validation/EcdsaValidation.sol b/src/plugins/validation/EcdsaValidation.sol index a2fcde05..5c9347c0 100644 --- a/src/plugins/validation/EcdsaValidation.sol +++ b/src/plugins/validation/EcdsaValidation.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.25; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; +import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; import {IPlugin, PluginManifest, PluginMetadata} from "../../interfaces/IPlugin.sol"; import {IValidation} from "../../interfaces/IValidation.sol"; @@ -13,9 +14,15 @@ import {IEcdsaValidation} from "./IEcdsaValidation.sol"; /// @title ECSDA Validation /// @author ERC-6900 Authors /// @notice This validation enables any ECDSA (secp256k1 curve) signature validation. It handles installation by -/// each entity (validationId). Uninstallation will NOT disable all installed validation entities. None of the -/// functions are installed on the account. Account states are to be retrieved from this global singleton directly. -/// Note: This validation also support composition that other validation can relay on entities in this validation +/// each entity (validationId). +/// Note: Uninstallation will NOT disable all installed validation entities. None of the functions are installed on +/// the account. Account states are to be retrieved from this global singleton directly. +/// +/// - This validation supports ERC-1271. The signature is valid if it is signed by the owner's private key +/// (if the owner is an EOA) or if it is a valid ERC-1271 signature from the +/// owner (if the owner is a contract). +/// +/// - This validation supports composition that other validation can relay on entities in this validation /// to validate partially or fully. contract EcdsaValidation is IEcdsaValidation, BasePlugin { using ECDSA for bytes32; @@ -28,6 +35,10 @@ contract EcdsaValidation is IEcdsaValidation, BasePlugin { uint256 internal constant _SIG_VALIDATION_PASSED = 0; uint256 internal constant _SIG_VALIDATION_FAILED = 1; + // bytes4(keccak256("isValidSignature(bytes32,bytes)")) + bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e; + bytes4 internal constant _1271_INVALID = 0xffffffff; + mapping(uint32 validationId => mapping(address account => address)) public signer; /// @inheritdoc IEcdsaValidation @@ -104,8 +115,23 @@ contract EcdsaValidation is IEcdsaValidation, BasePlugin { } /// @inheritdoc IValidation - function validateSignature(uint32, address, bytes32, bytes calldata) external pure override returns (bytes4) { - revert NotImplemented(); + /// @dev The signature is valid if it is signed by the owner's private key + /// (if the owner is an EOA) or if it is a valid ERC-1271 signature from the + /// owner (if the owner is a contract). Note that unlike the signature + /// validation used in `validateUserOp`, this does///*not** wrap the digest in + /// an "Ethereum Signed Message" envelope before checking the signature in + /// the EOA-owner case. + function validateSignature( + address account, + uint32 validationId, + address, + bytes32 digest, + bytes calldata signature + ) external view override returns (bytes4) { + if (SignatureChecker.isValidSignatureNow(signer[validationId][account], digest, signature)) { + return _1271_MAGIC_VALUE; + } + return _1271_INVALID; } // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index 87fb27f1..306e96a4 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -96,7 +96,11 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba revert NotImplemented(); } - function validateSignature(uint32 entityId, address, bytes32, bytes calldata) external pure returns (bytes4) { + function validateSignature(address, uint32 entityId, address, bytes32, bytes calldata) + external + pure + returns (bytes4) + { if (entityId == uint32(EntityId.SIG_VALIDATION)) { return 0xffffffff; } diff --git a/test/mocks/plugins/ReturnDataPluginMocks.sol b/test/mocks/plugins/ReturnDataPluginMocks.sol index fad0485f..3adbb6e4 100644 --- a/test/mocks/plugins/ReturnDataPluginMocks.sol +++ b/test/mocks/plugins/ReturnDataPluginMocks.sol @@ -85,7 +85,7 @@ contract ResultConsumerPlugin is BasePlugin, IValidation { } } - function validateSignature(uint32, address, bytes32, bytes calldata) external pure returns (bytes4) { + function validateSignature(address, uint32, address, bytes32, bytes calldata) external pure returns (bytes4) { revert NotImplemented(); } diff --git a/test/mocks/plugins/ValidationPluginMocks.sol b/test/mocks/plugins/ValidationPluginMocks.sol index 5aa3719d..8d9b6ce2 100644 --- a/test/mocks/plugins/ValidationPluginMocks.sol +++ b/test/mocks/plugins/ValidationPluginMocks.sol @@ -58,7 +58,12 @@ abstract contract MockBaseUserOpValidationPlugin is IValidation, IValidationHook revert NotImplemented(); } - function validateSignature(uint32, address, bytes32, bytes calldata) external pure override returns (bytes4) { + function validateSignature(address, uint32, address, bytes32, bytes calldata) + external + pure + override + returns (bytes4) + { revert NotImplemented(); } diff --git a/test/plugin/SingleOwnerPlugin.t.sol b/test/plugin/SingleOwnerPlugin.t.sol index 366f6cf8..6d57e5b7 100644 --- a/test/plugin/SingleOwnerPlugin.t.sol +++ b/test/plugin/SingleOwnerPlugin.t.sol @@ -156,7 +156,7 @@ contract SingleOwnerPluginTest is OptimizedTest { // sig check should fail assertEq( plugin.validateSignature( - TEST_DEFAULT_OWNER_FUNCTION_ID, address(this), digest, abi.encodePacked(r, s, v) + a, TEST_DEFAULT_OWNER_FUNCTION_ID, address(this), digest, abi.encodePacked(r, s, v) ), bytes4(0xFFFFFFFF) ); @@ -168,7 +168,7 @@ contract SingleOwnerPluginTest is OptimizedTest { // sig check should pass assertEq( plugin.validateSignature( - TEST_DEFAULT_OWNER_FUNCTION_ID, address(this), digest, abi.encodePacked(r, s, v) + a, TEST_DEFAULT_OWNER_FUNCTION_ID, address(this), digest, abi.encodePacked(r, s, v) ), _1271_MAGIC_VALUE ); @@ -179,7 +179,7 @@ contract SingleOwnerPluginTest is OptimizedTest { plugin.transferOwnership(TEST_DEFAULT_OWNER_FUNCTION_ID, address(contractOwner)); bytes memory signature = contractOwner.sign(digest); assertEq( - plugin.validateSignature(TEST_DEFAULT_OWNER_FUNCTION_ID, address(this), digest, signature), + plugin.validateSignature(a, TEST_DEFAULT_OWNER_FUNCTION_ID, address(this), digest, signature), _1271_MAGIC_VALUE ); } diff --git a/test/validation/EcdsaValidation.t.sol b/test/validation/EcdsaValidation.t.sol index d3750b03..aa9b24cb 100644 --- a/test/validation/EcdsaValidation.t.sol +++ b/test/validation/EcdsaValidation.t.sol @@ -10,20 +10,27 @@ import {ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; import {TEST_DEFAULT_VALIDATION_ID} from "../utils/TestConstants.sol"; +import {ContractOwner} from "../mocks/ContractOwner.sol"; contract EcdsaValidationTest is AccountTestBase { using MessageHashUtils for bytes32; + bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e; + address public ethRecipient; address public owner2; uint256 public owner2Key; UpgradeableModularAccount public account; + ContractOwner public contractOwner; + function setUp() public { ethRecipient = makeAddr("ethRecipient"); (owner2, owner2Key) = makeAddrAndKey("owner2"); account = ecdsaFactory.createAccount(owner1, 0); vm.deal(address(account), 100 ether); + + contractOwner = new ContractOwner(); } function test_userOpValidation() public { @@ -56,7 +63,7 @@ contract EcdsaValidationTest is AccountTestBase { assertEq(ethRecipient.balance, 1 wei); } - function test_runtime() public { + function test_runtimeValidate() public { vm.prank(owner1); account.executeWithAuthorization( abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, "")), @@ -67,7 +74,7 @@ contract EcdsaValidationTest is AccountTestBase { assertEq(ethRecipient.balance, 1 wei); } - function test_runtime_with2ndValidation() public { + function test_runtime_with2SameValidationInstalled() public { uint32 newValidationId = TEST_DEFAULT_OWNER_FUNCTION_ID + 1; vm.prank(address(entryPoint)); account.installValidation( @@ -87,4 +94,47 @@ contract EcdsaValidationTest is AccountTestBase { ); assertEq(ethRecipient.balance, 1 wei); } + + function testFuzz_isValidSignatureForEOAOwner(string memory salt, bytes32 digest) public { + // range bound the possible set of priv keys + (address signer, uint256 privateKey) = makeAddrAndKey(salt); + + address accountAddr = address(account); + + vm.startPrank(accountAddr); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, digest); + + // sig check should fail + assertEq( + ecdsaValidation.validateSignature( + accountAddr, TEST_DEFAULT_OWNER_FUNCTION_ID, address(this), digest, abi.encodePacked(r, s, v) + ), + bytes4(0xFFFFFFFF) + ); + + // transfer ownership to signer + ecdsaValidation.transferSigner(TEST_DEFAULT_OWNER_FUNCTION_ID, signer); + assertEq(signer, ecdsaValidation.signerOf(TEST_DEFAULT_OWNER_FUNCTION_ID, accountAddr)); + + // sig check should pass + assertEq( + ecdsaValidation.validateSignature( + accountAddr, TEST_DEFAULT_OWNER_FUNCTION_ID, address(this), digest, abi.encodePacked(r, s, v) + ), + _1271_MAGIC_VALUE + ); + } + + function testFuzz_isValidSignatureForContractOwner(bytes32 digest) public { + address accountAddr = address(account); + vm.startPrank(accountAddr); + ecdsaValidation.transferSigner(TEST_DEFAULT_OWNER_FUNCTION_ID, address(contractOwner)); + bytes memory signature = contractOwner.sign(digest); + assertEq( + ecdsaValidation.validateSignature( + accountAddr, TEST_DEFAULT_OWNER_FUNCTION_ID, address(this), digest, signature + ), + _1271_MAGIC_VALUE + ); + } } From 76857908d176cb3d73ff5cd3e534712b498c839a Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Tue, 16 Jul 2024 10:29:21 -0700 Subject: [PATCH 5/7] rename ecdsa to SingleSigner --- ...dation.sol => ISingleSignerValidation.sol} | 2 +- ...idation.sol => SingleSignerValidation.sol} | 12 ++++----- ...ure.sol => SingleSignerFactoryFixture.sol} | 12 ++++----- test/utils/AccountTestBase.sol | 12 ++++----- test/utils/OptimizedTest.sol | 10 ++++--- ...ion.t.sol => SingleSignerValidation.t.sol} | 26 ++++++++++--------- 6 files changed, 39 insertions(+), 35 deletions(-) rename src/plugins/validation/{IEcdsaValidation.sol => ISingleSignerValidation.sol} (96%) rename src/plugins/validation/{EcdsaValidation.sol => SingleSignerValidation.sol} (93%) rename test/mocks/{EcdsaFactoryFixture.sol => SingleSignerFactoryFixture.sol} (86%) rename test/validation/{EcdsaValidation.t.sol => SingleSignerValidation.t.sol} (81%) diff --git a/src/plugins/validation/IEcdsaValidation.sol b/src/plugins/validation/ISingleSignerValidation.sol similarity index 96% rename from src/plugins/validation/IEcdsaValidation.sol rename to src/plugins/validation/ISingleSignerValidation.sol index 61723a6b..0f814154 100644 --- a/src/plugins/validation/IEcdsaValidation.sol +++ b/src/plugins/validation/ISingleSignerValidation.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.25; import {IValidation} from "../../interfaces/IValidation.sol"; -interface IEcdsaValidation is IValidation { +interface ISingleSignerValidation is IValidation { /// @notice This event is emitted when Signer of the account's validation changes. /// @param account The account whose validation Signer changed. /// @param validationId The validationId for the account and the signer. diff --git a/src/plugins/validation/EcdsaValidation.sol b/src/plugins/validation/SingleSignerValidation.sol similarity index 93% rename from src/plugins/validation/EcdsaValidation.sol rename to src/plugins/validation/SingleSignerValidation.sol index 5c9347c0..e2d9d6ee 100644 --- a/src/plugins/validation/EcdsaValidation.sol +++ b/src/plugins/validation/SingleSignerValidation.sol @@ -9,7 +9,7 @@ import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/Signa import {IPlugin, PluginManifest, PluginMetadata} from "../../interfaces/IPlugin.sol"; import {IValidation} from "../../interfaces/IValidation.sol"; import {BasePlugin} from "../BasePlugin.sol"; -import {IEcdsaValidation} from "./IEcdsaValidation.sol"; +import {ISingleSignerValidation} from "./ISingleSignerValidation.sol"; /// @title ECSDA Validation /// @author ERC-6900 Authors @@ -24,11 +24,11 @@ import {IEcdsaValidation} from "./IEcdsaValidation.sol"; /// /// - This validation supports composition that other validation can relay on entities in this validation /// to validate partially or fully. -contract EcdsaValidation is IEcdsaValidation, BasePlugin { +contract SingleSignerValidation is ISingleSignerValidation, BasePlugin { using ECDSA for bytes32; using MessageHashUtils for bytes32; - string public constant NAME = "Ecdsa Validation"; + string public constant NAME = "SingleSigner Validation"; string public constant VERSION = "1.0.0"; string public constant AUTHOR = "ERC-6900 Authors"; @@ -41,12 +41,12 @@ contract EcdsaValidation is IEcdsaValidation, BasePlugin { mapping(uint32 validationId => mapping(address account => address)) public signer; - /// @inheritdoc IEcdsaValidation + /// @inheritdoc ISingleSignerValidation function signerOf(uint32 validationId, address account) external view returns (address) { return signer[validationId][account]; } - /// @inheritdoc IEcdsaValidation + /// @inheritdoc ISingleSignerValidation function transferSigner(uint32 validationId, address newSigner) external { _transferSigner(validationId, newSigner); } @@ -79,7 +79,7 @@ contract EcdsaValidation is IEcdsaValidation, BasePlugin { /// @inheritdoc IPlugin function onUninstall(bytes calldata data) external override { // ToDo: what does it mean in the world of composable validation world to uninstall one type of validation - // We can either get rid of all Ecdsa signers. What about the nested ones? + // We can either get rid of all SingleSigner signers. What about the nested ones? _transferSigner(abi.decode(data, (uint32)), address(0)); } diff --git a/test/mocks/EcdsaFactoryFixture.sol b/test/mocks/SingleSignerFactoryFixture.sol similarity index 86% rename from test/mocks/EcdsaFactoryFixture.sol rename to test/mocks/SingleSignerFactoryFixture.sol index 77073610..c3f663e3 100644 --- a/test/mocks/EcdsaFactoryFixture.sol +++ b/test/mocks/SingleSignerFactoryFixture.sol @@ -7,14 +7,14 @@ import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntry import {ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; -import {EcdsaValidation} from "../../src/plugins/validation/EcdsaValidation.sol"; +import {SingleSignerValidation} from "../../src/plugins/validation/SingleSignerValidation.sol"; import {OptimizedTest} from "../utils/OptimizedTest.sol"; import {TEST_DEFAULT_VALIDATION_ID} from "../utils/TestConstants.sol"; -contract EcdsaFactoryFixture is OptimizedTest { +contract SingleSignerFactoryFixture is OptimizedTest { UpgradeableModularAccount public accountImplementation; - EcdsaValidation public ecdsaValidation; + SingleSignerValidation public singleSignerValidation; bytes32 private immutable _PROXY_BYTECODE_HASH; uint32 public constant UNSTAKE_DELAY = 1 weeks; @@ -23,13 +23,13 @@ contract EcdsaFactoryFixture is OptimizedTest { address public self; - constructor(IEntryPoint _entryPoint, EcdsaValidation _ecdsaValidation) { + constructor(IEntryPoint _entryPoint, SingleSignerValidation _singleSignerValidation) { entryPoint = _entryPoint; accountImplementation = _deployUpgradeableModularAccount(_entryPoint); _PROXY_BYTECODE_HASH = keccak256( abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(address(accountImplementation), "")) ); - ecdsaValidation = _ecdsaValidation; + singleSignerValidation = _singleSignerValidation; self = address(this); } @@ -51,7 +51,7 @@ contract EcdsaFactoryFixture is OptimizedTest { // point proxy to actual implementation and init plugins UpgradeableModularAccount(payable(addr)).initializeWithValidation( - ValidationConfigLib.pack(address(ecdsaValidation), TEST_DEFAULT_VALIDATION_ID, true, false), + ValidationConfigLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ID, true, false), new bytes4[](0), pluginInstallData, "", diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index 42d020d0..a23f0151 100644 --- a/test/utils/AccountTestBase.sol +++ b/test/utils/AccountTestBase.sol @@ -5,7 +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 {EcdsaValidation} from "../../src/plugins/validation/EcdsaValidation.sol"; +import {SingleSignerValidation} from "../../src/plugins/validation/SingleSignerValidation.sol"; import {PluginEntity, PluginEntityLib} from "../../src/helpers/PluginEntityLib.sol"; import {IStandardExecutor, Call} from "../../src/interfaces/IStandardExecutor.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; @@ -14,7 +14,7 @@ import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {OptimizedTest} from "./OptimizedTest.sol"; import {TEST_DEFAULT_OWNER_FUNCTION_ID as EXT_CONST_TEST_DEFAULT_OWNER_FUNCTION_ID} from "./TestConstants.sol"; -import {EcdsaFactoryFixture} from "../mocks/EcdsaFactoryFixture.sol"; +import {SingleSignerFactoryFixture} from "../mocks/SingleSignerFactoryFixture.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; /// @dev This contract handles common boilerplate setup for tests using UpgradeableModularAccount with @@ -28,8 +28,8 @@ abstract contract AccountTestBase is OptimizedTest { SingleOwnerPlugin public singleOwnerPlugin; MSCAFactoryFixture public factory; - EcdsaValidation public ecdsaValidation; - EcdsaFactoryFixture public ecdsaFactory; + SingleSignerValidation public singleSignerValidation; + SingleSignerFactoryFixture public singleSignerFactory; address public owner1; uint256 public owner1Key; @@ -58,8 +58,8 @@ abstract contract AccountTestBase is OptimizedTest { singleOwnerPlugin = _deploySingleOwnerPlugin(); factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); - ecdsaValidation = _deployEcdsaValidation(); - ecdsaFactory = new EcdsaFactoryFixture(entryPoint, ecdsaValidation); + singleSignerValidation = _deploySingleSignerValidation(); + singleSignerFactory = new SingleSignerFactoryFixture(entryPoint, singleSignerValidation); account1 = factory.createAccount(owner1, 0); vm.deal(address(account1), 100 ether); diff --git a/test/utils/OptimizedTest.sol b/test/utils/OptimizedTest.sol index c2e5f9d6..f5e66dfc 100644 --- a/test/utils/OptimizedTest.sol +++ b/test/utils/OptimizedTest.sol @@ -7,7 +7,7 @@ import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntry import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; -import {EcdsaValidation} from "../../src/plugins/validation/EcdsaValidation.sol"; +import {SingleSignerValidation} from "../../src/plugins/validation/SingleSignerValidation.sol"; import {TokenReceiverPlugin} from "../../src/plugins/TokenReceiverPlugin.sol"; /// @dev This contract provides functions to deploy optimized (via IR) precompiled contracts. By compiling just @@ -57,9 +57,11 @@ abstract contract OptimizedTest is Test { : new TokenReceiverPlugin(); } - function _deployEcdsaValidation() internal returns (EcdsaValidation) { + function _deploySingleSignerValidation() internal returns (SingleSignerValidation) { return _isOptimizedTest() - ? EcdsaValidation(deployCode("out-optimized/EcdsaValidation.sol/EcdsaValidation.json")) - : new EcdsaValidation(); + ? SingleSignerValidation( + deployCode("out-optimized/SingleSignerValidation.sol/SingleSignerValidation.json") + ) + : new SingleSignerValidation(); } } diff --git a/test/validation/EcdsaValidation.t.sol b/test/validation/SingleSignerValidation.t.sol similarity index 81% rename from test/validation/EcdsaValidation.t.sol rename to test/validation/SingleSignerValidation.t.sol index aa9b24cb..eb2692c7 100644 --- a/test/validation/EcdsaValidation.t.sol +++ b/test/validation/SingleSignerValidation.t.sol @@ -12,7 +12,7 @@ import {AccountTestBase} from "../utils/AccountTestBase.sol"; import {TEST_DEFAULT_VALIDATION_ID} from "../utils/TestConstants.sol"; import {ContractOwner} from "../mocks/ContractOwner.sol"; -contract EcdsaValidationTest is AccountTestBase { +contract SingleSignerValidationTest is AccountTestBase { using MessageHashUtils for bytes32; bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e; @@ -27,7 +27,7 @@ contract EcdsaValidationTest is AccountTestBase { function setUp() public { ethRecipient = makeAddr("ethRecipient"); (owner2, owner2Key) = makeAddrAndKey("owner2"); - account = ecdsaFactory.createAccount(owner1, 0); + account = singleSignerFactory.createAccount(owner1, 0); vm.deal(address(account), 100 ether); contractOwner = new ContractOwner(); @@ -50,7 +50,7 @@ contract EcdsaValidationTest is AccountTestBase { bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); userOp.signature = _encodeSignature( - PluginEntityLib.pack(address(ecdsaValidation), TEST_DEFAULT_VALIDATION_ID), + PluginEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ID), GLOBAL_VALIDATION, abi.encodePacked(r, s, v) ); @@ -68,7 +68,9 @@ contract EcdsaValidationTest is AccountTestBase { account.executeWithAuthorization( abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, "")), _encodeSignature( - PluginEntityLib.pack(address(ecdsaValidation), TEST_DEFAULT_VALIDATION_ID), GLOBAL_VALIDATION, "" + PluginEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ID), + GLOBAL_VALIDATION, + "" ) ); assertEq(ethRecipient.balance, 1 wei); @@ -78,7 +80,7 @@ contract EcdsaValidationTest is AccountTestBase { uint32 newValidationId = TEST_DEFAULT_OWNER_FUNCTION_ID + 1; vm.prank(address(entryPoint)); account.installValidation( - ValidationConfigLib.pack(address(ecdsaValidation), newValidationId, true, false), + ValidationConfigLib.pack(address(singleSignerValidation), newValidationId, true, false), new bytes4[](0), abi.encode(newValidationId, owner2), "", @@ -89,7 +91,7 @@ contract EcdsaValidationTest is AccountTestBase { account.executeWithAuthorization( abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, "")), _encodeSignature( - PluginEntityLib.pack(address(ecdsaValidation), newValidationId), GLOBAL_VALIDATION, "" + PluginEntityLib.pack(address(singleSignerValidation), newValidationId), GLOBAL_VALIDATION, "" ) ); assertEq(ethRecipient.balance, 1 wei); @@ -106,19 +108,19 @@ contract EcdsaValidationTest is AccountTestBase { // sig check should fail assertEq( - ecdsaValidation.validateSignature( + singleSignerValidation.validateSignature( accountAddr, TEST_DEFAULT_OWNER_FUNCTION_ID, address(this), digest, abi.encodePacked(r, s, v) ), bytes4(0xFFFFFFFF) ); // transfer ownership to signer - ecdsaValidation.transferSigner(TEST_DEFAULT_OWNER_FUNCTION_ID, signer); - assertEq(signer, ecdsaValidation.signerOf(TEST_DEFAULT_OWNER_FUNCTION_ID, accountAddr)); + singleSignerValidation.transferSigner(TEST_DEFAULT_OWNER_FUNCTION_ID, signer); + assertEq(signer, singleSignerValidation.signerOf(TEST_DEFAULT_OWNER_FUNCTION_ID, accountAddr)); // sig check should pass assertEq( - ecdsaValidation.validateSignature( + singleSignerValidation.validateSignature( accountAddr, TEST_DEFAULT_OWNER_FUNCTION_ID, address(this), digest, abi.encodePacked(r, s, v) ), _1271_MAGIC_VALUE @@ -128,10 +130,10 @@ contract EcdsaValidationTest is AccountTestBase { function testFuzz_isValidSignatureForContractOwner(bytes32 digest) public { address accountAddr = address(account); vm.startPrank(accountAddr); - ecdsaValidation.transferSigner(TEST_DEFAULT_OWNER_FUNCTION_ID, address(contractOwner)); + singleSignerValidation.transferSigner(TEST_DEFAULT_OWNER_FUNCTION_ID, address(contractOwner)); bytes memory signature = contractOwner.sign(digest); assertEq( - ecdsaValidation.validateSignature( + singleSignerValidation.validateSignature( accountAddr, TEST_DEFAULT_OWNER_FUNCTION_ID, address(this), digest, signature ), _1271_MAGIC_VALUE From d66b6b8e7a2da079b7f87be00b306343b4b09973 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Tue, 16 Jul 2024 17:12:28 -0700 Subject: [PATCH 6/7] rename validationId in SingleSignerValidation to entityId --- .../validation/ISingleSignerValidation.sol | 12 ++--- .../validation/SingleSignerValidation.sol | 47 +++++++++---------- test/validation/SingleSignerValidation.t.sol | 8 ++-- 3 files changed, 33 insertions(+), 34 deletions(-) diff --git a/src/plugins/validation/ISingleSignerValidation.sol b/src/plugins/validation/ISingleSignerValidation.sol index 0f814154..2653b752 100644 --- a/src/plugins/validation/ISingleSignerValidation.sol +++ b/src/plugins/validation/ISingleSignerValidation.sol @@ -6,23 +6,23 @@ import {IValidation} from "../../interfaces/IValidation.sol"; interface ISingleSignerValidation is IValidation { /// @notice This event is emitted when Signer of the account's validation changes. /// @param account The account whose validation Signer changed. - /// @param validationId The validationId for the account and the signer. + /// @param entityId The entityId for the account and the signer. /// @param previousSigner The address of the previous signer. /// @param newSigner The address of the new signer. event SignerTransferred( - address indexed account, uint32 indexed validationId, address previousSigner, address newSigner + address indexed account, uint32 indexed entityId, address previousSigner, address newSigner ); error NotAuthorized(); /// @notice Transfer Signer of the account's validation to `newSigner`. - /// @param validationId The validationId for the account and the signer. + /// @param entityId The entityId for the account and the signer. /// @param newSigner The address of the new signer. - function transferSigner(uint32 validationId, address newSigner) external; + function transferSigner(uint32 entityId, address newSigner) external; /// @notice Get the signer of the `account`'s validation. - /// @param validationId The validationId for the account and the signer. + /// @param entityId The entityId for the account and the signer. /// @param account The account to get the signer of. /// @return The address of the signer. - function signerOf(uint32 validationId, address account) external view returns (address); + function signerOf(uint32 entityId, address account) external view returns (address); } diff --git a/src/plugins/validation/SingleSignerValidation.sol b/src/plugins/validation/SingleSignerValidation.sol index e2d9d6ee..5ae4517d 100644 --- a/src/plugins/validation/SingleSignerValidation.sol +++ b/src/plugins/validation/SingleSignerValidation.sol @@ -14,7 +14,7 @@ import {ISingleSignerValidation} from "./ISingleSignerValidation.sol"; /// @title ECSDA Validation /// @author ERC-6900 Authors /// @notice This validation enables any ECDSA (secp256k1 curve) signature validation. It handles installation by -/// each entity (validationId). +/// each entity (entityId). /// Note: Uninstallation will NOT disable all installed validation entities. None of the functions are installed on /// the account. Account states are to be retrieved from this global singleton directly. /// @@ -39,16 +39,16 @@ contract SingleSignerValidation is ISingleSignerValidation, BasePlugin { bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e; bytes4 internal constant _1271_INVALID = 0xffffffff; - mapping(uint32 validationId => mapping(address account => address)) public signer; + mapping(uint32 entityId => mapping(address account => address)) public signer; /// @inheritdoc ISingleSignerValidation - function signerOf(uint32 validationId, address account) external view returns (address) { - return signer[validationId][account]; + function signerOf(uint32 entityId, address account) external view returns (address) { + return signer[entityId][account]; } /// @inheritdoc ISingleSignerValidation - function transferSigner(uint32 validationId, address newSigner) external { - _transferSigner(validationId, newSigner); + function transferSigner(uint32 entityId, address newSigner) external { + _transferSigner(entityId, newSigner); } // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ @@ -72,8 +72,8 @@ contract SingleSignerValidation is ISingleSignerValidation, BasePlugin { /// @inheritdoc IPlugin function onInstall(bytes calldata data) external override { - (uint32 validationId, address newSigner) = abi.decode(data, (uint32, address)); - _transferSigner(validationId, newSigner); + (uint32 entityId, address newSigner) = abi.decode(data, (uint32, address)); + _transferSigner(entityId, newSigner); } /// @inheritdoc IPlugin @@ -84,7 +84,7 @@ contract SingleSignerValidation is ISingleSignerValidation, BasePlugin { } /// @inheritdoc IValidation - function validateUserOp(uint32 validationId, PackedUserOperation calldata userOp, bytes32 userOpHash) + function validateUserOp(uint32 entityId, PackedUserOperation calldata userOp, bytes32 userOpHash) external view override @@ -92,7 +92,7 @@ contract SingleSignerValidation is ISingleSignerValidation, BasePlugin { { // Validate the user op signature against the owner. (address sigSigner,,) = (userOpHash.toEthSignedMessageHash()).tryRecover(userOp.signature); - if (sigSigner == address(0) || sigSigner != signer[validationId][userOp.sender]) { + if (sigSigner == address(0) || sigSigner != signer[entityId][userOp.sender]) { return _SIG_VALIDATION_FAILED; } return _SIG_VALIDATION_PASSED; @@ -101,14 +101,14 @@ contract SingleSignerValidation is ISingleSignerValidation, BasePlugin { /// @inheritdoc IValidation function validateRuntime( address account, - uint32 validationId, + uint32 entityId, address sender, uint256, bytes calldata, bytes calldata ) external view override { // Validate that the sender is the owner of the account or self. - if (sender != signer[validationId][account]) { + if (sender != signer[entityId][account]) { revert NotAuthorized(); } return; @@ -121,14 +121,13 @@ contract SingleSignerValidation is ISingleSignerValidation, BasePlugin { /// validation used in `validateUserOp`, this does///*not** wrap the digest in /// an "Ethereum Signed Message" envelope before checking the signature in /// the EOA-owner case. - function validateSignature( - address account, - uint32 validationId, - address, - bytes32 digest, - bytes calldata signature - ) external view override returns (bytes4) { - if (SignatureChecker.isValidSignatureNow(signer[validationId][account], digest, signature)) { + function validateSignature(address account, uint32 entityId, address, bytes32 digest, bytes calldata signature) + external + view + override + returns (bytes4) + { + if (SignatureChecker.isValidSignatureNow(signer[entityId][account], digest, signature)) { return _1271_MAGIC_VALUE; } return _1271_INVALID; @@ -138,9 +137,9 @@ contract SingleSignerValidation is ISingleSignerValidation, BasePlugin { // ┃ Internal / Private functions ┃ // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - function _transferSigner(uint32 validationId, address newSigner) internal { - address previousSigner = signer[validationId][msg.sender]; - signer[validationId][msg.sender] = newSigner; - emit SignerTransferred(msg.sender, validationId, previousSigner, newSigner); + function _transferSigner(uint32 entityId, address newSigner) internal { + address previousSigner = signer[entityId][msg.sender]; + signer[entityId][msg.sender] = newSigner; + emit SignerTransferred(msg.sender, entityId, previousSigner, newSigner); } } diff --git a/test/validation/SingleSignerValidation.t.sol b/test/validation/SingleSignerValidation.t.sol index eb2692c7..af2b1c6e 100644 --- a/test/validation/SingleSignerValidation.t.sol +++ b/test/validation/SingleSignerValidation.t.sol @@ -77,12 +77,12 @@ contract SingleSignerValidationTest is AccountTestBase { } function test_runtime_with2SameValidationInstalled() public { - uint32 newValidationId = TEST_DEFAULT_OWNER_FUNCTION_ID + 1; + uint32 newEntityId = TEST_DEFAULT_OWNER_FUNCTION_ID + 1; vm.prank(address(entryPoint)); account.installValidation( - ValidationConfigLib.pack(address(singleSignerValidation), newValidationId, true, false), + ValidationConfigLib.pack(address(singleSignerValidation), newEntityId, true, false), new bytes4[](0), - abi.encode(newValidationId, owner2), + abi.encode(newEntityId, owner2), "", "" ); @@ -91,7 +91,7 @@ contract SingleSignerValidationTest is AccountTestBase { account.executeWithAuthorization( abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, "")), _encodeSignature( - PluginEntityLib.pack(address(singleSignerValidation), newValidationId), GLOBAL_VALIDATION, "" + PluginEntityLib.pack(address(singleSignerValidation), newEntityId), GLOBAL_VALIDATION, "" ) ); assertEq(ethRecipient.balance, 1 wei); From dc9ed7934fd36312d4d0b0870b1d760633af76a2 Mon Sep 17 00:00:00 2001 From: fangting-alchemy <119372438+fangting-alchemy@users.noreply.github.com> Date: Tue, 16 Jul 2024 17:23:58 -0700 Subject: [PATCH 7/7] [4/n] validation series - delete SingleOwnerPlugin (#97) --- README.md | 2 +- src/plugins/owner/SingleOwnerPlugin.sol | 191 ------------------- test/account/AccountLoupe.t.sol | 4 +- test/account/AccountReturnData.t.sol | 6 +- test/account/GlobalValidationTest.t.sol | 7 +- test/account/MultiValidation.t.sol | 26 +-- test/account/PerHookData.t.sol | 43 +++-- test/account/SelfCallAuthorization.t.sol | 2 +- test/account/UpgradeableModularAccount.t.sol | 32 ++-- test/mocks/MSCAFactoryFixture.sol | 81 -------- test/mocks/SingleSignerFactoryFixture.sol | 8 +- test/plugin/SingleOwnerPlugin.t.sol | 186 ------------------ test/plugin/TokenReceiverPlugin.t.sol | 5 +- test/samples/AllowlistPlugin.t.sol | 4 +- test/utils/AccountTestBase.sol | 34 ++-- test/utils/OptimizedTest.sol | 7 - test/utils/TestConstants.sol | 3 +- test/validation/SingleSignerValidation.t.sol | 22 +-- 18 files changed, 101 insertions(+), 562 deletions(-) delete mode 100644 src/plugins/owner/SingleOwnerPlugin.sol delete mode 100644 test/mocks/MSCAFactoryFixture.sol delete mode 100644 test/plugin/SingleOwnerPlugin.t.sol diff --git a/README.md b/README.md index fb3e9dbc..1c90c710 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ Reference implementation for [ERC-6900](https://eips.ethereum.org/EIPS/eip-6900). It is an early draft implementation. -The implementation includes an upgradable modular account with two plugins (`SingleOwnerPlugin` and `TokenReceiverPlugin`). It is compliant with ERC-6900 with the latest updates. +The implementation includes an upgradable modular account with two plugins (`SingleSignerValidation` and `TokenReceiverPlugin`). It is compliant with ERC-6900 with the latest updates. ## Important Callouts diff --git a/src/plugins/owner/SingleOwnerPlugin.sol b/src/plugins/owner/SingleOwnerPlugin.sol deleted file mode 100644 index 5cfa6592..00000000 --- a/src/plugins/owner/SingleOwnerPlugin.sol +++ /dev/null @@ -1,191 +0,0 @@ -// SPDX-License-Identifier: GPL-3.0 -pragma solidity ^0.8.25; - -import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; -import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; -import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; -import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; - -import {PluginManifest, PluginMetadata, SelectorPermission} from "../../interfaces/IPlugin.sol"; -import {IPlugin} from "../../interfaces/IPlugin.sol"; -import {IValidation} from "../../interfaces/IValidation.sol"; -import {BasePlugin, IERC165} from "../BasePlugin.sol"; - -/// @title Single Owner Plugin -/// @author ERC-6900 Authors -/// @notice This plugin allows an EOA or smart contract to own a modular account. -/// It also supports [ERC-1271](https://eips.ethereum.org/EIPS/eip-1271) signature -/// validation for both validating the signature on user operations and in -/// exposing its own `isValidSignature` method. This only works when the owner of -/// modular account also support ERC-1271. -/// -/// ERC-4337's bundler validation rules limit the types of contracts that can be -/// used as owners to validate user operation signatures. For example, the -/// contract's `isValidSignature` function may not use any forbidden opcodes -/// such as `TIMESTAMP` or `NUMBER`, and the contract may not be an ERC-1967 -/// proxy as it accesses a constant implementation slot not associated with -/// the account, violating storage access rules. This also means that the -/// owner of a modular account may not be another modular account if you want to -/// send user operations through a bundler. -contract SingleOwnerPlugin is IValidation, BasePlugin { - using ECDSA for bytes32; - using MessageHashUtils for bytes32; - - string internal constant _NAME = "Single Owner Plugin"; - string internal constant _VERSION = "1.0.0"; - string internal constant _AUTHOR = "ERC-6900 Authors"; - - uint256 internal constant _SIG_VALIDATION_PASSED = 0; - uint256 internal constant _SIG_VALIDATION_FAILED = 1; - - // bytes4(keccak256("isValidSignature(bytes32,bytes)")) - bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e; - bytes4 internal constant _1271_INVALID = 0xffffffff; - - mapping(uint32 id => mapping(address account => address)) public owners; - - /// @notice This event is emitted when ownership of the account changes. - /// @param account The account whose ownership changed. - /// @param previousOwner The address of the previous owner. - /// @param newOwner The address of the new owner. - event OwnershipTransferred(address indexed account, address indexed previousOwner, address indexed newOwner); - - error AlreadyInitialized(); - error NotAuthorized(); - error NotInitialized(); - - // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ - // ┃ Execution functions ┃ - // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - - /// @notice Transfer ownership of an account and ID to `newOwner`. The caller address (`msg.sender`) is user to - /// identify the account. - /// @param newOwner The address of the new owner. - function transferOwnership(uint32 id, address newOwner) external { - _transferOwnership(id, newOwner); - } - - // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ - // ┃ Plugin interface functions ┃ - // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - - /// @inheritdoc IPlugin - function onInstall(bytes calldata data) external override { - (uint32 id, address owner) = abi.decode(data, (uint32, address)); - _transferOwnership(id, owner); - } - - /// @inheritdoc IPlugin - function onUninstall(bytes calldata data) external override { - uint32 id = abi.decode(data, (uint32)); - _transferOwnership(id, address(0)); - } - - /// @inheritdoc IValidation - function validateRuntime( - address account, - uint32 entityId, - address sender, - uint256, - bytes calldata, - bytes calldata - ) external view override { - // Validate that the sender is the owner of the account or self. - if (sender != owners[entityId][account]) { - revert NotAuthorized(); - } - return; - } - - /// @inheritdoc IValidation - function validateUserOp(uint32 entityId, PackedUserOperation calldata userOp, bytes32 userOpHash) - external - view - override - returns (uint256) - { - // Validate the user op signature against the owner. - if ( - SignatureChecker.isValidSignatureNow( - owners[entityId][msg.sender], userOpHash.toEthSignedMessageHash(), userOp.signature - ) - ) { - return _SIG_VALIDATION_PASSED; - } - return _SIG_VALIDATION_FAILED; - } - - // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ - // ┃ Execution view functions ┃ - // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - - /// @inheritdoc IValidation - /// @dev The signature is valid if it is signed by the owner's private key - /// (if the owner is an EOA) or if it is a valid ERC-1271 signature from the - /// owner (if the owner is a contract). Note that unlike the signature - /// validation used in `validateUserOp`, this does///*not** wrap the digest in - /// an "Ethereum Signed Message" envelope before checking the signature in - /// the EOA-owner case. - function validateSignature(address account, uint32 entityId, address, bytes32 digest, bytes calldata signature) - external - view - override - returns (bytes4) - { - if (SignatureChecker.isValidSignatureNow(owners[entityId][account], digest, signature)) { - return _1271_MAGIC_VALUE; - } - return _1271_INVALID; - } - - // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ - // ┃ Plugin view functions ┃ - // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - - /// @inheritdoc IPlugin - function pluginManifest() external pure override returns (PluginManifest memory) { - PluginManifest memory manifest; - return manifest; - } - - /// @inheritdoc IPlugin - function pluginMetadata() external pure virtual override returns (PluginMetadata memory) { - PluginMetadata memory metadata; - metadata.name = _NAME; - metadata.version = _VERSION; - metadata.author = _AUTHOR; - - // Permission strings - string memory modifyOwnershipPermission = "Modify Ownership"; - - // Permission descriptions - metadata.permissionDescriptors = new SelectorPermission[](1); - metadata.permissionDescriptors[0] = SelectorPermission({ - functionSelector: this.transferOwnership.selector, - permissionDescription: modifyOwnershipPermission - }); - - return metadata; - } - - // ┏━━━━━━━━━━━━━━━┓ - // ┃ EIP-165 ┃ - // ┗━━━━━━━━━━━━━━━┛ - - /// @inheritdoc BasePlugin - function supportsInterface(bytes4 interfaceId) public view override(BasePlugin, IERC165) returns (bool) { - return interfaceId == type(IValidation).interfaceId || super.supportsInterface(interfaceId); - } - - // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ - // ┃ Internal / Private functions ┃ - // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - - // Transfers ownership and emits and event. - function _transferOwnership(uint32 id, address newOwner) internal { - address previousOwner = owners[id][msg.sender]; - owners[id][msg.sender] = newOwner; - // Todo: include id in event - emit OwnershipTransferred(msg.sender, previousOwner, newOwner); - } -} diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol index b65bf3c9..f0670848 100644 --- a/test/account/AccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -115,7 +115,7 @@ contract AccountLoupeTest is CustomValidationTestBase { } function test_pluginLoupe_getValidationHooks() public { - PluginEntity[] memory hooks = account1.getPreValidationHooks(_ownerValidation); + PluginEntity[] memory hooks = account1.getPreValidationHooks(_signerValidation); assertEq(hooks.length, 2); assertEq( @@ -154,7 +154,7 @@ contract AccountLoupeTest is CustomValidationTestBase { bytes[] memory installDatas = new bytes[](2); return ( - _ownerValidation, + _signerValidation, true, true, new bytes4[](0), diff --git a/test/account/AccountReturnData.t.sol b/test/account/AccountReturnData.t.sol index eaf8b8a7..bb081325 100644 --- a/test/account/AccountReturnData.t.sol +++ b/test/account/AccountReturnData.t.sol @@ -10,7 +10,7 @@ import { ResultConsumerPlugin } from "../mocks/plugins/ReturnDataPluginMocks.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; -import {TEST_DEFAULT_OWNER_FUNCTION_ID} from "../utils/TestConstants.sol"; +import {TEST_DEFAULT_VALIDATION_ENTITY_ID} from "../utils/TestConstants.sol"; // Tests all the different ways that return data can be read from plugins through an account contract AccountReturnDataTest is AccountTestBase { @@ -58,7 +58,7 @@ contract AccountReturnDataTest is AccountTestBase { (address(regularResultContract), 0, abi.encodeCall(RegularResultContract.foo, ())) ), _encodeSignature( - PluginEntityLib.pack(address(singleOwnerPlugin), TEST_DEFAULT_OWNER_FUNCTION_ID), + PluginEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID), GLOBAL_VALIDATION, "" ) @@ -86,7 +86,7 @@ contract AccountReturnDataTest is AccountTestBase { bytes memory retData = account1.executeWithAuthorization( abi.encodeCall(account1.executeBatch, (calls)), _encodeSignature( - PluginEntityLib.pack(address(singleOwnerPlugin), TEST_DEFAULT_OWNER_FUNCTION_ID), + PluginEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID), GLOBAL_VALIDATION, "" ) diff --git a/test/account/GlobalValidationTest.t.sol b/test/account/GlobalValidationTest.t.sol index 433116bc..7ef7fdc5 100644 --- a/test/account/GlobalValidationTest.t.sol +++ b/test/account/GlobalValidationTest.t.sol @@ -26,7 +26,8 @@ contract GlobalValidationTest is AccountTestBase { account2 = UpgradeableModularAccount(payable(factory.getAddress(owner2, 0))); vm.deal(address(account2), 100 ether); - _ownerValidation = PluginEntityLib.pack(address(singleOwnerPlugin), TEST_DEFAULT_OWNER_FUNCTION_ID); + _signerValidation = + PluginEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID); ethRecipient = makeAddr("ethRecipient"); vm.deal(ethRecipient, 1 wei); @@ -48,7 +49,7 @@ contract GlobalValidationTest is AccountTestBase { // Generate signature bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner2Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = _encodeSignature(_ownerValidation, 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; @@ -65,7 +66,7 @@ contract GlobalValidationTest is AccountTestBase { vm.prank(owner2); account2.executeWithAuthorization( abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, "")), - _encodeSignature(_ownerValidation, GLOBAL_VALIDATION, "") + _encodeSignature(_signerValidation, GLOBAL_VALIDATION, "") ); assertEq(ethRecipient.balance, 2 wei); diff --git a/test/account/MultiValidation.t.sol b/test/account/MultiValidation.t.sol index 2dbae711..641fcefb 100644 --- a/test/account/MultiValidation.t.sol +++ b/test/account/MultiValidation.t.sol @@ -12,22 +12,22 @@ import {PluginEntity} from "../../src/interfaces/IPluginManager.sol"; import {IStandardExecutor} from "../../src/interfaces/IStandardExecutor.sol"; import {PluginEntityLib} from "../../src/helpers/PluginEntityLib.sol"; import {ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol"; -import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; +import {SingleSignerValidation} from "../../src/plugins/validation/SingleSignerValidation.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; -import {TEST_DEFAULT_OWNER_FUNCTION_ID} from "../utils/TestConstants.sol"; +import {TEST_DEFAULT_VALIDATION_ENTITY_ID} from "../utils/TestConstants.sol"; contract MultiValidationTest is AccountTestBase { using ECDSA for bytes32; using MessageHashUtils for bytes32; - SingleOwnerPlugin public validator2; + SingleSignerValidation public validator2; address public owner2; uint256 public owner2Key; function setUp() public { - validator2 = new SingleOwnerPlugin(); + validator2 = new SingleSignerValidation(); (owner2, owner2Key) = makeAddrAndKey("owner2"); } @@ -35,16 +35,16 @@ contract MultiValidationTest is AccountTestBase { function test_overlappingValidationInstall() public { vm.prank(address(entryPoint)); account1.installValidation( - ValidationConfigLib.pack(address(validator2), TEST_DEFAULT_OWNER_FUNCTION_ID, true, true), + ValidationConfigLib.pack(address(validator2), TEST_DEFAULT_VALIDATION_ENTITY_ID, true, true), new bytes4[](0), - abi.encode(TEST_DEFAULT_OWNER_FUNCTION_ID, owner2), + abi.encode(TEST_DEFAULT_VALIDATION_ENTITY_ID, owner2), "", "" ); PluginEntity[] memory validations = new PluginEntity[](2); - validations[0] = PluginEntityLib.pack(address(singleOwnerPlugin), TEST_DEFAULT_OWNER_FUNCTION_ID); - validations[1] = PluginEntityLib.pack(address(validator2), TEST_DEFAULT_OWNER_FUNCTION_ID); + validations[0] = PluginEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID); + validations[1] = PluginEntityLib.pack(address(validator2), TEST_DEFAULT_VALIDATION_ENTITY_ID); bytes4[] memory selectors0 = account1.getSelectors(validations[0]); bytes4[] memory selectors1 = account1.getSelectors(validations[1]); @@ -64,14 +64,14 @@ contract MultiValidationTest is AccountTestBase { abi.encodeWithSelector( UpgradeableModularAccount.RuntimeValidationFunctionReverted.selector, address(validator2), - 0, + 1, abi.encodeWithSignature("NotAuthorized()") ) ); account1.executeWithAuthorization( abi.encodeCall(IStandardExecutor.execute, (address(0), 0, "")), _encodeSignature( - PluginEntityLib.pack(address(validator2), TEST_DEFAULT_OWNER_FUNCTION_ID), GLOBAL_VALIDATION, "" + PluginEntityLib.pack(address(validator2), TEST_DEFAULT_VALIDATION_ENTITY_ID), GLOBAL_VALIDATION, "" ) ); @@ -79,7 +79,7 @@ contract MultiValidationTest is AccountTestBase { account1.executeWithAuthorization( abi.encodeCall(IStandardExecutor.execute, (address(0), 0, "")), _encodeSignature( - PluginEntityLib.pack(address(validator2), TEST_DEFAULT_OWNER_FUNCTION_ID), GLOBAL_VALIDATION, "" + PluginEntityLib.pack(address(validator2), TEST_DEFAULT_VALIDATION_ENTITY_ID), GLOBAL_VALIDATION, "" ) ); } @@ -105,7 +105,7 @@ contract MultiValidationTest is AccountTestBase { bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner2Key, userOpHash.toEthSignedMessageHash()); userOp.signature = _encodeSignature( - PluginEntityLib.pack(address(validator2), TEST_DEFAULT_OWNER_FUNCTION_ID), + PluginEntityLib.pack(address(validator2), TEST_DEFAULT_VALIDATION_ENTITY_ID), GLOBAL_VALIDATION, abi.encodePacked(r, s, v) ); @@ -120,7 +120,7 @@ contract MultiValidationTest is AccountTestBase { userOp.nonce = 1; (v, r, s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); userOp.signature = _encodeSignature( - PluginEntityLib.pack(address(validator2), TEST_DEFAULT_OWNER_FUNCTION_ID), + PluginEntityLib.pack(address(validator2), TEST_DEFAULT_VALIDATION_ENTITY_ID), GLOBAL_VALIDATION, abi.encodePacked(r, s, v) ); diff --git a/test/account/PerHookData.t.sol b/test/account/PerHookData.t.sol index 546a156c..bcb53171 100644 --- a/test/account/PerHookData.t.sol +++ b/test/account/PerHookData.t.sol @@ -37,8 +37,9 @@ contract PerHookDataTest is CustomValidationTestBase { PreValidationHookData[] memory preValidationHookData = new PreValidationHookData[](1); preValidationHookData[0] = PreValidationHookData({index: 0, validationData: abi.encodePacked(_counter)}); - userOp.signature = - _encodeSignature(_ownerValidation, GLOBAL_VALIDATION, preValidationHookData, abi.encodePacked(r, s, v)); + userOp.signature = _encodeSignature( + _signerValidation, GLOBAL_VALIDATION, preValidationHookData, abi.encodePacked(r, s, v) + ); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); userOps[0] = userOp; @@ -59,8 +60,9 @@ contract PerHookDataTest is CustomValidationTestBase { validationData: abi.encodePacked(address(0x1234123412341234123412341234123412341234)) }); - userOp.signature = - _encodeSignature(_ownerValidation, GLOBAL_VALIDATION, preValidationHookData, abi.encodePacked(r, s, v)); + userOp.signature = _encodeSignature( + _signerValidation, GLOBAL_VALIDATION, preValidationHookData, abi.encodePacked(r, s, v) + ); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); userOps[0] = userOp; @@ -80,7 +82,7 @@ contract PerHookDataTest is CustomValidationTestBase { (PackedUserOperation memory userOp, bytes32 userOpHash) = _getCounterUserOP(); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = _encodeSignature(_ownerValidation, 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; @@ -104,8 +106,9 @@ contract PerHookDataTest is CustomValidationTestBase { preValidationHookData[0] = PreValidationHookData({index: 0, validationData: abi.encodePacked(_counter)}); preValidationHookData[1] = PreValidationHookData({index: 1, validationData: abi.encodePacked(_counter)}); - userOp.signature = - _encodeSignature(_ownerValidation, GLOBAL_VALIDATION, preValidationHookData, abi.encodePacked(r, s, v)); + userOp.signature = _encodeSignature( + _signerValidation, GLOBAL_VALIDATION, preValidationHookData, abi.encodePacked(r, s, v) + ); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); userOps[0] = userOp; @@ -142,8 +145,9 @@ contract PerHookDataTest is CustomValidationTestBase { PreValidationHookData[] memory preValidationHookData = new PreValidationHookData[](1); preValidationHookData[0] = PreValidationHookData({index: 0, validationData: abi.encodePacked(beneficiary)}); - userOp.signature = - _encodeSignature(_ownerValidation, GLOBAL_VALIDATION, preValidationHookData, abi.encodePacked(r, s, v)); + userOp.signature = _encodeSignature( + _signerValidation, GLOBAL_VALIDATION, preValidationHookData, abi.encodePacked(r, s, v) + ); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); userOps[0] = userOp; @@ -166,8 +170,9 @@ contract PerHookDataTest is CustomValidationTestBase { PreValidationHookData[] memory preValidationHookData = new PreValidationHookData[](1); preValidationHookData[0] = PreValidationHookData({index: 0, validationData: ""}); - userOp.signature = - _encodeSignature(_ownerValidation, GLOBAL_VALIDATION, preValidationHookData, abi.encodePacked(r, s, v)); + userOp.signature = _encodeSignature( + _signerValidation, GLOBAL_VALIDATION, preValidationHookData, abi.encodePacked(r, s, v) + ); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); userOps[0] = userOp; @@ -195,7 +200,7 @@ contract PerHookDataTest is CustomValidationTestBase { UpgradeableModularAccount.execute, (address(_counter), 0 wei, abi.encodeCall(Counter.increment, ())) ), - _encodeSignature(_ownerValidation, GLOBAL_VALIDATION, preValidationHookData, "") + _encodeSignature(_signerValidation, GLOBAL_VALIDATION, preValidationHookData, "") ); assertEq(_counter.number(), 1); @@ -222,7 +227,7 @@ contract PerHookDataTest is CustomValidationTestBase { UpgradeableModularAccount.execute, (address(_counter), 0 wei, abi.encodeCall(Counter.increment, ())) ), - _encodeSignature(_ownerValidation, GLOBAL_VALIDATION, preValidationHookData, "") + _encodeSignature(_signerValidation, GLOBAL_VALIDATION, preValidationHookData, "") ); } @@ -241,7 +246,7 @@ contract PerHookDataTest is CustomValidationTestBase { UpgradeableModularAccount.execute, (address(_counter), 0 wei, abi.encodeCall(Counter.increment, ())) ), - _encodeSignature(_ownerValidation, GLOBAL_VALIDATION, "") + _encodeSignature(_signerValidation, GLOBAL_VALIDATION, "") ); } @@ -259,7 +264,7 @@ contract PerHookDataTest is CustomValidationTestBase { UpgradeableModularAccount.execute, (address(_counter), 0 wei, abi.encodeCall(Counter.increment, ())) ), - _encodeSignature(_ownerValidation, GLOBAL_VALIDATION, preValidationHookData, "") + _encodeSignature(_signerValidation, GLOBAL_VALIDATION, preValidationHookData, "") ); } @@ -280,7 +285,7 @@ contract PerHookDataTest is CustomValidationTestBase { ); account1.executeWithAuthorization( abi.encodeCall(UpgradeableModularAccount.execute, (beneficiary, 1 wei, "")), - _encodeSignature(_ownerValidation, GLOBAL_VALIDATION, preValidationHookData, "") + _encodeSignature(_signerValidation, GLOBAL_VALIDATION, preValidationHookData, "") ); } @@ -295,7 +300,7 @@ contract PerHookDataTest is CustomValidationTestBase { UpgradeableModularAccount.execute, (address(_counter), 0 wei, abi.encodeCall(Counter.increment, ())) ), - _encodeSignature(_ownerValidation, GLOBAL_VALIDATION, preValidationHookData, "") + _encodeSignature(_signerValidation, GLOBAL_VALIDATION, preValidationHookData, "") ); } @@ -341,11 +346,11 @@ contract PerHookDataTest is CustomValidationTestBase { bytes memory packedPreValidationHooks = abi.encode(preValidationHooks, preValidationHookData); return ( - _ownerValidation, + _signerValidation, true, true, new bytes4[](0), - abi.encode(TEST_DEFAULT_OWNER_FUNCTION_ID, owner1), + abi.encode(TEST_DEFAULT_VALIDATION_ENTITY_ID, owner1), packedPreValidationHooks, "" ); diff --git a/test/account/SelfCallAuthorization.t.sol b/test/account/SelfCallAuthorization.t.sol index 516f9f34..c490eea4 100644 --- a/test/account/SelfCallAuthorization.t.sol +++ b/test/account/SelfCallAuthorization.t.sol @@ -303,7 +303,7 @@ contract SelfCallAuthorizationTest is AccountTestBase { UpgradeableModularAccount.installValidation, (ValidationConfigLib.pack(comprehensivePluginValidation, false, false), selectors, "", "", "") ), - _encodeSignature(_ownerValidation, GLOBAL_VALIDATION, "") + _encodeSignature(_signerValidation, GLOBAL_VALIDATION, "") ); } diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index dda78c66..54af25c6 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -14,14 +14,14 @@ import {PluginManifest} from "../../src/interfaces/IPlugin.sol"; import {IAccountLoupe} from "../../src/interfaces/IAccountLoupe.sol"; import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; import {Call} from "../../src/interfaces/IStandardExecutor.sol"; -import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; +import {SingleSignerValidation} from "../../src/plugins/validation/SingleSignerValidation.sol"; import {TokenReceiverPlugin} from "../../src/plugins/TokenReceiverPlugin.sol"; import {Counter} from "../mocks/Counter.sol"; import {ComprehensivePlugin} from "../mocks/plugins/ComprehensivePlugin.sol"; import {MockPlugin} from "../mocks/MockPlugin.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; -import {TEST_DEFAULT_OWNER_FUNCTION_ID} from "../utils/TestConstants.sol"; +import {TEST_DEFAULT_VALIDATION_ENTITY_ID} from "../utils/TestConstants.sol"; contract UpgradeableModularAccountTest is AccountTestBase { using ECDSA for bytes32; @@ -77,7 +77,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { // Generate signature bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = _encodeSignature(_ownerValidation, 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; @@ -95,9 +95,9 @@ contract UpgradeableModularAccountTest is AccountTestBase { callData: abi.encodeCall( UpgradeableModularAccount.execute, ( - address(singleOwnerPlugin), + address(singleSignerValidation), 0, - abi.encodeCall(SingleOwnerPlugin.transferOwnership, (TEST_DEFAULT_OWNER_FUNCTION_ID, owner2)) + abi.encodeCall(SingleSignerValidation.transferSigner, (TEST_DEFAULT_VALIDATION_ENTITY_ID, owner2)) ) ), accountGasLimits: _encodeGas(VERIFICATION_GAS_LIMIT, CALL_GAS_LIMIT), @@ -110,7 +110,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { // Generate signature bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner2Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = _encodeSignature(_ownerValidation, 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; @@ -136,7 +136,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { // Generate signature bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner2Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = _encodeSignature(_ownerValidation, 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; @@ -162,7 +162,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { // Generate signature bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = _encodeSignature(_ownerValidation, 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; @@ -190,7 +190,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { // Generate signature bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = _encodeSignature(_ownerValidation, 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; @@ -221,7 +221,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { // Generate signature bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = _encodeSignature(_ownerValidation, 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; @@ -409,16 +409,16 @@ contract UpgradeableModularAccountTest is AccountTestBase { } function test_transferOwnership() public { - assertEq(singleOwnerPlugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, address(account1)), owner1); + assertEq(singleSignerValidation.signerOf(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), owner1); vm.prank(address(entryPoint)); account1.execute( - address(singleOwnerPlugin), + address(singleSignerValidation), 0, - abi.encodeCall(SingleOwnerPlugin.transferOwnership, (TEST_DEFAULT_OWNER_FUNCTION_ID, owner2)) + abi.encodeCall(SingleSignerValidation.transferSigner, (TEST_DEFAULT_VALIDATION_ENTITY_ID, owner2)) ); - assertEq(singleOwnerPlugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, address(account1)), owner2); + assertEq(singleSignerValidation.signerOf(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), owner2); } function test_isValidSignature() public { @@ -426,10 +426,10 @@ contract UpgradeableModularAccountTest is AccountTestBase { (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, message); - // singleOwnerPlugin.ownerOf(address(account1)); + // singleSignerValidation.ownerOf(address(account1)); bytes memory signature = - abi.encodePacked(address(singleOwnerPlugin), TEST_DEFAULT_OWNER_FUNCTION_ID, r, s, v); + abi.encodePacked(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID, r, s, v); bytes4 validationResult = IERC1271(address(account1)).isValidSignature(message, signature); diff --git a/test/mocks/MSCAFactoryFixture.sol b/test/mocks/MSCAFactoryFixture.sol deleted file mode 100644 index 8ca3a51f..00000000 --- a/test/mocks/MSCAFactoryFixture.sol +++ /dev/null @@ -1,81 +0,0 @@ -// SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.19; - -import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; -import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; -import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; - -import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; -import {ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol"; -import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; - -import {OptimizedTest} from "../utils/OptimizedTest.sol"; -import {TEST_DEFAULT_OWNER_FUNCTION_ID} from "../utils/TestConstants.sol"; - -/** - * @title MSCAFactoryFixture - * @dev a factory that initializes UpgradeableModularAccounts with a single plugin, SingleOwnerPlugin - * intended for unit tests and local development, not for production. - */ -contract MSCAFactoryFixture is OptimizedTest { - UpgradeableModularAccount public accountImplementation; - SingleOwnerPlugin public singleOwnerPlugin; - bytes32 private immutable _PROXY_BYTECODE_HASH; - - uint32 public constant UNSTAKE_DELAY = 1 weeks; - - IEntryPoint public entryPoint; - - constructor(IEntryPoint _entryPoint, SingleOwnerPlugin _singleOwnerPlugin) { - entryPoint = _entryPoint; - accountImplementation = _deployUpgradeableModularAccount(_entryPoint); - _PROXY_BYTECODE_HASH = keccak256( - abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(address(accountImplementation), "")) - ); - singleOwnerPlugin = _singleOwnerPlugin; - } - - /** - * create an account, and return its address. - * returns the address even if the account is already deployed. - * Note that during user operation execution, this method is called only if the account is not deployed. - * This method returns an existing account address so that entryPoint.getSenderAddress() would work even after - * account creation - */ - function createAccount(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) { - bytes memory pluginInstallData = abi.encode(TEST_DEFAULT_OWNER_FUNCTION_ID, owner); - // not necessary to check return addr since next call will fail if so - new ERC1967Proxy{salt: getSalt(owner, salt)}(address(accountImplementation), ""); - - // point proxy to actual implementation and init plugins - UpgradeableModularAccount(payable(addr)).initializeWithValidation( - ValidationConfigLib.pack(address(singleOwnerPlugin), TEST_DEFAULT_OWNER_FUNCTION_ID, true, true), - new bytes4[](0), - pluginInstallData, - "", - "" - ); - } - - return UpgradeableModularAccount(payable(addr)); - } - - /** - * calculate the counterfactual address of this account as it would be returned by createAccount() - */ - function getAddress(address owner, uint256 salt) public view returns (address) { - return Create2.computeAddress(getSalt(owner, salt), _PROXY_BYTECODE_HASH); - } - - function addStake() external payable { - entryPoint.addStake{value: msg.value}(UNSTAKE_DELAY); - } - - function getSalt(address owner, uint256 salt) public pure returns (bytes32) { - return keccak256(abi.encodePacked(owner, salt)); - } -} diff --git a/test/mocks/SingleSignerFactoryFixture.sol b/test/mocks/SingleSignerFactoryFixture.sol index c3f663e3..b3da73ec 100644 --- a/test/mocks/SingleSignerFactoryFixture.sol +++ b/test/mocks/SingleSignerFactoryFixture.sol @@ -10,7 +10,7 @@ import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAcc import {SingleSignerValidation} from "../../src/plugins/validation/SingleSignerValidation.sol"; import {OptimizedTest} from "../utils/OptimizedTest.sol"; -import {TEST_DEFAULT_VALIDATION_ID} from "../utils/TestConstants.sol"; +import {TEST_DEFAULT_VALIDATION_ENTITY_ID} from "../utils/TestConstants.sol"; contract SingleSignerFactoryFixture is OptimizedTest { UpgradeableModularAccount public accountImplementation; @@ -45,13 +45,15 @@ contract SingleSignerFactoryFixture is OptimizedTest { // short circuit if exists if (addr.code.length == 0) { - bytes memory pluginInstallData = abi.encode(TEST_DEFAULT_VALIDATION_ID, owner); + bytes memory pluginInstallData = abi.encode(TEST_DEFAULT_VALIDATION_ENTITY_ID, owner); // not necessary to check return addr since next call will fail if so new ERC1967Proxy{salt: getSalt(owner, salt)}(address(accountImplementation), ""); // point proxy to actual implementation and init plugins UpgradeableModularAccount(payable(addr)).initializeWithValidation( - ValidationConfigLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ID, true, false), + ValidationConfigLib.pack( + address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID, true, true + ), new bytes4[](0), pluginInstallData, "", diff --git a/test/plugin/SingleOwnerPlugin.t.sol b/test/plugin/SingleOwnerPlugin.t.sol deleted file mode 100644 index 6d57e5b7..00000000 --- a/test/plugin/SingleOwnerPlugin.t.sol +++ /dev/null @@ -1,186 +0,0 @@ -// SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.19; - -import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.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 {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; - -import {ContractOwner} from "../mocks/ContractOwner.sol"; -import {OptimizedTest} from "../utils/OptimizedTest.sol"; -import {TEST_DEFAULT_OWNER_FUNCTION_ID} from "../utils/TestConstants.sol"; - -contract SingleOwnerPluginTest is OptimizedTest { - using ECDSA for bytes32; - using MessageHashUtils for bytes32; - - SingleOwnerPlugin public plugin; - EntryPoint public entryPoint; - - bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e; - address public a; - address public b; - - address public owner1; - address public owner2; - ContractOwner public contractOwner; - - // Event declarations (needed for vm.expectEmit) - event OwnershipTransferred(address indexed account, address indexed previousOwner, address indexed newOwner); - - function setUp() public { - plugin = _deploySingleOwnerPlugin(); - entryPoint = new EntryPoint(); - - a = makeAddr("a"); - b = makeAddr("b"); - owner1 = makeAddr("owner1"); - owner2 = makeAddr("owner2"); - contractOwner = new ContractOwner(); - } - - // Tests: - // - uninitialized owner is zero address - // - transferOwnership result is returned via owner afterwards - // - transferOwnership emits OwnershipTransferred event - // - owner() returns correct value after transferOwnership - // - owner() does not return a different account's owner - // - requireFromOwner succeeds when called by owner - // - requireFromOwner reverts when called by non-owner - - function test_uninitializedOwner() public { - vm.startPrank(a); - assertEq(address(0), plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); - } - - function test_ownerInitialization() public { - vm.startPrank(a); - assertEq(address(0), plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); - plugin.transferOwnership(TEST_DEFAULT_OWNER_FUNCTION_ID, owner1); - assertEq(owner1, plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); - } - - function test_ownerInitializationEvent() public { - vm.startPrank(a); - assertEq(address(0), plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); - - vm.expectEmit(true, true, true, true); - emit OwnershipTransferred(a, address(0), owner1); - - plugin.transferOwnership(TEST_DEFAULT_OWNER_FUNCTION_ID, owner1); - assertEq(owner1, plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); - } - - function test_ownerMigration() public { - vm.startPrank(a); - assertEq(address(0), plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); - plugin.transferOwnership(TEST_DEFAULT_OWNER_FUNCTION_ID, owner1); - assertEq(owner1, plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); - plugin.transferOwnership(TEST_DEFAULT_OWNER_FUNCTION_ID, owner2); - assertEq(owner2, plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); - } - - function test_ownerMigrationEvents() public { - vm.startPrank(a); - assertEq(address(0), plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); - - vm.expectEmit(true, true, true, true); - emit OwnershipTransferred(a, address(0), owner1); - - plugin.transferOwnership(TEST_DEFAULT_OWNER_FUNCTION_ID, owner1); - assertEq(owner1, plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); - - vm.expectEmit(true, true, true, true); - emit OwnershipTransferred(a, owner1, owner2); - - plugin.transferOwnership(TEST_DEFAULT_OWNER_FUNCTION_ID, owner2); - assertEq(owner2, plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); - } - - function test_ownerForSender() public { - vm.startPrank(a); - assertEq(address(0), plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); - plugin.transferOwnership(TEST_DEFAULT_OWNER_FUNCTION_ID, owner1); - assertEq(owner1, plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); - vm.startPrank(b); - assertEq(address(0), plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, b)); - plugin.transferOwnership(TEST_DEFAULT_OWNER_FUNCTION_ID, owner2); - assertEq(owner2, plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, b)); - } - - function test_requireOwner() public { - vm.startPrank(a); - assertEq(address(0), plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); - plugin.transferOwnership(TEST_DEFAULT_OWNER_FUNCTION_ID, owner1); - assertEq(owner1, plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); - plugin.validateRuntime(a, TEST_DEFAULT_OWNER_FUNCTION_ID, owner1, 0, "", ""); - - vm.startPrank(b); - vm.expectRevert(SingleOwnerPlugin.NotAuthorized.selector); - plugin.validateRuntime(b, TEST_DEFAULT_OWNER_FUNCTION_ID, owner1, 0, "", ""); - } - - function testFuzz_validateUserOpSig(string memory salt, PackedUserOperation memory userOp) public { - // range bound the possible set of priv keys - (address signer, uint256 privateKey) = makeAddrAndKey(salt); - - vm.startPrank(a); - bytes32 userOpHash = entryPoint.getUserOpHash(userOp); - (uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, userOpHash.toEthSignedMessageHash()); - - // sig cannot cover the whole userop struct since userop struct has sig field - userOp.signature = abi.encodePacked(r, s, v); - - // sig check should fail - uint256 success = plugin.validateUserOp(TEST_DEFAULT_OWNER_FUNCTION_ID, userOp, userOpHash); - assertEq(success, 1); - - // transfer ownership to signer - plugin.transferOwnership(TEST_DEFAULT_OWNER_FUNCTION_ID, signer); - assertEq(signer, plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); - - // sig check should pass - success = plugin.validateUserOp(TEST_DEFAULT_OWNER_FUNCTION_ID, userOp, userOpHash); - assertEq(success, 0); - } - - function testFuzz_isValidSignatureForEOAOwner(string memory salt, bytes32 digest) public { - // range bound the possible set of priv keys - (address signer, uint256 privateKey) = makeAddrAndKey(salt); - - vm.startPrank(a); - (uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, digest); - - // sig check should fail - assertEq( - plugin.validateSignature( - a, TEST_DEFAULT_OWNER_FUNCTION_ID, address(this), digest, abi.encodePacked(r, s, v) - ), - bytes4(0xFFFFFFFF) - ); - - // transfer ownership to signer - plugin.transferOwnership(TEST_DEFAULT_OWNER_FUNCTION_ID, signer); - assertEq(signer, plugin.owners(TEST_DEFAULT_OWNER_FUNCTION_ID, a)); - - // sig check should pass - assertEq( - plugin.validateSignature( - a, TEST_DEFAULT_OWNER_FUNCTION_ID, address(this), digest, abi.encodePacked(r, s, v) - ), - _1271_MAGIC_VALUE - ); - } - - function testFuzz_isValidSignatureForContractOwner(bytes32 digest) public { - vm.startPrank(a); - plugin.transferOwnership(TEST_DEFAULT_OWNER_FUNCTION_ID, address(contractOwner)); - bytes memory signature = contractOwner.sign(digest); - assertEq( - plugin.validateSignature(a, TEST_DEFAULT_OWNER_FUNCTION_ID, address(this), digest, signature), - _1271_MAGIC_VALUE - ); - } -} diff --git a/test/plugin/TokenReceiverPlugin.t.sol b/test/plugin/TokenReceiverPlugin.t.sol index 2f52a988..1e198f0b 100644 --- a/test/plugin/TokenReceiverPlugin.t.sol +++ b/test/plugin/TokenReceiverPlugin.t.sol @@ -8,7 +8,7 @@ import {IERC1155Receiver} from "@openzeppelin/contracts/token/ERC1155/IERC1155Re import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {TokenReceiverPlugin} from "../../src/plugins/TokenReceiverPlugin.sol"; -import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; +import {SingleSignerFactoryFixture} from "../mocks/SingleSignerFactoryFixture.sol"; import {MockERC721} from "../mocks/MockERC721.sol"; import {MockERC1155} from "../mocks/MockERC1155.sol"; import {OptimizedTest} from "../utils/OptimizedTest.sol"; @@ -33,7 +33,8 @@ contract TokenReceiverPluginTest is OptimizedTest, IERC1155Receiver { function setUp() public { entryPoint = new EntryPoint(); - MSCAFactoryFixture factory = new MSCAFactoryFixture(entryPoint, _deploySingleOwnerPlugin()); + SingleSignerFactoryFixture factory = + new SingleSignerFactoryFixture(entryPoint, _deploySingleSignerValidation()); acct = factory.createAccount(address(this), 0); plugin = _deployTokenReceiverPlugin(); diff --git a/test/samples/AllowlistPlugin.t.sol b/test/samples/AllowlistPlugin.t.sol index 4a8fc4ff..441fbdcb 100644 --- a/test/samples/AllowlistPlugin.t.sol +++ b/test/samples/AllowlistPlugin.t.sol @@ -305,11 +305,11 @@ contract AllowlistPluginTest is CustomValidationTestBase { bytes memory packedPreValidationHooks = abi.encode(preValidationHooks, preValidationHookData); return ( - _ownerValidation, + _signerValidation, true, true, new bytes4[](0), - abi.encode(TEST_DEFAULT_OWNER_FUNCTION_ID, owner1), + abi.encode(TEST_DEFAULT_VALIDATION_ENTITY_ID, owner1), packedPreValidationHooks, "" ); diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index a23f0151..526365d0 100644 --- a/test/utils/AccountTestBase.sol +++ b/test/utils/AccountTestBase.sol @@ -9,39 +9,36 @@ import {SingleSignerValidation} from "../../src/plugins/validation/SingleSignerV import {PluginEntity, PluginEntityLib} from "../../src/helpers/PluginEntityLib.sol"; import {IStandardExecutor, Call} from "../../src/interfaces/IStandardExecutor.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; -import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {OptimizedTest} from "./OptimizedTest.sol"; -import {TEST_DEFAULT_OWNER_FUNCTION_ID as EXT_CONST_TEST_DEFAULT_OWNER_FUNCTION_ID} from "./TestConstants.sol"; +import {TEST_DEFAULT_VALIDATION_ENTITY_ID as EXT_CONST_TEST_DEFAULT_VALIDATION_ENTITY_ID} from + "./TestConstants.sol"; import {SingleSignerFactoryFixture} from "../mocks/SingleSignerFactoryFixture.sol"; -import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; /// @dev This contract handles common boilerplate setup for tests using UpgradeableModularAccount with -/// SingleOwnerPlugin. +/// SingleSignerValidation. abstract contract AccountTestBase is OptimizedTest { using PluginEntityLib for PluginEntity; using MessageHashUtils for bytes32; EntryPoint public entryPoint; address payable public beneficiary; - SingleOwnerPlugin public singleOwnerPlugin; - MSCAFactoryFixture public factory; SingleSignerValidation public singleSignerValidation; - SingleSignerFactoryFixture public singleSignerFactory; + SingleSignerFactoryFixture public factory; address public owner1; uint256 public owner1Key; UpgradeableModularAccount public account1; - PluginEntity internal _ownerValidation; + PluginEntity internal _signerValidation; uint8 public constant SELECTOR_ASSOCIATED_VALIDATION = 0; uint8 public constant GLOBAL_VALIDATION = 1; // Re-declare the constant to prevent derived test contracts from having to import it - uint32 public constant TEST_DEFAULT_OWNER_FUNCTION_ID = EXT_CONST_TEST_DEFAULT_OWNER_FUNCTION_ID; + uint32 public constant TEST_DEFAULT_VALIDATION_ENTITY_ID = EXT_CONST_TEST_DEFAULT_VALIDATION_ENTITY_ID; uint256 public constant CALL_GAS_LIMIT = 100000; uint256 public constant VERIFICATION_GAS_LIMIT = 1200000; @@ -56,15 +53,14 @@ abstract contract AccountTestBase is OptimizedTest { (owner1, owner1Key) = makeAddrAndKey("owner1"); beneficiary = payable(makeAddr("beneficiary")); - singleOwnerPlugin = _deploySingleOwnerPlugin(); - factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); singleSignerValidation = _deploySingleSignerValidation(); - singleSignerFactory = new SingleSignerFactoryFixture(entryPoint, singleSignerValidation); + factory = new SingleSignerFactoryFixture(entryPoint, singleSignerValidation); account1 = factory.createAccount(owner1, 0); vm.deal(address(account1), 100 ether); - _ownerValidation = PluginEntityLib.pack(address(singleOwnerPlugin), TEST_DEFAULT_OWNER_FUNCTION_ID); + _signerValidation = + PluginEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID); } function _runExecUserOp(address target, bytes memory callData) internal { @@ -107,7 +103,7 @@ abstract contract AccountTestBase is OptimizedTest { (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); userOp.signature = _encodeSignature( - PluginEntityLib.pack(address(singleOwnerPlugin), TEST_DEFAULT_OWNER_FUNCTION_ID), + PluginEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID), GLOBAL_VALIDATION, abi.encodePacked(r, s, v) ); @@ -160,7 +156,7 @@ abstract contract AccountTestBase is OptimizedTest { account1.executeWithAuthorization( callData, _encodeSignature( - PluginEntityLib.pack(address(singleOwnerPlugin), TEST_DEFAULT_OWNER_FUNCTION_ID), + PluginEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID), GLOBAL_VALIDATION, "" ) @@ -175,7 +171,7 @@ abstract contract AccountTestBase is OptimizedTest { account1.executeWithAuthorization( callData, _encodeSignature( - PluginEntityLib.pack(address(singleOwnerPlugin), TEST_DEFAULT_OWNER_FUNCTION_ID), + PluginEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID), GLOBAL_VALIDATION, "" ) @@ -189,15 +185,15 @@ abstract contract AccountTestBase is OptimizedTest { abi.encodeCall( account1.execute, ( - address(singleOwnerPlugin), + address(singleSignerValidation), 0, abi.encodeCall( - SingleOwnerPlugin.transferOwnership, (TEST_DEFAULT_OWNER_FUNCTION_ID, address(this)) + SingleSignerValidation.transferSigner, (TEST_DEFAULT_VALIDATION_ENTITY_ID, address(this)) ) ) ), _encodeSignature( - PluginEntityLib.pack(address(singleOwnerPlugin), TEST_DEFAULT_OWNER_FUNCTION_ID), + PluginEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID), GLOBAL_VALIDATION, "" ) diff --git a/test/utils/OptimizedTest.sol b/test/utils/OptimizedTest.sol index f5e66dfc..d884193f 100644 --- a/test/utils/OptimizedTest.sol +++ b/test/utils/OptimizedTest.sol @@ -6,7 +6,6 @@ import {Test} from "forge-std/Test.sol"; import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; -import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {SingleSignerValidation} from "../../src/plugins/validation/SingleSignerValidation.sol"; import {TokenReceiverPlugin} from "../../src/plugins/TokenReceiverPlugin.sol"; @@ -45,12 +44,6 @@ abstract contract OptimizedTest is Test { : new UpgradeableModularAccount(entryPoint); } - function _deploySingleOwnerPlugin() internal returns (SingleOwnerPlugin) { - return _isOptimizedTest() - ? SingleOwnerPlugin(deployCode("out-optimized/SingleOwnerPlugin.sol/SingleOwnerPlugin.json")) - : new SingleOwnerPlugin(); - } - function _deployTokenReceiverPlugin() internal returns (TokenReceiverPlugin) { return _isOptimizedTest() ? TokenReceiverPlugin(deployCode("out-optimized/TokenReceiverPlugin.sol/TokenReceiverPlugin.json")) diff --git a/test/utils/TestConstants.sol b/test/utils/TestConstants.sol index 1bd467ad..c15b2dd3 100644 --- a/test/utils/TestConstants.sol +++ b/test/utils/TestConstants.sol @@ -1,5 +1,4 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.25; -uint32 constant TEST_DEFAULT_OWNER_FUNCTION_ID = 0; -uint32 constant TEST_DEFAULT_VALIDATION_ID = 1; +uint32 constant TEST_DEFAULT_VALIDATION_ENTITY_ID = 1; diff --git a/test/validation/SingleSignerValidation.t.sol b/test/validation/SingleSignerValidation.t.sol index af2b1c6e..a983ea93 100644 --- a/test/validation/SingleSignerValidation.t.sol +++ b/test/validation/SingleSignerValidation.t.sol @@ -9,7 +9,7 @@ import {PluginEntityLib} from "../../src/helpers/PluginEntityLib.sol"; import {ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; -import {TEST_DEFAULT_VALIDATION_ID} from "../utils/TestConstants.sol"; +import {TEST_DEFAULT_VALIDATION_ENTITY_ID} from "../utils/TestConstants.sol"; import {ContractOwner} from "../mocks/ContractOwner.sol"; contract SingleSignerValidationTest is AccountTestBase { @@ -27,7 +27,7 @@ contract SingleSignerValidationTest is AccountTestBase { function setUp() public { ethRecipient = makeAddr("ethRecipient"); (owner2, owner2Key) = makeAddrAndKey("owner2"); - account = singleSignerFactory.createAccount(owner1, 0); + account = factory.createAccount(owner1, 0); vm.deal(address(account), 100 ether); contractOwner = new ContractOwner(); @@ -50,7 +50,7 @@ contract SingleSignerValidationTest is AccountTestBase { bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); userOp.signature = _encodeSignature( - PluginEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ID), + PluginEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID), GLOBAL_VALIDATION, abi.encodePacked(r, s, v) ); @@ -68,7 +68,7 @@ contract SingleSignerValidationTest is AccountTestBase { account.executeWithAuthorization( abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, "")), _encodeSignature( - PluginEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ID), + PluginEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID), GLOBAL_VALIDATION, "" ) @@ -77,7 +77,7 @@ contract SingleSignerValidationTest is AccountTestBase { } function test_runtime_with2SameValidationInstalled() public { - uint32 newEntityId = TEST_DEFAULT_OWNER_FUNCTION_ID + 1; + uint32 newEntityId = TEST_DEFAULT_VALIDATION_ENTITY_ID + 1; vm.prank(address(entryPoint)); account.installValidation( ValidationConfigLib.pack(address(singleSignerValidation), newEntityId, true, false), @@ -109,19 +109,19 @@ contract SingleSignerValidationTest is AccountTestBase { // sig check should fail assertEq( singleSignerValidation.validateSignature( - accountAddr, TEST_DEFAULT_OWNER_FUNCTION_ID, address(this), digest, abi.encodePacked(r, s, v) + accountAddr, TEST_DEFAULT_VALIDATION_ENTITY_ID, address(this), digest, abi.encodePacked(r, s, v) ), bytes4(0xFFFFFFFF) ); // transfer ownership to signer - singleSignerValidation.transferSigner(TEST_DEFAULT_OWNER_FUNCTION_ID, signer); - assertEq(signer, singleSignerValidation.signerOf(TEST_DEFAULT_OWNER_FUNCTION_ID, accountAddr)); + singleSignerValidation.transferSigner(TEST_DEFAULT_VALIDATION_ENTITY_ID, signer); + assertEq(signer, singleSignerValidation.signerOf(TEST_DEFAULT_VALIDATION_ENTITY_ID, accountAddr)); // sig check should pass assertEq( singleSignerValidation.validateSignature( - accountAddr, TEST_DEFAULT_OWNER_FUNCTION_ID, address(this), digest, abi.encodePacked(r, s, v) + accountAddr, TEST_DEFAULT_VALIDATION_ENTITY_ID, address(this), digest, abi.encodePacked(r, s, v) ), _1271_MAGIC_VALUE ); @@ -130,11 +130,11 @@ contract SingleSignerValidationTest is AccountTestBase { function testFuzz_isValidSignatureForContractOwner(bytes32 digest) public { address accountAddr = address(account); vm.startPrank(accountAddr); - singleSignerValidation.transferSigner(TEST_DEFAULT_OWNER_FUNCTION_ID, address(contractOwner)); + singleSignerValidation.transferSigner(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(contractOwner)); bytes memory signature = contractOwner.sign(digest); assertEq( singleSignerValidation.validateSignature( - accountAddr, TEST_DEFAULT_OWNER_FUNCTION_ID, address(this), digest, signature + accountAddr, TEST_DEFAULT_VALIDATION_ENTITY_ID, address(this), digest, signature ), _1271_MAGIC_VALUE );