AA-216 Proposal: Packing UserOperation and paymaster gas limits #363

shahafn · 2023-11-02T11:52:53Z

This PR is the biggest change we propose for erc4337 v0.7
While the changes below are distinct, it was best to implement this as a single PR

The changes:

The current verificationGasLimit field was used for three different purposes (account validation, paymaster validation, and paymaster postOp). We discovered it makes the paymaster vulnerable.
As a result, we added two new gas fields: paymasterValidationGasLimit and postOpGasLimit
Instead of adding more fields to the UserOperation struct, we encode all four paymaster-related parameters into a single “paymasterData” field.
This means that if no paymaster is in use, there is only a single “zeros” field, instead of four.
We also decided to pack the two account gas parameters (verificationGasLimit and callGasLimit) and pack them into a single field.
So in total, we added two more parameters but reduced the size of a simple UserOperation.
Note that all the above fields are usually not in use by on-chain contracts (account or paymaster)
When used off-chain, we introduced a separation between the “UserOperation” structure (the JSON API used to contact the bundler) and the “PackedUserOperation” structure in Solidity, which is passed to the contracts (the EntryPoint, account’s validateUserOp, and paymaster’s validatePaymasterUserOp)

Changes to Account

The validateUserOp method now has a new method signature, with the new PackedUserOperation structure
Account should be upgraded to the new version to support the new EntryPoint v0.7

Supporting multiple entrypoints is still possible:

validateUserOperation(PackedUserOperation) is the new method that supports the new entrypoint.
validateUserOperation(UserOperationV060) can be implemented to support the old entryPoint.
The execute (and executeBatch) should be modified to support calls from both entrypoint contracts

Changes to Paymasters

Paymaster’s paymasterAndData field processing now needs to account for both the 20 bytes of paymaster address and the 32 bytes of gas fields.
We added PAYMASTER_DATA_OFFSET constant to indicate the offset where paymaster-specific data starts

- Adding packed paymasterGasLimits for validatePaymasterUserOp and postOp - Packing gas limits of validateUserOp and call gas limits into accountGasLimits

contracts/core/EntryPoint.sol

contracts/core/UserOperationLib.sol

contracts/samples/DepositPaymaster.sol

contracts/samples/VerifyingPaymaster.sol

drortirosh · 2023-12-03T06:35:23Z

contracts/samples/gnosis/EIP4337Manager.sol

