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

[N-05] Use Custom Errors Instead of Revert Strings #247

Closed
wants to merge 1 commit into from

Conversation

nlordell
Copy link
Collaborator

@nlordell nlordell commented Feb 1, 2024

This PR addresses issue N-05 from the audit report.

In particular this PR changes the Safe 4337 module contract to use custom error codes instead of revert messages. This has a positive impact on the code size of the contracts.

That being said, the 4337 entry point contract v0.6.0 has special support for revert strings, and as such I do not believe this to be a positive change in the context that the 4337 module is designed to work for the v0.6.0 contracts. In particular, we believe that prioritizing debuggability is more important than contract code size improvements or potential gas savings in the error paths. With that in mind, I would recommend this change to not be included for the module supporting the v0.6.0 version of the entry point contract.

However, the special support in the entry point contract with revert string was removed in recent versions of the contract. As of yet, v0.7.0 is unreleased, but these changes should be included when the module is upgraded to v0.7.0 of the contracts once released.

@nlordell nlordell added invalid This doesn't seem right audit review A PR to address an issue found during an audit labels Feb 1, 2024
@nlordell nlordell requested a review from a team as a code owner February 1, 2024 09:56
@nlordell nlordell requested review from rmeissner, akshay-ap, mmv08 and remedcu and removed request for a team February 1, 2024 09:56
@coveralls
Copy link

Pull Request Test Coverage Report for Build 7740417871

  • 0 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 82.514%

Totals Coverage Status
Change from base Build 7668997253: 0.4%
Covered Lines: 114
Relevant Lines: 125

💛 - Coveralls

@nlordell
Copy link
Collaborator Author

nlordell commented Feb 7, 2024

Closing as this will not be implemented.

@nlordell nlordell closed this Feb 7, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 7, 2024
@nlordell nlordell deleted the audit/n-05 branch February 28, 2024 13:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
audit review A PR to address an issue found during an audit invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants