Skip to content

Commit

Permalink
remove AA40 - global verification gas check (#447)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@drortirosh
drortirosh authored Feb 11, 2024
1 parent 549a29b commit b399197
Show file tree
Hide file tree
Showing 5 changed files with 200 additions and 36 deletions.
29 changes: 19 additions & 10 deletions contracts/core/EntryPoint.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -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;
Expand Down Expand Up @@ -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];
Expand All @@ -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
Expand All @@ -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");
}
}
}

Expand Down Expand Up @@ -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 |
Expand All @@ -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(
Expand All @@ -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;
Expand Down
40 changes: 20 additions & 20 deletions reports/gas-checker.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 ║
╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝

2 changes: 1 addition & 1 deletion test/UserOp.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
77 changes: 73 additions & 4 deletions test/entrypointsimulations.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ import {
SimpleAccountFactory,
SimpleAccountFactory__factory,
SimpleAccount__factory,
TestCounter__factory
TestCounter__factory,
TestPaymasterWithPostOp,
TestPaymasterWithPostOp__factory
} from '../typechain'
import {
ONE_ETH,
Expand All @@ -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 () {
Expand Down Expand Up @@ -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<UserOperation> {
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()
Expand Down
Loading

0 comments on commit b399197

Please sign in to comment.