-
Notifications
You must be signed in to change notification settings - Fork 457
fix(audit): Merkle library infos #1597
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(audit): Merkle library infos #1597
Conversation
cf11051 to
e87f933
Compare
…proof.length is 0
…l other functions
6710b90 to
5d53a67
Compare
ypatil12
left a comment
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.
I-05 LGTM
ypatil12
left a comment
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.
I-06 LGTM
ypatil12
left a comment
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.
I-07 LGTM
ypatil12
left a comment
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.
I-08 LGTM
ypatil12
left a comment
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.
I-09 LGTM
ypatil12
left a comment
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.
I-11 LGTM
ypatil12
left a comment
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.
I-14
|
For each function should we add where (if at all) we use it elsewhere in the protocol. Example: @dev Used by `RewardsCoordinator.xyz` |
ypatil12
left a comment
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.
Isn't this L-03 too?
|
We do indeed capture L-03 within these fixes -- a consequence of closing I-03 |
|
@ypatil12 Rather than specify in the file, since that could get unwieldy (especially for others referencing / copying the library), I've created a new documentation file for the library -- think it captures what you're looking for? |
ypatil12
left a comment
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.
LGTM
<!--
🚨 ATTENTION! 🚨
This PR template is REQUIRED. PRs not following this format will be
closed without review.
Requirements:
- PR title must follow commit conventions:
https://www.conventionalcommits.org/en/v1.0.0/
- Label your PR with the correct type (e.g., 🐛 Bug, ✨ Enhancement, 🧪
Test, etc.)
- Provide clear and specific details in each section
-->
**Motivation:**
As part of an audit for the Merkle library, several low and
informational findings were found. This PR consolidates all
informational findings to fix, as well as additional commits for
findings discovered along the way, with a specific commit for each
finding.
All Lows are separated into individual PRs into the
[release-dev/merkle-audit-fixes](https://github.com/Layr-Labs/eigenlayer-contracts/tree/release-dev/merkle-audit-fixes)
branch
**Modifications:**
* Completely uplifted natspec, matching current EigenLayer quality and
standards
* Four new error codes:
* `InvalidIndex` for indices outside of the Merkle tree's max index
* `LeavesNotPowerOfTwo` for leaves (particularly for SHA256 functions)
that don't match the expected length condition
* `NoLeaves` for an empty `leaves` array
* `NotEnoughLeaves` for leaves (particularly for SHA256 functions) that
are not 2 or greater
* Explicit return in `processInclusionProofKeccak` for 0 length proofs
to return the leaf
* Requirement in `processInclusionProof(Keccak|Sha256)` that the index
is 0, i.e. the index was for a leaf within the tree
* Refactoring in `merkleize(Sha256|Keccak)` and `getProofKeccak` of
logic for readability and performance
* Newly added `getProofSha256` to mirror `getProofKeccak` to reduce
burden of producing roots offchain
* Newly added `isPowerOfTwo` helper function
* Newly added documentation file for the Merkle library
**Result:**
* Improved documentation/natspec
* Four new error codes
* Refactored and simplified code
* Newly added `getProofSha256` and `isPowerOfTwo` functions
* Updated and added unit tests
<!--
🚨 ATTENTION! 🚨
This PR template is REQUIRED. PRs not following this format will be
closed without review.
Requirements:
- PR title must follow commit conventions:
https://www.conventionalcommits.org/en/v1.0.0/
- Label your PR with the correct type (e.g., 🐛 Bug, ✨ Enhancement, 🧪
Test, etc.)
- Provide clear and specific details in each section
-->
**Motivation:**
In response to a recent audit report, we are closing out Lows and Infos
related to the Merkle library.
**Modifications:**
* [fix(audit): Merkle library infos
(#1597)](a98493f)
* [fix(L-01): prevent uninitialized roots from being used
(#1586)](30ec964)
**Result:**
Cleaner, safer code
Motivation:
As part of an audit for the Merkle library, several low and informational findings were found. This PR consolidates all informational findings to fix, as well as additional commits for findings discovered along the way, with a specific commit for each finding.
All Lows are separated into individual PRs into the release-dev/merkle-audit-fixes branch
Modifications:
InvalidIndexfor indices outside of the Merkle tree's max indexLeavesNotPowerOfTwofor leaves (particularly for SHA256 functions) that don't match the expected length conditionNoLeavesfor an emptyleavesarrayNotEnoughLeavesfor leaves (particularly for SHA256 functions) that are not 2 or greaterprocessInclusionProofKeccakfor 0 length proofs to return the leafprocessInclusionProof(Keccak|Sha256)that the index is 0, i.e. the index was for a leaf within the treemerkleize(Sha256|Keccak)andgetProofKeccakof logic for readability and performancegetProofSha256to mirrorgetProofKeccakto reduce burden of producing roots offchainisPowerOfTwohelper functionResult:
getProofSha256andisPowerOfTwofunctions