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

Fix/cantina fixes #139

Merged
merged 31 commits into from
Aug 19, 2024
Merged

Conversation

Aboudjem
Copy link
Contributor

@Aboudjem Aboudjem commented Aug 15, 2024

  • # 25: Gas Optimization in K1Validator.validateUserOp
    Swapped isValidSignatureNow with isValidSignatureNowCalldata to reduce gas costs.
    Link

  • # 26: Unnecessary Module Type Check on Uninstall
    Removed redundant type checks during module uninstall to streamline the process.
    Link

  • # 24: Registry Threshold Validation
    Added checks to ensure the threshold doesn’t exceed the number of attesters.
    Link

  • # 15: Validator Installation Check in initializeAccount
    Ensured at least one validator is installed during account initialization to avoid issues.
    Link

  • # 14: Optimized _uninstallValidator Flow
    Modified the logic to check for an empty validator list after removal, improving efficiency.
    Link

  • # 10: Documentation and Typo Fixes
    Cleared up various typos and improved doc clarity across the codebase.
    Link

  • # 7: ModuleType Range Validation
    Added a range check for ModuleType to ensure it stays within valid bounds.
    Link

  • # 50: Risk of Account Becoming Unrecoverable
    Discussed scenarios where an account could become unrecoverable if all validators are removed. Explored potential solutions like fallback validators or social recovery mechanisms.
    Link

  • # 40: Missing Memory-Safe Assembly Annotation
    Added missing memory-safe annotations to assembly blocks where applicable.
    Link

  • # 11: Custom userOp.signature Encoding Documentation
    Documented the custom encoding used for userOp.signature in enableMode.
    Link

  • # 5: Missing Owner Address Validation in K1ValidatorFactory
    Added a check to ensure the factory owner address is non-zero for consistency with other factories.
    Link

  • # 4: Missing Return Parameters in NatSpec
    Added descriptions for missing return parameters in NatSpec comments to improve code documentation.
    Link

  • # 3: RegistryFactory createAccount Assumes Bootstrap.initNexus Will Be Used
    Documented that RegistryFactory.createAccount is only compatible with an inner Bootstrap.initNexus call as its initialization method.
    Link


…to be more generic in IModuleManagerEventsAndErrors.sol
@Aboudjem Aboudjem self-assigned this Aug 15, 2024
Copy link

openzeppelin-code bot commented Aug 15, 2024

Fix/cantina fixes

Generated at commit: a2730fcf2f383d44b4335dce7590de7396e83901

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
0
1
0
7
24
32

For more details view the full report in OpenZeppelin Code Inspector

Copy link

codecov bot commented Aug 15, 2024

Codecov Report

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

Project coverage is 75.67%. Comparing base (902c284) to head (b1f51b2).
Report is 6 commits behind head on remediations/cantina-spearbit.

Files Patch % Lines
contracts/base/ExecutionHelper.sol 66.66% 1 Missing ⚠️
Additional details and impacted files
@@                        Coverage Diff                        @@
##           remediations/cantina-spearbit     #139      +/-   ##
=================================================================
+ Coverage                          75.60%   75.67%   +0.07%     
=================================================================
  Files                                 13       13              
  Lines                                664      666       +2     
  Branches                             153      154       +1     
=================================================================
+ Hits                                 502      504       +2     
  Misses                               162      162              
Files Coverage Δ
contracts/Nexus.sol 62.85% <100.00%> (-0.22%) ⬇️
contracts/base/ModuleManager.sol 83.92% <100.00%> (+0.19%) ⬆️
contracts/base/RegistryAdapter.sol 100.00% <ø> (ø)
contracts/factory/K1ValidatorFactory.sol 100.00% <ø> (ø)
contracts/factory/RegistryFactory.sol 84.00% <100.00%> (+0.32%) ⬆️
contracts/modules/validators/K1Validator.sol 90.47% <100.00%> (ø)
contracts/base/ExecutionHelper.sol 58.16% <66.66%> (ø)

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 33f1a01...b1f51b2. Read the comment docs.

Copy link
Contributor

@livingrockrises livingrockrises left a comment

Choose a reason for hiding this comment

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

review i

/// @dev Checks if there is at least one validator installed.
/// @return True if there is at least one validator, otherwise false.
function _hasValidators() internal view returns (bool) {
return _getAccountStorage().validators.getNext(address(0x01)) != address(0x01);
Copy link

Choose a reason for hiding this comment

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

I just checked the SentinelListLib again. There's a bug in this lib when you call popAll() it leaves the list in an uninitialized state instead of an initialized but empty state.

You're not using popAll() but it's better to fix it and change this check to also ensure the next is not the zero address (uninitialized):

return _getAccountStorage().validators.getNext(address(0x01)) != address(0x01) && _getAccountStorage().validators.getNext(address(0x01)) != address(0x00);

Copy link
Contributor

Choose a reason for hiding this comment

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

would this comment be appropriate then? @MrToph

// Sentinel pointing to itself means the list is empty, so check this after removal

Copy link
Contributor

Choose a reason for hiding this comment

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

just double check above comment. actual fix is done @MrToph

Copy link

Choose a reason for hiding this comment

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

You can write this instead

// Sentinel pointing to itself / zero means the list is empty / uninitialized, so check this after removal

Copy link
Contributor

Choose a reason for hiding this comment

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

ok changing this

Copy link

🤖 Slither Analysis Report 🔎

Slither report

# Slither report

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

locked-ether

🟡 Impact: Medium
🔴 Confidence: High

utils/RegistryBootstrap.sol#L33-L165

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 5bd4851 into remediations/cantina-spearbit Aug 19, 2024
4 of 8 checks passed
@livingrockrises livingrockrises deleted the fix/cantina-fixes branch August 19, 2024 11:13
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