bcnmy / nexus

Nexus by Biconomy: ERC-7579 Modular Smart Account for Enhanced Account Abstraction
https://github.com/bcnmy/nexus/wiki
MIT License
27 stars 5 forks source link

check registry everytime fallback and hook is triggered #151

Closed livingrockrises closed 2 months ago

livingrockrises commented 2 months ago

https://cantina.xyz/code/d1d4b139-9705-4367-9468-297b7078674e/findings/23

I don't think we can do this in validateUserOp to check validator with the registry..

github-actions[bot] commented 2 months ago

:robot: Slither Analysis Report :mag_right:

Slither report

# Slither report **THIS CHECKLIST IS NOT COMPLETE**. Use `--show-ignored-findings` to show all the results. Summary - [constable-states](#constable-states) (2 results) (Optimization) ## constable-states Impact: Optimization :red_circle: Confidence: High - [ ] ID-0 [RegistryAdapter.registry]([base/RegistryAdapter.sol#L10](https://github.com/bcnmy/nexus/blob/a3eb1703b2804a017e71efd981ab221f9ae4c3dd/contracts/contracts/base/RegistryAdapter.sol#L10)) should be constant [base/RegistryAdapter.sol#L10](https://github.com/bcnmy/nexus/blob/a3eb1703b2804a017e71efd981ab221f9ae4c3dd/contracts/contracts/base/RegistryAdapter.sol#L10) - [ ] ID-1 [RegistryFactory.threshold]([factory/RegistryFactory.sol#L39](https://github.com/bcnmy/nexus/blob/a3eb1703b2804a017e71efd981ab221f9ae4c3dd/contracts/contracts/factory/RegistryFactory.sol#L39)) should be constant [factory/RegistryFactory.sol#L39](https://github.com/bcnmy/nexus/blob/a3eb1703b2804a017e71efd981ab221f9ae4c3dd/contracts/contracts/factory/RegistryFactory.sol#L39)

This comment was automatically generated by the GitHub Actions workflow.

github-actions[bot] commented 2 months ago

Changes to gas cost

Generated at commit: a3eb1703b2804a017e71efd981ab221f9ae4c3dd, compared to commit: 63801893c88354be9e02c5c7304040f904491e31

🧾 Summary (5% most significant diffs)

Contract Method Avg (+/-) %
Nexus execute
uninstallModule
+17,505 ❌
+271 ❌
+55.32%
+2.66%

Full diff report 👇
| Contract | Deployment Cost (+/-) | Method | Min (+/-) | % | Avg (+/-) | % | Median (+/-) | % | Max (+/-) | % | # Calls (+/-) | |:-|-:|:-|-:|-:|-:|-:|-:|-:|-:|-:|-:| | **Nexus** | 5,649,989 (-173,483) | _execute_
_executeFromExecutor_
_initializeAccount_
_installModule_
_isModuleInstalled_
_uninstallModule_
_validateUserOp_ | 6,159 (-38)
14,485 (-14)
111,052 (0)
32,612 (-8)
611 (0)
5,616 (-16)
7,142 (0) | **-0.61%**
**-0.10%**
**0.00%**
**-0.02%**
**0.00%**
**-0.28%**
**0.00%** | 49,150 (+17,505)
19,781 (-14)
130,887 (+15)
38,462 (-8)
804 (-6)
10,446 (+271)
8,713 (-29) | **+55.32%**
**-0.07%**
**+0.01%**
**-0.02%**
**-0.74%**
**+2.66%**
**-0.33%** | 37,859 (+1,233)
19,611 (-14)
130,952 (0)
40,139 (-8)
780 (0)
12,058 (+260)
7,142 (0) | **+3.37%**
**-0.07%**
**0.00%**
**-0.02%**
**0.00%**
**+2.20%**
**0.00%** | 142,937 (+94,997)
25,417 (-14)
130,952 (0)
43,926 (-8)
2,839 (0)
12,642 (+568)
35,933 (+119) | **+198.16%**
**-0.06%**
**0.00%**
**-0.02%**
**0.00%**
**+4.70%**
**+0.33%** | 76 (+13)
4 (0)
309 (+58)
23 (0)
331 (+58)
6 (0)
351 (+63) | | **MockPaymaster** | 1,161,593 (0) | _getHash_ | 2,087 (0) | **0.00%** | 2,318 (+41) | **+1.80%** | 2,427 (+152) | **+6.68%** | 2,697 (+233) | **+9.46%** | 17 (+5) | | **Bootstrap** | 2,556,353 (+4,080) | _initNexusScoped_ | 62,207 (0) | **0.00%** | 82,042 (+15) | **+0.02%** | 82,107 (0) | **0.00%** | 82,107 (0) | **0.00%** | 309 (+58) |
codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 75.29%. Comparing base (847828b) to head (3f96bf9).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## m_dev_cantina #151 +/- ## ================================================= + Coverage 75.21% 75.29% +0.07% ================================================= Files 13 13 Lines 686 688 +2 Branches 134 134 ================================================= + Hits 516 518 +2 Misses 170 170 ``` | [Files](https://app.codecov.io/gh/bcnmy/nexus/pull/151?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy) | Coverage Δ | | |---|---|---| | [contracts/base/ModuleManager.sol](https://app.codecov.io/gh/bcnmy/nexus/pull/151?src=pr&el=tree&filepath=contracts%2Fbase%2FModuleManager.sol&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy#diff-Y29udHJhY3RzL2Jhc2UvTW9kdWxlTWFuYWdlci5zb2w=) | `82.96% <100.00%> (+0.18%)` | :arrow_up: | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/bcnmy/nexus/pull/151?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/bcnmy/nexus/pull/151?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy). Last update [847828b...3f96bf9](https://app.codecov.io/gh/bcnmy/nexus/pull/151?dropdown=coverage&src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bcnmy).
livingrockrises commented 2 months ago

closing this as discussed

"I think it makes sense to leave it as is. Querying validators could brick an account and will violate 4337 rules. Querying hooks and fallbacks isnt really that useful, unless the fallback is also an executor in which case it is queried everytime its used anyways"