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.

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

Cantina Spearbit fixes #147

Merged
merged 121 commits into from
Aug 27, 2024
Merged

Cantina Spearbit fixes #147

merged 121 commits into from
Aug 27, 2024

Conversation

livingrockrises
Copy link
Contributor

@livingrockrises livingrockrises commented Aug 20, 2024

FYI all approved remediations from Cantina auditors were sitting in this branch, as per the full previous scope and findings.
https://github.com/bcnmy/nexus/tree/remediations/cantina-spearbit

from remediations/cantina-spearbit I created this m_dev_cantina just to merge back with dev branch because dev branch has some fixes merged from our Cyfrin contest.

some changes in new scope/code change have been directly merged to dev or m_dev_cantina (which is merged to dev as well as seen above)

Filipp Makarov and others added 30 commits July 18, 2024 13:31
…reshold parameters in createAccount and computeAccountAddress functions
… threshold parameters in createAccount and computeAccountAddress functions
add hooks to multiple other functions
Copy link

github-actions bot commented Aug 21, 2024

Changes to gas cost

Generated at commit: 881c292c4e602198a419017fe329775770c07f2f, compared to commit: 327e1e4a64f93b1321929ad6e19a95b4eaa54da1

🧾 Summary (5% most significant diffs)

Contract Method Avg (+/-) %
Bootstrap getInitNexusScopedCalldata -495 ✅ -8.11%
NexusAccountFactory computeAccountAddress +47 ❌ +3.84%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
Bootstrap 2,552,273 (+122,327) getInitNexusScopedCalldata 5,606 (-495) -8.11% 5,606 (-495) -8.11% 5,606 (-495) -8.11% 5,606 (-495) -8.11% 918 (0)
NexusAccountFactory 743,559 (-73,000) computeAccountAddress
createAccount
1,272 (+47)
212,621 (+278)
+3.84%
+0.13%
1,272 (+47)
230,242 (+278)
+3.84%
+0.12%
1,272 (+47)
232,761 (+278)
+3.84%
+0.12%
1,272 (+47)
232,761 (+278)
+3.84%
+0.12%
609 (0)
8 (0)
Nexus 5,641,328 (+287,704) execute
executeFromExecutor
initializeAccount
installModule
uninstallModule
validateUserOp
6,175 (-15)
14,477 (-30)
111,052 (+304)
32,620 (+179)
5,632 (-2,472)
7,142 (-5)
-0.24%
-0.21%
+0.27%
+0.55%
-30.50%
-0.07%
49,006 (+43)
19,773 (-8)
130,887 (+304)
38,470 (+203)
10,175 (-75)
8,713 (-5)
+0.09%
-0.04%
+0.23%
+0.53%
-0.73%
-0.06%
37,875 (-15)
19,603 (+15)
130,952 (+304)
40,147 (+178)
11,798 (+1,915)
7,142 (-5)
-0.04%
+0.08%
+0.23%
+0.45%
+19.38%
-0.07%
142,953 (-15)
25,409 (-34)
130,952 (+304)
43,934 (+178)
12,074 (-691)
35,933 (-5)
-0.01%
-0.13%
+0.23%
+0.41%
-5.41%
-0.01%
76 (0)
4 (0)
309 (0)
23 (0)
6 (0)
351 (0)

Copy link

codecov bot commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 66.91729% with 44 lines in your changes missing coverage. Please review.

Project coverage is 75.21%. Comparing base (7166cae) to head (847828b).
Report is 21 commits behind head on dev.

Files Patch % Lines
contracts/base/ModuleManager.sol 54.09% 28 Missing ⚠️
contracts/modules/validators/K1Validator.sol 64.28% 5 Missing ⚠️
contracts/Nexus.sol 80.00% 3 Missing ⚠️
contracts/base/BaseAccount.sol 0.00% 3 Missing ⚠️
contracts/base/ExecutionHelper.sol 82.35% 3 Missing ⚠️
contracts/factory/RegistryFactory.sol 88.23% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #147      +/-   ##
==========================================
+ Coverage   72.36%   75.21%   +2.85%     
==========================================
  Files          13       13              
  Lines         673      686      +13     
  Branches      147      134      -13     
==========================================
+ Hits          487      516      +29     
+ Misses        186      170      -16     
Files Coverage Δ
contracts/base/RegistryAdapter.sol 100.00% <ø> (ø)
contracts/base/Storage.sol 66.66% <ø> (ø)
contracts/factory/K1ValidatorFactory.sol 100.00% <100.00%> (+33.33%) ⬆️
contracts/factory/NexusAccountFactory.sol 100.00% <100.00%> (+38.46%) ⬆️
contracts/factory/RegistryFactory.sol 84.00% <88.23%> (+21.73%) ⬆️
contracts/Nexus.sol 62.57% <80.00%> (-0.59%) ⬇️
contracts/base/BaseAccount.sol 59.45% <0.00%> (-1.66%) ⬇️
contracts/base/ExecutionHelper.sol 58.16% <82.35%> (+1.78%) ⬆️
contracts/modules/validators/K1Validator.sol 83.33% <64.28%> (-7.15%) ⬇️
contracts/base/ModuleManager.sol 82.77% <54.09%> (-3.21%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 327e1e4...847828b. Read the comment docs.

Copy link
Collaborator

@VGabriel45 VGabriel45 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Collaborator

@filmakarov filmakarov left a comment

Choose a reason for hiding this comment

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

Approving without proper reviewing tbh, very busy with preparing smart sessions for audit, can't properly check all 62 changed files. I think as soon as Auditors are good with changes, I'm good as well

Copy link

🤖 Slither Analysis Report 🔎

Slither report

# Slither report

THIS CHECKLIST IS NOT COMPLETE. Use --show-ignored-findings to show all the results.
Summary

constable-states

Impact: Optimization
🔴 Confidence: High

base/RegistryAdapter.sol#L10

factory/RegistryFactory.sol#L39

_This comment was automatically generated by the GitHub Actions workflow._

@livingrockrises livingrockrises merged commit f1df70d into dev Aug 27, 2024
5 of 8 checks passed
@livingrockrises livingrockrises deleted the m_dev_cantina branch August 27, 2024 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants