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

AA-527 handleOps should process validateUserOp returning wrong data size #533

Merged
merged 15 commits into from
Feb 16, 2025
Merged

AA-527 handleOps should process validateUserOp returning wrong data size #533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f4724e7
use callValidateUserOp instead of sender.validateUserOp
drortirosh Feb 2, 2025
6048b95
optmized but actually costs more
drortirosh Feb 2, 2025
e5f9787
added opIndex to FailedOp
drortirosh Feb 2, 2025
b61bccf
more FailedOp instead of Error
drortirosh Feb 2, 2025
43124e5
cleanup saveFreePtr/restoreFreePtr
drortirosh Feb 4, 2025
c571011
cleaup copy of legacy contracts
drortirosh Feb 4, 2025
d09fc31
pr issue
drortirosh Feb 5, 2025
057e4a3
Merge branch 'develop' into callValidateUserOp-catch-revert
drortirosh Feb 6, 2025
ad58580
assembly: use if instead of mul
drortirosh Feb 9, 2025
cf22e10
gascalc
drortirosh Feb 9, 2025
1575598
return value failure check
drortirosh Feb 10, 2025
0235fb3
lint
drortirosh Feb 11, 2025
683b2c8
Merge branch 'develop' into callValidateUserOp-catch-revert
drortirosh Feb 16, 2025
4705bd6
Merge branch 'develop' into callValidateUserOp-catch-revert
drortirosh Feb 16, 2025
e48063b
update Simple7702Account
drortirosh Feb 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 54 additions & 25 deletions contracts/core/EntryPoint.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT
bytes memory context = getMemoryBytesFromOffset(opInfo.contextOffset);
bool success;
{
uint256 saveFreePtr;
assembly ("memory-safe") {
saveFreePtr := mload(0x40)
}
uint256 saveFreePtr = getFreePtr();
bytes calldata callData = userOp.callData;
bytes memory innerCall;
bytes4 methodSig;
Expand All @@ -119,8 +116,8 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT
assembly ("memory-safe") {
success := call(gas(), address(), 0, add(innerCall, 0x20), mload(innerCall), 0, 32)
collected := mload(0)
mstore(0x40, saveFreePtr)
}
restoreFreePtr(saveFreePtr);
}
if (!success) {
bytes32 innerRevertCode;
Expand Down Expand Up @@ -231,7 +228,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT
//address(1) is special marker of "signature error"
require(
address(aggregator) != address(1),
"AA96 invalid aggregator"
FailedOp(totalOps + i, "AA96 invalid aggregator")
);

if (address(aggregator) != address(0)) {
Expand Down Expand Up @@ -493,8 +490,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT
uint256 opIndex,
PackedUserOperation calldata op,
UserOpInfo memory opInfo,
uint256 requiredPrefund,
uint256 verificationGasLimit
uint256 requiredPrefund
)
internal
returns (
Expand All @@ -513,15 +509,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT
? 0
: requiredPrefund - bal;
}
try
IAccount(sender).validateUserOp{
gas: verificationGasLimit
}(op, opInfo.userOpHash, missingAccountFunds)
returns (uint256 _validationData) {
validationData = _validationData;
} catch {
revert FailedOpWithRevert(opIndex, "AA23 reverted", Exec.getReturnData(REVERT_REASON_MAX_LEN));
}
validationData = _callValidateUserOp(op, opInfo, missingAccountFunds, opIndex);
if (paymaster == address(0)) {
DepositInfo storage senderInfo = deposits[sender];
uint256 deposit = senderInfo.deposit;
Expand All @@ -533,6 +521,33 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT
}
}

// call sender.validateUserOp()
// handle wrong output size with FailedOp
function _callValidateUserOp(PackedUserOperation calldata op, UserOpInfo memory opInfo, uint256 missingAccountFunds, uint256 opIndex)
internal returns (uint256 validationData) {
uint256 saveFreePtr = getFreePtr();
bytes memory callData = abi.encodeCall(IAccount.validateUserOp, (op, opInfo.userOpHash, missingAccountFunds));
uint256 gasLimit = opInfo.mUserOp.verificationGasLimit;
address sender = opInfo.mUserOp.sender;
bool success;
assembly ("memory-safe"){
success := call(gasLimit, sender, 0, add(callData, 0x20), mload(callData), 0, 32)
validationData := mload(0)
// any return data size other than 32 is considered failure
if iszero(eq(returndatasize(), 32)) {
success := 0
}
}
restoreFreePtr(saveFreePtr);
if (!success) {
if(sender.code.length == 0) {
revert FailedOp(opIndex, "AA20 account not deployed");
} else {
revert FailedOpWithRevert(opIndex, "AA23 reverted", Exec.getReturnData(REVERT_REASON_MAX_LEN));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking about it a little more.
I don't know if that's important, but technically it is not checked here if the call reverted or not, only that the return data is not of the correct length. However, if the call did revert, you don't even load it's return data so the emitter even will always be empty, right?
Does it matter? I think it may be useful to see the revert message if there is a way to do it, especially if the UserOp got on-chain and reverted. It could be two different events - FailedOpWithRevert and FailedOpWithBadValidationData. What do you think?
Or at least, maybe change the string to AA23 wrong return or something like that?

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 check only the size==32, but the assembly code above makes any differrent return value size (either revert or return data) to be size=0
so checking "size==32" actually means "didn't revert, AND size is 32"
thus no-account causes a size=0 with no revert, but handled by this exception case.
also, assembly { return (0,0) } (or any other assembly-level return with size!=32) would be treated as revert.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand what you did, I am asking if that's actually the right thing to do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean? success is defined as "call didn't revert AND datasize is 32"
since I have to use assembly, I prefer to do the AND also in assembly, and expose only the result to solidity.
I could reverse the terms: have "bool success", and clear it if datasize!=32... it has the same impact, only that the solidity code ends up doing if (success) { ... }

Copy link
Collaborator

@forshtat forshtat Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

There are two distinct failure cases:

  1. The call reverted
  2. The call retuned value of wrong size

I know there is no practical difference to us. But it may be beneficial for wallet developers to be able to tell the difference, maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only way to return the wrong data size is by NOT inheriting the IAccount interface, and implement it differently (or, by using assembly return)
I think neither modes are something we want to explicitly support.
(btw: if solidity "try/catch" worked correctly, then all these cases would be considered "revert"...)

}
}
}

/**
* In case the request has a paymaster:
* - Validate paymaster has enough deposit.
Expand Down Expand Up @@ -663,15 +678,14 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT
mUserOp.paymasterPostOpGasLimit |
mUserOp.maxFeePerGas |
mUserOp.maxPriorityFeePerGas;
require(maxGasValues <= type(uint120).max, "AA94 gas values overflow");
require(maxGasValues <= type(uint120).max, FailedOp(opIndex, "AA94 gas values overflow"));

uint256 requiredPreFund = _getRequiredPrefund(mUserOp);
validationData = _validateAccountPrepayment(
opIndex,
userOp,
outOpInfo,
requiredPreFund,
verificationGasLimit
requiredPreFund
);

if (!_validateAndUpdateNonce(mUserOp.sender, mUserOp.nonce)) {
Expand Down Expand Up @@ -782,7 +796,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT

/**
* The gas price this UserOp agrees to pay.
* Relayer/block builder might submit the TX with higher priorityFee, but the user should not.
* Relayer/block builder might submit the TX with higher priorityFee, but the user should not be affected.
* @param mUserOp - The userOp to get the gas price from.
*/
function getUserOpGasPrice(
Expand All @@ -799,6 +813,12 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT
}
}

/// @inheritdoc IEntryPoint
function delegateAndRevert(address target, bytes calldata data) external {
(bool success, bytes memory ret) = target.delegatecall(data);
revert DelegateAndRevert(success, ret);
}

