From 0524529465d917f08c1137e63a78c5f17f336b6b Mon Sep 17 00:00:00 2001 From: shahafn Date: Tue, 2 Jan 2024 11:19:12 +0200 Subject: [PATCH 1/4] AA-238 Removing obsolete gnosis safe 4337 module (#389) * Removing obsolete gnosis safe module --- .solcover.js | 2 - contracts/package.json | 1 - contracts/samples/gnosis/EIP4337Fallback.sol | 89 ------ contracts/samples/gnosis/EIP4337Manager.sol | 197 ------------ .../samples/gnosis/GnosisAccountFactory.sol | 61 ---- package.json | 1 - scripts/prepack-contracts-package.sh | 4 +- test/gnosis.test.ts | 285 ------------------ yarn.lock | 5 - 9 files changed, 2 insertions(+), 643 deletions(-) delete mode 100644 contracts/samples/gnosis/EIP4337Fallback.sol delete mode 100644 contracts/samples/gnosis/EIP4337Manager.sol delete mode 100644 contracts/samples/gnosis/GnosisAccountFactory.sol delete mode 100644 test/gnosis.test.ts diff --git a/.solcover.js b/.solcover.js index 186efeaf..851e37d6 100644 --- a/.solcover.js +++ b/.solcover.js @@ -2,8 +2,6 @@ module.exports = { skipFiles: [ "test", "samples/bls/lib", - //solc-coverage fails to compile our Manager module. - "samples/gnosis", "utils/Exec.sol" ], configureYulOptimizer: true, diff --git a/contracts/package.json b/contracts/package.json index a2af65b9..f51ca31d 100644 --- a/contracts/package.json +++ b/contracts/package.json @@ -24,7 +24,6 @@ "url": "https://github.com/eth-infinitism/account-abstraction/issues" }, "devDependencies": { - "@gnosis.pm/safe-contracts": "^1.3.0", "@nomiclabs/hardhat-ethers": "^2.0.2", "@nomiclabs/hardhat-waffle": "^2.0.1", "@uniswap/v3-periphery": "^1.4.3" diff --git a/contracts/samples/gnosis/EIP4337Fallback.sol b/contracts/samples/gnosis/EIP4337Fallback.sol deleted file mode 100644 index 4cc0978d..00000000 --- a/contracts/samples/gnosis/EIP4337Fallback.sol +++ /dev/null @@ -1,89 +0,0 @@ -//SPDX-License-Identifier: GPL -pragma solidity ^0.8.7; - -/* solhint-disable no-inline-assembly */ - -import "@gnosis.pm/safe-contracts/contracts/handler/DefaultCallbackHandler.sol"; -import "@gnosis.pm/safe-contracts/contracts/GnosisSafe.sol"; -import "@openzeppelin/contracts/interfaces/IERC1271.sol"; -import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; -import "../../interfaces/IAccount.sol"; -import "./EIP4337Manager.sol"; - -using ECDSA for bytes32; - -/** - * The GnosisSafe enables adding custom functions implementation to the Safe by setting a 'fallbackHandler'. - * This 'fallbackHandler' adds an implementation of 'validateUserOp' to the GnosisSafe. - * Note that the implementation of the 'validateUserOp' method is located in the EIP4337Manager. - * Upon receiving the 'validateUserOp', a Safe with EIP4337Fallback enabled makes a 'delegatecall' to EIP4337Manager. - */ -contract EIP4337Fallback is DefaultCallbackHandler, IAccount, IERC1271 { - bytes4 internal constant ERC1271_MAGIC_VALUE = 0x1626ba7e; - - address immutable public eip4337manager; - constructor(address _eip4337manager) { - eip4337manager = _eip4337manager; - } - - /** - * delegate the contract call to the EIP4337Manager - */ - function delegateToManager() internal returns (bytes memory) { - // delegate entire msg.data (including the appended "msg.sender") to the EIP4337Manager - // will work only for GnosisSafe contracts - GnosisSafe safe = GnosisSafe(payable(msg.sender)); - (bool success, bytes memory ret) = safe.execTransactionFromModuleReturnData(eip4337manager, 0, msg.data, Enum.Operation.DelegateCall); - if (!success) { - assembly { - revert(add(ret, 32), mload(ret)) - } - } - return ret; - } - - /** - * called from the Safe. delegate actual work to EIP4337Manager - */ - function validateUserOp(UserOperation calldata, bytes32, uint256) override external returns (uint256 deadline){ - bytes memory ret = delegateToManager(); - return abi.decode(ret, (uint256)); - } - - /** - * Helper for wallet to get the next nonce. - */ - function getNonce() public returns (uint256 nonce) { - bytes memory ret = delegateToManager(); - (nonce) = abi.decode(ret, (uint256)); - } - - /** - * called from the Safe. delegate actual work to EIP4337Manager - */ - function executeAndRevert( - address, - uint256, - bytes memory, - Enum.Operation - ) external { - delegateToManager(); - } - - function isValidSignature( - bytes32 _hash, - bytes memory _signature - ) external override view returns (bytes4) { - bytes32 hash = _hash.toEthSignedMessageHash(); - address recovered = hash.recover(_signature); - - GnosisSafe safe = GnosisSafe(payable(address(msg.sender))); - - // Validate signatures - if (safe.isOwner(recovered)) { - return ERC1271_MAGIC_VALUE; - } else { - return 0xffffffff; - } - } -} diff --git a/contracts/samples/gnosis/EIP4337Manager.sol b/contracts/samples/gnosis/EIP4337Manager.sol deleted file mode 100644 index 2a687ada..00000000 --- a/contracts/samples/gnosis/EIP4337Manager.sol +++ /dev/null @@ -1,197 +0,0 @@ -//SPDX-License-Identifier: GPL -pragma solidity ^0.8.7; - -/* solhint-disable avoid-low-level-calls */ -/* solhint-disable no-inline-assembly */ -/* solhint-disable reason-string */ - -import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; -import "@gnosis.pm/safe-contracts/contracts/GnosisSafe.sol"; -import "@gnosis.pm/safe-contracts/contracts/base/Executor.sol"; -import "@gnosis.pm/safe-contracts/contracts/examples/libraries/GnosisSafeStorage.sol"; -import "./EIP4337Fallback.sol"; -import "../../interfaces/IAccount.sol"; -import "../../interfaces/IEntryPoint.sol"; -import "../../utils/Exec.sol"; - - using ECDSA for bytes32; - -/** - * Main EIP4337 module. - * Called (through the fallback module) using "delegate" from the GnosisSafe as an "IAccount", - * so must implement validateUserOp - * holds an immutable reference to the EntryPoint - * Inherits GnosisSafe so that it can reference the memory storage - */ -contract EIP4337Manager is IAccount, GnosisSafeStorage, Executor { - - address public immutable eip4337Fallback; - address public immutable entryPoint; - - // return value in case of signature failure, with no time-range. - // equivalent to _packValidationData(true,0,0); - uint256 constant internal SIG_VALIDATION_FAILED = 1; - - address internal constant SENTINEL_MODULES = address(0x1); - - constructor(address anEntryPoint) { - entryPoint = anEntryPoint; - eip4337Fallback = address(new EIP4337Fallback(address(this))); - } - - /** - * delegate-called (using execFromModule) through the fallback, so "real" msg.sender is attached as last 20 bytes - */ - function validateUserOp(UserOperation calldata userOp, bytes32 userOpHash, uint256 missingAccountFunds) - external override returns (uint256 validationData) { - address msgSender = address(bytes20(msg.data[msg.data.length - 20 :])); - require(msgSender == entryPoint, "account: not from entrypoint"); - - GnosisSafe pThis = GnosisSafe(payable(address(this))); - bytes32 hash = userOpHash.toEthSignedMessageHash(); - address recovered = hash.recover(userOp.signature); - require(threshold == 1, "account: only threshold 1"); - if (!pThis.isOwner(recovered)) { - validationData = SIG_VALIDATION_FAILED; - } - - // mimic normal Safe nonce behaviour: prevent parallel nonces - require(userOp.nonce < type(uint64).max, "account: nonsequential nonce"); - - if (missingAccountFunds > 0) { - //Note: MAY pay more than the minimum, to deposit for future transactions - (bool success,) = payable(msgSender).call{value : missingAccountFunds}(""); - (success); - //ignore failure (its EntryPoint's job to verify, not account.) - } - } - - /** - * Execute a call but also revert if the execution fails. - * The default behavior of the Safe is to not revert if the call fails, - * which is challenging for integrating with ERC4337 because then the - * EntryPoint wouldn't know to emit the UserOperationRevertReason event, - * which the frontend/client uses to capture the reason for the failure. - */ - function executeAndRevert( - address to, - uint256 value, - bytes memory data, - Enum.Operation operation - ) external { - address msgSender = address(bytes20(msg.data[msg.data.length - 20 :])); - require(msgSender == entryPoint, "account: not from entrypoint"); - require(msg.sender == eip4337Fallback, "account: not from EIP4337Fallback"); - - bool success = execute( - to, - value, - data, - operation, - type(uint256).max - ); - - bytes memory returnData = Exec.getReturnData(type(uint256).max); - // Revert with the actual reason string - // Adopted from: https://github.com/Uniswap/v3-periphery/blob/464a8a49611272f7349c970e0fadb7ec1d3c1086/contracts/base/Multicall.sol#L16-L23 - if (!success) { - if (returnData.length < 68) revert(); - assembly { - returnData := add(returnData, 0x04) - } - revert(abi.decode(returnData, (string))); - } - } - - /** - * Helper for wallet to get the next nonce. - */ - function getNonce() public view returns (uint256) { - return IEntryPoint(entryPoint).getNonce(address(this), 0); - } - - /** - * set up a safe as EIP-4337 enabled. - * called from the GnosisSafeAccountFactory during construction time - * - enable 3 modules (this module, fallback and the entrypoint) - * - this method is called with delegateCall, so the module (usually itself) is passed as parameter, and "this" is the safe itself - */ - function setup4337Modules( - EIP4337Manager manager //the manager (this contract) - ) external { - GnosisSafe safe = GnosisSafe(payable(address(this))); - require(!safe.isModuleEnabled(manager.entryPoint()), "setup4337Modules: entrypoint already enabled"); - require(!safe.isModuleEnabled(manager.eip4337Fallback()), "setup4337Modules: eip4337Fallback already enabled"); - safe.enableModule(manager.entryPoint()); - safe.enableModule(manager.eip4337Fallback()); - } - - /** - * replace EIP4337 module, to support a new EntryPoint. - * must be called using execTransaction and Enum.Operation.DelegateCall - * @param prevModule returned by getCurrentEIP4337Manager - * @param oldManager the old EIP4337 manager to remove, returned by getCurrentEIP4337Manager - * @param newManager the new EIP4337Manager, usually with a new EntryPoint - */ - function replaceEIP4337Manager(address prevModule, EIP4337Manager oldManager, EIP4337Manager newManager) public { - GnosisSafe pThis = GnosisSafe(payable(address(this))); - address oldFallback = oldManager.eip4337Fallback(); - require(pThis.isModuleEnabled(oldFallback), "replaceEIP4337Manager: oldManager is not active"); - pThis.disableModule(oldFallback, oldManager.entryPoint()); - pThis.disableModule(prevModule, oldFallback); - - address eip4337fallback = newManager.eip4337Fallback(); - - pThis.enableModule(newManager.entryPoint()); - pThis.enableModule(eip4337fallback); - pThis.setFallbackHandler(eip4337fallback); - - validateEip4337(pThis, newManager); - } - - /** - * Validate this gnosisSafe is callable through the EntryPoint. - * the test is might be incomplete: we check that we reach our validateUserOp and fail on signature. - * we don't test full transaction - */ - function validateEip4337(GnosisSafe safe, EIP4337Manager manager) public { - - // this prevents mistaken replaceEIP4337Manager to disable the module completely. - // minimal signature that pass "recover" - bytes memory sig = new bytes(65); - sig[64] = bytes1(uint8(27)); - sig[2] = bytes1(uint8(1)); - sig[35] = bytes1(uint8(1)); - uint256 nonce = uint256(IEntryPoint(manager.entryPoint()).getNonce(address(safe), 0)); - UserOperation memory userOp = UserOperation(address(safe), nonce, "", "", 0, 1000000, 0, 0, 0, "", sig); - UserOperation[] memory userOps = new UserOperation[](1); - userOps[0] = userOp; - IEntryPoint _entryPoint = IEntryPoint(payable(manager.entryPoint())); - try _entryPoint.handleOps(userOps, payable(msg.sender)) { - revert("validateEip4337: handleOps must fail"); - } catch (bytes memory error) { - if (keccak256(error) != keccak256(abi.encodeWithSignature("FailedOp(uint256,string)", 0, "AA24 signature error"))) { - revert(string(error)); - } - } - } - /** - * enumerate modules, and find the currently active EIP4337 manager (and previous module) - * @return prev prev module, needed by replaceEIP4337Manager - * @return manager the current active EIP4337Manager - */ - function getCurrentEIP4337Manager(GnosisSafe safe) public view returns (address prev, address manager) { - prev = address(SENTINEL_MODULES); - (address[] memory modules,) = safe.getModulesPaginated(SENTINEL_MODULES, 100); - for (uint i = 0; i < modules.length; i++) { - address module = modules[i]; - try EIP4337Fallback(module).eip4337manager() returns (address _manager) { - return (prev, _manager); - } - // solhint-disable-next-line no-empty-blocks - catch {} - prev = module; - } - return (address(0), address(0)); - } -} diff --git a/contracts/samples/gnosis/GnosisAccountFactory.sol b/contracts/samples/gnosis/GnosisAccountFactory.sol deleted file mode 100644 index 459846a7..00000000 --- a/contracts/samples/gnosis/GnosisAccountFactory.sol +++ /dev/null @@ -1,61 +0,0 @@ -// SPDX-License-Identifier: GPL-3.0 -pragma solidity ^0.8.12; - -import "@openzeppelin/contracts/utils/Create2.sol"; -import "@gnosis.pm/safe-contracts/contracts/proxies/GnosisSafeProxyFactory.sol"; -import "./EIP4337Manager.sol"; - -/** - * A wrapper factory contract to deploy GnosisSafe as an ERC-4337 account contract. - */ -contract GnosisSafeAccountFactory { - - GnosisSafeProxyFactory public immutable proxyFactory; - address public immutable safeSingleton; - EIP4337Manager public immutable eip4337Manager; - - constructor(GnosisSafeProxyFactory _proxyFactory, address _safeSingleton, EIP4337Manager _eip4337Manager) { - proxyFactory = _proxyFactory; - safeSingleton = _safeSingleton; - eip4337Manager = _eip4337Manager; - } - - function createAccount(address owner,uint256 salt) public returns (address) { - address addr = getAddress(owner, salt); - uint codeSize = addr.code.length; - if (codeSize > 0) { - return addr; - } - return address(proxyFactory.createProxyWithNonce( - safeSingleton, getInitializer(owner), salt)); - } - - function getInitializer(address owner) internal view returns (bytes memory) { - address[] memory owners = new address[](1); - owners[0] = owner; - uint threshold = 1; - address eip4337fallback = eip4337Manager.eip4337Fallback(); - - bytes memory setup4337Modules = abi.encodeCall( - EIP4337Manager.setup4337Modules, (eip4337Manager)); - - return abi.encodeCall(GnosisSafe.setup, ( - owners, threshold, - address (eip4337Manager), setup4337Modules, - eip4337fallback, - address(0), 0, payable(0) //no payment receiver - )); - } - - /** - * calculate the counterfactual address of this account as it would be returned by createAccount() - * (uses the same "create2 signature" used by GnosisSafeProxyFactory.createProxyWithNonce) - */ - function getAddress(address owner,uint256 salt) public view returns (address) { - bytes memory initializer = getInitializer(owner); - //copied from deployProxyWithNonce - bytes32 salt2 = keccak256(abi.encodePacked(keccak256(initializer), salt)); - bytes memory deploymentData = abi.encodePacked(proxyFactory.proxyCreationCode(), uint256(uint160(safeSingleton))); - return Create2.computeAddress(bytes32(salt2), keccak256(deploymentData), address (proxyFactory)); - } -} diff --git a/package.json b/package.json index 400563c5..d7cfc162 100644 --- a/package.json +++ b/package.json @@ -54,7 +54,6 @@ "typechain": "^8.1.0" }, "dependencies": { - "@gnosis.pm/safe-contracts": "^1.3.0", "@nomiclabs/hardhat-etherscan": "^2.1.6", "@openzeppelin/contracts": "^4.2.0", "@thehubbleproject/bls": "^0.5.1", diff --git a/scripts/prepack-contracts-package.sh b/scripts/prepack-contracts-package.sh index 8d4c69aa..de4725d3 100755 --- a/scripts/prepack-contracts-package.sh +++ b/scripts/prepack-contracts-package.sh @@ -7,13 +7,13 @@ if git status contracts | grep -v 'nothing to commit'|tee /dev/stderr |grep -q U exit 1 fi -yarn clean +yarn clean yarn compile cd contracts rm -rf artifacts types dist mkdir -p artifacts -cp `find ../artifacts/contracts -type f | grep -v -E 'Test|dbg|gnosis|bls|IOracle'` artifacts/ +cp `find ../artifacts/contracts -type f | grep -v -E 'Test|dbg|bls|IOracle'` artifacts/ npx typechain --target ethers-v5 --out-dir types artifacts/** npx tsc index.ts -d --outDir dist diff --git a/test/gnosis.test.ts b/test/gnosis.test.ts deleted file mode 100644 index b763e1ab..00000000 --- a/test/gnosis.test.ts +++ /dev/null @@ -1,285 +0,0 @@ -import './aa.init' -import { ethers } from 'hardhat' -import { Signer } from 'ethers' -import { - EIP4337Fallback__factory, - EIP4337Manager, - EIP4337Manager__factory, - EntryPoint, - EntryPoint__factory, - GnosisSafe, - GnosisSafeAccountFactory, - GnosisSafeAccountFactory__factory, - GnosisSafeProxy, - GnosisSafeProxyFactory__factory, - GnosisSafe__factory, - TestCounter, - TestCounter__factory -} from '../typechain' -import { - AddressZero, - createAddress, - createAccountOwner, - deployEntryPoint, - getBalance, - HashZero, - isDeployed, decodeRevertReason -} from './testutils' -import { fillAndSign } from './UserOp' -import { defaultAbiCoder, hexConcat, hexZeroPad, parseEther } from 'ethers/lib/utils' -import { expect } from 'chai' - -describe('Gnosis Proxy', function () { - this.timeout(30000) - - let ethersSigner: Signer - let safeSingleton: GnosisSafe - let owner: Signer - let ownerAddress: string - let proxy: GnosisSafeProxy - let manager: EIP4337Manager - let entryPoint: EntryPoint - let counter: TestCounter - let proxySafe: GnosisSafe - let safe_execTxCallData: string - - let accountFactory: GnosisSafeAccountFactory - - before('before', async function () { - // EIP4337Manager fails to compile with solc-coverage - if (process.env.COVERAGE != null) { - return this.skip() - } - - const provider = ethers.provider - ethersSigner = provider.getSigner() - - // standard safe singleton contract (implementation) - safeSingleton = await new GnosisSafe__factory(ethersSigner).deploy() - // standard safe proxy factory - const proxyFactory = await new GnosisSafeProxyFactory__factory(ethersSigner).deploy() - entryPoint = await deployEntryPoint() - manager = await new EIP4337Manager__factory(ethersSigner).deploy(entryPoint.address) - owner = createAccountOwner() - ownerAddress = await owner.getAddress() - counter = await new TestCounter__factory(ethersSigner).deploy() - - accountFactory = await new GnosisSafeAccountFactory__factory(ethersSigner) - .deploy(proxyFactory.address, safeSingleton.address, manager.address) - - await accountFactory.createAccount(ownerAddress, 0) - // we use our accountFactory to create and configure the proxy. - // but the actual deployment is done internally by the gnosis factory - const ev = await proxyFactory.queryFilter(proxyFactory.filters.ProxyCreation()) - const addr = ev[0].args.proxy - - proxy = - proxySafe = GnosisSafe__factory.connect(addr, owner) - - await ethersSigner.sendTransaction({ - to: proxy.address, - value: parseEther('0.1') - }) - - const counter_countCallData = counter.interface.encodeFunctionData('count') - safe_execTxCallData = manager.interface.encodeFunctionData('executeAndRevert', [counter.address, 0, counter_countCallData, 0]) - }) - let beneficiary: string - beforeEach(() => { - beneficiary = createAddress() - }) - - it('#getCurrentEIP4337Manager', async () => { - // need some manager to query the current manager of a safe - const tempManager = await new EIP4337Manager__factory(ethersSigner).deploy(AddressZero) - const { manager: curManager } = await tempManager.getCurrentEIP4337Manager(proxySafe.address) - expect(curManager).to.eq(manager.address) - }) - - it('should validate', async function () { - await manager.callStatic.validateEip4337(proxySafe.address, manager.address, { gasLimit: 10e6 }) - }) - - it('should fail from wrong entrypoint', async function () { - const op = await fillAndSign({ - sender: proxy.address - }, owner, entryPoint, 'getNonce') - - const anotherEntryPoint = await new EntryPoint__factory(ethersSigner).deploy() - - // await expect( - expect(await anotherEntryPoint.handleOps([op], beneficiary) - .catch(e => decodeRevertReason(e))) - .to.include('account: not from entrypoint') - }) - - it('should fail on invalid userop', async function () { - let op = await fillAndSign({ - sender: proxy.address, - nonce: 1234, - callGasLimit: 1e6, - callData: safe_execTxCallData - }, owner, entryPoint, 'getNonce') - await expect(entryPoint.handleOps([op], beneficiary)).to.revertedWith('AA25 invalid account nonce') - - op = await fillAndSign({ - sender: proxy.address, - callGasLimit: 1e6, - callData: safe_execTxCallData - }, owner, entryPoint, 'getNonce') - // invalidate the signature - op.callGasLimit = 1 - await expect(entryPoint.handleOps([op], beneficiary)).to.revertedWith('FailedOp(0, "AA24 signature error")') - }) - - it('should exec', async function () { - const op = await fillAndSign({ - sender: proxy.address, - callGasLimit: 1e6, - callData: safe_execTxCallData - }, owner, entryPoint, 'getNonce') - const rcpt = await entryPoint.handleOps([op], beneficiary).then(async r => r.wait()) - console.log('gasUsed=', rcpt.gasUsed, rcpt.transactionHash) - - const ev = rcpt.events!.find(ev => ev.event === 'UserOperationEvent')! - expect(ev.args!.success).to.eq(true) - expect(await getBalance(beneficiary)).to.eq(ev.args!.actualGasCost) - }) - - it('should revert with reason', async function () { - const counter_countFailCallData = counter.interface.encodeFunctionData('countFail') - const safe_execFailTxCallData = manager.interface.encodeFunctionData('executeAndRevert', [counter.address, 0, counter_countFailCallData, 0]) - - const op = await fillAndSign({ - sender: proxy.address, - callGasLimit: 1e6, - callData: safe_execFailTxCallData - }, owner, entryPoint, 'getNonce') - - const rcpt = await entryPoint.handleOps([op], beneficiary).then(async r => r.wait()) - console.log('gasUsed=', rcpt.gasUsed, rcpt.transactionHash) - - // decode the revertReason - const ev = rcpt.events!.find(ev => ev.event === 'UserOperationRevertReason')! - let message: string = ev.args!.revertReason - if (message.startsWith('0x08c379a0')) { - // Error(string) - message = defaultAbiCoder.decode(['string'], '0x' + message.substring(10)).toString() - } - expect(message).to.eq('count failed') - }) - - let counterfactualAddress: string - it('should create account', async function () { - const initCode = hexConcat([ - accountFactory.address, - accountFactory.interface.encodeFunctionData('createAccount', [ownerAddress, 123]) - ]) - - counterfactualAddress = await accountFactory.callStatic.getAddress(ownerAddress, 123) - expect(!await isDeployed(counterfactualAddress)) - - await ethersSigner.sendTransaction({ - to: counterfactualAddress, - value: parseEther('0.1') - }) - const op = await fillAndSign({ - sender: counterfactualAddress, - initCode, - verificationGasLimit: 400000 - }, owner, entryPoint, 'getNonce') - - const rcpt = await entryPoint.handleOps([op], beneficiary).then(async r => r.wait()) - console.log('gasUsed=', rcpt.gasUsed, rcpt.transactionHash) - expect(await isDeployed(counterfactualAddress)) - - const newCode = await ethers.provider.getCode(counterfactualAddress) - expect(newCode.length).eq(324) - }) - - it('another op after creation', async function () { - if (counterfactualAddress == null) this.skip() - expect(await isDeployed(counterfactualAddress)) - - const op = await fillAndSign({ - sender: counterfactualAddress, - callData: safe_execTxCallData - }, owner, entryPoint, 'getNonce') - - const rcpt = await entryPoint.handleOps([op], beneficiary).then(async r => r.wait()) - console.log('gasUsed=', rcpt.gasUsed, rcpt.transactionHash) - }) - - it('should validate ERC1271 signatures', async function () { - const safe = EIP4337Fallback__factory.connect(proxySafe.address, ethersSigner) - - const message = ethers.utils.hexlify(ethers.utils.toUtf8Bytes('hello erc1271')) - const dataHash = ethers.utils.arrayify(ethers.utils.keccak256(message)) - - const sig = await owner.signMessage(dataHash) - expect(await safe.isValidSignature(dataHash, sig)).to.be.eq('0x1626ba7e') - - // make an sig invalid - const badWallet = ethers.Wallet.createRandom() - const badSig = await badWallet.signMessage(dataHash) - expect(await safe.isValidSignature(dataHash, badSig)).to.be.not.eq('0x1626ba7e') - }) - - context('#replaceEIP4337', () => { - let signature: string - let newEntryPoint: EntryPoint - let newFallback: string - let newManager: EIP4337Manager - let oldManager: string - let prev: string - - before(async () => { - // sig is r{32}s{32}v{1}. for trusting the caller, r=address, v=1 - signature = hexConcat([ - hexZeroPad(ownerAddress, 32), - HashZero, - '0x01']) - newEntryPoint = await new EntryPoint__factory(ethersSigner).deploy() - - newManager = await new EIP4337Manager__factory(ethersSigner).deploy(newEntryPoint.address) - newFallback = await newManager.eip4337Fallback(); - [prev, oldManager] = await manager.getCurrentEIP4337Manager(proxySafe.address) - }) - - it('should reject to replace if wrong old manager', async () => { - const replaceManagerCallData = manager.interface.encodeFunctionData('replaceEIP4337Manager', - [prev, newManager.address, oldManager]) - // using call from module, so it return value.. - const proxyFromModule = proxySafe.connect(entryPoint.address) - const ret = await proxyFromModule.callStatic.execTransactionFromModuleReturnData(manager.address, 0, replaceManagerCallData, 1) - const [errorStr] = defaultAbiCoder.decode(['string'], ret.returnData.replace(/0x.{8}/, '0x')) - expect(errorStr).to.equal('replaceEIP4337Manager: oldManager is not active') - }) - - it('should replace manager', async function () { - const oldFallback = await manager.eip4337Fallback() - expect(await proxySafe.isModuleEnabled(entryPoint.address)).to.equal(true) - expect(await proxySafe.isModuleEnabled(oldFallback)).to.equal(true) - - expect(oldManager.toLowerCase()).to.eq(manager.address.toLowerCase()) - await ethersSigner.sendTransaction({ - to: ownerAddress, - value: parseEther('33') - }) - - const replaceManagerCallData = manager.interface.encodeFunctionData('replaceEIP4337Manager', - [prev, oldManager, newManager.address]) - await proxySafe.execTransaction(manager.address, 0, replaceManagerCallData, 1, 1e6, 0, 0, AddressZero, AddressZero, signature).then(async r => r.wait()) - - // console.log(rcpt.events?.slice(-1)[0].event) - - expect(await proxySafe.isModuleEnabled(newEntryPoint.address)).to.equal(true) - expect(await proxySafe.isModuleEnabled(newFallback)).to.equal(true) - expect(await proxySafe.isModuleEnabled(entryPoint.address)).to.equal(false) - expect(await proxySafe.isModuleEnabled(oldFallback)).to.equal(false) - - const { manager: curManager } = await manager.getCurrentEIP4337Manager(proxySafe.address) - expect(curManager).to.eq(newManager.address) - }) - }) -}) diff --git a/yarn.lock b/yarn.lock index d9201748..ba215488 100644 --- a/yarn.lock +++ b/yarn.lock @@ -549,11 +549,6 @@ resolved "https://registry.yarnpkg.com/@fastify/busboy/-/busboy-2.0.0.tgz#f22824caff3ae506b18207bad4126dbc6ccdb6b8" integrity sha512-JUFJad5lv7jxj926GPgymrWQxxjPYuJNiNjNMzqT+HiuP6Vl3dk5xzG+8sTX96np0ZAluvaMzPsjhHZ5rNuNQQ== -"@gnosis.pm/safe-contracts@^1.3.0": - version "1.3.0" - resolved "https://registry.yarnpkg.com/@gnosis.pm/safe-contracts/-/safe-contracts-1.3.0.tgz#316741a7690d8751a1f701538cfc9ec80866eedc" - integrity sha512-1p+1HwGvxGUVzVkFjNzglwHrLNA67U/axP0Ct85FzzH8yhGJb4t9jDjPYocVMzLorDoWAfKicGy1akPY9jXRVw== - "@humanwhocodes/config-array@^0.11.11": version "0.11.11" resolved "https://registry.yarnpkg.com/@humanwhocodes/config-array/-/config-array-0.11.11.tgz#88a04c570dbbc7dd943e4712429c3df09bc32844" From b92deca9b84afe13f1494cdc4b23688254418a7c Mon Sep 17 00:00:00 2001 From: Dror Tirosh Date: Wed, 3 Jan 2024 16:15:30 +0200 Subject: [PATCH 2/4] add separate test for paymaster with postOp (#391) * add separate test for paymaster with postOp we use our "gas tests" to check the impact of specific version. it is important to to break tests. This PR make "simple paymaster" simple again, and adds a separte test of "paymaster with postOp" tests are ordered so that they can be directly diff'ed with (say) v0.6.0 contracts * separate TestPaymasterWithPostOp --- contracts/test/TestPaymasterAcceptAll.sol | 11 +---- contracts/test/TestPaymasterWithPostOp.sol | 30 +++++++++++++ gascalc/4-paymaster-postop.gas.ts | 45 +++++++++++++++++++ ...master.gas.ts => 5-token-paymaster.gas.ts} | 0 reports/gas-checker.txt | 32 ++++++++----- 5 files changed, 96 insertions(+), 22 deletions(-) create mode 100644 contracts/test/TestPaymasterWithPostOp.sol create mode 100644 gascalc/4-paymaster-postop.gas.ts rename gascalc/{4-token-paymaster.gas.ts => 5-token-paymaster.gas.ts} (100%) diff --git a/contracts/test/TestPaymasterAcceptAll.sol b/contracts/test/TestPaymasterAcceptAll.sol index f97ccd4a..7db7dc2c 100644 --- a/contracts/test/TestPaymasterAcceptAll.sol +++ b/contracts/test/TestPaymasterAcceptAll.sol @@ -2,7 +2,6 @@ pragma solidity ^0.8.12; import "../core/BasePaymaster.sol"; -/* solhint-disable no-empty-blocks */ /** * test paymaster, that pays for everything, without any check. @@ -22,14 +21,6 @@ contract TestPaymasterAcceptAll is BasePaymaster { internal virtual override view returns (bytes memory context, uint256 validationData) { (userOp, userOpHash, maxCost); - // return a context, as it is used for EntryPoint gas checking. - return ("1", 0); - } - - function _postOp( - PostOpMode mode, - bytes calldata context, - uint256 actualGasCost - ) internal override { + return ("", 0); } } diff --git a/contracts/test/TestPaymasterWithPostOp.sol b/contracts/test/TestPaymasterWithPostOp.sol new file mode 100644 index 00000000..694dde51 --- /dev/null +++ b/contracts/test/TestPaymasterWithPostOp.sol @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: GPL-3.0-only +pragma solidity ^0.8.12; + +import "./TestPaymasterAcceptAll.sol"; +/* solhint-disable no-empty-blocks */ + +/** + * test paymaster, that pays for everything, without any check. + * explicitly returns a context, to test cost (for entrypoint) to call postOp + */ +contract TestPaymasterWithPostOp is TestPaymasterAcceptAll { + + constructor(IEntryPoint _entryPoint) TestPaymasterAcceptAll(_entryPoint) { + } + + function _validatePaymasterUserOp(UserOperation calldata userOp, bytes32 userOpHash, uint256 maxCost) + internal virtual override view + returns (bytes memory context, uint256 validationData) { + (userOp, userOpHash, maxCost); + // return a context, to force a call for postOp. + return ("1", 0); + } + + function _postOp( + PostOpMode mode, + bytes calldata context, + uint256 actualGasCost + ) internal override { + } +} diff --git a/gascalc/4-paymaster-postop.gas.ts b/gascalc/4-paymaster-postop.gas.ts new file mode 100644 index 00000000..c7495dc7 --- /dev/null +++ b/gascalc/4-paymaster-postop.gas.ts @@ -0,0 +1,45 @@ +import { parseEther } from 'ethers/lib/utils' +import { TestPaymasterWithPostOp__factory } from '../typechain' +import { ethers } from 'hardhat' +import { GasChecker } from './GasChecker' +import { Create2Factory } from '../src/Create2Factory' +import { hexValue } from '@ethersproject/bytes' + +const ethersSigner = ethers.provider.getSigner() + +context('Paymaster with PostOp', function () { + this.timeout(60000) + const g = new GasChecker() + + let paymasterAddress: string + + before(async () => { + const paymasterInit = hexValue(new TestPaymasterWithPostOp__factory(ethersSigner).getDeployTransaction(g.entryPoint().address).data!) + paymasterAddress = await new Create2Factory(ethers.provider, ethersSigner).deploy(paymasterInit, 0) + const paymaster = TestPaymasterWithPostOp__factory.connect(paymasterAddress, ethersSigner) + await paymaster.addStake(1, { value: 1 }) + await g.entryPoint().depositTo(paymaster.address, { value: parseEther('10') }) + }) + + it('paymaster with PostOp', async function () { + await g.addTestRow({ title: 'paymaster+postOp', count: 1, paymaster: paymasterAddress, diffLastGas: false }) + await g.addTestRow({ + title: 'paymaster+postOp with diff', + count: 2, + paymaster: paymasterAddress, + diffLastGas: true + }) + }) + + it('paymaster with postOp 10', async function () { + if (g.skipLong()) this.skip() + + await g.addTestRow({ title: 'paymaster+postOp', count: 10, paymaster: paymasterAddress, diffLastGas: false }) + await g.addTestRow({ + title: 'paymaster+postOp with diff', + count: 11, + paymaster: paymasterAddress, + diffLastGas: true + }) + }) +}) diff --git a/gascalc/4-token-paymaster.gas.ts b/gascalc/5-token-paymaster.gas.ts similarity index 100% rename from gascalc/4-token-paymaster.gas.ts rename to gascalc/5-token-paymaster.gas.ts diff --git a/reports/gas-checker.txt b/reports/gas-checker.txt index 3fac0df2..a848178d 100644 --- a/reports/gas-checker.txt +++ b/reports/gas-checker.txt @@ -12,36 +12,44 @@ ║ │ │ │ (delta for │ (compared to ║ ║ │ │ │ one UserOp) │ account.exec()) ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple │ 1 │ 81918 │ │ ║ +║ simple │ 1 │ 81930 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ ║ simple - diff from previous │ 2 │ │ 44187 │ 15173 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple │ 10 │ 479730 │ │ ║ +║ simple │ 10 │ 479742 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ ║ simple - diff from previous │ 11 │ │ 44247 │ 15233 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster │ 1 │ 89813 │ │ ║ +║ simple paymaster │ 1 │ 88071 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster with diff │ 2 │ │ 44796 │ 15782 ║ +║ simple paymaster with diff │ 2 │ │ 43022 │ 14008 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster │ 10 │ 493254 │ │ ║ +║ simple paymaster │ 10 │ 475654 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster with diff │ 11 │ │ 44820 │ 15806 ║ +║ simple paymaster with diff │ 11 │ │ 43105 │ 14091 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx 5k │ 1 │ 182975 │ │ ║ +║ big tx 5k │ 1 │ 182987 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ ║ big tx - diff from previous │ 2 │ │ 144698 │ 19438 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx 5k │ 10 │ 1485374 │ │ ║ +║ big tx 5k │ 10 │ 1485386 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ ║ big tx - diff from previous │ 11 │ │ 144759 │ 19499 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ token paymaster │ 1 │ 148244 │ │ ║ +║ paymaster+postOp │ 1 │ 89837 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ token paymaster with diff │ 2 │ │ 72920 │ 43906 ║ +║ paymaster+postOp with diff │ 2 │ │ 44808 │ 15794 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ token paymaster │ 10 │ 804795 │ │ ║ +║ paymaster+postOp │ 10 │ 493254 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ token paymaster with diff │ 11 │ │ 73015 │ 44001 ║ +║ paymaster+postOp with diff │ 11 │ │ 44880 │ 15866 ║ +╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ +║ token paymaster │ 1 │ 148256 │ │ ║ +╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ +║ token paymaster with diff │ 2 │ │ 72896 │ 43882 ║ +╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ +║ token paymaster │ 10 │ 804843 │ │ ║ +╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ +║ token paymaster with diff │ 11 │ │ 73003 │ 43989 ║ ╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝ From 63d8226fd3fff6a8091eb696808d02a43224f532 Mon Sep 17 00:00:00 2001 From: Dror Tirosh Date: Wed, 3 Jan 2024 16:25:40 +0200 Subject: [PATCH 3/4] AA-244 Upgrade open zeppelin 5.0 (#384) * upgrade solidity to 0.8.23 * Use OpenZeppelin v0.5 --- contracts/core/BasePaymaster.sol | 2 +- contracts/core/EntryPoint.sol | 5 +-- contracts/samples/SimpleAccount.sol | 15 +++---- contracts/samples/VerifyingPaymaster.sol | 4 +- .../samples/callback/TokenCallbackHandler.sol | 12 +---- contracts/test/TestExpiryAccount.sol | 5 +-- hardhat.config.ts | 4 +- package.json | 2 +- reports/gas-checker.txt | 44 +++++++++---------- test/entrypoint.test.ts | 4 +- test/samples/TokenPaymaster.test.ts | 13 +++--- test/testutils.ts | 15 +++---- test/verifying_paymaster.test.ts | 2 +- yarn.lock | 8 ++-- 14 files changed, 62 insertions(+), 73 deletions(-) diff --git a/contracts/core/BasePaymaster.sol b/contracts/core/BasePaymaster.sol index b6a4540d..60bf4c69 100644 --- a/contracts/core/BasePaymaster.sol +++ b/contracts/core/BasePaymaster.sol @@ -16,7 +16,7 @@ import "./Helpers.sol"; abstract contract BasePaymaster is IPaymaster, Ownable { IEntryPoint public immutable entryPoint; - constructor(IEntryPoint _entryPoint) { + constructor(IEntryPoint _entryPoint) Ownable(msg.sender) { entryPoint = _entryPoint; } diff --git a/contracts/core/EntryPoint.sol b/contracts/core/EntryPoint.sol index f619874b..1bf9984c 100644 --- a/contracts/core/EntryPoint.sol +++ b/contracts/core/EntryPoint.sol @@ -1,6 +1,5 @@ // SPDX-License-Identifier: GPL-3.0 -pragma solidity ^0.8.12; - +pragma solidity ^0.8.23; /* solhint-disable avoid-low-level-calls */ /* solhint-disable no-inline-assembly */ @@ -17,7 +16,7 @@ import "./UserOperationLib.sol"; // we also require '@gnosis.pm/safe-contracts' and both libraries have 'IERC165.sol', leading to conflicts import "@openzeppelin/contracts/utils/introspection/ERC165.sol" as OpenZeppelin; -import "@openzeppelin/contracts/security/ReentrancyGuard.sol"; +import "@openzeppelin/contracts/utils/ReentrancyGuard.sol"; /* * Account-Abstraction (EIP-4337) singleton EntryPoint implementation. diff --git a/contracts/samples/SimpleAccount.sol b/contracts/samples/SimpleAccount.sol index 10f5d12b..ecdb19e0 100644 --- a/contracts/samples/SimpleAccount.sol +++ b/contracts/samples/SimpleAccount.sol @@ -1,14 +1,14 @@ // SPDX-License-Identifier: GPL-3.0 -pragma solidity ^0.8.12; +pragma solidity ^0.8.23; /* solhint-disable avoid-low-level-calls */ /* solhint-disable no-inline-assembly */ /* solhint-disable reason-string */ import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; +import "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; import "@openzeppelin/contracts/proxy/utils/Initializable.sol"; import "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; - import "../core/BaseAccount.sol"; import "./callback/TokenCallbackHandler.sol"; @@ -19,8 +19,6 @@ import "./callback/TokenCallbackHandler.sol"; * has a single signer that can send requests through the entryPoint. */ contract SimpleAccount is BaseAccount, TokenCallbackHandler, UUPSUpgradeable, Initializable { - using ECDSA for bytes32; - address public owner; IEntryPoint private immutable _entryPoint; @@ -37,7 +35,6 @@ contract SimpleAccount is BaseAccount, TokenCallbackHandler, UUPSUpgradeable, In return _entryPoint; } - // solhint-disable-next-line no-empty-blocks receive() external payable {} @@ -99,14 +96,14 @@ contract SimpleAccount is BaseAccount, TokenCallbackHandler, UUPSUpgradeable, In /// implement template method of BaseAccount function _validateSignature(UserOperation calldata userOp, bytes32 userOpHash) internal override virtual returns (uint256 validationData) { - bytes32 hash = userOpHash.toEthSignedMessageHash(); - if (owner != hash.recover(userOp.signature)) + bytes32 hash = MessageHashUtils.toEthSignedMessageHash(userOpHash); + if (owner != ECDSA.recover(hash, userOp.signature)) return SIG_VALIDATION_FAILED; return 0; } function _call(address target, uint256 value, bytes memory data) internal { - (bool success, bytes memory result) = target.call{value : value}(data); + (bool success, bytes memory result) = target.call{value: value}(data); if (!success) { assembly { revert(add(result, 32), mload(result)) @@ -125,7 +122,7 @@ contract SimpleAccount is BaseAccount, TokenCallbackHandler, UUPSUpgradeable, In * deposit more funds for this account in the entryPoint */ function addDeposit() public payable { - entryPoint().depositTo{value : msg.value}(address(this)); + entryPoint().depositTo{value: msg.value}(address(this)); } /** diff --git a/contracts/samples/VerifyingPaymaster.sol b/contracts/samples/VerifyingPaymaster.sol index 5dcf0b15..33036f6e 100644 --- a/contracts/samples/VerifyingPaymaster.sol +++ b/contracts/samples/VerifyingPaymaster.sol @@ -7,6 +7,7 @@ pragma solidity ^0.8.12; import "../core/BasePaymaster.sol"; import "../core/UserOperationLib.sol"; import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; +import "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; /** * A sample paymaster that uses external service to decide whether to pay for the UserOp. * The paymaster trusts an external signer to sign the transaction. @@ -18,7 +19,6 @@ import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; */ contract VerifyingPaymaster is BasePaymaster { - using ECDSA for bytes32; using UserOperationLib for UserOperation; address public immutable verifyingSigner; @@ -78,7 +78,7 @@ contract VerifyingPaymaster is BasePaymaster { //ECDSA library supports both 64 and 65-byte long signatures. // we only "require" it here so that the revert reason on invalid signature will be of "VerifyingPaymaster", and not "ECDSA" require(signature.length == 64 || signature.length == 65, "VerifyingPaymaster: invalid signature length in paymasterAndData"); - bytes32 hash = ECDSA.toEthSignedMessageHash(getHash(userOp, validUntil, validAfter)); + bytes32 hash = MessageHashUtils.toEthSignedMessageHash(getHash(userOp, validUntil, validAfter)); //don't revert on signature failure: return SIG_VALIDATION_FAILED if (verifyingSigner != ECDSA.recover(hash, signature)) { diff --git a/contracts/samples/callback/TokenCallbackHandler.sol b/contracts/samples/callback/TokenCallbackHandler.sol index d7ed9cbb..c4078525 100644 --- a/contracts/samples/callback/TokenCallbackHandler.sol +++ b/contracts/samples/callback/TokenCallbackHandler.sol @@ -4,7 +4,6 @@ pragma solidity ^0.8.12; /* solhint-disable no-empty-blocks */ import "@openzeppelin/contracts/utils/introspection/IERC165.sol"; -import "@openzeppelin/contracts/token/ERC777/IERC777Recipient.sol"; import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol"; import "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol"; @@ -12,16 +11,7 @@ import "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol"; * Token callback handler. * Handles supported tokens' callbacks, allowing account receiving these tokens. */ -contract TokenCallbackHandler is IERC777Recipient, IERC721Receiver, IERC1155Receiver { - function tokensReceived( - address, - address, - address, - uint256, - bytes calldata, - bytes calldata - ) external pure override { - } +contract TokenCallbackHandler is IERC721Receiver, IERC1155Receiver { function onERC721Received( address, diff --git a/contracts/test/TestExpiryAccount.sol b/contracts/test/TestExpiryAccount.sol index 294f4aaf..b613397e 100644 --- a/contracts/test/TestExpiryAccount.sol +++ b/contracts/test/TestExpiryAccount.sol @@ -11,7 +11,6 @@ import "../samples/SimpleAccount.sol"; * also, the "since" value is not really useful, only for testing the entrypoint. */ contract TestExpiryAccount is SimpleAccount { - using ECDSA for bytes32; mapping(address => uint48) public ownerAfter; mapping(address => uint48) public ownerUntil; @@ -37,8 +36,8 @@ contract TestExpiryAccount is SimpleAccount { /// implement template method of BaseAccount function _validateSignature(UserOperation calldata userOp, bytes32 userOpHash) internal override view returns (uint256 validationData) { - bytes32 hash = userOpHash.toEthSignedMessageHash(); - address signer = hash.recover(userOp.signature); + bytes32 hash = MessageHashUtils.toEthSignedMessageHash(userOpHash); + address signer = ECDSA.recover(hash,userOp.signature); uint48 _until = ownerUntil[signer]; uint48 _after = ownerAfter[signer]; diff --git a/hardhat.config.ts b/hardhat.config.ts index a76e49b0..b2c1a3fb 100644 --- a/hardhat.config.ts +++ b/hardhat.config.ts @@ -25,7 +25,7 @@ function getNetwork (name: string): { url: string, accounts: { mnemonic: string } const optimizedComilerSettings = { - version: '0.8.17', + version: '0.8.23', settings: { optimizer: { enabled: true, runs: 1000000 }, viaIR: true @@ -38,7 +38,7 @@ const optimizedComilerSettings = { const config: HardhatUserConfig = { solidity: { compilers: [{ - version: '0.8.15', + version: '0.8.23', settings: { optimizer: { enabled: true, runs: 1000000 } } diff --git a/package.json b/package.json index d7cfc162..e9980034 100644 --- a/package.json +++ b/package.json @@ -55,7 +55,7 @@ }, "dependencies": { "@nomiclabs/hardhat-etherscan": "^2.1.6", - "@openzeppelin/contracts": "^4.2.0", + "@openzeppelin/contracts": "^5.0.0", "@thehubbleproject/bls": "^0.5.1", "@typechain/hardhat": "^2.3.0", "@types/mocha": "^9.0.0", diff --git a/reports/gas-checker.txt b/reports/gas-checker.txt index a848178d..96f43322 100644 --- a/reports/gas-checker.txt +++ b/reports/gas-checker.txt @@ -2,9 +2,9 @@ the destination is "account.entryPoint()", which is known to be "hot" address used by this account it little higher than EOA call: its an exec from entrypoint (or account owner) into account contract, verifying msg.sender and exec to target) ╔══════════════════════════╤════════╗ -║ gas estimate "simple" │ 29014 ║ +║ gas estimate "simple" │ 28979 ║ ╟──────────────────────────┼────────╢ -║ gas estimate "big tx 5k" │ 125260 ║ +║ gas estimate "big tx 5k" │ 125224 ║ ╚══════════════════════════╧════════╝ ╔════════════════════════════════╤═══════╤═══════════════╤════════════════╤═════════════════════╗ @@ -12,44 +12,44 @@ ║ │ │ │ (delta for │ (compared to ║ ║ │ │ │ one UserOp) │ account.exec()) ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple │ 1 │ 81930 │ │ ║ +║ simple │ 1 │ 81905 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple - diff from previous │ 2 │ │ 44187 │ 15173 ║ +║ simple - diff from previous │ 2 │ │ 44177 │ 15198 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple │ 10 │ 479742 │ │ ║ +║ simple │ 10 │ 479663 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple - diff from previous │ 11 │ │ 44247 │ 15233 ║ +║ simple - diff from previous │ 11 │ │ 44165 │ 15186 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster │ 1 │ 88071 │ │ ║ +║ simple paymaster │ 1 │ 88055 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster with diff │ 2 │ │ 43022 │ 14008 ║ +║ simple paymaster with diff │ 2 │ │ 43033 │ 14054 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster │ 10 │ 475654 │ │ ║ +║ simple paymaster │ 10 │ 475497 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster with diff │ 11 │ │ 43105 │ 14091 ║ +║ simple paymaster with diff │ 11 │ │ 43092 │ 14113 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx 5k │ 1 │ 182987 │ │ ║ +║ big tx 5k │ 1 │ 182962 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx - diff from previous │ 2 │ │ 144698 │ 19438 ║ +║ big tx - diff from previous │ 2 │ │ 144676 │ 19452 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx 5k │ 10 │ 1485386 │ │ ║ +║ big tx 5k │ 10 │ 1485271 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx - diff from previous │ 11 │ │ 144759 │ 19499 ║ +║ big tx - diff from previous │ 11 │ │ 144737 │ 19513 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ paymaster+postOp │ 1 │ 89837 │ │ ║ +║ paymaster+postOp │ 1 │ 89761 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ paymaster+postOp with diff │ 2 │ │ 44808 │ 15794 ║ +║ paymaster+postOp with diff │ 2 │ │ 44771 │ 15792 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ paymaster+postOp │ 10 │ 493254 │ │ ║ +║ paymaster+postOp │ 10 │ 492869 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ paymaster+postOp with diff │ 11 │ │ 44880 │ 15866 ║ +║ paymaster+postOp with diff │ 11 │ │ 44795 │ 15816 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ token paymaster │ 1 │ 148256 │ │ ║ +║ token paymaster │ 1 │ 148295 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ token paymaster with diff │ 2 │ │ 72896 │ 43882 ║ +║ token paymaster with diff │ 2 │ │ 72950 │ 43971 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ token paymaster │ 10 │ 804843 │ │ ║ +║ token paymaster │ 10 │ 805308 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ token paymaster with diff │ 11 │ │ 73003 │ 43989 ║ +║ token paymaster with diff │ 11 │ │ 73069 │ 44090 ║ ╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝ diff --git a/test/entrypoint.test.ts b/test/entrypoint.test.ts index dc235517..ebc3acee 100644 --- a/test/entrypoint.test.ts +++ b/test/entrypoint.test.ts @@ -711,7 +711,9 @@ describe('EntryPoint', function () { }).then(async r => r.wait()) const error = rcpt.events?.find(ev => ev.event === 'UserOperationRevertReason') - expect(decodeRevertReason(error?.args?.revertReason)).to.eql('Error(ReentrancyGuard: reentrant call)', 'execution of handleOps inside a UserOp should revert') + // console.log(rcpt.events!.map(e => ({ ev: e.event, ...objdump(e.args!) }))) + + expect(decodeRevertReason(error?.args?.revertReason)).to.eql('ReentrancyGuardReentrantCall()', 'execution of handleOps inside a UserOp should revert') }) it('should report failure on insufficient verificationGas after creation', async () => { const op0 = await fillAndSign({ diff --git a/test/samples/TokenPaymaster.test.ts b/test/samples/TokenPaymaster.test.ts index 00752888..35d7aada 100644 --- a/test/samples/TokenPaymaster.test.ts +++ b/test/samples/TokenPaymaster.test.ts @@ -30,7 +30,7 @@ import { createAccountOwner, decodeRevertReason, deployEntryPoint, - fund + fund, objdump } from '../testutils' import { fillUserOp, signUserOp } from '../UserOp' @@ -156,13 +156,13 @@ describe('TokenPaymaster', function () { // await expect( expect(await entryPoint.handleOps([op], beneficiaryAddress, { gasLimit: 1e7 }) .catch(e => decodeRevertReason(e))) - .to.include('ERC20: insufficient allowance') + .to.match(/FailedOpWithRevert\(0,"AA33 reverted",ERC20InsufficientAllowance/) await token.sudoApprove(account.address, paymaster.address, ethers.constants.MaxUint256) expect(await entryPoint.handleOps([op], beneficiaryAddress, { gasLimit: 1e7 }) .catch(e => decodeRevertReason(e))) - .to.include('ERC20: transfer amount exceeds balance') + .to.match(/FailedOpWithRevert\(0,"AA33 reverted",ERC20InsufficientBalance/) await ethers.provider.send('evm_revert', [snapshot]) }) @@ -295,7 +295,7 @@ describe('TokenPaymaster', function () { await ethers.provider.send('evm_revert', [snapshot]) }) - it('should use cached token price if the one supplied by the client if it is worse', async function () { + it('should use cached token price if the one supplied by the client is worse', async function () { const snapshot = await ethers.provider.send('evm_snapshot', []) await token.transfer(account.address, parseEther('1')) await token.sudoApprove(account.address, paymaster.address, ethers.constants.MaxUint256) @@ -307,6 +307,7 @@ describe('TokenPaymaster', function () { const paymasterAndData = generatePaymasterAndData(paymasterAddress, overrideTokenPrice) let op = await fillUserOp({ sender: account.address, + maxFeePerGas: 1000000000, paymasterAndData, callData }, entryPoint) @@ -406,8 +407,10 @@ describe('TokenPaymaster', function () { const decodedLogs = tx.logs.map(it => { return testInterface.parseLog(it) }) + console.log(decodedLogs.map((e: any) => ({ ev: e.name, ...objdump(e.args!) }))) + const postOpRevertReason = decodeRevertReason(decodedLogs[2].args.revertReason) - assert.equal(postOpRevertReason, 'PostOpReverted(Error(ERC20: transfer amount exceeds balance))') + assert.include(postOpRevertReason, 'PostOpReverted(ERC20InsufficientBalance') const userOpSuccess = decodedLogs[3].args.success assert.equal(userOpSuccess, false) assert.equal(decodedLogs.length, 4) diff --git a/test/testutils.ts b/test/testutils.ts index d8c79a41..144b044b 100644 --- a/test/testutils.ts +++ b/test/testutils.ts @@ -15,7 +15,7 @@ import { SimpleAccountFactory__factory, SimpleAccount__factory, SimpleAccountFactory, - TestAggregatedAccountFactory, TestPaymasterRevertCustomError__factory + TestAggregatedAccountFactory, TestPaymasterRevertCustomError__factory, TestERC20__factory } from '../typechain' import { BytesLike } from '@ethersproject/bytes' import { expect } from 'chai' @@ -158,11 +158,11 @@ export function rethrow (): (e: Error) => void { } const decodeRevertReasonContracts = new Interface([ - ...[ - ...EntryPoint__factory.createInterface().fragments, - ...TestPaymasterRevertCustomError__factory.createInterface().fragments - ].filter(f => f.type === 'error') -]) // + ...EntryPoint__factory.createInterface().fragments, + ...TestPaymasterRevertCustomError__factory.createInterface().fragments, + ...TestERC20__factory.createInterface().fragments, // for OZ errors, + 'error ECDSAInvalidSignature()' +]) // .filter(f => f.type === 'error')) export function decodeRevertReason (data: string | Error, nullIfNoMatch = true): string | null { if (typeof data !== 'string') { @@ -172,6 +172,7 @@ export function decodeRevertReason (data: string | Error, nullIfNoMatch = true): const methodSig = data.slice(0, 10) const dataParams = '0x' + data.slice(10) + // can't add Error(string) to xface... if (methodSig === '0x08c379a0') { const [err] = ethers.utils.defaultAbiCoder.decode(['string'], dataParams) // eslint-disable-next-line @typescript-eslint/restrict-template-expressions @@ -183,8 +184,6 @@ export function decodeRevertReason (data: string | Error, nullIfNoMatch = true): try { const err = decodeRevertReasonContracts.parseError(data) - // let args: any[] = err.args as any - // treat any error "bytes" argument as possible error to decode (e.g. FailedOpWithRevert, PostOpReverted) const args = err.args.map((arg: any, index) => { switch (err.errorFragment.inputs[index].type) { diff --git a/test/verifying_paymaster.test.ts b/test/verifying_paymaster.test.ts index 6c8ee729..b95d92f8 100644 --- a/test/verifying_paymaster.test.ts +++ b/test/verifying_paymaster.test.ts @@ -70,7 +70,7 @@ describe('EntryPoint with VerifyingPaymaster', function () { }, accountOwner, entryPoint) expect(await simulateValidation(userOp, entryPoint.address) .catch(e => decodeRevertReason(e))) - .to.include('ECDSA: invalid signature') + .to.include('ECDSAInvalidSignature') }) describe('with wrong signature', () => { diff --git a/yarn.lock b/yarn.lock index ba215488..97ad2333 100644 --- a/yarn.lock +++ b/yarn.lock @@ -898,10 +898,10 @@ resolved "https://registry.yarnpkg.com/@openzeppelin/contracts/-/contracts-3.4.2-solc-0.7.tgz#38f4dbab672631034076ccdf2f3201fab1726635" integrity sha512-W6QmqgkADuFcTLzHL8vVoNBtkwjvQRpYIAom7KiUNoLKghyx3FgH0GBjt8NRvigV1ZmMOBllvE1By1C+bi8WpA== -"@openzeppelin/contracts@^4.2.0": - version "4.9.3" - resolved "https://registry.yarnpkg.com/@openzeppelin/contracts/-/contracts-4.9.3.tgz#00d7a8cf35a475b160b3f0293a6403c511099364" - integrity sha512-He3LieZ1pP2TNt5JbkPA4PNT9WC3gOTOlDcFGJW4Le4QKqwmiNJCRt44APfxMxvq7OugU/cqYuPcSBzOw38DAg== +"@openzeppelin/contracts@^5.0.0": + version "5.0.0" + resolved "https://registry.yarnpkg.com/@openzeppelin/contracts/-/contracts-5.0.0.tgz#ee0e4b4564f101a5c4ee398cd4d73c0bd92b289c" + integrity sha512-bv2sdS6LKqVVMLI5+zqnNrNU/CA+6z6CmwFXm/MzmOPBRSO5reEJN7z0Gbzvs0/bv/MZZXNklubpwy3v2+azsw== "@resolver-engine/core@^0.3.3": version "0.3.3" From 346ab2d25673f83cbda8f62f9bb861201f0e7b38 Mon Sep 17 00:00:00 2001 From: Dror Tirosh Date: Wed, 3 Jan 2024 16:36:38 +0200 Subject: [PATCH 4/4] AA-237: paymaster: check EntryPoint's interface (#370) * AA-237: paymaster: check EntryPoint's interface verify the paymaster is configured with EntryPoint with the expected IEntryPoint interface. * update sample for contract with supportsInterface() method --- contracts/core/BasePaymaster.sol | 8 ++++++++ reports/gas-checker.txt | 12 ++++++------ test/paymaster.test.ts | 11 ++++++++++- test/samples/OracleHelper.test.ts | 14 ++++++++++---- 4 files changed, 34 insertions(+), 11 deletions(-) diff --git a/contracts/core/BasePaymaster.sol b/contracts/core/BasePaymaster.sol index 60bf4c69..dee42b2d 100644 --- a/contracts/core/BasePaymaster.sol +++ b/contracts/core/BasePaymaster.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.12; /* solhint-disable reason-string */ import "@openzeppelin/contracts/access/Ownable.sol"; +import "@openzeppelin/contracts/utils/introspection/IERC165.sol"; import "../interfaces/IPaymaster.sol"; import "../interfaces/IEntryPoint.sol"; import "./Helpers.sol"; @@ -17,9 +18,16 @@ abstract contract BasePaymaster is IPaymaster, Ownable { IEntryPoint public immutable entryPoint; constructor(IEntryPoint _entryPoint) Ownable(msg.sender) { + _validateEntryPointInterface(_entryPoint); entryPoint = _entryPoint; } + //sanity check: make sure this EntryPoint was compiled against the same + // IEntryPoint of this paymaster + function _validateEntryPointInterface(IEntryPoint _entryPoint) internal virtual { + require(IERC165(address(_entryPoint)).supportsInterface(type(IEntryPoint).interfaceId), "IEntryPoint interface mismatch"); + } + /// @inheritdoc IPaymaster function validatePaymasterUserOp( UserOperation calldata userOp, diff --git a/reports/gas-checker.txt b/reports/gas-checker.txt index 96f43322..210d20be 100644 --- a/reports/gas-checker.txt +++ b/reports/gas-checker.txt @@ -20,13 +20,13 @@ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ ║ simple - diff from previous │ 11 │ │ 44165 │ 15186 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster │ 1 │ 88055 │ │ ║ +║ simple paymaster │ 1 │ 88043 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster with diff │ 2 │ │ 43033 │ 14054 ║ +║ simple paymaster with diff │ 2 │ │ 43021 │ 14042 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ ║ simple paymaster │ 10 │ 475497 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster with diff │ 11 │ │ 43092 │ 14113 ║ +║ simple paymaster with diff │ 11 │ │ 43080 │ 14101 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ ║ big tx 5k │ 1 │ 182962 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ @@ -42,14 +42,14 @@ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ ║ paymaster+postOp │ 10 │ 492869 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ paymaster+postOp with diff │ 11 │ │ 44795 │ 15816 ║ +║ paymaster+postOp with diff │ 11 │ │ 44831 │ 15852 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ ║ token paymaster │ 1 │ 148295 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ ║ token paymaster with diff │ 2 │ │ 72950 │ 43971 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ token paymaster │ 10 │ 805308 │ │ ║ +║ token paymaster │ 10 │ 805344 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ token paymaster with diff │ 11 │ │ 73069 │ 44090 ║ +║ token paymaster with diff │ 11 │ │ 73021 │ 44042 ║ ╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝ diff --git a/test/paymaster.test.ts b/test/paymaster.test.ts index d5806397..5e5e0544 100644 --- a/test/paymaster.test.ts +++ b/test/paymaster.test.ts @@ -7,7 +7,7 @@ import { LegacyTokenPaymaster__factory, TestCounter__factory, SimpleAccountFactory, - SimpleAccountFactory__factory, EntryPoint + SimpleAccountFactory__factory, EntryPoint, TokenCallbackHandler__factory } from '../typechain' import { AddressZero, @@ -68,6 +68,15 @@ describe('EntryPoint with paymaster', function () { ownerAddr = await ethersSigner.getAddress() }) + it('paymaster should revert on wrong entryPoint type', async () => { + const notEntryPoint = await new TokenCallbackHandler__factory(ethersSigner).deploy() + // a contract that has "supportsInterface" but with different interface value.. + await expect(new LegacyTokenPaymaster__factory(ethersSigner).deploy(factory.address, 'ttt', notEntryPoint.address)) + .to.be.revertedWith('IEntryPoint interface mismatch') + await expect(new LegacyTokenPaymaster__factory(ethersSigner).deploy(factory.address, 'ttt', AddressZero)) + .to.be.revertedWith('') + }) + it('owner should have allowance to withdraw funds', async () => { expect(await paymaster.allowance(pmAddr, ownerAddr)).to.equal(ethers.constants.MaxUint256) expect(await paymaster.allowance(pmAddr, otherAddr)).to.equal(0) diff --git a/test/samples/OracleHelper.test.ts b/test/samples/OracleHelper.test.ts index 1dd05390..eae60b8b 100644 --- a/test/samples/OracleHelper.test.ts +++ b/test/samples/OracleHelper.test.ts @@ -4,6 +4,8 @@ import { ethers } from 'hardhat' import { AddressZero } from '../testutils' import { + EntryPoint, + EntryPoint__factory, TestERC20, TestERC20__factory, TestOracle2, @@ -102,6 +104,8 @@ describe('OracleHelper', function () { // @ts-ignore const testEnv: TestEnv = {} + let entryPoint: EntryPoint + before(async function () { const ethersSigner = ethers.provider.getSigner() testEnv.owner = await ethersSigner.getAddress() @@ -124,9 +128,11 @@ describe('OracleHelper', function () { testEnv.token = await new TestERC20__factory(ethersSigner).deploy(18) + entryPoint = await new EntryPoint__factory(ethersSigner).deploy() + testEnv.paymaster = await new TokenPaymaster__factory(ethersSigner).deploy( testEnv.token.address, - AddressZero, + entryPoint.address, AddressZero, testEnv.owner, // cannot approve to AddressZero testEnv.tokenPaymasterConfig, @@ -164,7 +170,7 @@ describe('OracleHelper', function () { const ethersSigner = ethers.provider.getSigner() testEnv.paymaster = await new TokenPaymaster__factory(ethersSigner).deploy( testEnv.token.address, - AddressZero, + entryPoint.address, AddressZero, testEnv.owner, // cannot approve to AddressZero testEnv.tokenPaymasterConfig, @@ -215,7 +221,7 @@ describe('OracleHelper', function () { const ethersSigner = ethers.provider.getSigner() testEnv.paymaster = await new TokenPaymaster__factory(ethersSigner).deploy( testEnv.token.address, - AddressZero, + entryPoint.address, AddressZero, testEnv.owner, // cannot approve to AddressZero testEnv.tokenPaymasterConfig, @@ -245,7 +251,7 @@ describe('OracleHelper', function () { const ethersSigner = ethers.provider.getSigner() testEnv.paymaster = await new TokenPaymaster__factory(ethersSigner).deploy( testEnv.token.address, - AddressZero, + entryPoint.address, AddressZero, testEnv.owner, // cannot approve to AddressZero testEnv.tokenPaymasterConfig,