Skip to content

Conversation

@henriquemarlon
Copy link
Member

With the stronger fuzzer introduced in Foundry v1.5.0, a new failing fuzz case was detected in:

Merkle.getHashOfLeafAtIndex(bytes calldata data, uint256 leafIndex)

The issue is caused by this line:

uint256 start = leafIndex << MerkleConstants.LOG2_LEAF_SIZE;

For very large leafIndex values, this shift can silently wrap around modulo 2^256, making start small again. This may cause the condition start < data.length to incorrectly pass, leading to an invalid hash being returned instead of the pristine leaf.

Although this does not affect deployed contracts (where leafIndex is safely bounded), it breaks fuzz tests on Foundry v1.5.0.

This PR adds a reversible shift check to prevent wrap-around:

if (start < data.length && (start >> MerkleConstants.LOG2_LEAF_SIZE) == leafIndex)

This guarantees that the shift did not overflow and restores correct behavior under fuzzing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These changes seem to pertain more to your other PR #226.
My suggestion is, now that you have write permissions to the repo:

  • Address all suggested changes.

  • Rebase fix/integer-overflow on top of fix/dockerized-env-tests

git checkout fix/integer-overflow
git rebase -i fix/dockerized-env-tests # Might require conflicts to be resolved
  • Push your branches from your fork to the upstream dave repo and make these PRs point to them.
git remote add upstream https://github.com/cartesi/dave.git
git fetch upstream
git push upstream fix/dockerized-env-tests
git push upstream fix/integer-overflow
  • Make your PRs point to the new branches on the dave repo (not to your fork branches)

  • Make this PR point have fix/dockerized-env-tests as its base branch

apt-get update && apt-get install -y --no-install-recommends curl ca-certificates git
curl -L https://foundry.paradigm.xyz | bash
~/.foundry/bin/foundryup --install stable
~/.foundry/bin/foundryup --install v1.5.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

When bumping Foundry, we should make sure to:

  • update the Foundry version on the CI config.
  • run the Forge formatter on all Forge projects.

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