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

Vanilla 1271 for isValidSignatureWithSender in K1Validator #169

Closed

Conversation

filmakarov
Copy link
Collaborator

Addresses #168

I made the order:

  1. try 7739
    if not,
  2. try vanilla 1271
    assuming most protocols will switch to 7739

However, interested in hearing your thoughts if I should switch, as checking 1271 is very cheap compared to parsing 7739 data.

Copy link

openzeppelin-code bot commented Sep 18, 2024

Vanilla 1271 for isValidSignatureWithSender in K1Validator

Generated at commit: 1e700fcde8d369331f795dbdfda990b550bc84d4

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
0
1
0
7
25
33

For more details view the full report in OpenZeppelin Code Inspector

@livingrockrises
Copy link
Contributor

Addresses #168

I made the order:

  1. try 7739
    if not,
  2. try vanilla 1271
    assuming most protocols will switch to 7739

However, interested in hearing your thoughts if I should switch, as checking 1271 is very cheap compared to parsing 7739 data.

tbh depends on what validator supports so looks ok.

@livingrockrises livingrockrises self-requested a review September 18, 2024 09:20
Copy link

github-actions bot commented Sep 19, 2024

Changes to gas cost

Generated at commit: 6a0a57de22c49910ecb5e9c4202ff2321b657caa, compared to commit: 16273fdb3baa26e03c8b536ca3a9156197c8410a

🧾 Summary (5% most significant diffs)

Contract Method Avg (+/-) %
MockValidator isOwner -45 ✅ -6.16%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
MockValidator 998,224 (+18,788) isOwner 685 (-45) -6.16% 685 (-45) -6.16% 685 (-45) -6.16% 685 (-45) -6.16% 274 (-1)
Nexus 4,785,628 (-54,084) execute
initializeAccount
isModuleInstalled
uninstallModule
validateUserOp
6,426 (0)
114,400 (0)
865 (0)
5,868 (0)
13,973 (0)
0.00%
0.00%
0.00%
0.00%
0.00%
46,634 (-2,767)
134,234 (-1)
1,192 (+1)
10,586 (-8)
15,545 (-1)
-5.60%
-0.00%
+0.08%
-0.08%
-0.01%
38,126 (0)
134,300 (0)
1,168 (0)
12,423 (-12)
13,973 (0)
0.00%
0.00%
0.00%
-0.10%
0.00%
143,204 (0)
134,300 (0)
3,212 (0)
12,424 (-12)
42,860 (0)
0.00%
0.00%
0.00%
-0.10%
0.00%
73 (-3)
306 (-3)
328 (-3)
6 (0)
347 (-4)
MockPaymaster 1,093,531 (0) getHash 2,184 (0) 0.00% 2,388 (+8) +0.34% 2,483 (0) 0.00% 2,681 (0) 0.00% 16 (-1)
Bootstrap 1,930,562 (0) initNexusScoped 63,412 (0) 0.00% 83,246 (-1) -0.00% 83,312 (0) 0.00% 83,312 (0) 0.00% 306 (-3)

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.82%. Comparing base (7f8410d) to head (1e700fc).
Report is 4 commits behind head on dev.

Files with missing lines Patch % Lines
contracts/modules/validators/K1Validator.sol 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #169      +/-   ##
==========================================
- Coverage   93.94%   93.82%   -0.13%     
==========================================
  Files          14       14              
  Lines         727      729       +2     
  Branches      165      139      -26     
==========================================
+ Hits          683      684       +1     
- Misses         44       45       +1     
Files with missing lines Coverage Δ
contracts/Nexus.sol 93.84% <ø> (-0.65%) ⬇️
contracts/base/ModuleManager.sol 91.39% <100.00%> (-0.14%) ⬇️
contracts/modules/validators/K1Validator.sol 86.27% <75.00%> (+0.56%) ⬆️

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 7f8410d...1e700fc. Read the comment docs.

Copy link

🤖 Slither Analysis Report 🔎

Slither report

# Slither report

THIS CHECKLIST IS NOT COMPLETE. Use --show-ignored-findings to show all the results.
Summary
🟡 - unused-return (1 results) (Medium)

unused-return

🟡 Impact: Medium
🟡 Confidence: Medium

base/ERC7739Validator.sol#L176-L206

constable-states

Impact: Optimization
🔴 Confidence: High

base/RegistryAdapter.sol#L10

factory/RegistryFactory.sol#L39

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

@filmakarov filmakarov mentioned this pull request Sep 19, 2024
4 tasks
@filmakarov
Copy link
Collaborator Author

Well, I think this approach is not safe.
It still allows for replay attacks for 2 SA's with the same owner.
Need to reconsider.

@livingrockrises
Copy link
Contributor

Well, I think this approach is not safe. It still allows for replay attacks for 2 SA's with the same owner. Need to reconsider.

note to self, we have already merged some code. need to revert that and go with changes / no changes at all.

@filmakarov
Copy link
Collaborator Author

check #177

@filmakarov
Copy link
Collaborator Author

filmakarov commented Sep 23, 2024

closing in favour of #177

@filmakarov filmakarov closed this Sep 23, 2024
@livingrockrises livingrockrises deleted the feat/k1-to-support-vanilla-erc1271-via-7579-interface branch October 14, 2024 10:42
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.

2 participants