[AA]: fix inconsistent userOpHash by souradeep-das · Pull Request #757 · bobanetwork/boba_legacy

souradeep-das · 2023-05-01T13:03:58Z

📋 closes https://github.com/bobanetwork/boba/issues/690

Overview

Add upstream changes to AA contracts (primarily fix inconsistent userOpHash issue)

To merge after https://github.com/bobanetwork/boba/pull/698 is merged

Changes

add token callback handler on SimpleAccount (to support receiving ERC721/1155)
fix: inconsistent userOpHash issue
prevent recursive calls in handleOps method
move nonce validation from individual Account to EntryPoint

Notes

To merge after https://github.com/bobanetwork/boba/pull/698 is merged
"move nonce validation from individual Account to EntryPoint" is not a security vulnerability, but was introduced to ease integration with wallets/explorers (added as last commit can be removed 0bc894c)
bundler side expected changes for this is in be7b45b

Testing

/

codecov-commenter · 2023-05-01T13:55:54Z

Codecov Report

Patch coverage: 64.00% and project coverage change: +0.01 🎉

Comparison is base (a07ee58) 52.42% compared to head (2ab9a53) 52.44%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #757      +/-   ##
===========================================
+ Coverage    52.42%   52.44%   +0.01%     
===========================================
  Files          167      168       +1     
  Lines         7622     7644      +22     
  Branches      1572     1572              
===========================================
+ Hits          3996     4009      +13     
- Misses        3626     3635       +9

Flag	Coverage Δ
coverage	`52.44% <64.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...nt-abstraction/contracts/samples/SimpleAccount.sol	`93.61% <ø> (ø)`
...ontracts/samples/callback/TokenCallbackHandler.sol	`0.00% <0.00%> (ø)`
.../account-abstraction/contracts/core/EntryPoint.sol	`98.20% <100.00%> (+0.01%)`	⬆️
...oba/account-abstraction/contracts/core/Helpers.sol	`100.00% <100.00%> (ø)`
...abstraction/contracts/interfaces/UserOperation.sol	`88.88% <100.00%> (+5.55%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

InoMurko · 2023-05-01T19:10:10Z

     */
-    function nonce() public view virtual returns (uint256);
+    function getNonce() public view virtual returns (uint256) {
+        return entryPoint().getNonce(address(this), 0);


was just discussing this on twitter lol, why is nonce handling handled on the entrypoint contract?

I guess it makes sense since the bundler submitter nonce actually gets bumped not the userop sender

yeah! if user account contracts are allowed to control nonces - (or not have nonces) - the userOpHash might not be a unique value and that might confuse wallets/explorers. Because of that they moved the validation to the EntryPoint. found more details here :)
https://docs.google.com/document/d/1MywdH_TCkyEjD3QusLZ_kUZg4ZEI00qp97mBze9JI4k/edit#heading=h.gyhqxhuyd59n

InoMurko · 2023-05-01T19:11:00Z

        }

        uint256 collected = 0;
+        emit BeforeExecution();


what is this? :D

oh since there can be recursive calls to handleOps() (and the handleAggregatedOps()), understanding the order of emitted events and parsing is difficult. This event emitted before every execution is like a separator for each execution (if recursive)

InoMurko · 2023-05-01T19:11:16Z


        UserOpInfo[] memory opInfos = new UserOpInfo[](totalOps);

+        emit BeforeExecution();


souradeep-das · 2023-05-02T17:53:48Z

note: to be merged after https://github.com/bobanetwork/boba/pull/698 and ensure tests pass
thanks for the review guys!

* add token callback handler on SimpleAccount * fix: userOpHash packing * prevent recursive calls into handleOps * move nonce validation from individual Account to EntryPoint * add bundler changes for nonce change to EP (cherry picked from commit cc4e205)

