From f199ddb3c157725d6b0bd328ab33130a31d12da8 Mon Sep 17 00:00:00 2001 From: Enrico Bottazzi <85900164+enricobottazzi@users.noreply.github.com> Date: Sun, 1 Oct 2023 16:43:45 +0200 Subject: [PATCH] chore: add audit comments --- backend/README.md | 4 +++ backend/examples/summa_solvency_flow.rs | 42 +++++++++++++++++++++++++ backend/src/apis/address_ownership.rs | 8 +++++ backend/src/apis/round.rs | 19 +++++++++++ backend/src/contracts/signer.rs | 3 ++ backend/src/tests.rs | 12 ++++++- 6 files changed, 87 insertions(+), 1 deletion(-) diff --git a/backend/README.md b/backend/README.md index 5168e2b4..3d1efd50 100644 --- a/backend/README.md +++ b/backend/README.md @@ -48,6 +48,10 @@ SIGNATURE_VERIFICATION_MESSAGE="Summa proof of solvency for CryptoExchange" carg ### Generating Verifiers for Backend +/* + Can we replace this whole process with a bash script +*/ + The following steps are optional and are only required if you need to update the verifier contracts for the backend: 1. **Build the Verifier Contracts**: diff --git a/backend/examples/summa_solvency_flow.rs b/backend/examples/summa_solvency_flow.rs index 76a0861d..31eb8543 100644 --- a/backend/examples/summa_solvency_flow.rs +++ b/backend/examples/summa_solvency_flow.rs @@ -38,6 +38,11 @@ async fn main() -> Result<(), Box> { ) .unwrap(); + /* + I think that this part related to `address_hashes` can be safely removed. + The example should be as simple as possible. + */ + // Retrieve hashed addresses using the `keccak256` method. let address_hashes = address_ownership_client .get_ownership_proofs() @@ -45,6 +50,12 @@ async fn main() -> Result<(), Box> { .map(|x| keccak256(encode(&[Token::String(x.cex_address.clone())]))) .collect::>(); + /* + If I understand correctly, the function `dispatch_proof_of_address_ownership` returns a `Result<(), Box>`. + When we call unwrap on the result, it will panic if the result is an error. + Is that correct? + */ + // Dispatch the proof of address ownership. // the `dispatch_proof_of_address_ownership` function sends a transaction to the Summa contract. address_ownership_client @@ -97,9 +108,23 @@ async fn main() -> Result<(), Box> { ) .unwrap(); + /* + I still get the error: + ``assert_eq` of unit values detected. This will always succeed + for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unit_cmp + `#[deny(clippy::unit_cmp)]` on by defaultclippyClick for full compiler diagnostic` + + I think that, similarly to what you do with `dispatch_proof_of_address_ownership` about, we can remove the `assert_eq` statement. + Instead, by just calling round.dispatch_solvency_proof().await.unwrap(), it will panic if the result is an error. + + */ // Sends the solvency proof, which should ideally complete without errors. assert_eq!(round.dispatch_solvency_proof().await.unwrap(), ()); + /* + Remove the follwing println statement. + */ + // You can also use the `solvency_proof_submitted_filter` method to check if the solvency proof is submitted. // println!("{:?}", summa_contract // .solvency_proof_submitted_filter() @@ -111,6 +136,10 @@ async fn main() -> Result<(), Box> { // 3. Generate Inclusion Proof // + /* + Remove the follwing comment. It seems that what you are doing here is different from what you would do in a production setup. + */ + // In a production setup, the CEX should first dispatch the solvency proof to update the Merkle sum tree's root before generating any inclusion proofs. // Otherwise, users might distrust the provided `root_hash` in the inclusion proof, as it hasn't been published on-chain. let inclusion_proof = round.get_proof_of_inclusion(USER_INDEX).unwrap(); @@ -140,6 +169,15 @@ async fn main() -> Result<(), Box> { 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 would specify that the balances are the balances of the user on the CEX at `snapshot_time`. + */ + // Verify the `leaf_hash` from the proof file. // It's assumed that both `user_name` and `balances` are provided by the CEX. let user_name = "dxGaEAii".to_string(); @@ -151,6 +189,10 @@ async fn main() -> Result<(), Box> { generate_leaf_hash::(user_name.clone(), balances.clone()) ); + /* + This whole conversion seems unnecessary to me. Look at the comment in `Round` + */ + // Before verifying `root_hath`, convert type of `proof` and `public_inputs` to the type of `Bytes` and `Vec`. let proof: Bytes = Bytes::from(inclusion_proof.get_proof().clone()); let public_inputs: Vec = inclusion_proof diff --git a/backend/src/apis/address_ownership.rs b/backend/src/apis/address_ownership.rs index 7b56348b..68ed7697 100644 --- a/backend/src/apis/address_ownership.rs +++ b/backend/src/apis/address_ownership.rs @@ -10,6 +10,9 @@ pub struct AddressOwnership { } impl AddressOwnership { + /* + Build signer outside of the constructor and pass it in as a parameter. + */ pub fn new( signer_key: &str, chain_id: u64, @@ -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? + */ + // This function dispatches the proof of address ownership. Before calling this function, // ensure externally that the provided `addresses` in `address_ownership_proof` are not already registered // on the Summa contract. diff --git a/backend/src/apis/round.rs b/backend/src/apis/round.rs index c1f47b45..76089c24 100644 --- a/backend/src/apis/round.rs +++ b/backend/src/apis/round.rs @@ -44,6 +44,11 @@ impl SolvencyProof { } } +/* + 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`. +*/ #[derive(Debug, Clone, Serialize, Deserialize)] pub struct MstInclusionProof { public_inputs: Vec>, @@ -66,6 +71,9 @@ pub struct Snapshot { timestamp: u64, snapshot: Snapshot, @@ -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. + */ pub fn new( signer_key: &str, chain_id: u64, @@ -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? + */ pub async fn dispatch_solvency_proof(&mut self) -> Result<(), Box> { let proof: SolvencyProof = match self.snapshot.generate_proof_of_solvency() { Ok(p) => p, @@ -202,6 +217,10 @@ where let circuit = MstInclusionCircuit::::init(self.mst.clone(), user_index); + /* + Why can't we get the `calldata` using `gen_proof_solidity_calldata` similar to `generate_proof_of_solvency`? + */ + // Currently, default manner of generating a inclusion proof for solidity-verifier. let proof = gen_evm_proof_shplonk( &self.trusted_setup[0].0, diff --git a/backend/src/contracts/signer.rs b/backend/src/contracts/signer.rs index 5b4f3ccc..deec9a86 100644 --- a/backend/src/contracts/signer.rs +++ b/backend/src/contracts/signer.rs @@ -41,6 +41,9 @@ impl SummaSigner { } } + /* + This method is never used. Can it be removed? + */ pub fn get_deployment_address>( path: P, chain_id: u64, diff --git a/backend/src/tests.rs b/backend/src/tests.rs index faf41cee..c4dff6d1 100644 --- a/backend/src/tests.rs +++ b/backend/src/tests.rs @@ -132,6 +132,9 @@ mod test { .dispatch_proof_of_address_ownership() .await; + /* + Replace it with assert!(ownership_submitted_result.is_ok()); + */ assert_eq!(ownership_submitted_result.is_ok(), true); let logs = summa_contract @@ -200,6 +203,9 @@ mod test { }, ]; + /* + Check comment in `examples/summa_solvency_flow.rs` for the following line + */ assert_eq!(round.dispatch_solvency_proof().await.unwrap(), ()); // After sending transaction of proof of solvency, logs should be updated @@ -224,6 +230,10 @@ mod test { ); // Test inclusion proof + + /* + This whole conversion seems unnecessary to me. Look at the comment in `Round` + */ let inclusion_proof = round.get_proof_of_inclusion(0).unwrap(); let proof = Bytes::from(inclusion_proof.get_proof().clone()); let public_inputs: Vec = inclusion_proof @@ -244,7 +254,7 @@ mod test { .await .unwrap(); - assert_eq!(verified, true); + assert!(verified); drop(anvil); }