zBlock-2 / summa-solvency

Monorepo for Summa Proof of Solvency Protocol
Apache License 2.0
0 stars 3 forks source link

Wrong verifying key contract permutation length can be considered valid by validateVKPermutationsLength #10

Open obatirou opened 2 months ago

obatirou commented 2 months ago

The function validateVKPermutationsLength is used to validate the number of permutations in the verifying key contract corresponds to the circuit used to generate proof for the number of cryptocurrencies the custodian committed to. The Summa contract assumes that permutations commitments begins at bytes 0x2e0 in the vKContract (source) This can lead to wrongfully consider valid vkContracts with different permutation commitments from the circuit used by the custodian. A simple case would be a verifying contract with 2 fixed commitments but one permutation commitment less than the one being used. In this case, all others things being equals, the permutations commitments would not begin at bytes 0x2e0 but at 0x0320. As the length of the bytes of the contracts are still equals due to one permutation in less, validateVKPermutationsLength would consider this vkcontract valid but in fact is not.

sebastiantf commented 2 months ago

I suppose this is kinda analogous / related to how SnarkVerifier loads exactly 0x0560 bytes from the vk contract https://github.com/zBlock-2/summa-solvency/blob/fec83a747ead213261aecfaf4a01b43fff9731ee/contracts/src/SnarkVerifier.sol#L218

If the no. of commitments change, then the length of the bytecode loaded also changes, no? But I suppose extcodesize could be used while sacrificing some gas costs for this.

But I think the point I'm tryna make is maybe Summa.sol is justified (borrows the behavior in SnarkVerifier.sol) in using a hardcoded length in lieu of SnarkVerifier using hardcoded ptr location / lengths. But the latter the generated whereas the former is written. So I suppose there should some manual/automated verification that this pointer is correct on each iteration?

Although I did not notice any of the verifiers using pointers starting from the commitments. They all seem to only use the ptrs until neg_s_g2_y_2: https://github.com/zBlock-2/summa-solvency/blob/fec83a747ead213261aecfaf4a01b43fff9731ee/contracts/src/VerifyingKey.sol#L28

0xalizk commented 1 month ago

I don't understand this part:

A simple case would be a verifying contract with 2 fixed commitments but one permutation commitment less than the one being used Could you elaborate?

obatirou commented 1 month ago

I don't understand this part:

A simple case would be a verifying contract with 2 fixed commitments but one permutation commitment less than the one being used Could you elaborate?

Sure, it is a follow up of this discussion https://discord.com/channels/877252171983360072/1230201530582568962/1230453113199263777 Commitments are hardcoded in the Halo2VerifyingKey https://github.com/zBlock-2/summa-solvency/blob/fec83a747ead213261aecfaf4a01b43fff9731ee/contracts/src/VerifyingKey.sol#L30

Hence, if a different VerifyingKey contract is created with same bytecode size but different commitments, it can be considered valid by validateVKPermutationsLength function https://github.com/zBlock-2/summa-solvency/blob/fec83a747ead213261aecfaf4a01b43fff9731ee/contracts/src/Summa.sol#L151

In the current one, https://github.com/zBlock-2/summa-solvency/blob/fec83a747ead213261aecfaf4a01b43fff9731ee/contracts/src/VerifyingKey.sol there are 1 fixed commitments and 10 permutation commitments. We could imagine a new contracts with 2 fixed commitments and 9 permutations commitments, this one would still be considered valid by validateVKPermutationsLength function even if the number of permutations commitments is invalid.

sifnoc commented 5 days ago

Although I did not notice any of the verifiers using pointers starting from the commitments. They all seem to only use the ptrs until neg_s_g2_y_2:

The permutation commitments are utilized during the verification process in SnarkVerifier. This verifier copies all verifying keys as shown here: https://github.com/zBlock-2/summa-solvency/blob/fec83a747ead213261aecfaf4a01b43fff9731ee/contracts/src/SnarkVerifier.sol#L218

sifnoc commented 5 days ago

To further clarify the example, let's consider how manipulation could occur in the current Summa implementation. A prover wishing to manipulate the validateVKPermutationsLength function might modify the circuit to accept additional public inputs. Although this scenario does not perfectly align with the case discussed, as the increase in fixed commitments would change the base number of permutations from 2 to 3 in the formula, resulting in a modified formula: numPermutations = 3 + (balanceByteRange/2) * numberOfCurrencies instead of https://github.com/zBlock-2/summa-solvency/blob/fec83a747ead213261aecfaf4a01b43fff9731ee/contracts/src/Summa.sol#L147 This demonstrates that adjustments can be made by altering the circuit configuration.

Subsequently, the prover would need to send the commitment transaction with more public inputs. Here, I assume the Summa contract has been modified to accommodate two public inputs by adjusting the arguments here: https://github.com/zBlock-2/summa-solvency/blob/fec83a747ead213261aecfaf4a01b43fff9731ee/contracts/src/Summa.sol#L239

It's important to note that the prover would need to modify other hardcoded numbers to achieve a valid result from the validateVKPermutationsLength by altering the number of permutations, without changing the hardcoded startOffsetForPermutations. More importantly, users can easily notice a commitment transaction designed for two fixed columns, which is inconsistent with having just one public input.

Furthermore, another variable, offsetAfterLastPermutation, depends on numPermutations as well as startOffsetForPermutations. https://github.com/zBlock-2/summa-solvency/blob/fec83a747ead213261aecfaf4a01b43fff9731ee/contracts/src/Summa.sol#L154-L156

It can also produce a valid result with an incorrect number of currencies and a different balanceByteRange. For example:

This outcome follows the formula: https://github.com/zBlock-2/summa-solvency/blob/fec83a747ead213261aecfaf4a01b43fff9731ee/contracts/src/Summa.sol#L147-L149

However, such discrepancies are typically obvious to users when they attempt to verify their proofs.