Skip to content

Conversation

@nadir-akhtar
Copy link
Collaborator

@nadir-akhtar nadir-akhtar commented Aug 6, 2025

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:

  • 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

@nadir-akhtar nadir-akhtar changed the title Nadir/merkle infos fix(audit): Merkle library infos Aug 6, 2025
@nadir-akhtar nadir-akhtar force-pushed the release-dev/merkle-audit-fixes branch from cf11051 to e87f933 Compare August 6, 2025 21:34
@nadir-akhtar nadir-akhtar marked this pull request as ready for review August 7, 2025 01:11
Copy link
Collaborator

@ypatil12 ypatil12 left a comment

Choose a reason for hiding this comment

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

I-05 LGTM

Copy link
Collaborator

@ypatil12 ypatil12 left a comment

Choose a reason for hiding this comment

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

I-06 LGTM

Copy link
Collaborator

@ypatil12 ypatil12 left a comment

Choose a reason for hiding this comment

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

I-07 LGTM

Copy link
Collaborator

@ypatil12 ypatil12 left a comment

Choose a reason for hiding this comment

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

I-08 LGTM

Copy link
Collaborator

@ypatil12 ypatil12 left a comment

Choose a reason for hiding this comment

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

I-09 LGTM

Copy link
Collaborator

@ypatil12 ypatil12 left a comment

Choose a reason for hiding this comment

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

I-11 LGTM

Copy link
Collaborator

@ypatil12 ypatil12 left a comment

Choose a reason for hiding this comment

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

I-14

@ypatil12
Copy link
Collaborator

ypatil12 commented Aug 7, 2025

For each function should we add where (if at all) we use it elsewhere in the protocol.

Example:

@dev Used by `RewardsCoordinator.xyz`

Copy link
Collaborator

@ypatil12 ypatil12 left a 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?

@nadir-akhtar
Copy link
Collaborator Author

We do indeed capture L-03 within these fixes -- a consequence of closing I-03

@nadir-akhtar
Copy link
Collaborator Author

@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?

@nadir-akhtar nadir-akhtar requested a review from ypatil12 August 7, 2025 23:23
Copy link
Collaborator

@ypatil12 ypatil12 left a comment

Choose a reason for hiding this comment

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

LGTM

@nadir-akhtar nadir-akhtar merged commit a98493f into release-dev/merkle-audit-fixes Aug 8, 2025
11 checks passed
@nadir-akhtar nadir-akhtar deleted the nadir/merkle-infos branch August 8, 2025 20:27
nadir-akhtar added a commit that referenced this pull request Aug 12, 2025
<!-- 
    🚨 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
nadir-akhtar added a commit that referenced this pull request Aug 13, 2025
<!-- 
    🚨 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
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.

3 participants