Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add audit comments #168

Merged
merged 1 commit into from
Nov 3, 2023
Merged

chore: add audit comments #168

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions contracts/src/Summa.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,17 @@ contract Summa is Ownable {
uint256 amount;
}

//Verifier contracts
// Verifier contracts
IVerifier private immutable solvencyVerifier;
IVerifier private immutable inclusionVerifier;

//All address ownership proofs submitted by the CEX
// All address ownership proofs submitted by the CEX
AddressOwnershipProof[] public addressOwnershipProofs;

//Convenience mapping to check if an address has already been verified
// Convenience mapping to check if an address has already been verified
/*
Boolean type is better than uint256 for this mapping, at least more than 2,100 gas is saved per call
*/
mapping(bytes32 => uint256) public ownershipProofByAddress;

// MST roots corresponding to successfully verified solvency proofs by timestamp
Expand All @@ -64,6 +67,15 @@ contract Summa is Ownable {
inclusionVerifier = _inclusionVerifier;
}

/*
// While this duplicate method might elevate deployment costs, the trade-off is a reduction in usage and maintenance costs.
function submitProofOfAddressOwnership(
AddressOwnershipProof memory _addressOwnershipProof
) public onlyOwner {
...
}
*/

/**
* @dev Submit an optimistic proof of address ownership for a CEX. The proof is subject to an off-chain verification as it's not feasible to verify the signatures of non-EVM chains in an Ethereum smart contract.
* @param _addressOwnershipProofs The list of address ownership proofs
Expand All @@ -77,6 +89,9 @@ contract Summa is Ownable {
);
uint256 index = ownershipProofByAddress[addressHash];
require(index == 0, "Address already verified");
/*
Is there any reason to assign value `i + 1` to `ownershipProofByAddress[addressHash]`?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to store the array index, but starting with 1 allows to distinguish a case when the proof has not yet been submitted (the storage slots are zero by default).

*/
ownershipProofByAddress[addressHash] = i + 1;
addressOwnershipProofs.push(_addressOwnershipProofs[i]);
require(
Expand Down Expand Up @@ -125,6 +140,10 @@ contract Summa is Ownable {
emit SolvencyProofSubmitted(timestamp, inputs[0], assets);
}

/*
It would be helpful to provide a description of the public inputs for the `verifySolvencyProof` and `verifyInclusionProof` methods.
*/

/**
* Verify the proof of CEX solvency
* @param proof ZK proof
Expand Down