* Inomurko/bump bundler (#698) * bump bundler, limit dependency bumps * uncomment bundler related stuff * build bundler in docker * build fixes * fix tests * fix bundler building * uncomment _disableInitializers * fix running DTL * v1.0.0 * fixing starting bundler and tests, default config * local unsafe, linting fix in intg tests * custom errors fixes * new api for simple account contract * use simple account factory proxy * use simple account factory proxy in SimpleAccountAPI * use simple account factory proxy in SimpleAccountAPI * use wrappers to get around custom errors * update entrypoint wrapper (#745) * sponsoring fee fixed * stricter validation for staking * remove debug namespace, fix return for unaavailable rpc methods * addressing Souradeeps comments --------- Co-authored-by: Souradeep Das <dsouradeep2@gmail.com> (cherry picked from commit 6368c72) * fix: qsp30 (#773) fix: BOB1-30 (cherry picked from commit 7feda88) * run op fix (#771) (cherry picked from commit 176cd3c) * close-server (#768) (cherry picked from commit 72021af) * [AA]: fix inconsistent userOpHash (#757) * add token callback handler on SimpleAccount * fix: userOpHash packing * prevent recursive calls into handleOps * move nonce validation from individual Account to EntryPoint * add bundler changes for nonce change to EP (cherry picked from commit cc4e205) * ValidationManager account for signature expiration (#775) * resolve #753 * Update packages/boba/bundler/src/modules/ValidationManager.ts Co-authored-by: Ino Murko <ino.murko.github@protonmail.com> * fix bool * validAfter/Until integrationt tests, validAfter * cleanup * regex * integration_tests * integration & unit tests --------- Co-authored-by: Ino Murko <ino.murko.github@protonmail.com> (cherry picked from commit ef02bee) * npm release workflow for bundler-sdk (#749) (cherry picked from commit 718141f) * Fix/banxa and bridges (#772) * adding boba network * fixing boba bridge url * fixing bridge integration * replace code by selectors * remove hardcoded symbol * enable banxa only for mainnet * Available bridge inable only for mainnet * adding support for testnet * update conditional for other bridges * implemented the available bridges with typescript * unit test cases for available bridges * typo in Available bridges --------- Co-authored-by: alvaro-ricotta <alvaro.e.ricotta@gmail.com> Co-authored-by: alvaro-ricotta <81116391+alvaro-ricotta@users.noreply.github.com> Co-authored-by: Ino Murko <ino.murko.github@protonmail.com> (cherry picked from commit dcf9b7e) * add validation of entryPoint and wrapper (#779) * add validaiton of entryPoint and wrapper (cherry picked from commit 41f3150) --------- Co-authored-by: Souradeep Das <dsouradeep2@gmail.com> Co-authored-by: Riedl Kevin, Bsc <kevin.riedl@wavect.io> Co-authored-by: Sahil K <86316370+sk-enya@users.noreply.github.com>

souradeep-das added 4 commits May 1, 2023 16:51

add token callback handler on SimpleAccount

d97ee5d

fix: userOpHash packing

81794d3

prevent recursive calls into handleOps

2ab9a53

validate nonce in EntryPoint instead of Account

0bc894c

add bundler changes for nonce change to EP

be7b45b

souradeep-das marked this pull request as ready for review May 1, 2023 14:48

souradeep-das requested review from InoMurko and wsdt May 1, 2023 14:48

InoMurko reviewed May 1, 2023

View reviewed changes

InoMurko approved these changes May 1, 2023

View reviewed changes

souradeep-das changed the title ~~[AA]: fix inconsistnet userOpHash~~ [AA]: fix inconsistent userOpHash May 2, 2023

wsdt approved these changes May 2, 2023

View reviewed changes

Merge branch 'develop' into souradeep/aa_update_userOpHash

5342377

merge develop

7fa4832

souradeep-das merged commit cc4e205 into develop May 4, 2023

souradeep-das deleted the souradeep/aa_update_userOpHash branch May 4, 2023 12:54

souradeep-das mentioned this pull request May 10, 2023

upgrade AA to 0.6.0 #802

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AA]: fix inconsistent userOpHash#757

[AA]: fix inconsistent userOpHash#757
souradeep-das merged 7 commits intodevelopfrom
souradeep/aa_update_userOpHash

souradeep-das commented May 1, 2023 •

edited

Loading

Uh oh!

codecov-commenter commented May 1, 2023

Uh oh!

InoMurko May 1, 2023 •

edited

Loading

Uh oh!

souradeep-das May 2, 2023

Uh oh!

InoMurko May 1, 2023

Uh oh!

souradeep-das May 2, 2023

Uh oh!

InoMurko May 2, 2023

Uh oh!

InoMurko May 1, 2023

Uh oh!

souradeep-das commented May 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		UserOpInfo[] memory opInfos = new UserOpInfo[](totalOps);

		emit BeforeExecution();

Conversation

souradeep-das commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Changes

Notes

Testing

Uh oh!

codecov-commenter commented May 1, 2023

Codecov Report

Uh oh!

InoMurko May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

souradeep-das May 2, 2023

Choose a reason for hiding this comment

Uh oh!

InoMurko May 1, 2023

Choose a reason for hiding this comment

Uh oh!

souradeep-das May 2, 2023

Choose a reason for hiding this comment

Uh oh!

InoMurko May 2, 2023

Choose a reason for hiding this comment

Uh oh!

InoMurko May 1, 2023

Choose a reason for hiding this comment

Uh oh!

souradeep-das commented May 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

souradeep-das commented May 1, 2023 •

edited

Loading

InoMurko May 1, 2023 •

edited

Loading