Refactor backend by following internal audit comments #173

sifnoc · 2023-10-31T15:22:45Z

Updates:

Create bash script(fixed the revert issue)
Removed address_hashes
Refactored MstInclusionProof struct
Fix error propagation on the async functions with try operator
Fix some assesrt code
Removed function get_deployment_address
Updated README
- Build a Solvency Verifier Contract: the verifier generated base on specific ptau file.
- Generating Verifiers for Backend: Introduce bash script instead of
Checked a unix timestamp as u64

…kend

enricobottazzi · 2023-11-01T09:46:58Z

backend/scripts/update_verifier_contracts.sh

+# Build the verifier contracts
+echo "1. Building verifier contracts"
+cd ../zk_prover
+cargo run --example gen_inclusion_verifier


Can you have it run on release mode?

enricobottazzi · 2023-11-01T09:59:56Z

backend/README.md

@@ -48,7 +48,27 @@ SIGNATURE_VERIFICATION_MESSAGE="Summa proof of solvency for CryptoExchange" carg

 ### Generating Verifiers for Backend


A few things here:

Here the script is not only generating new Verifiers but the new Summa contract which takes also the new verifiers in the constructors

The README should also explain that by default the backend is working with a Summa contract based on certain parameters. You might want to generate a new one if and only if you want to work with different parameters and a different ptau file!

Good point! That's missing in here. I will update for that.

enricobottazzi · 2023-11-01T10:02:32Z

backend/examples/summa_solvency_flow.rs

