-
Notifications
You must be signed in to change notification settings - Fork 10
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
Remediations re 7739 update #216
Conversation
filmakarov
commented
Nov 8, 2024
•
edited
Loading
edited
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #216 +/- ##
==========================================
- Coverage 84.68% 84.45% -0.24%
==========================================
Files 13 13
Lines 849 849
Branches 271 249 -22
==========================================
- Hits 719 717 -2
- Misses 115 117 +2
Partials 15 15
Continue to review full report in Codecov by Sentry.
|
Remediations re 7739 update
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
@@ -226,7 +227,9 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra | |||
/// @dev Delegates the validation to a validator module specified within the signature data. | |||
function isValidSignature(bytes32 hash, bytes calldata signature) external view virtual override returns (bytes4) { | |||
// Handle potential ERC7739 support detection request | |||
if (checkERC7739Support(hash, signature)) return SUPPORTS_ERC7739; | |||
if (signature.length == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if signature length is 0 then check? is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it's in 7739 spec
contracts/Nexus.sol
Outdated
if (IValidator(next).isValidSignatureWithSender(msg.sender, hash, signature) == SUPPORTS_ERC7739) return true; | ||
} | ||
// Forces the compiler to optimize for smaller bytecode size. | ||
if (uint256(hash) == (~signature.length / 0xffff) * 0x7739) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious what does ~ do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is bitwise negation. so it makes 0xfff..fff out of 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed
Changes to gas cost
🧾 Summary (5% most significant diffs)
Full diff report 👇
|
🤖 Slither Analysis Report 🔎Slither report
# Slither report
_This comment was automatically generated by the GitHub Actions workflow._
THIS CHECKLIST IS NOT COMPLETE. Use
locked-ether🟡 Impact: Medium
utils/NexusBootstrap.sol#L33-L165 constable-statesImpact: Optimization
|