From f11b1fe194cdde49d8d38030b45851c6829bb859 Mon Sep 17 00:00:00 2001 From: JinHwan Date: Mon, 26 Aug 2024 18:46:19 +0900 Subject: [PATCH] Mitigating high impact issues (#300) * feat: added unblind username feature * feat: checking maximum value of converted username * feat: added warning about possiblity of private data leakage while verifying process * feat: added warning about 'validateVKPermutationsLength' method * fix typo --- backend/README.md | 3 ++- contracts/src/Summa.sol | 7 +++++ prover/Cargo.toml | 1 + prover/src/circuits/univariate_grand_sum.rs | 10 ++++++- prover/src/entry.rs | 29 +++++++++++++++++++-- 5 files changed, 46 insertions(+), 4 deletions(-) diff --git a/backend/README.md b/backend/README.md index 59905f15..9f413bdc 100644 --- a/backend/README.md +++ b/backend/README.md @@ -144,4 +144,5 @@ The result will display as: 4. Verifying the proof on contract verifier for User #0: true ``` -**Note:** In a production environment, users can independently verify their proof using public interfaces, such as Etherscan. +**Note:** In a production environment, users can independently verify their proof using public interfaces, such as Etherscan. +Also, It's crucial for the prover to inform users about the potential risk of private data leakage, specifically their balances, during the proof verification process. This is especially important when using a third-party endpoint to query `verify_inclusion_proof` method. diff --git a/contracts/src/Summa.sol b/contracts/src/Summa.sol index 66904f83..085d8790 100644 --- a/contracts/src/Summa.sol +++ b/contracts/src/Summa.sol @@ -136,6 +136,13 @@ contract Summa is Ownable { * @param vkContract The address of the verifying key contract * @param numberOfCurrencies The number of cryptocurrencies whose polynomials are committed in the proof * @return isValid True if the number of permutations in the verifying key corresponds to the number of cryptocurrencies + * + * WARNING: The permutation length may not be correctly calculated by this method if the prover deliberately + * tries to deceive the process. This issue cannot be resolved even if we change the approach to rely on user input + * for the length instead of calculating it within the method. The ultimate solution is to implement a validation process for the + * verifying key contract that can be performed by the user themselves. This issue will be addressed in the following: + * https://github.com/summa-dev/summa-solvency/issues/299 + */ */ function validateVKPermutationsLength( address vkContract, diff --git a/prover/Cargo.toml b/prover/Cargo.toml index 435f5692..1e5fb4f8 100644 --- a/prover/Cargo.toml +++ b/prover/Cargo.toml @@ -9,6 +9,7 @@ edition = "2021" dev-graph = ["halo2_proofs/dev-graph", "plotters"] profiling = [] no_range_check = [] +unblind-username = [] [dependencies] diff --git a/prover/src/circuits/univariate_grand_sum.rs b/prover/src/circuits/univariate_grand_sum.rs index 75f60b41..fb6b378a 100644 --- a/prover/src/circuits/univariate_grand_sum.rs +++ b/prover/src/circuits/univariate_grand_sum.rs @@ -72,7 +72,15 @@ where [(); N_CURRENCIES + 1]:, { fn configure(meta: &mut ConstraintSystem) -> Self { - let username = meta.advice_column(); + // By default, the username is in a blinded column like any other private inputs in common Halo2 circuit. + // However, this makes it indistinguishable between fake user data and real user data that has zero values in Summa circuit. + // Therefore, we have introduced a feature that allows the username to be unblinded instead of blinded, which may be necessary for the prover in the future. + // Refer to this comment for more details: https://github.com/zBlock-2/summa-solvency/issues/9#issuecomment-2143427298 + let username: Column = if cfg!(feature = "unblind-username") { + meta.unblinded_advice_column() + } else { + meta.advice_column() + }; let balances = [(); N_CURRENCIES].map(|_| meta.unblinded_advice_column()); diff --git a/prover/src/entry.rs b/prover/src/entry.rs index daaef76c..e9810b7d 100644 --- a/prover/src/entry.rs +++ b/prover/src/entry.rs @@ -1,6 +1,7 @@ +use halo2_proofs::halo2curves::bn256::Fr as Fp; use num_bigint::BigUint; -use crate::utils::big_intify_username; +use crate::utils::{big_intify_username, fp_to_big_uint}; /// An entry in the Merkle Sum Tree from the database of the CEX. /// It contains the username and the balances of the user. @@ -13,8 +14,19 @@ pub struct Entry { impl Entry { pub fn new(username: String, balances: [BigUint; N_CURRENCIES]) -> Result { + let username_as_big_uint = big_intify_username(&username); + let max_allowed_value = fp_to_big_uint(Fp::zero() - Fp::one()); + + // Ensure the username, when converted to a BigUint, does not exceed the field modulus + // This prevents potential overflow issues by asserting that the username's numeric value + // is within the allowable range defined by the field modulus + // Please refer to https://github.com/zBlock-2/audit-report/blob/main/versionB.md#4-high-missing-username-range-check-in-big_intify_username--big_uint_to_fp + if username_as_big_uint > max_allowed_value { + return Err("The value that converted username should not exceed field modulus"); + } + Ok(Entry { - username_as_big_uint: big_intify_username(&username), + username_as_big_uint, balances, username, }) @@ -42,3 +54,16 @@ impl Entry { &self.username } } + +#[cfg(test)] +#[test] +fn test_entry_new() { + let short_username_entry = Entry::new(String::from("userA"), [BigUint::from(0u32)]); + assert!(short_username_entry.is_ok()); + + let long_username_entry = Entry::new( + String::from("userABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"), + [BigUint::from(0u32)], + ); + assert!(long_username_entry.is_err()) +}