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

refactor: address informational security review findings #909

Merged
merged 2 commits into from
Dec 5, 2024

Conversation

nadir-akhtar
Copy link
Contributor

@nadir-akhtar nadir-akhtar commented Nov 27, 2024

This PR addresses some of the informational findings found during the internal security review. Specifically, this PR:

  • Arranges functions to match our EigenLayer style guide
  • Makes a note on processClaims() about an array of too large a size potentially causing a user to hit gas limits, and recommending a smaller array in that circumstance

@nadir-akhtar nadir-akhtar changed the title fix: address internal review findings refactor: address internal review findings Nov 27, 2024
@nadir-akhtar nadir-akhtar changed the title refactor: address internal review findings refactor: address informational security review findings Nov 27, 2024
Copy link
Contributor

@0xrajath 0xrajath left a comment

Choose a reason for hiding this comment

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

I get why it should be reshuffled according to the Solidity guide. But I also don't think we should introduce such a large diff for audit purposes just to adhere to that.

@nadir-akhtar
Copy link
Contributor Author

I spoke with @wadealexc and refactored the PR accordingly.

He told me that we don't actually follow the Solidity style guide for this repo, though no documented internal style guide exists. After talking to him, I now understand that we order functions differently from the style guide, specifically in the following order:

  • external/public functions grouped by relevance. Note that these are interchangable for style/ordering purposes: external and public functionscan be grouped together however it makes sense based on their purpose.
  • internal functions. These are to be ordered by state-changing internal functions first, and view/pure functions interchangably after.
  • external/public view functions, also interchangable.

As such, the PR diff as it stands now is smaller.

That said, this is a PR remedying an informational finding, the lowest possible severity. I recommend merging in the change so that auditors work off of the intended latest version. Moreover, there are no functionality changes with this PR, so it shouldn't be that disruptive, and it will ensure our codebase aligns with our general style guide for consistency and clarity. However, I also understand not wanting to disrupt auditors' understanding of the target commit. @0xrajath

@nadir-akhtar nadir-akhtar merged commit c27424d into dev Dec 5, 2024
16 of 18 checks passed
@nadir-akhtar nadir-akhtar deleted the nadir/rewards-v2 branch December 5, 2024 21:40
0xClandestine pushed a commit that referenced this pull request Dec 6, 2024
* docs: add comment for processClaims() about large arrays hitting gas limit

* refactor: correct internal function arrangement
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