chore: add audit comments #163

enricobottazzi · 2023-10-02T08:09:44Z

No description provided.

sifnoc

In this audit comments, many findings are related to supporting the off-chain verification with full_verifier.

sifnoc · 2023-10-31T08:24:51Z

backend/examples/summa_solvency_flow.rs

+    /*
+        If I understand correctly, the function `dispatch_proof_of_address_ownership` returns a `Result<(), Box<dyn Error>>`.
+        When we call unwrap on the result, it will panic if the result is an error.
+        Is that correct?


Yes, It is correct. I will modify all error propagation in the async functions.

sifnoc · 2023-10-31T08:40:35Z

backend/examples/summa_solvency_flow.rs

@@ -38,13 +38,24 @@ async fn main() -> Result<(), Box<dyn Error>> {
    )
    .unwrap();

+    /*
+        I think that this part related to `address_hashes` can be safely removed.
+        The example should be as simple as possible.


That makes sense. There isn't any check procedure for the Submit solvency proof too.
FYI, The only reason for adding this checking step is due to the require term in the contract:

summa-solvency/contracts/src/Summa.sol

Lines 107 to 110 in ffd8259

require(

addressOwnershipProofs.length != 0,

"The CEX has not submitted any address ownership proofs"

);

sifnoc · 2023-10-31T09:14:34Z

backend/examples/summa_solvency_flow.rs

@@ -140,6 +169,15 @@ async fn main() -> Result<(), Box<dyn Error>> {

    let public_inputs = downloaded_inclusion_proof.get_public_inputs();

+    /*
+        Why leaf_hash is public_inputs[0][0] while mst_root is public_inputs[1]?
+        Shouldn't public_inputs be a vector with 2 elements?


I've overridden public_inputs to be a one-dimensional vector for simplicity.
However, the reason public_inputs was initially structured as a two-dimensional vector was due to the way the full_verifier function receives and processes it.

summa-solvency/zk_prover/src/circuits/utils.rs

Lines 114 to 119 in ffd8259

pub fn full_verifier(

params: &ParamsKZG<Bn256>,

vk: &VerifyingKey<G1Affine>,

proof: Vec<u8>,

public_inputs: Vec<Vec<Fp>>,

) -> bool {

sifnoc · 2023-10-31T09:32:02Z

backend/src/apis/address_ownership.rs

@@ -29,6 +32,11 @@ impl AddressOwnership {
        &self.address_ownership_proofs
    }

+    /*
+        If I understand correctly, an error in submit_proof_of_address_ownership is propagated to the caller.
+        Is that correct?


Yes, the try operator, "?", returns an Error when one occurs.

sifnoc · 2023-10-31T09:39:13Z

backend/src/apis/round.rs

+/*
+   Since we are now using the `MstInclusionProof` only for on-chain verification,
+   why don't we replace the data type with something that can be directly consumed by the contract?
+   Similar to `SolvencyProof`.


We still support off-chain verification using the full_verifier method.
That's why I haven't changed the public_inputs type.

I think we can set the on-chain verifcation (via the smart contract interface) as standard way to perform the proof of inclusion verification. This is because of the nice UX that it provides + full transparency on the code being executed compared to the other available solutions.

Therefore I suggest to remove every dependency that relates to off-chain verification and foucs on the on-chain path.

sifnoc · 2023-10-31T09:41:45Z

backend/src/apis/round.rs

@@ -78,6 +86,9 @@ where
    [usize; N_ASSETS + 1]: Sized,
    [usize; 2 * (1 + N_ASSETS)]: Sized,
 {
+    /*
+       Build signer outside of the constructor and pass it in as a parameter.
+    */


I will handle this at #155 Refactor signers

sifnoc · 2023-10-31T09:44:21Z

backend/src/apis/round.rs

@@ -104,6 +115,10 @@ where
        self.timestamp
    }

+    /*
+        If I understand correctly, an error in submit_proof_of_solvency is propagated to the caller.
+        Is that correct?


Yes, the try operator, "?", returns an Error when one occurs.

sifnoc · 2023-10-31T09:52:33Z

backend/src/apis/round.rs

@@ -202,6 +217,10 @@ where
        let circuit =
            MstInclusionCircuit::<LEVELS, N_ASSETS, N_BYTES>::init(self.mst.clone(), user_index);

+        /*
+            Why can't we get the `calldata` using `gen_proof_solidity_calldata` similar to `generate_proof_of_solvency`?
+        */


We can use gen_proof_solidity_calldata similar to generate_proof_of_solvency.
However, as I mentioned earlier, the type of public_inputs is closely related to the full_verifier, which is why I've kept it as is.

See comments above

sifnoc · 2023-10-31T09:52:54Z

backend/src/contracts/signer.rs

@@ -41,6 +41,9 @@ impl SummaSigner {
        }
    }

+    /*
+    This method is never used. Can it be removed?
+     */


Yes! I will remove this.

chore: add audit comments

f199ddb

enricobottazzi requested a review from sifnoc October 2, 2023 08:09

enricobottazzi linked an issue Oct 30, 2023 that may be closed by this pull request

Internal Audit of Backend #162

Closed

1 task

sifnoc reviewed Oct 31, 2023

View reviewed changes

enricobottazzi closed this Nov 3, 2023

enricobottazzi deleted the enrico/backend-audit branch November 21, 2023 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add audit comments #163

chore: add audit comments #163

enricobottazzi commented Oct 2, 2023

sifnoc left a comment

sifnoc Oct 31, 2023

sifnoc Oct 31, 2023

sifnoc Oct 31, 2023

sifnoc Oct 31, 2023

sifnoc Oct 31, 2023

enricobottazzi Oct 31, 2023

sifnoc Oct 31, 2023

sifnoc Oct 31, 2023

sifnoc Oct 31, 2023

enricobottazzi Oct 31, 2023

sifnoc Oct 31, 2023

	require(
	addressOwnershipProofs.length != 0,
	"The CEX has not submitted any address ownership proofs"
	);

	pub fn full_verifier(
	params: &ParamsKZG<Bn256>,
	vk: &VerifyingKey<G1Affine>,
	proof: Vec<u8>,
	public_inputs: Vec<Vec<Fp>>,
	) -> bool {

chore: add audit comments #163

chore: add audit comments #163

Conversation

enricobottazzi commented Oct 2, 2023

sifnoc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment