Open sebastiantf opened 8 months ago
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
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
.
The values submitted & stored in the submitCommitment()
are being checked against the inputs in verifyInclusionProof()
:
https://github.com/zBlock-2/summa-solvency-schneier/blob/95d63fe1a55935542810138aa5d8de7f50f4e94b/contracts/src/Summa.sol#L198-L207
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
Description
Came across a few bugs while looking at some bugs reported in https://github.com/0xPARC/zk-bug-tracker that are similar:
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
, sinceuint256
values in Solidity could overflow the field and cause issuesWondering 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