From add996164f876f6673fc0ddd44005542b566812c Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 12 Jul 2024 01:56:09 +0200 Subject: [PATCH] identify / multisig with ERC1271 --- .../modules/recovery/AccountAllSignatures.sol | 1 - .../modules/recovery/AccountERC1271.sol | 4 +- .../abstraction/mocks/AdvancedAccount.sol | 44 +------------------ test/abstraction/accountECDSA.test.js | 19 +++++--- test/abstraction/accountERC1271.test.js | 19 ++++---- test/abstraction/accountMultisig.test.js | 36 +++++++++------ test/helpers/erc4337.js | 38 +++++++++++----- test/helpers/identity.js | 43 ++++++++++++++++++ test/helpers/p256.js | 2 +- 9 files changed, 119 insertions(+), 87 deletions(-) create mode 100644 test/helpers/identity.js diff --git a/contracts/abstraction/account/modules/recovery/AccountAllSignatures.sol b/contracts/abstraction/account/modules/recovery/AccountAllSignatures.sol index 5bfd7e78c6d..471a9b680e4 100644 --- a/contracts/abstraction/account/modules/recovery/AccountAllSignatures.sol +++ b/contracts/abstraction/account/modules/recovery/AccountAllSignatures.sol @@ -17,7 +17,6 @@ abstract contract AccountAllSignatures is AccountECDSA, AccountERC1271 { bytes32 userOpHash ) internal virtual override(AccountECDSA, AccountERC1271) returns (address) { (SignatureType sigType, bytes memory sigData) = abi.decode(signature, (SignatureType, bytes)); - if (sigType == SignatureType.ECDSA) { return AccountECDSA._recoverSigner(sigData, userOpHash); } else if (sigType == SignatureType.ERC1271) { diff --git a/contracts/abstraction/account/modules/recovery/AccountERC1271.sol b/contracts/abstraction/account/modules/recovery/AccountERC1271.sol index 91df58ed346..bc2eb152fd7 100644 --- a/contracts/abstraction/account/modules/recovery/AccountERC1271.sol +++ b/contracts/abstraction/account/modules/recovery/AccountERC1271.sol @@ -14,8 +14,6 @@ abstract contract AccountERC1271 is Account { bytes32 msgHash = MessageHashUtils.toEthSignedMessageHash(userOpHash); (address signer, bytes memory sig) = abi.decode(signature, (address, bytes)); - return SignatureChecker.isValidERC1271SignatureNow(signer, msgHash, sig) - ? signer - : address(0); + return SignatureChecker.isValidERC1271SignatureNow(signer, msgHash, sig) ? signer : address(0); } } diff --git a/contracts/abstraction/mocks/AdvancedAccount.sol b/contracts/abstraction/mocks/AdvancedAccount.sol index 642ebdf12bf..dc4dd237a16 100644 --- a/contracts/abstraction/mocks/AdvancedAccount.sol +++ b/contracts/abstraction/mocks/AdvancedAccount.sol @@ -8,49 +8,9 @@ import {ERC1155Holder} from "../../token/ERC1155/utils/ERC1155Holder.sol"; import {Account} from "../account/Account.sol"; import {AccountCommon} from "../account/AccountCommon.sol"; import {AccountMultisig} from "../account/modules/AccountMultisig.sol"; -import {AccountECDSA} from "../account/modules/recovery/AccountECDSA.sol"; -import {AccountERC1271} from "../account/modules/recovery/AccountERC1271.sol"; +import {AccountAllSignatures} from "../account/modules/recovery/AccountAllSignatures.sol"; -contract AdvancedAccountECDSA is AccessControl, AccountCommon, AccountECDSA, AccountMultisig { - bytes32 public constant SIGNER_ROLE = keccak256("SIGNER_ROLE"); - uint256 private _requiredSignatures; - - constructor( - IEntryPoint entryPoint_, - address admin_, - address[] memory signers_, - uint256 requiredSignatures_ - ) AccountCommon(entryPoint_) { - _grantRole(DEFAULT_ADMIN_ROLE, admin_); - for (uint256 i = 0; i < signers_.length; ++i) { - _grantRole(SIGNER_ROLE, signers_[i]); - } - _requiredSignatures = requiredSignatures_; - } - - function supportsInterface( - bytes4 interfaceId - ) public view virtual override(AccessControl, ERC1155Holder) returns (bool) { - return super.supportsInterface(interfaceId); - } - - function requiredSignatures() public view virtual override returns (uint256) { - return _requiredSignatures; - } - - function _isAuthorized(address user) internal view virtual override returns (bool) { - return hasRole(SIGNER_ROLE, user); - } - - function _processSignature( - bytes memory signature, - bytes32 userOpHash - ) internal virtual override(Account, AccountMultisig) returns (bool, address, uint48, uint48) { - return super._processSignature(signature, userOpHash); - } -} - -contract AdvancedAccountERC1271 is AccessControl, AccountCommon, AccountERC1271, AccountMultisig { +contract AdvancedAccount is AccessControl, AccountCommon, AccountAllSignatures, AccountMultisig { bytes32 public constant SIGNER_ROLE = keccak256("SIGNER_ROLE"); uint256 private _requiredSignatures; diff --git a/test/abstraction/accountECDSA.test.js b/test/abstraction/accountECDSA.test.js index 8c7fb875820..190bf70763c 100644 --- a/test/abstraction/accountECDSA.test.js +++ b/test/abstraction/accountECDSA.test.js @@ -3,16 +3,23 @@ const { expect } = require('chai'); const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); const { ERC4337Helper } = require('../helpers/erc4337'); +const { IdentityHelper } = require('../helpers/identity'); async function fixture() { const accounts = await ethers.getSigners(); - accounts.user = accounts.shift(); + accounts.relayer = accounts.shift(); accounts.beneficiary = accounts.shift(); - const target = await ethers.deployContract('CallReceiverMock'); + // 4337 helper const helper = new ERC4337Helper('SimpleAccountECDSA'); - await helper.wait(); - const sender = await helper.newAccount(accounts.user); + const identity = new IdentityHelper(); + + // environment + const target = await ethers.deployContract('CallReceiverMock'); + + // create 4337 account controlled by ECDSA + const signer = await identity.newECDSASigner(); + const sender = await helper.newAccount(signer); return { accounts, @@ -31,7 +38,7 @@ describe('AccountECDSA', function () { describe('execute operation', function () { beforeEach('fund account', async function () { - await this.accounts.user.sendTransaction({ to: this.sender, value: ethers.parseEther('1') }); + await this.accounts.relayer.sendTransaction({ to: this.sender, value: ethers.parseEther('1') }); }); describe('account not deployed yet', function () { @@ -57,7 +64,7 @@ describe('AccountECDSA', function () { describe('account already deployed', function () { beforeEach(async function () { - await this.sender.deploy(); + await this.sender.deploy(this.accounts.relayer); }); it('success: call', async function () { diff --git a/test/abstraction/accountERC1271.test.js b/test/abstraction/accountERC1271.test.js index a16dfb6f752..14dfbea0845 100644 --- a/test/abstraction/accountERC1271.test.js +++ b/test/abstraction/accountERC1271.test.js @@ -3,26 +3,22 @@ const { expect } = require('chai'); const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); const { ERC4337Helper } = require('../helpers/erc4337'); -const { P256Signer } = require('../helpers/p256'); +const { IdentityHelper } = require('../helpers/identity'); async function fixture() { const accounts = await ethers.getSigners(); - accounts.user = accounts.shift(); + accounts.relayer = accounts.shift(); accounts.beneficiary = accounts.shift(); // 4337 helper const helper = new ERC4337Helper('SimpleAccountERC1271'); - await helper.wait(); + const identity = new IdentityHelper(); // environment const target = await ethers.deployContract('CallReceiverMock'); - const identifyFactory = await ethers.deployContract('IdentityP256Factory'); - // create P256 key and identity contract - const signer = P256Signer.random(); - signer.address = await identifyFactory.predict(signer.publicKey); // override address of the signer - signer.sigParams.prefixAddress = true; - await identifyFactory.create(signer.publicKey); + // create 4337 account controlled by P256 + const signer = await identity.newP256Signer(); const sender = await helper.newAccount(signer); return { @@ -31,6 +27,7 @@ async function fixture() { helper, entrypoint: helper.entrypoint, factory: helper.factory, + signer, sender, }; } @@ -42,7 +39,7 @@ describe('AccountERC1271', function () { describe('execute operation', function () { beforeEach('fund account', async function () { - await this.accounts.user.sendTransaction({ to: this.sender, value: ethers.parseEther('1') }); + await this.accounts.relayer.sendTransaction({ to: this.sender, value: ethers.parseEther('1') }); }); describe('account not deployed yet', function () { @@ -68,7 +65,7 @@ describe('AccountERC1271', function () { describe('account already deployed', function () { beforeEach(async function () { - await this.sender.deploy(this.accounts.user); + await this.sender.deploy(this.accounts.relayer); }); it('success: call', async function () { diff --git a/test/abstraction/accountMultisig.test.js b/test/abstraction/accountMultisig.test.js index 1f0c33320a9..44562f9771b 100644 --- a/test/abstraction/accountMultisig.test.js +++ b/test/abstraction/accountMultisig.test.js @@ -3,19 +3,28 @@ const { expect } = require('chai'); const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); const { ERC4337Helper } = require('../helpers/erc4337'); +const { IdentityHelper } = require('../helpers/identity'); async function fixture() { const accounts = await ethers.getSigners(); - accounts.user = accounts.shift(); + accounts.relayer = accounts.shift(); accounts.beneficiary = accounts.shift(); - accounts.signers = Array(3) - .fill() - .map(() => accounts.shift()); + // 4337 helper + const helper = new ERC4337Helper('AdvancedAccount'); + const identity = new IdentityHelper(); + + // environment const target = await ethers.deployContract('CallReceiverMock'); - const helper = new ERC4337Helper('AdvancedAccountECDSA'); - await helper.wait(); - const sender = await helper.newAccount(accounts.user, [accounts.signers, 2]); // 2-of-3 + + // create 4337 account controlled by multiple signers + const signers = await Promise.all([ + identity.newECDSASigner(), // secp256k1 + identity.newP256Signer(), // secp256r1 + identity.newP256Signer(), // secp256r1 + identity.newECDSASigner(), // secp256k1 + ]); + const sender = await helper.newAccount(accounts.relayer, [signers, 2]); // 2-of-4 return { accounts, @@ -23,6 +32,7 @@ async function fixture() { helper, entrypoint: helper.entrypoint, factory: helper.factory, + signers, sender, }; } @@ -34,7 +44,7 @@ describe('AccountMultisig', function () { describe('execute operation', function () { beforeEach('fund account', async function () { - await this.accounts.user.sendTransaction({ to: this.sender, value: ethers.parseEther('1') }); + await this.accounts.relayer.sendTransaction({ to: this.sender, value: ethers.parseEther('1') }); }); describe('account not deployed yet', function () { @@ -48,7 +58,7 @@ describe('AccountMultisig', function () { ]), }) .then(op => op.addInitCode()) - .then(op => op.sign(this.accounts.signers)); + .then(op => op.sign(this.signers, true)); await expect(this.entrypoint.handleOps([operation.packed], this.accounts.beneficiary)) .to.emit(this.entrypoint, 'AccountDeployed') @@ -72,7 +82,7 @@ describe('AccountMultisig', function () { this.target.interface.encodeFunctionData('mockFunctionExtra'), ]), }) - .then(op => op.sign(this.accounts.signers)); + .then(op => op.sign(this.signers, true)); await expect(this.entrypoint.handleOps([operation.packed], this.accounts.beneficiary)) .to.emit(this.target, 'MockFunctionCalledExtra') @@ -88,7 +98,7 @@ describe('AccountMultisig', function () { this.target.interface.encodeFunctionData('mockFunctionExtra'), ]), }) - .then(op => op.sign([this.accounts.signers[0], this.accounts.signers[2]])); + .then(op => op.sign([this.signers[0], this.signers[2]], true)); await expect(this.entrypoint.handleOps([operation.packed], this.accounts.beneficiary)) .to.emit(this.target, 'MockFunctionCalledExtra') @@ -104,7 +114,7 @@ describe('AccountMultisig', function () { this.target.interface.encodeFunctionData('mockFunctionExtra'), ]), }) - .then(op => op.sign([this.accounts.signers[2]])); + .then(op => op.sign([this.signers[2]], true)); await expect(this.entrypoint.handleOps([operation.packed], this.accounts.beneficiary)) .to.be.revertedWithCustomError(this.entrypoint, 'FailedOp') @@ -120,7 +130,7 @@ describe('AccountMultisig', function () { this.target.interface.encodeFunctionData('mockFunctionExtra'), ]), }) - .then(op => op.sign([this.accounts.user, this.accounts.signers[2]])); + .then(op => op.sign([this.accounts.relayer, this.signers[2]], true)); await expect(this.entrypoint.handleOps([operation.packed], this.accounts.beneficiary)) .to.be.revertedWithCustomError(this.entrypoint, 'FailedOp') diff --git a/test/helpers/erc4337.js b/test/helpers/erc4337.js index d225771ca0a..419f42129ac 100644 --- a/test/helpers/erc4337.js +++ b/test/helpers/erc4337.js @@ -1,9 +1,12 @@ const { ethers } = require('hardhat'); +const { SignatureType } = require('./identity'); + function pack(left, right) { return ethers.solidityPacked(['uint128', 'uint128'], [left, right]); } +/// Global ERC-4337 environment helper. class ERC4337Helper { constructor(account = 'SimpleAccountECDSA') { this.entrypointAsPromise = ethers.deployContract('EntryPoint'); @@ -20,19 +23,20 @@ class ERC4337Helper { return this; } - async newAccount(user, extraArgs = [], salt = ethers.randomBytes(32)) { + async newAccount(signer, extraArgs = [], salt = ethers.randomBytes(32)) { await this.wait(); const initCode = await this.account - .getDeployTransaction(this.entrypoint, user, ...extraArgs) + .getDeployTransaction(this.entrypoint, signer, ...extraArgs) .then(tx => this.factory.interface.encodeFunctionData('$deploy', [0, salt, tx.data])) .then(deployCode => ethers.concat([this.factory.target, deployCode])); const instance = await this.entrypoint.getSenderAddress .staticCall(initCode) - .then(address => this.account.attach(address).connect(user)); + .then(address => this.account.attach(address).connect(signer)); return new AbstractAccount(instance, initCode, this); } } +/// Represent one ERC-4337 account contract. class AbstractAccount extends ethers.BaseContract { constructor(instance, initCode, context) { super(instance.target, instance.interface, instance.runner, instance.deployTx); @@ -69,6 +73,7 @@ class AbstractAccount extends ethers.BaseContract { } } +/// Represent one user operation class UserOperation { constructor(params) { this.sender = params.sender; @@ -128,14 +133,27 @@ class UserOperation { return this; } - async sign(signer = this.sender.runner) { - this.signature = await Promise.all( - (Array.isArray(signer) ? signer : [signer]) - .sort((signer1, signer2) => signer1.address - signer2.address) - .map(signer => signer.signMessage(ethers.getBytes(this.hash))), - ).then(signatures => - Array.isArray(signer) ? ethers.AbiCoder.defaultAbiCoder().encode(['bytes[]'], [signatures]) : signatures[0], + async sign(signer = this.sender.runner, withTypePrefix = false) { + const signers = (Array.isArray(signer) ? signer : [signer]).sort( + (signer1, signer2) => signer1.address - signer2.address, + ); + const signatures = await Promise.all( + signers.map(signer => + Promise.resolve(signer.signMessage(ethers.getBytes(this.hash))).then(signature => + withTypePrefix + ? ethers.AbiCoder.defaultAbiCoder().encode( + ['uint8', 'bytes'], + [signer.type ?? SignatureType.ECDSA, signature], + ) + : signature, + ), + ), ); + + this.signature = Array.isArray(signer) + ? ethers.AbiCoder.defaultAbiCoder().encode(['bytes[]'], [signatures]) + : signatures[0]; + return this; } } diff --git a/test/helpers/identity.js b/test/helpers/identity.js new file mode 100644 index 00000000000..7076e402028 --- /dev/null +++ b/test/helpers/identity.js @@ -0,0 +1,43 @@ +const { ethers } = require('hardhat'); + +const { Enum } = require('./enums'); +const { P256Signer } = require('./p256'); + +const SignatureType = Enum('ECDSA', 'ERC1271'); + +class IdentityHelper { + constructor() { + this.p256FactoryAsPromise = ethers.deployContract('IdentityP256Factory'); + this.rsaFactoryAsPromise = ethers.deployContract('IdentityRSAFactory'); + } + + async wait() { + this.p256Factory = await this.p256FactoryAsPromise; + this.rsaFactory = await this.rsaFactoryAsPromise; + return this; + } + + async newECDSASigner() { + return Object.assign(ethers.Wallet.createRandom(), { type: SignatureType.ECDSA }); + } + + async newP256Signer(sigParams = { prefixAddress: true }) { + await this.wait(); + + const signer = P256Signer.random(); + return Promise.all([this.p256Factory.predict(signer.publicKey), this.p256Factory.create(signer.publicKey)]).then( + ([address]) => Object.assign(signer, { address, sigParams, type: SignatureType.ERC1271 }), + ); + } + + async newRSASigner() { + await this.wait(); + + return Promise.reject('Not implemented yet'); + } +} + +module.exports = { + SignatureType, + IdentityHelper, +}; diff --git a/test/helpers/p256.js b/test/helpers/p256.js index 8ee2436c74e..12e5efbf0ba 100644 --- a/test/helpers/p256.js +++ b/test/helpers/p256.js @@ -38,7 +38,7 @@ class P256Signer { ? ethers.solidityPacked(['uint256', 'uint256', 'uint8'], [r, s, recovery]) : ethers.solidityPacked(['uint256', 'uint256'], [r, s]); return this.sigParams.prefixAddress - ? ethers.AbiCoder.defaultAbiCoder().encode([ 'address', 'bytes' ], [ this.address, signature ]) + ? ethers.AbiCoder.defaultAbiCoder().encode(['address', 'bytes'], [this.address, signature]) : signature; } }