/**
* The offset of the given bytes in memory.
* @param data - The bytes to get the offset of.
Expand All @@ -823,10 +843,19 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT
}
}

/// @inheritdoc IEntryPoint
function delegateAndRevert(address target, bytes calldata data) external {
(bool success, bytes memory ret) = target.delegatecall(data);
revert DelegateAndRevert(success, ret);
// safe free memory pointer.
function getFreePtr() internal pure returns (uint256 ptr) {
assembly ("memory-safe") {
ptr := mload(0x40)
}
}

// restore free memory pointer.
// no allocated memory since saveFreePtr was called is allowed to be accessed after this call.
function restoreFreePtr(uint256 ptr) internal pure {
assembly ("memory-safe") {
mstore(0x40, ptr)
}
}

function _getUnusedGasPenalty(uint256 gasUsed, uint256 gasLimit) internal pure returns (uint256) {
Expand Down
2 changes: 1 addition & 1 deletion contracts/samples/Simple7702Account.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ contract Simple7702Account is BaseAccount, IERC165, IERC1271, ERC1155Holder, ERC

// temporary address of entryPoint v0.8
function entryPoint() public pure override returns (IEntryPoint) {
return IEntryPoint(0xE2b1C20D236ECe93b91f0656A9428C072e3F88Ad);
return IEntryPoint(0x9dab3AEd4B71E1AF22550b28ebae79c955F7041e);
}

/**
Expand Down
32 changes: 16 additions & 16 deletions reports/gas-checker.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,36 +12,36 @@
║ │ │ │ (delta for │ (compared to ║
║ │ │ │ one UserOp) │ account.exec()) ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple │ 1 │ 77824 │ │ ║
║ simple │ 1 │ 77763 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple - diff from previous │ 2 │ │ 41996 │ 12737 ║
║ simple - diff from previous │ 2 │ │ 41910 │ 12651 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple │ 10 │ 455792 │ │ ║
║ simple │ 10 │ 455206 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple - diff from previous │ 11 │ │ 42033 │ 12774 ║
║ simple - diff from previous │ 11 │ │ 41982 │ 12723 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster │ 1 │ 83675 │ │ ║
║ simple paymaster │ 1 │ 83624 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster with diff │ 2 │ │ 40524 │ 11265 ║
║ simple paymaster with diff │ 2 │ │ 40484 │ 11225 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster │ 10 │ 448582 │ │ ║
║ simple paymaster │ 10 │ 448035 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster with diff │ 11 │ │ 40593 │ 11334 ║
║ simple paymaster with diff │ 11 │ │ 40491 │ 11232 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ big tx 5k │ 1 │ 167613 │ │ ║
║ big tx 5k │ 1 │ 167551 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ big tx - diff from previous │ 2 │ │ 131215 │ 16513 ║
║ big tx - diff from previous │ 2 │ │ 131165 │ 16463 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ big tx 5k │ 10 │ 1348706 │ │ ║
║ big tx 5k │ 10 │ 1348102 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ big tx - diff from previous │ 11 │ │ 131228 │ 16526 ║
║ big tx - diff from previous │ 11 │ │ 131164 │ 16462 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ paymaster+postOp │ 1 │ 85070 │ │ ║
║ paymaster+postOp │ 1 │ 85019 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ paymaster+postOp with diff │ 2 │ │ 41930 │ 12671 ║
║ paymaster+postOp with diff │ 2 │ │ 41879 │ 12620 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ paymaster+postOp │ 10 │ 462525 │ │ ║
║ paymaster+postOp │ 10 │ 462062 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ paymaster+postOp with diff │ 11 │ │ 41993 │ 12734 ║
║ paymaster+postOp with diff │ 11 │ │ 41915 │ 12656 ║
╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝

49 changes: 44 additions & 5 deletions test/entrypoint.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,19 @@ import {
TestRevertAccount__factory,
TestSignatureAggregator,
TestSignatureAggregator__factory,
TestWarmColdAccount__factory
TestWarmColdAccount__factory,
SimpleAccount__factory
} from '../typechain'
import { DefaultsForUserOp, fillAndSign, fillSignAndPack, getUserOpHash, packUserOp, simulateValidation } from './UserOp'

import {
DefaultsForUserOp,
fillAndSign,
fillSignAndPack,
fillUserOp,
getUserOpHash,
packUserOp,
simulateValidation
} from './UserOp'
import { PackedUserOperation, UserOperation } from './UserOperation'
import { PopulatedTransaction } from 'ethers/lib/ethers'
import { ethers } from 'hardhat'
Expand All @@ -40,9 +50,6 @@ import { BytesLike } from '@ethersproject/bytes'
import { toChecksumAddress } from 'ethereumjs-util'
import { getERC165InterfaceID } from '../src/Utils'
import { UserOperationEventEvent } from '../typechain/contracts/interfaces/IEntryPoint'
import { before } from 'mocha'

import Debug from 'debug'
import {
AddressZero,
calcGasUsage,
Expand All @@ -66,6 +73,7 @@ import {
TWO_ETH,
unpackAccountGasFees
} from './testutils'
import Debug from 'debug'

const debug = Debug('entrypoint.test')

Expand Down Expand Up @@ -656,6 +664,37 @@ describe('EntryPoint', function () {
expect(await getBalance(simpleAccount.address)).to.eq(inititalAccountBalance)
})

it('should fail with AA20 if account not deployed', async () => {
const userop = await fillUserOp({
sender: createAddress(),
nonce: 0
}, entryPoint)
const beneficiary = createAddress()
await expect(entryPoint.handleOps([packUserOp(userop)], beneficiary)).to.revertedWith('AA20 account not deployed')
})

it('should fail with AA23 if account reverts', async () => {
const userop = await fillUserOp({
sender: entryPoint.address, // existing but not a real account
nonce: 0
}, entryPoint)
const beneficiary = createAddress()
await expect(entryPoint.handleOps([packUserOp(userop)], beneficiary).catch(rethrow())).to.be
.revertedWith('FailedOpWithRevert(0,"AA23 reverted",)')
})

it('should fail with AA23 (and original error) if account reverts', async () => {
// deploy an account with broken entrypoint, so it always reverts with "not from EntryPoint"
const revertingAccount = await new SimpleAccount__factory(ethersSigner).deploy(createAddress())
const userop = await fillUserOp({
sender: revertingAccount.address,
nonce: 0
}, entryPoint)
const beneficiary = createAddress()
await expect(entryPoint.handleOps([packUserOp(userop)], beneficiary).catch(rethrow())).to.be
.revertedWith('FailedOpWithRevert(0,"AA23 reverted",Error(account: not from EntryPoint)')
})

it('account should pay a penalty for unused gas only above threshold', async function () {
if (process.env.COVERAGE != null) {
return
Expand Down