@@ -163,7 +163,7 @@ contract EIP4337Manager is IAccount, GnosisSafeStorage, Executor {
        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 userOp = UserOperation(address(safe), nonce, "", "", bytes32(bytes3(0x0f4240)), 0, 0, 0, "", sig);


why is this needed?

What is "this" that you're asking about? bytes32(bytes3(0x0f4240)) is there since it's the account gas limits that are now packed.

should use packGasLimit(callGasLimit, verificationGasLimit)

same func can be used for pmgas and for acc.gas

contracts/test/MaliciousAccount.sol

drortirosh · 2023-12-05T10:50:00Z

contracts/core/UserOperationLib.sol

+        return (uint128(bytes16(accountGasLimits)), uint128(uint256(accountGasLimits)));
+    }
+
+    function unpackPaymasterStaticFields(


I don't think we ever decode the paymaster itself, only paymasterGaslimits
(the reason is that this method is expected to be used from within validatePaymasterUserOp itself)

We do decode it..

contracts/samples/DepositPaymaster.sol

drortirosh · 2023-12-05T10:54:17Z

contracts/samples/TokenPaymaster.sol

@@ -123,15 +123,15 @@ contract TokenPaymaster is BasePaymaster, UniswapHelper, OracleHelper {
    override
    returns (bytes memory context, uint256 validationResult) {unchecked {
            uint256 priceMarkup = tokenPaymasterConfig.priceMarkup;
-            uint256 paymasterAndDataLength = userOp.paymasterAndData.length - 20;
+            uint256 paymasterAndDataLength = userOp.paymasterAndData.length - 52;


we need to define these as constants, in UserOpLib. its not good to have so many explicit numerics spread in our code.
e.g.
PAYMASTER_GASLIMIT_OFFSET (offset of (paymasterGasLimit,paymasterPostOpGasLimit))
PAYMASTER_DATA_OFFSET (offset of "rest of paymaster data")

drortirosh · 2023-12-05T11:12:13Z

contracts/samples/VerifyingPaymaster.sol

-                    userOp.callGasLimit,
-                    userOp.verificationGasLimit,
+                    userOp.accountGasLimits,
+                    uint256(bytes32(userOp.paymasterAndData[20:52])),


afaik, bytes32 is enough (bytes32(userOp.paymasterAndData[PAYMASTER_GASLIMT_OFFSET]))

drortirosh · 2023-12-05T11:15:51Z

contracts/core/EntryPoint.sol

@@ -325,8 +329,12 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
                "AA93 invalid paymasterAndData"
            );
            mUserOp.paymaster = address(bytes20(paymasterAndData[:20]));
+            mUserOp.paymasterVerificationGasLimit = uint128(bytes16(paymasterAndData[20:36]));


use decode helper

need to check if need all those in mUserOp

drortirosh · 2023-12-05T11:17:46Z

contracts/core/EntryPoint.sol

        } else {
            mUserOp.paymaster = address(0);
+            mUserOp.paymasterVerificationGasLimit = 0;


Solidity structs are pre-initialized to all-zeros.

Except when one does crazy stuff like recycling memory by using the free-pointer back - which we do :) So we can't assume zeros anywhere.

Yes, we can assume zeros
all storage variables in solidity are initialized to zero - either "stack" or memory variables (structs and arrays).
(there are Yul injected functions "allocate_and_zero_memory..." for that)

The paymaster initialization to address(0) was there, I only added the other fields. Do you want to remove all of them?

Yes, we can assume zeros all storage variables in solidity are initialized to zero - either "stack" or memory variables (structs and arrays). (there are Yul injected functions "allocate_and_zero_memory..." for that)

Interesting, I wonder why they do that. Solidity always allocates new memory (containing zeros) so why do they need to explicitly write zeros? (We do, due to our asm code, but normal solidity doesn't)

contracts/core/EntryPoint.sol

contracts/samples/gnosis/EIP4337Manager.sol

contracts/interfaces/UserOperation.sol

drortirosh · 2023-12-05T12:09:08Z

test/paymaster.test.ts

          sender: account.address,
-          paymasterAndData: paymaster.address,
+          paymaster: paymaster.address,
+          paymasterPostOpGasLimit: 3e5,


just like other fields, fillAndSign should have default value for postOpGasLimit.

I don't think that postOp should have a default non-zero value

drortirosh · 2023-12-05T12:17:05Z

test/samples/TokenPaymaster.test.ts

@@ -313,7 +312,7 @@ describe('TokenPaymaster', function () {
    })

    const preChargeTokens = decodedLogs[0].args.value
-    const requiredGas = BigNumber.from(op.callGasLimit).add(BigNumber.from(op.verificationGasLimit).mul(2)).add(op.preVerificationGas).add(40000 /*  REFUND_POSTOP_COST */)
+    const requiredGas = BigNumber.from(op.callGasLimit).add(BigNumber.from(op.verificationGasLimit).add(BigNumber.from(op.paymasterVerificationGasLimit))).add(BigNumber.from(op.paymasterPostOpGasLimit)).add(op.preVerificationGas).add(40000 /*  REFUND_POSTOP_COST */)


we're missing
bnSum = function(){ return [...arguments].reduce((sum,x)=>bn.from(x).add(sum)) }
and then
bnSum(op.callGasLimit, op.paymasterVerificationGasLimit, op.paymasterPostOpGasLimit, 40000)

test/UserOp.ts

test/entrypoint.test.ts

drortirosh · 2023-12-05T12:25:28Z

test/entrypoint.test.ts

@@ -275,14 +276,18 @@ describe('EntryPoint', function () {
        // we need maxFeeperGas > block.basefee + maxPriorityFeePerGas so requiredPrefund onchain is basefee + maxPriorityFeePerGas
        maxFeePerGas: block.baseFeePerGas.mul(3),


actually, this field looks anachronism: why doesn't it use DefaultUserOp or fillUserOp ?

I didn't touch this code at all

test/simple-wallet.test.ts

test/testutils.ts

drortirosh · 2023-12-05T12:41:09Z

test/verifying_paymaster.test.ts

@@ -43,9 +43,17 @@ describe('EntryPoint with VerifyingPaymaster', function () {

  describe('#parsePaymasterAndData', () => {
    it('should parse data properly', async () => {
-      const paymasterAndData = hexConcat([paymaster.address, defaultAbiCoder.encode(['uint48', 'uint48'], [MOCK_VALID_UNTIL, MOCK_VALID_AFTER]), MOCK_SIG])
+      const paymasterAndData = hexConcat([


missing helper packPaymasterAndData(paymasterr, pmGas, postOpGas, data?)
we don't want to mix "protocol" encoding (pm, gas), with "app" encoding (time-range)

forshtat · 2023-12-10T18:06:54Z

contracts/core/EntryPoint.sol

-                gasleft() < callGasLimit + mUserOp.verificationGasLimit + 5000
+                gasleft() <
+                callGasLimit +
+                mUserOp.paymasterPostOpGasLimit +


At this point we already have a context, so if it is empty the postOp will not be called. I think it may make sense to allow the transaction to go on in this case, instead of always adding paymasterPostOpGasLimit to the required gas left?
Say, the UserOp have requested 1'000'000 gas for a postOp but it does not need it any more, why should this check fail?

why should we (entrypoint) waste gas to check it? it is the paymaster's job to put a valid postOpGasLimit, even if context is empty.
There is another edge case: validatePaymasterUserOp() returns a context, and the postOpGasLimit` is zero: in this case, we still execute the callData and revert the postOp (on OOG) - again, because it is not the EntryPoint's job to validate the userop fields.

forshtat · 2023-12-10T18:24:12Z

contracts/core/EntryPoint.sol

@@ -313,8 +318,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
    ) internal pure {
        mUserOp.sender = userOp.sender;


Why not have the entire UserOperation => MemoryUserOp conversion in the UserOperationLib library? Construction of a MemoryUserOp takes a lot of space and will call to the UserOperationLib at least twice.

This copy is an internal optimization of EntryPoint. we might even remove copying some fields if we find they are not needed (e.g. used exactly once)
UserOperationLib is for general-use functions, that are expected to be used even outside the EntryPoint itself.

contracts/core/UserOperationLib.sol

contracts/samples/LegacyTokenPaymaster.sol

contracts/samples/TokenPaymaster.sol

contracts/samples/VerifyingPaymaster.sol

contracts/samples/gnosis/EIP4337Manager.sol

test/UserOp.ts

test/entrypoint.test.ts

          sender: testWarmColdAccount.address
        }
        const beneficiaryAddress = createAddress()
+        const badOpPacked = packUserOp(badOp)


test/entrypoint.test.ts

PackedUserOp merge develop

contracts/core/EntryPoint.sol

shahafn added 2 commits November 2, 2023 13:52

Packing gas limits

3181436

- Adding packed paymasterGasLimits for validatePaymasterUserOp and postOp - Packing gas limits of validateUserOp and call gas limits into accountGasLimits

Packing paymaster gas fields into paymasterAndData

1e2fe50

drortirosh reviewed Dec 3, 2023

View reviewed changes

shahafn added 10 commits December 3, 2023 16:52

bug fixes

bef7e4d

Fixing entrypoint tests

41bad54

Fixing (almost all) tests

f27b2d6

Fixing TokenPaymaster tests

287fe60

Fixing safe tests

b2a9e61

Lint

46fb3ba

Fixing gas-calc, tsc

7494872

Lint

0cc1d67

ethers tsc

6ffd042

gas calc again

56a5710

drortirosh reviewed Dec 5, 2023

View reviewed changes

contracts/interfaces/UserOperation.sol Show resolved Hide resolved

drortirosh reviewed Dec 5, 2023

View reviewed changes

forshtat requested changes Dec 10, 2023

View reviewed changes

drortirosh added the Proposal-change-v0.7 label Dec 12, 2023

shahafn changed the title ~~Packing gas limits~~ AA-216 Packing gas limits Dec 18, 2023

drortirosh changed the title ~~AA-216 Packing gas limits~~ AA-216 Proposal: Packing UserOperation and paymaster gas limits Dec 19, 2023

drortirosh reviewed Dec 19, 2023

View reviewed changes

contracts/core/UserOperationLib.sol Outdated Show resolved Hide resolved

shahafn added 3 commits December 21, 2023 00:06

Fixes

2e3ceff

Rename UserOperation to PackedUserOperation

d95187b

Gas calc

415ef69

drortirosh reviewed Dec 21, 2023

View reviewed changes

shahafn marked this pull request as ready for review December 21, 2023 13:40

Merge remote-tracking branch 'origin/develop' into packed-gas-limits

ffdb104

shahafn force-pushed the packed-gas-limits branch from 4a41d5e to ffdb104 Compare December 21, 2023 17:21

drortirosh added 2 commits January 1, 2024 19:34

Merge branch 'develop' into packed-gas-limits

7b6949a

remove debug comments

47a57fa

drortirosh added 4 commits January 3, 2024 17:15

Merge branch 'develop' into packed-gas-limits

a54d10b

changes

08ef38e

fix tokenpaymaster test

052a8ad

Merge pull request #393 from eth-infinitism/dror-packeduserop

87a8eac

PackedUserOp merge develop

forshtat previously approved these changes Jan 3, 2024

View reviewed changes

drortirosh dismissed forshtat’s stale review via ca96976 January 3, 2024 16:54

fixed pr comments

277c379

drortirosh force-pushed the packed-gas-limits branch from ca96976 to 277c379 Compare January 3, 2024 16:55

add paymaster constants to BasePaymaster

effa64f

drortirosh force-pushed the packed-gas-limits branch from 5426ba2 to effa64f Compare January 4, 2024 09:15

drortirosh reviewed Jan 4, 2024

View reviewed changes

contracts/core/EntryPoint.sol Show resolved Hide resolved

shahafn added 4 commits January 7, 2024 20:14

PR fix

8ff7490

Removing redundant initializer

24cf39e

gas calc

b9504f4

Fix test

6252387

drortirosh approved these changes Jan 8, 2024

View reviewed changes

shahafn merged commit e8896ac into develop Jan 8, 2024
8 checks passed

shahafn deleted the packed-gas-limits branch January 8, 2024 17:02

drortirosh mentioned this pull request Jan 10, 2024

Packing UserOperation and paymaster gas limits #393

Merged

jwindawi mentioned this pull request Jan 12, 2024

Community Call #4 erc6900/resources#23

Closed

This was referenced Jan 25, 2024

Evaluate 4337 Module Changes For EntryPoint 0.7.0 safe-global/safe-modules#215

Closed

Use the new PackedUserOperation struct with packed gas fields from entrypoint 0.7.0 safe-global/safe-modules#225

Closed

neavra mentioned this pull request Jan 27, 2024

Incompatibility between UserOperation struct & PackedUserOperation due to EntryPoint v0.7 changes stackup-wallet/stackup-bundler#372

Open

cherry-yl-sh mentioned this pull request Mar 28, 2024

EntryPoint 0.7 support AAStarCommunity/aa-bundler#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AA-216 Proposal: Packing UserOperation and paymaster gas limits #363

AA-216 Proposal: Packing UserOperation and paymaster gas limits #363

shahafn commented Nov 2, 2023 •

edited by drortirosh

Loading

drortirosh Dec 3, 2023

shahafn Dec 3, 2023 •

edited

Loading

drortirosh Dec 3, 2023 •

edited

Loading

drortirosh Dec 5, 2023

shahafn Dec 20, 2023

drortirosh Dec 5, 2023

shahafn Dec 20, 2023

drortirosh Dec 5, 2023

drortirosh Dec 5, 2023

drortirosh Dec 5, 2023

yoavw Jan 8, 2024

drortirosh Jan 8, 2024 •

edited

Loading

shahafn Jan 8, 2024

yoavw Jan 8, 2024

drortirosh Dec 5, 2023

shahafn Jan 7, 2024

drortirosh Dec 5, 2023

drortirosh Dec 5, 2023

shahafn Jan 8, 2024

drortirosh Dec 5, 2023

forshtat Dec 10, 2023

drortirosh Dec 11, 2023

forshtat Dec 10, 2023

drortirosh Dec 11, 2023

This comment was marked as outdated.

		@@ -275,14 +276,18 @@ describe('EntryPoint', function () {
		// we need maxFeeperGas > block.basefee + maxPriorityFeePerGas so requiredPrefund onchain is basefee + maxPriorityFeePerGas
		maxFeePerGas: block.baseFeePerGas.mul(3),

		@@ -313,8 +318,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
		) internal pure {
		mUserOp.sender = userOp.sender;

AA-216 Proposal: Packing UserOperation and paymaster gas limits #363

AA-216 Proposal: Packing UserOperation and paymaster gas limits #363

Conversation

shahafn commented Nov 2, 2023 • edited by drortirosh Loading

The changes:

Changes to Account

Supporting multiple entrypoints is still possible:

Changes to Paymasters

Choose a reason for hiding this comment

shahafn Dec 3, 2023 • edited Loading

Choose a reason for hiding this comment

drortirosh Dec 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drortirosh Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

shahafn commented Nov 2, 2023 •

edited by drortirosh

Loading

shahafn Dec 3, 2023 •

edited

Loading

drortirosh Dec 3, 2023 •

edited

Loading

drortirosh Jan 8, 2024 •

edited

Loading