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.

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

Security concerns in Summa.sol #23

Open
hrishibhat opened this issue Apr 30, 2024 · 0 comments
Open

Security concerns in Summa.sol #23

hrishibhat opened this issue Apr 30, 2024 · 0 comments

Comments

@hrishibhat
Copy link

  • No timestamp validation:
    function submitCommitment(
        bytes calldata snarkProof,
        bytes calldata grandSumProof,
        uint256[] memory totalBalances,
        uint256 timestamp // @audit : Future timestamp can be used. This can be used to manipulate 

timestamp is expected to be time at which the exchange has taken snapshot of all the balances but this timestamp is not validated. As this can be set to a future timestamp. This may lead to potential manipulations by the exchange owner by combining off-chain and on-chain processes:

  • Creates inconsistencies/confusion by not maintaining a chronological order in the commitment.
  • Delaying the proof verification by promising a future commitment.
    To mitigate this add the following vallidation checks to timestamp
  • Add a check to make sure the timestamp is not in the future.
  • Store the last submitted timestamp and check the new timestamp is larger than the previous timestamp.
+ uint256 public lastSubmitted;

    function submitCommitment(
        bytes calldata snarkProof,
        bytes calldata grandSumProof,
        uint256[] memory totalBalances,
        uint256 timestamp
    ) public onlyOwner {
        // Check input length
        require(totalBalances.length > 0, "Invalid total balances length");
        require(
            grandSumProof.length == (totalBalances.length * 0x40),
            "Invalid grand sum proof length"
        );
+       require(timestamp < block.timestamp, "Cannot submit future commitment");
+       require(timestamp > lastSubmitted, "Incorrect timestamp");
+       lastSubmitted = timestamp;
        ....

Fixed cryptocurrencyNames and cryptocurrencyChains:
cryptocurrencyNames and cryptocurrencyChains are currently set within the summa config inside the constructor. However this could limit flexibility if the currencies or the chains changes over time. Allow for dynamic resizing of cryptocurrencyNames and cryptocurrencyChains.
While this may require commitment versioning so as to not impact the previous versions of commitments when the currency count changes.

No validation for cryptocurrencyChains of the reported balances in a commitment:
Commitment submissions do check if the cryptocurrencyChains that the reported totalBalances from the respective chains or even checked against the chains parameters within address ownership proofs within submitProofOfAddressOwnership .
The following sanity check :
require(bytes(cryptocurrencies[i].chain).length <= config.cryptocurrencyChains.length, "Cryptocurrency chain mismatch");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant