RISC Zero Crypto SCs Security Review Report

Groth16Verifier is a contract to check the Groth16 proof. Inside the contract it checks that public inputs are from the scalar field. It is generated via snarkjs which had a bug in checkField function and checked the element to be small from Base field instead of being small from Scalar Field (https://github.com/iden3/snarkjs/commit/8035774be493ac09e322cd1335e1a1d0ea3979d9). So an attacker can generate a number that is between Scalar Field and Base field which is an alias to a number smaller than Scalar Field (count is: q - r = 147946756881789318990833708069417712966).

function checkField(v) {
    if iszero(lt(v, q)) {
        mstore(0, 0)
        return(0, 0x20)
    }
}

Use this fix https://github.com/iden3/snarkjs/blame/bda5de30766cdb36da29e834715e56680c131807/templates/verifier_groth16.sol.ejs#L59

The mentioned contracts import OZ's Ownable.sol, as the contracts are non-upgradeable in the router and have some very important onlyOwner functionality, it is especially important that transfers of ownership should be handled with care.

import { Ownable } from "openzeppelin/contracts/access/Ownable.sol";

Use OpenZeppelin's Ownable2Step.sol.

The removeVerifier() function in the router contract allows the removal of verifiers associated with specific selectors. However, the function does not include a check to verify if the selector is already in the tombstone state before attempting to remove it. As a result, redundant operations may occur if users or developers attempt to remove a verifier that is already in the tombstone state, leading to potential confusion.

Lack of feedback about the state of the selector may lead to confusion or misunderstanding about whether the removal operation was successful or necessary.

function removeVerifier(bytes4 selector) external onlyOwner {
    // Simple check to reduce the chance of accidents.
    // NOTE: If there ever _is_ a reason to remove a selector that has never been set, the owner
    // can call addVerifier with the tombstone address.
    if (verifiers[selector] == UNSET) {
        revert SelectorUnknown({selector: selector});
    }
    verifiers[selector] = TOMBSTONE;
}

Implement an additional check in the removeVerifier() function to verify if the selector is already in the tombstone state before attempting to remove it.