Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove AA40 - global verification gas check #447

Merged
merged 6 commits into from Feb 11, 2024
Merged

remove AA40 - global verification gas check #447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
40a4af2
remove AA40 - global verification gas check
drortirosh Feb 1, 2024
3caa62e
formatting
drortirosh Feb 8, 2024
e9164de
verificationGasLimit to cover also nonce increment
drortirosh Feb 9, 2024
277e9db
test AA26, AA36
drortirosh Feb 11, 2024
eb29f85
remove estimations
drortirosh Feb 11, 2024
4126ec5
Merge branch 'develop' into remove-AA40-verification-gas-check
drortirosh Feb 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 19 additions & 10 deletions contracts/core/EntryPoint.sol
View file
Open in desktop
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why this variable makes things more efficient than accessing mUserOp.paymasterVerificationGasLimit directly below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use it twice in this method. so it saves (a little..) to keep the memory var on the stack.

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");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor issue, why not incorporate the Nonce verification into _validateAccountPrepayment? It seems that doing so would make the responsibilities of each method clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the method names declare their responsibilities:

  • _validateAccountPrepayment - performs calls validateUserOp, and validates its response (the payment).
  • _validateAndUpdateNonce - performs nonce validation
    (yes, we could wrap them both in "account validation), which does nothing but call them both)

}

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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We removed this since we now check each of the limits separately. Is it guaranteed that this can no longer happen even with EntryPoint overhead? I assume it is, just checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right. we didn't cover some validations. Now account validation gas limit would cover also nonce check and copy

}
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
Open in desktop
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
Open in desktop
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
Open in desktop
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