[WIP] Potential Smart Contract Range Check

[sebastiantf]

Description

Came across a few bugs while looking at some bugs reported in https://github.com/0xPARC/zk-bug-tracker that are similar:

• Semaphore: Missing Smart Contract Range Check
• Zk-Kit: Missing Smart Contract Range Check
• a16z ZkDrops: Missing Nullifier Range Check
• EY Nightfall: Missing Nullifier Range Check

Essentially the smart contracts used with the projects seem to be missing range checks for public inputs to ensure they're less than the SNARK_SCALAR_FIELD , since uint256 values in Solidity could overflow the field and cause issues

uint256 constant SNARK_SCALAR_FIELD = 21888242871839275222246405745257275088548364400416034343698204186575808495617;

Wondering if similar checks should be present in the Summa contracts where uint256 public inputs are used like:

• uint256[] rootBalances
• uint256 mstRoot
• uint256[] memory publicInputs

and possibly others.

This is just an initial observation and need more work & help to validate/invalidate this. Nevertheless documenting it here

PS: Semaphore has since changed their logic to have no stark restrictions on groupIds. But the other codebases listed above still seem to use such range checks

[0xnirlin]

I think there could be an issue with the public input and root, both of them will be hashes, probably bytes 32 hashes converted into uint256, now I don't know how are these values further handled but the hash converted into uint256 may cross this value

uint256 constant SNARK_SCALAR_FIELD = 21888242871839275222246405745257275088548364400416034343698204186575808495617;

Run this simple code

[flyingnobita]

These values are being inputs to submitCommitment() and verifyInclusionProof(), both in Summa.sol

For submitCommitment(), the contract simply acts as an immutable storage for these submitted values. They are read for off-chain verification of exchange liabilities thus they aren't being computed or have risk of overflow in the contract.

For verifyInclusionProof(), the solidity verifier, InclusionVerifier.sol, is generated by halo2-solidity-verifier from MstInclusionCircuit (in circuits\merkle_sum_tree.rs). This circuit contain the RangeCheck chip that we've been looking at. So I don't think further range check is necessary in Summa.sol.

[sebastiantf]

The values submitted & stored in the submitCommitment() are being checked against the inputs in verifyInclusionProof():

summa-solvency-schneier/contracts/src/Summa.sol

Lines 198 to 207 in 95d63fe

	require(
	commitments[timestamp].mstRoot == publicInputs[1],
	"Invalid MST root"
	);
	for (uint i = 2; i < publicInputs.length; i++) {
	require(
	commitments[timestamp].rootBalances[i - 2] == publicInputs[i],
	"Invalid root balance"
	);
	}

In the zkDrops contract, nullifierHash is being range checked even if it is being fed into the verifier contract that is also generated based on the circuits: https://github.com/a16z/zkdrops/blob/a4e58bdad8391ffc133c3643c449be5d18b69832/zkdrops-contracts/contracts/PrivateAirdrop.sol#L38-L46