-    utils::keccak256,
+use std::{
+    error::Error,
+    fs::{remove_file, File},


unused depedency

enricobottazzi · 2023-11-01T10:06:49Z

backend/README.md

@@ -97,12 +114,16 @@ If executed successfully, you'll see:

 ### 2. Submit Proof of Solvency

-This step is crucial for two primary reasons: first, to validate the root hash of the Merkle Sum Tree (`mst_root`); and second, to ensure that the assets held by the CEX exceed their liabilities, as confirmed through the proof verification on the Summa contract.
-The CEX must submit this proof of solvency to the Summa contract. Currently, it's a mandatory requirement to provide this proof before generating the inclusion proof for each user in the current round.
+This step is also crucial for two primary reasons: 


Leaving the comment here because I can't do otherwise. For section "### 4. Verify Proof of Inclusion" can you please specify that in production this can be performed independently by a user using an interface such as Etherscan?

I understand your point. Are you suggesting that we emphasize to the CEX that the user can independently verify their proof using a public interface like Etherscan?

alxkzmn

Why did you decide to remove get_deployment_address? Now you need to supply this address manually when creating the signer, and the deployments file generation is unnecessary.

sifnoc · 2023-11-02T05:17:36Z

Why did you decide to remove get_deployment_address? Now you need to supply this address manually when creating the signer, and the deployments file generation is unnecessary.

Well, I found that the method get_deployment_address was not being used effectively in both the tests and example/summa_solvency_flow. It seemed more straightforward to retrieve the address directly from the summa_contract instance.
While I can see its potential usefulness in real-world scenarios, it didn't seem necessary for current implementation.
If you have any other examples or scenarios in mind where it could be useful, I'd love to hear your thoughts.

enricobottazzi

LGTM ∑

sifnoc · 2023-11-02T12:37:34Z

@alxkzmn Now the Signer accept Address and also Path of file contains address as config.

alxkzmn · 2023-11-03T05:05:52Z

Awesome!

* chore: add audit comments (#168) * Refactor backend by following internal audit comments (#173) * feat: create bash script for updating verifier interface files in backend * fix: error propagation with try operator; remove unnecessaries * refactor: changed data type in 'MstInclusionProof' * fix: generate solvency verifier contract * chore: remove left over * chore: update README * fix: remove left over; assert term * fix: update README; small fixes * feat: Signer accepts address or file path for init * Refactor contract according to V1 consolidation spec (#169) * Refactor smart contract * Update smart contract module readme --------- Co-authored-by: JinHwan <[email protected]>

* chore: add audit comments (#168) * Refactor backend by following internal audit comments (#173) * feat: create bash script for updating verifier interface files in backend * fix: error propagation with try operator; remove unnecessaries * refactor: changed data type in 'MstInclusionProof' * fix: generate solvency verifier contract * chore: remove left over * chore: update README * fix: remove left over; assert term * fix: update README; small fixes * feat: Signer accepts address or file path for init * feat: added mutex lock to signer; used it as ref * fix: mutex deadlock in signer * chore: minor updates * feat: `Tree` trait * fix: move `verify_proof` logic to `Tree` trait * feat: added method implementation to `Tree` trait and moved outside of utils * refactor: signer spawn provider internally; updated comments * fix: round now use Tree trait * refactor: Round needs MST and Assets instead of csv files path * fix: rollback applying csv_parser for AddressOwnership and Assets in Snapshot * fix: Solvency::init fn accept type that has Tree trait * fix: updates for summa solvency contract v1.1 * chore: rename `compute_leaves` api * fix: Rounds accept 'Tree' trait object * chore: removed and updated comments * chore: removed env variables 'SIGNATURE_VERIFICATION_MESSAGE' --------- Co-authored-by: JinHwan <[email protected]> Co-authored-by: sifnoc <[email protected]>

sifnoc added 7 commits October 30, 2023 10:56

feat: create bash script for updating verifier interface files in bac…

44ee5c7

…kend

fix: error propagation with try operator; remove unnecessaries

5b2f510

refactor: changed data type in 'MstInclusionProof'

897027a

fix: generate solvency verifier contract

d8ef0cb

chore: remove left over

ab5d31b

chore: update README

ce6c0c6

fix: remove left over; assert term

98a2039

sifnoc marked this pull request as ready for review November 1, 2023 05:58

sifnoc requested review from enricobottazzi and alxkzmn and removed request for enricobottazzi November 1, 2023 05:58

enricobottazzi requested changes Nov 1, 2023

View reviewed changes

fix: update README; small fixes

fda1e0b

sifnoc force-pushed the audit-backend-updates branch from 7313ce3 to fda1e0b Compare November 2, 2023 04:26

alxkzmn reviewed Nov 2, 2023

View reviewed changes

enricobottazzi self-requested a review November 2, 2023 10:26

enricobottazzi approved these changes Nov 2, 2023

View reviewed changes

feat: Signer accepts address or file path for init

e1df685

alxkzmn merged commit 400fc49 into master Nov 3, 2023
4 checks passed

enricobottazzi linked an issue Nov 3, 2023 that may be closed by this pull request

Internal Audit of Backend #162

Closed

1 task

enricobottazzi deleted the audit-backend-updates branch November 21, 2023 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor backend by following internal audit comments #173

Refactor backend by following internal audit comments #173

sifnoc commented Oct 31, 2023 •

edited

Loading

enricobottazzi Nov 1, 2023

enricobottazzi Nov 1, 2023

sifnoc Nov 1, 2023

enricobottazzi Nov 1, 2023

enricobottazzi Nov 1, 2023

sifnoc Nov 1, 2023

enricobottazzi Nov 1, 2023

alxkzmn left a comment

sifnoc commented Nov 2, 2023

enricobottazzi left a comment

sifnoc commented Nov 2, 2023

alxkzmn commented Nov 3, 2023

		@@ -48,7 +48,27 @@ SIGNATURE_VERIFICATION_MESSAGE="Summa proof of solvency for CryptoExchange" carg

		### Generating Verifiers for Backend

Refactor backend by following internal audit comments #173

Refactor backend by following internal audit comments #173

Conversation

sifnoc commented Oct 31, 2023 • edited Loading

enricobottazzi Nov 1, 2023

Choose a reason for hiding this comment

enricobottazzi Nov 1, 2023

Choose a reason for hiding this comment

sifnoc Nov 1, 2023

Choose a reason for hiding this comment

enricobottazzi Nov 1, 2023

Choose a reason for hiding this comment

enricobottazzi Nov 1, 2023

Choose a reason for hiding this comment

sifnoc Nov 1, 2023

Choose a reason for hiding this comment

enricobottazzi Nov 1, 2023

Choose a reason for hiding this comment

alxkzmn left a comment

Choose a reason for hiding this comment

sifnoc commented Nov 2, 2023

enricobottazzi left a comment

Choose a reason for hiding this comment

sifnoc commented Nov 2, 2023

alxkzmn commented Nov 3, 2023

sifnoc commented Oct 31, 2023 •

edited

Loading