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

Check the proof size is a multiple of 60 #99

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mininny
Copy link
Collaborator

@mininny mininny commented Dec 10, 2024

Description

The Go slow implementation and the Solidity implementation of the RISCV virtual machine are supposed to be equivalent.

The Go slow implementation ensures that the proof size is a multiple of 60.

	proofContentOffset := shortToU64(uint16(stateContentOffset) + paddedStateSize + 32)

	if and(b32asBEWord(calldataload(shortToU64(uint16(stateContentOffset)+paddedStateSize))), shortToU256(60-1)) != (U256{}) {
		// proof offset must be stateContentOffset+paddedStateSize+32
		// proof size: 64-5+1=60 * 32 byte leaf,
		// but multiple memProof can be used, so the proofSize must be a multiple of 60
		panic("invalid proof offset input")
	}

However, the Solidity implementation doesn't check the _proof size.

            if iszero(eq(_proof.offset, add(add(_stateData.offset, paddedLen(stateSize())), 32))) {
                // 132+stateSize+padding+32 = expected proof offset
                revert(0, 0)
            }
            function proofContentOffset() -> out {
                // since we can't reference proof.offset in functions, blame Yul
                // 132+362+(32-362%32)+32=548
                out := 548
            }
            if iszero(eq(_proof.offset, proofContentOffset())) { revert(0, 0) }

The Solidity implementation should implement a check to ensure that the _proof size is a multiple of 32 * 60 as expected.

@mininny mininny force-pushed the feature/mininny/audit-1 branch 2 times, most recently from 1d6d143 to 95f2a0e Compare December 10, 2024 14:54
@mininny mininny force-pushed the feature/mininny/audit-1 branch from 95f2a0e to b9189bb Compare December 10, 2024 14:56
@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.89%. Comparing base (8ff9318) to head (b4d5cf6).

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #99   +/-   ##
=======================================
  Coverage   61.89%   61.89%           
=======================================
  Files          27       27           
  Lines        4091     4091           
=======================================
  Hits         2532     2532           
  Misses       1427     1427           
  Partials      132      132           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mininny mininny requested review from clabby and ImTei December 13, 2024 06:55
@BlocksOnAChain BlocksOnAChain added the Audit finding grouping for our audit findings label Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Audit finding grouping for our audit findings
Projects
Development

Successfully merging this pull request may close these issues.

3 participants