Need clarify with input / output sizes

[venezuela01]

    /// @return r the sum of two points of G1
    function add(G1Point memory p1, G1Point memory p2) internal 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 := call(sub(gas(), 2000), 6, 0, input, 0xc0, r, 0x60)
        }
        require(success, "Call to precompiled contract for add failed");
    }

input is unit[4], which is 32*4 = 128 bytes, which is 0x80 in hex
r is a G1Point which is 32*2 = 64 bytes, which is 0x40 in hex
I don't understand why we have 0xc0 and 0x60 in call(sub(gas(), 2000), 6, 0, input, 0xc0, r, 0x60), is there anything I missed?

    /// @return r the product of a point on G1 and a scalar, i.e.
    /// p == p.mul(1) and p.add(p) == p.mul(2) for all points p.
    function mul(G1Point memory p, uint s) internal returns (G1Point memory r) {
        uint[3] memory input;
        input[0] = p.X;
        input[1] = p.Y;
        input[2] = s;
        bool success;
        assembly {
            success := call(sub(gas(), 2000), 7, 0, input, 0x80, r, 0x60)
        }
        require(success, "Call to precompiled contract for mul failed");
    }

input is unit[3] which is 32*3 = 96 bytes or 0x60 in hex
r is G1Point which is 32*2 = 64 bytes or 0x40 in hex
I don't understand the line call(sub(gas(), 2000), 7, 0, input, 0x80, r, 0x60) either, can anyone explain it for me?

Source: https://github.com/oxen-io/eth-sn-contracts/blob/e3f6bdc4eef75bce44b44e076cae0e6163b046e4/contracts/libraries/BN256G1.sol

Thank you.

[darcys22]

This is simply passing the variables to the EVM precompiles. The ethereum clients have these curve operations built in

https://www.evm.codes/precompiled?fork=shanghai

[venezuela01]

This is simply passing the variables to the EVM precompiles. The ethereum clients have these curve operations built in

https://www.evm.codes/precompiled?fork=shanghai

Thanks, I'm aware of the EVM precompiled contracts. However, it seems my question hasn't been answered.

Aren't the input size and output size incorrect in the code?

[darcys22]

Deleted my other response cause I'm not sure anymore

ill need to investigate this further, this code has been copied and pasted and worked happily without any issues.

there is a chance that those size params arnt correct and just have had no effect on the functionality

[venezuela01]

there is a chance that those size params arnt correct and just have had no effect on the functionality

Yes, I think we were just lucky. The precompiled ecAdd and ecMul functions consume a fixed size of input and completely ignore our input size, so nothing bad happened. However, that doesn't mean our implementation is correct.

https://github.com/ethereum/go-ethereum/blob/master/core/vm/contracts.go#L442

func runBn256Add(input []byte) ([]byte, error) {
	x, err := newCurvePoint(getData(input, 0, 64))
	if err != nil {
		return nil, err
	}
	y, err := newCurvePoint(getData(input, 64, 64))
	if err != nil {
		return nil, err
	}
	res := new(bn256.G1)
	res.Add(x, y)
	return res.Marshal(), nil
}

https://github.com/ethereum/go-ethereum/blob/master/core/vm/contracts.go#L484

func runBn256ScalarMul(input []byte) ([]byte, error) {
	p, err := newCurvePoint(getData(input, 0, 64))
	if err != nil {
		return nil, err
	}
	res := new(bn256.G1)
	res.ScalarMult(p, new(big.Int).SetBytes(getData(input, 64, 32)))
	return res.Marshal(), nil
}

[venezuela01]

ill need to investigate this further, this code has been copied and pasted and worked happily without any issues.

Just curious, where did you copy the code from? If it's from an open-source project, I think it's worth reminding the original author that their code is not correct. If it's from AI-generated code, in my experience, AI frequently makes this type of error.

[darcys22]

Yeah looks like that param is completely unused on the ethereum client.

Source was here
https://github.com/maticnetwork/pos-commit-poc

[venezuela01]

1. The maticnetwork/pos-commit-poc repository originally copied its code from jstoxrocky/zksnarks_example.
2. The jstoxrocky/zksnarks_example repository initially generated its code from Zokrates/ZoKrates.
3. The original ZoKrates code had nearly five years of history without any complaints.
4. Zokrates/ZoKrates fixed this issue after it was reported.

Could you also address this instance?

This issue demonstrates the potential risk of a supply chain attack. This time we were lucky to detect it early, and fortunate that the code was not seriously risky. However, if the other time some upstream code is not entirely safe, it could eventually affect us in the future.

[venezuela01]

Fixed by Zellic 3.5: Wrong input length to static call as part of Zellic audit triage

Thanks @darcys22 @Doy-lee and @KeeJef.

Just curious, did the Zellic audit team independently rediscover this issue by reviewing the code, or did they review this GitHub issue and confirm that this is a potential issue that needs to be addressed?

I'm asking because I'm curious about the coverage of their audit. If they independently rediscover an issue we found before, then it's a good signal that they have conducted an extensive audit with high coverage.

[KeeJef]

Full audit report will be released soon with findings, just finalising things now

[venezuela01]

Hi @KeeJef Thanks for publishing the audit report.

I've read:

• https://token.getsession.org/blog/zellic-rewards-contract-audit
• https://github.com/Zellic/publications/blob/master/Session%20Token%20-%20Zellic%20Audit%20Report.pdf

However, my previous question was not answered, please allow me to rephrase it:

Did the Zellic audit team identify the input/output size issue independently through their code review, or did they merely confirm its existence after reviewing our GitHub issue?

The keyword is independently.

Understanding this is helpful for making an educated guess of the thoroughness of their audit. If they independently rediscovered a known issue, it suggests their audit has a lower chance of missing other vulnerabilities. Conversely, if they missed something we previously found, then it raises concerns about potential blind spots in other parts of their audit process.

Is my question clear enough?

[KeeJef]

My understanding is that they did not review any GitHub issues before determining their findings. We simply provided them with the commit hash, the repository link to the contracts folder, and a high-level outline of the contracts and interaction paths before the audit began. We did not provide any background on previously raised GitHub issues, and they did not bring up any community-identified issues with us.

We specifically directed Zellic to focus attention in their audit on our cryptographic implementation of BLS validation logic, which is where they spent most of their time and where we had the biggest blind spots, they also looked our other contracts, but those contracts implement much more basic functionality.

[venezuela01]

My understanding is that they did not review any GitHub issues before determining their findings. We simply provided them with the commit hash, the repository link to the contracts folder, and a high-level outline of the contracts and interaction paths before the audit began. We did not provide any background on previously raised GitHub issues, and they did not bring up any community-identified issues with us.

We specifically directed Zellic to focus attention in their audit on our cryptographic implementation of BLS validation logic, which is where they spent most of their time and where we had the biggest blind spots, they also looked our other contracts, but those contracts implement much more basic functionality.

Thanks, that's a very detailed answer to my question. I appreciate the effort you put into it.

[venezuela01]

Close this issue as it's already fixed. Thank you everyone!