From b3991970052dec0d457d3d6b914a9d29d23d6b1f Mon Sep 17 00:00:00 2001 From: Dror Tirosh Date: Sun, 11 Feb 2024 19:54:14 +0200 Subject: [PATCH] remove AA40 - global verification gas check (#447) * remove AA40, and instead add separate gas check for paymaster and for factory/account This way, staked entity that cause verification over-gas usage can't escape from blaming. * verificationGasLimit to cover also nonce increment * test AA26, AA36 --- contracts/core/EntryPoint.sol | 29 ++++++---- reports/gas-checker.txt | 40 +++++++------- test/UserOp.ts | 2 +- test/entrypointsimulations.test.ts | 77 ++++++++++++++++++++++++-- test/testutils.ts | 88 +++++++++++++++++++++++++++++- 5 files changed, 200 insertions(+), 36 deletions(-) diff --git a/contracts/core/EntryPoint.sol b/contracts/core/EntryPoint.sol index 6f5006198..0a71565dc 100644 --- a/contracts/core/EntryPoint.sol +++ b/contracts/core/EntryPoint.sol @@ -441,7 +441,8 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, uint256 opIndex, PackedUserOperation calldata op, UserOpInfo memory opInfo, - uint256 requiredPrefund + uint256 requiredPrefund, + uint256 verificationGasLimit ) internal returns ( @@ -462,7 +463,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, } try IAccount(sender).validateUserOp{ - gas: mUserOp.verificationGasLimit + gas: verificationGasLimit }(op, opInfo.userOpHash, missingAccountFunds) returns (uint256 _validationData) { validationData = _validationData; @@ -498,6 +499,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, uint256 requiredPreFund ) internal returns (bytes memory context, uint256 validationData) { unchecked { + uint256 preGas = gasleft(); MemoryUserOp memory mUserOp = opInfo.mUserOp; address paymaster = mUserOp.paymaster; DepositInfo storage paymasterInfo = deposits[paymaster]; @@ -506,8 +508,9 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, revert FailedOp(opIndex, "AA31 paymaster deposit too low"); } paymasterInfo.deposit = deposit - requiredPreFund; + uint256 pmVerificationGasLimit = mUserOp.paymasterVerificationGasLimit; try - IPaymaster(paymaster).validatePaymasterUserOp{gas: mUserOp.paymasterVerificationGasLimit}( + IPaymaster(paymaster).validatePaymasterUserOp{gas: pmVerificationGasLimit}( op, opInfo.userOpHash, requiredPreFund @@ -518,6 +521,9 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, } catch { revert FailedOpWithRevert(opIndex, "AA33 reverted", Exec.getReturnData(REVERT_REASON_MAX_LEN)); } + if (preGas - gasleft() > pmVerificationGasLimit) { + revert FailedOp(opIndex, "AA36 over paymasterVerificationGasLimit"); + } } } @@ -597,8 +603,9 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, // Validate all numeric values in userOp are well below 128 bit, so they can safely be added // and multiplied without causing overflow. + uint256 verificationGasLimit = mUserOp.verificationGasLimit; uint256 maxGasValues = mUserOp.preVerificationGas | - mUserOp.verificationGasLimit | + verificationGasLimit | mUserOp.callGasLimit | mUserOp.paymasterVerificationGasLimit | mUserOp.paymasterPostOpGasLimit | @@ -611,13 +618,20 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, opIndex, userOp, outOpInfo, - requiredPreFund + requiredPreFund, + verificationGasLimit ); if (!_validateAndUpdateNonce(mUserOp.sender, mUserOp.nonce)) { revert FailedOp(opIndex, "AA25 invalid account nonce"); } + unchecked { + if (preGas - gasleft() > verificationGasLimit) { + revert FailedOp(opIndex, "AA26 over verificationGasLimit"); + } + } + bytes memory context; if (mUserOp.paymaster != address(0)) { (context, paymasterValidationData) = _validatePaymasterPrepayment( @@ -628,11 +642,6 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, ); } unchecked { - uint256 gasUsed = preGas - gasleft(); - - if (mUserOp.verificationGasLimit + mUserOp.paymasterVerificationGasLimit < gasUsed) { - revert FailedOp(opIndex, "AA40 over verificationGasLimit"); - } outOpInfo.prefund = requiredPreFund; outOpInfo.contextOffset = getOffsetOfMemoryBytes(context); outOpInfo.preOpGas = preGas - gasleft() + userOp.preVerificationGas; diff --git a/reports/gas-checker.txt b/reports/gas-checker.txt index e8909ae54..dd125a346 100644 --- a/reports/gas-checker.txt +++ b/reports/gas-checker.txt @@ -12,44 +12,44 @@ ║ │ │ │ (delta for │ (compared to ║ ║ │ │ │ one UserOp) │ account.exec()) ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple │ 1 │ 79958 │ │ ║ +║ simple │ 1 │ 79954 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple - diff from previous │ 2 │ │ 42156 │ 13177 ║ +║ simple - diff from previous │ 2 │ │ 42176 │ 13197 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple │ 10 │ 459549 │ │ ║ +║ simple │ 10 │ 459617 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple - diff from previous │ 11 │ │ 42211 │ 13232 ║ +║ simple - diff from previous │ 11 │ │ 42243 │ 13264 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster │ 1 │ 86060 │ │ ║ +║ simple paymaster │ 1 │ 86097 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster with diff │ 2 │ │ 40971 │ 11992 ║ +║ simple paymaster with diff │ 2 │ │ 41032 │ 12053 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster │ 10 │ 454974 │ │ ║ +║ simple paymaster │ 10 │ 455440 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ simple paymaster with diff │ 11 │ │ 40975 │ 11996 ║ +║ simple paymaster with diff │ 11 │ │ 41060 │ 12081 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx 5k │ 1 │ 181002 │ │ ║ +║ big tx 5k │ 1 │ 181010 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx - diff from previous │ 2 │ │ 142666 │ 17442 ║ +║ big tx - diff from previous │ 2 │ │ 142662 │ 17438 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx 5k │ 10 │ 1465047 │ │ ║ +║ big tx 5k │ 10 │ 1465175 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ big tx - diff from previous │ 11 │ │ 142710 │ 17486 ║ +║ big tx - diff from previous │ 11 │ │ 142682 │ 17458 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ paymaster+postOp │ 1 │ 87725 │ │ ║ +║ paymaster+postOp │ 1 │ 87774 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ paymaster+postOp with diff │ 2 │ │ 42636 │ 13657 ║ +║ paymaster+postOp with diff │ 2 │ │ 42685 │ 13706 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ paymaster+postOp │ 10 │ 471608 │ │ ║ +║ paymaster+postOp │ 10 │ 472098 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ paymaster+postOp with diff │ 11 │ │ 42681 │ 13702 ║ +║ paymaster+postOp with diff │ 11 │ │ 42730 │ 13751 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ token paymaster │ 1 │ 128754 │ │ ║ +║ token paymaster │ 1 │ 128791 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ token paymaster with diff │ 2 │ │ 66363 │ 37384 ║ +║ token paymaster with diff │ 2 │ │ 66400 │ 37421 ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ token paymaster │ 10 │ 726214 │ │ ║ +║ token paymaster │ 10 │ 726704 │ │ ║ ╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢ -║ token paymaster with diff │ 11 │ │ 66431 │ 37452 ║ +║ token paymaster with diff │ 11 │ │ 66480 │ 37501 ║ ╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝ diff --git a/test/UserOp.ts b/test/UserOp.ts index 8993cb45e..2d8ddd5b2 100644 --- a/test/UserOp.ts +++ b/test/UserOp.ts @@ -29,7 +29,7 @@ export function packUserOp (userOp: UserOperation): PackedUserOperation { const accountGasLimits = packAccountGasLimits(userOp.verificationGasLimit, userOp.callGasLimit) const gasFees = packAccountGasLimits(userOp.maxPriorityFeePerGas, userOp.maxFeePerGas) let paymasterAndData = '0x' - if (userOp.paymaster.length >= 20 && userOp.paymaster !== AddressZero) { + if (userOp.paymaster?.length >= 20 && userOp.paymaster !== AddressZero) { paymasterAndData = packPaymasterData(userOp.paymaster as string, userOp.paymasterVerificationGasLimit, userOp.paymasterPostOpGasLimit, userOp.paymasterData as string) } return { diff --git a/test/entrypointsimulations.test.ts b/test/entrypointsimulations.test.ts index d5121677d..fe3205256 100644 --- a/test/entrypointsimulations.test.ts +++ b/test/entrypointsimulations.test.ts @@ -7,7 +7,9 @@ import { SimpleAccountFactory, SimpleAccountFactory__factory, SimpleAccount__factory, - TestCounter__factory + TestCounter__factory, + TestPaymasterWithPostOp, + TestPaymasterWithPostOp__factory } from '../typechain' import { ONE_ETH, @@ -17,12 +19,13 @@ import { fund, getAccountAddress, getAccountInitCode, - getBalance, deployEntryPoint + getBalance, deployEntryPoint, decodeRevertReason, findSimulationUserOpWithMin, findUserOpWithMin } from './testutils' -import { fillSignAndPack, simulateHandleOp, simulateValidation } from './UserOp' +import { fillAndSign, fillSignAndPack, packUserOp, simulateHandleOp, simulateValidation } from './UserOp' import { BigNumber, Wallet } from 'ethers' -import { hexConcat } from 'ethers/lib/utils' +import { hexConcat, parseEther } from 'ethers/lib/utils' +import { UserOperation } from './UserOperation' const provider = ethers.provider describe('EntryPointSimulations', function () { @@ -243,6 +246,72 @@ describe('EntryPointSimulations', function () { }) }) + describe('over-validation test', () => { + // coverage skews gas checks. + if (process.env.COVERAGE != null) { + return + } + + let vgl: number + let pmVgl: number + let paymaster: TestPaymasterWithPostOp + let sender: string + let owner: Wallet + async function userOpWithGas (vgl: number, pmVgl = 0): Promise { + return fillAndSign({ + sender, + verificationGasLimit: vgl, + paymaster: pmVgl !== 0 ? paymaster.address : undefined, + paymasterVerificationGasLimit: pmVgl, + paymasterPostOpGasLimit: pmVgl, + maxFeePerGas: 1, + maxPriorityFeePerGas: 1 + }, owner, entryPoint) + } + before(async () => { + owner = createAccountOwner() + paymaster = await new TestPaymasterWithPostOp__factory(ethersSigner).deploy(entryPoint.address) + await entryPoint.depositTo(paymaster.address, { value: parseEther('1') }) + const { proxy: account } = await createAccount(ethersSigner, owner.address, entryPoint.address) + sender = account.address + await fund(account) + pmVgl = await findSimulationUserOpWithMin(async n => userOpWithGas(1e6, n), entryPoint, 1, 500000) + vgl = await findSimulationUserOpWithMin(async n => userOpWithGas(n, pmVgl), entryPoint, 3000, 500000) + + const userOp = await userOpWithGas(vgl, pmVgl) + + await simulateValidation(packUserOp(userOp), entryPoint.address) + .catch(e => { throw new Error(decodeRevertReason(e)!) }) + }) + describe('compare to execution', () => { + let execVgl: number + let execPmVgl: number + const diff = 2000 + before(async () => { + execPmVgl = await findUserOpWithMin(async n => userOpWithGas(1e6, n), false, entryPoint, 1, 500000) + execVgl = await findUserOpWithMin(async n => userOpWithGas(n, execPmVgl), false, entryPoint, 1, 500000) + }) + it('account verification simulation cost should be higher than execution', function () { + console.log('simulation account validation', vgl, 'above exec:', vgl - execVgl) + expect(vgl).to.be.within(execVgl + 1, execVgl + diff, `expected simulation verificationGas to be 1..${diff} above actual, but was ${vgl - execVgl}`) + }) + it('paymaster verification simulation cost should be higher than execution', function () { + console.log('simulation paymaster validation', pmVgl, 'above exec:', pmVgl - execPmVgl) + expect(pmVgl).to.be.within(execPmVgl + 1, execPmVgl + diff, `expected simulation verificationGas to be 1..${diff} above actual, but was ${pmVgl - execPmVgl}`) + }) + }) + it('should revert with AA2x if verificationGasLimit is low', async function () { + expect(await simulateValidation(packUserOp(await userOpWithGas(vgl - 1, pmVgl)), entryPoint.address) + .catch(decodeRevertReason)) + .to.match(/AA26/) + }) + it('should revert with AA3x if paymasterVerificationGasLimit is low', async function () { + expect(await simulateValidation(packUserOp(await userOpWithGas(vgl, pmVgl - 1)), entryPoint.address) + .catch(decodeRevertReason)) + .to.match(/AA36/) + }) + }) + describe('#simulateHandleOp', () => { it('should simulate creation', async () => { const accountOwner1 = createAccountOwner() diff --git a/test/testutils.ts b/test/testutils.ts index 59a8c16c8..14370a7f2 100644 --- a/test/testutils.ts +++ b/test/testutils.ts @@ -23,6 +23,8 @@ import { BytesLike, Hexable } from '@ethersproject/bytes' import { expect } from 'chai' import { Create2Factory } from '../src/Create2Factory' import { debugTransaction } from './debugTx' +import { UserOperation } from './UserOperation' +import { packUserOp, simulateValidation } from './UserOp' export const AddressZero = ethers.constants.AddressZero export const HashZero = ethers.constants.HashZero @@ -168,8 +170,10 @@ const decodeRevertReasonContracts = new Interface([ export function decodeRevertReason (data: string | Error, nullIfNoMatch = true): string | null { if (typeof data !== 'string') { const err = data as any - data = (err.data ?? err.error.data) as string + data = (err.data ?? err.error?.data) as string + if (typeof data !== 'string') throw err } + const methodSig = data.slice(0, 10) const dataParams = '0x' + data.slice(10) @@ -351,3 +355,85 @@ export function packValidationData (validationData: ValidationData): BigNumber { .add(validationData.validUntil).shl(160) .add(validationData.aggregator) } + +// find the lowest number in the range min..max where testFunc returns true +export async function findMin (testFunc: (index: number) => Promise, min: number, max: number, delta = 5): Promise { + if (await testFunc(min)) { + throw new Error(`increase range: function already true at ${min}`) + } + if (!await testFunc(max)) { + throw new Error(`no result: function is false for max value in ${min}..${max}`) + } + while (true) { + const avg = Math.floor((max + min) / 2) + if (await testFunc(avg)) { + max = avg + } else { + min = avg + } + // console.log('== ', min, '...', max, max - min) + if (Math.abs(max - min) < delta) { + return max + } + } +} + +/** + * find the lowest value that when creating a userop, still doesn't revert and + * doesn't emit UserOperationPrefundTooLow + * note: using eth_snapshot/eth_revert, since we actually submit calls to handleOps + * @param f function that return a signed userop, with parameter-under-test set to "n" + * @param min range minimum. the function is expected to return false + * @param max range maximum. the function is expected to be true + * @param entryPoint entrypoint for "fillAndSign" of userops + */ +export async function findUserOpWithMin (f: (n: number) => Promise, expectExec: boolean, entryPoint: EntryPoint, min: number, max: number, delta = 2): Promise { + const beneficiary = ethers.provider.getSigner().getAddress() + return await findMin( + async n => { + const snapshot = await ethers.provider.send('evm_snapshot', []) + try { + const userOp = await f(n) + // console.log('== userop=', userOp) + const rcpt = await entryPoint.handleOps([packUserOp(userOp)], beneficiary, { gasLimit: 1e6 }) + .then(async r => r.wait()) + if (rcpt?.events?.find(e => e.event === 'UserOperationPrefundTooLow') != null) { + // console.log('min', n, 'UserOperationPrefundTooLow') + return false + } + if (expectExec) { + const useropEvent = rcpt?.events?.find(e => e.event === 'UserOperationEvent') + if (useropEvent?.args?.success !== true) { + // console.log(rcpt?.events?.map((e: any) => ({ ev: e.event, ...objdump(e.args!) }))) + + // console.log('min', n, 'success=false') + return false + } + } + // console.log('min', n, 'ok') + return true + } catch (e) { + // console.log('min', n, 'ex=', decodeRevertReason(e as Error)) + return false + } finally { + await ethers.provider.send('evm_revert', [snapshot]) + } + }, min, max, delta + ) +} + +export async function findSimulationUserOpWithMin (f: (n: number) => Promise, entryPoint: EntryPoint, min: number, max: number, delta = 2): Promise { + return await findMin( + async n => { + try { + const userOp = await f(n) + await simulateValidation(packUserOp(userOp), entryPoint.address) + // console.log('sim', n, 'ok') + return true + } catch (e) { + // console.log('sim', n, 'ex=', decodeRevertReason(e as Error)) + return false + } + }, min, max, delta + ) +}