Zokrates / ZoKrates

A toolbox for zkSNARKs on Ethereum
https://zokrates.github.io
GNU Lesser General Public License v3.0
1.8k stars 360 forks source link

Need Clarification on Input/Output Size for `addition`/`scalar_mul` in Generated `verifier.sol` #1364

Closed venezuela01 closed 4 months ago

venezuela01 commented 6 months ago

Description

The zokrates export-verifier command generates a verifier.sol file, sourcing its code templates from solidity.rs. Upon closer inspection of the code, there seems to be something wrong in input and output sizes:

However, the staticcall specifies 0xc0 as the input size and 0x60 as the output size, both of which are larger than necessary.

A similar discrepancy is observed in scalar_mul. I haven't exhaustively inspected other calls/static calls.

It appears that this code has been functioning without issues for years, arguably due to luck, as EVM precompiled contracts ignore the input and output sizes for Bn256Add and Bn256ScalarMul, as seen in the Bn256Add and Bn256ScalarMul.

Could anyone confirm this? Thanks.

Earliest commit: fc60aa97e88d7c195517d8d69226763d088268ce

    /// @return r the sum of two points of G1
    function addition(G1Point memory p1, G1Point memory p2) internal view returns (G1Point memory r) {
        uint[4] memory input;
        input[0] = p1.X;
        input[1] = p1.Y;
        input[2] = p2.X;
        input[3] = p2.Y;
        bool success;
        assembly {
            success := staticcall(sub(gas(), 2000), 6, input, 0xc0, r, 0x60)
            // Use "invalid" to make gas estimation work
            switch success case 0 { invalid() }
        }
        require(success);
    }
    /// @return r the product of a point on G1 and a scalar, i.e.
    /// p == p.scalar_mul(1) and p.addition(p) == p.scalar_mul(2) for all points p.
    function scalar_mul(G1Point memory p, uint s) internal view returns (G1Point memory r) {
        uint[3] memory input;
        input[0] = p.X;
        input[1] = p.Y;
        input[2] = s;
        bool success;
        assembly {
            success := staticcall(sub(gas(), 2000), 7, input, 0x80, r, 0x60)
            // Use "invalid" to make gas estimation work
            switch success case 0 { invalid() }
        }
        require (success);
    }
venezuela01 commented 5 months ago

Any comments? @petehunt @dark64 @Schaeff

dark64 commented 5 months ago

Any comments? @petehunt @dark64 @Schaeff

Hmm great catch, it does look like the input and output sizes are incorrect judging by the precompile implementation you linked.

venezuela01 commented 5 months ago

There are many downstream projects that have copied and pasted code from ZoKrates, and now all the children and grandchildren inherit the incorrect sizes. It would be nice if someone could fix it upstream. I'm not a Solidity developer; I just catch it occasionally when reviewing code in other projects. It would be great if someone more knowledgeable than me could fix the code. ;-)