Skip to content

Sherlock audit fixes#61

Merged
eason1981 merged 42 commits intomainfrom
sherlock-audit-fixes
Dec 9, 2025
Merged

Sherlock audit fixes#61
eason1981 merged 42 commits intomainfrom
sherlock-audit-fixes

Conversation

@will-brevis
Copy link
Contributor

Temporary PR for an alternative web-view of changes made to the auditing branch.

@will-brevis
Copy link
Contributor Author

will-brevis commented Nov 3, 2025

For any auditors visiting this PR, the fix commit for 204 has a typo and is addressed in the fix for 233 (a similar issue).

The VM also no longer works, as we need to pull in some P3 updates for the updated FRI code and changed some lookups.

will-brevis and others added 25 commits November 10, 2025 12:52
…duceWithMaxBits for koalabear field

adjusts the hint for koalabear (2^31 - 2^24 + 1) by constraining the
lower 24 bits to 0 when the upper 7 bits are all set, as well as
adjusting the corresponding range checks and other constants
…constrained REM/REMU operations when divisor is 0
…eds to guarantee the VM execution trace is complete (#844)
@CergyK
Copy link

CergyK commented Nov 20, 2025

Here are my comments on some of the fixes (please consider the others LGTM):

#144 -> Fix ok, here are some potentially similar issues (input length mismatch) which could be fixed with zip_eq?

https://github.com/brevis-network/pico/blob/sherlock-audit-fixes/vm/src/compiler/recursion/circuit/fri.rs#L271
https://github.com/brevis-network/pico/blob/sherlock-audit-fixes/vm/src/compiler/recursion/circuit/merkle_tree.rs#L150
https://github.com/brevis-network/pico/blob/sherlock-audit-fixes/vm/src/instances/compiler/riscv_circuit/convert/builder.rs#L274
https://github.com/brevis-network/pico/blob/sherlock-audit-fixes/vm/src/machine/verifier.rs#L93

#124 -> Outdated comment
https://github.com/brevis-network/pico/blob/sherlock-audit-fixes/gnark/koalabear/koalabear.go#L377-L378

#230 -> The assert is stripped out from the recursion program;
https://github.com/brevis-network/pico/blob/sherlock-audit-fixes/vm/src/compiler/recursion/circuit/stark.rs#L142

#233 -> Same here. Assert would be stripped out, so it brings no additional security guarantees

#226 -> Would need to update Cargo.lock in perf/bench_apps

@will-brevis
Copy link
Contributor Author

Regarding #230 and #233, these are meant to be sanity checks to be performed by our official proof verifier, or anyone using our recursion machine builder. We do not want to encode these into any constraints since they are already satisfied by proof shape constraints or execution completeness checks, nor was the original issue asking us to.

For #226, sure, we can trigger a rebuild for bench_apps which will cause the resolver to update the P3 dependency.

#124, this comment will be updated

#144, I will add additional zip_eq invocations.

@will-brevis will-brevis changed the base branch from audit to main November 25, 2025 03:07
Copy link
Contributor

@eason1981 eason1981 left a comment

Choose a reason for hiding this comment

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

LGTM

@eason1981 eason1981 merged commit 76a3214 into main Dec 9, 2025
7 checks passed
@eason1981 eason1981 deleted the sherlock-audit-fixes branch December 9, 2025 06:45
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.

7 participants