AZTEC Protocol Audit

• 1 Summary
  • 1.1 Audit Dashboard
  • 1.2 Audit Goals
  • 1.3 System Overview
  • 1.4 Key Observations/Recommendations
• 2 Issue Overview
• 3 Issue Details
  • 3.1 Bilateral swaps cannot be trustless without additional contract
  • 3.2 Owner of the protocol can drain all funds
  • 3.3 Tokens are not minted in confidentialTransferFrom
  • 3.4 ZkAssetOwnable should use OpenZeppelin's Ownable.sol
  • 3.5 System can be irrevocably blocked by the owner
  • 3.6 The ValidatorAddress in the proof data is unchecked during deposit
  • 3.7 Multiple Integer Overflows in joinSplit.sol
• 4 Threat Model
  • 4.1 Overview
  • 4.2 Detail
• 5 Tool-Based Analysis
  • 5.1 Mythril
  • 5.2 Harvey
  • 5.3 Ethlint
  • 5.4 Surya
• 6 Test Coverage Measurement
• Appendix 1 - File Hashes
• Appendix 2 - Severity
  • A.2.2 - Minor
  • A.2.3 - Medium
  • A.2.4 - Major
  • A.2.5 - Critical
• Appendix 3 - Disclosure

1 Summary

The AZTEC Protocol is an exciting new project that promises, for the first time, to bring privacy (confidential transactions) into the Ethereum ecosystem. This is done by managing sets of Zero Knowledge Notes which can represent value bound to standard ERC20 tokens. The existence and ownership of these notes are known, but their values are hidden using a novel Zero Knowledge proof construction.

For the purposes of this audit, we are primarily focused on analyzing the smart contract system deployed by the AZTEC protocol. Verifying the security promises made by the Zero Knowledge constructions in the AZTEC Protocol is beyond our particular expertise, and will be focused on by another group at a later date.

1.1 Audit Dashboard

Audit Details

• Project Name: CreditMint / Aztec Protocol
• Client Name: AZTEC
• Client Contact: Arnaud Schenk ([email protected]), Tom Pocock ([email protected])
• Lead Auditor: Dean Pierce
• Co-auditors: Sergii Kravchenko, Shayan Eskandari
• Date: April 8, 2019

Number of issues by severity

Update: This audit was done in April 2019 on commit 7a020f4. At the time of the audit the Aztec project was under active development and since then the code base has changed significantly. The remediation phase was delayed due to the ongoing changes and discussions regarding the relevance and importance of the reported issues.

1.2 Audit Goals

The focus of the audit was to verify that the smart contract system is secure, resilient and working according to its specifications. The audit activities can be grouped in the following three categories:

Security: Identifying security related issues within each contract and within the system of contracts.

Sound Architecture: Evaluation of the architecture of this system through the lens of established smart contract best practices and general software best practices.

Code Correctness and Quality: A full review of the contract source code. The primary areas of focus include:

• Correctness
• Readability
• Sections of code with high complexity
• Improving scalability
• Quantity and quality of test coverage

1.3 System Overview

There are two main components in the AZTEC protocol. The ACE (AZTEC Cryptography Engine), and the ZkAssets that use the ACE. The ACE is deployed and managed by a central trusted authority, while new ZkAsset contracts can be deployed and managed by anyone.

The AZTEC Cryptography Engine

The ACE contains the "Note Registry", which is the central repository for "zero knowledge notes", the cloaked assets used in the AZTEC system. This also means the ACE holds all of the ERC20 tokens represented by the set of notes. The ACE also manages a set of "validators" which hold all the on chain code for validating the various zero knowledge proofs for interacting with the ZkNotes.

The key validator is the JoinSplit validator, which lets note owners to combine the values of a set of notes they own, destroy them, and create a new set of notes with an arbitrary set of owners. While the existence of every ZkNote, as well as the owner of each note, is public information; the value of each note is cloaked. This lets note owners transfer value to others without ever having to reveal how much value they are transferring.

The ZkAsset

There are many options when deploying a ZkAsset. For the most part, a ZkAsset can be created by anyone and is bound to a particular ERC20 token, and on creation, opens up a new NoteRegistry in the ACE contract. These assets have functions that will follow similar naming and behavior with ERC20 contracts, such as confidentialTransfer(), confidentialTransferFrom(), and confidentialApprove().

Documentation

The following documentations were available to the audit team:

• A series of blog posts explaining AZTEC:
  • Confidential transactions have arrived, a dive into the AZTEC Protocol
  • AZTEC under the hood: range proofs
• The main README, and each package has its own README, such as packages/protocol.
• The White Paper explaining the concept of the AZTEC Protocol.

Scope

The scope of this audit is the main smart contracts of AZTEC Protocol:

• The AZTEC Crypto Engine (ACE)
• The JointSplit proof validation smart contract
• The Swap proof validation smart contract
• The Dividend/interest payment smart contract
• The base AZTEC EIP-1724 reference implementation

It should be noted that the zero-knowledge proof algorithms, trusted setup, and the mathematics behind these proofs are not in the scope of this audit.

1.4 Key Observations/Recommendations

The AZTEC Protocol code is incredibly well written. The tests are extensive, and there is a definite focus on security. Lots of the code is written in assembly, but for the most part, it is due to heavy math operations rather than minor gas improvements. The code also utilizes emerging standards, such as EIP712 for encoding and signing data, and the AZTEC Protocol itself is in the process of becoming a standard, with EIP1723 for the Cryptography Engine, and EIP1724 as a Confidential Token standard.

Minimize the impact of owner key loss

In the current setup, the ACE has an owner. That owner is very powerful, as they can add and remove validators, and even update the reference string, which is the base of the Zero Knowledge Construction in the AZTEC Protocol. This gives them several avenues for draining NoteRegistries. Any steps to reduce the impact of an attacker gaining owner capabilities over the ACE should be considered. This includes ideas like making the list of validators of an ACE static and/or setting up a proxy ownership contract with security controls like revokable delays and multisig approvals.

Enforce a revokable delay for ACE owner switching

When ownership is changed, there should be a period of time, 72 hours for example, where the existing owner can revoke the change in ownership. To keep the code clean, this can be done in the form of an ownership contract, which would act as a multisig delayed proxy that relays commands to the ACE when appropriate conditions are met. Such a contract could also be used to put delay controls around modifying validators or the reference string.

Document a detailed Emergency Response Plan

Assume an attacker has managed to obtain access to the owner key for an ACE. What happens next? Trying to wrestle the ACE back from the attacker is not a great option. Any controls that could be put in to evict the attacker could also be used by the attacker to retain control. The best option is to permanently lock the ACE in such a way that users are able to extract the value from the ZkNotes they currently hold, but no new notes could be created. Public documentation on exactly how this process would work helps to build confidence in the platform.

ACE compartmentalization

One approach to mitigating overpowered ACE owners is to compartmentalize out experimental ACEs. A more conservative ACE could be permanently locked such that no new validators could be added. ZkAssets built off of this ACE could be more attractive for users who only really need joinSplit functionality. A more experimental ACE with more cutting edge features could also be deployed. If a vulnerability is discovered in any of the loaded validators in the experimental ACE, or if an attacker obtains ownership access, then assets on the more conservative ACE would remain protected.

ZkAsset Ownership

At the moment, the concept of Ownership on ZkAssets is relatively weak. Ownership is not available on a standard ZkAsset, and ZkAssetOwnable has an owner field but does not conform to the OpenZeppelin Ownable standard, specifically the ownership is not updatable once set.

If ZkAssets did conform to the standard, the owners would be able to set strict filters on which validators are supported per ZkAsset, and crucially, lock the ownership afterward by setting it to an address like 0x00..00DEAD. This conservative setup will protect users of cloaked assets, even in the event of owner compromise.

ZkAsset Factory

One approach to ensure that participating ZkAssets conform to expected standards is for the ACE to point to a ZkAssetFactory, and that Factory could instantiate new ZkAssets. The ACE could choose to only create NoteRegistries for ZkAssets created through the factory.

Open Questions for ZK Construction Auditors

• Can subtle vulnerabilities in validators be created?
• Can subtle vulnerabilities in validators automatically be detected?
• Can a malicious reference string be generated such that the hard coded joinSplit validator can be used to forge ZkNotes in existing NoteRegistries?
• What is the computational difficulty in exposing the value of any ZkNote given the proof data from the transaction that created it?

2 Issue Overview

The following table contains all the issues discovered during the audit. The issues are ordered based on their severity. A more detailed description of the levels of severity can be found in the Severity Definitions Appendix. The table also contains the status of any discovered issue.

3 Issue Details

3.1 Bilateral swaps cannot be trustless without additional contract

Description

There should be a way to make atomic trustless bilateral swap transactions using a current set of contracts. Unfortunately, there is no way how to do this. One of the counterparties can always front-run the other one and spend both notes (his own note and counterparty's note) in one transaction. So there is no way to make this swap without trusting the counterparty. But if trader trusts the counterparty, no bilateral swap is needed, join-split proof can be used to make the trade.

Remediation

Create additional settlement contract that allows making bilateral swaps in an atomic and trustless way. This contract is mentioned in specifications, but it's not present in the codebase.

Note from AZTEC:

(we disagree that creating a settlement contract is a required part of the protocol for us to provide, and that this is an issue. The swap proof is useful on its own, and needs to be accessible even without going through a settlement contract, and any loss of funds would be the result of a trust abuse by a scammer which a settlement contract would not solve)

3.2 Owner of the protocol can drain all funds

Description

The owner of ACE contract should be able to add or remove proof validators. He can create a malicious validator that will verify the proof and transfer free tokens to the owner's account.

1. The owner (=attacker) creates a validator that validates any input and returns _proofOutput that would convert to (0, 0, attacker_address, amount_to_steal) and adds it to the validators list in ACE contract.

(bytes memory inputNotes,
        bytes memory outputNotes,
        address publicOwner,
        int256 publicValue) = _proofOutput.extractProofOutput();

2. The attacker should validate the proof by calling validateProof function of ACE contract.
3. The attacker calls confidentialTransferFrom in related ZkAsset contract and funds are being transferred on his account.

Remediation

One of the possible solutions could look like that:

1. Add timelock to setProof function, e.g. validator will be added only 2 weeks after the owner added it. So people would be able to withdraw their funds in case of the attack.
2. Disable the ability to invalidate standard join-split validator. That will ensure that people are able to withdraw their funds and actually solves another issue. The downside of that option is that the owner cannot protect the protocol if join-split is compromised.

3.3 Tokens are not minted in confidentialTransferFrom

Description

ZkAssetMintable contract is responsible for supplying tokens to the ACE contract. If some tokens were confidentially minted, they are publicly minted only when there is a need for it (i.e., if there are not enough funds to withdraw). It's done automatically in confidentialTransfer function. It should be also done automatically in confidentialTransferFrom.

Examples

No minting is happening in confidentialTransferFrom so this function may revert if there are not enough tokens in totalSupply to withdraw.

Remediation

Add minting to confidentialTransferFrom function if there are not enough funds for withdrawal.

3.4 ZkAssetOwnable should use OpenZeppelin's Ownable.sol

Description

ZkAssetOwnable uses its own simplified ownership model, where an owner is assigned on creation, and can never be changed.

It is suggested to use the more popular Ownable.sol from OpenZeppelin (as is used by ACE), which would be more in line with a users expectations of contract ownership.

Examples

Owner is set manually rather than through the standard Ownable contract: code/packages/protocol/contracts/ZkAsset/ZkAssetOwnable.sol:L35

owner = msg.sender;

Remediation

contract ZkAssetOwnable is ZkAsset, Ownable { The main driver for this is granting ZkAsset owners the ability to lock their validator filters, perhaps their minting and burning as well, in such a way that users of the asset can be assured that no future changes can be made.

3.5 System can be irrevocably blocked by the owner

Description

The owner of the protocol can invalidate basic validator (JOIN_SPLIT_PROOF) that is hardcoded into ZkAsset contracts and used in confidentialTransfer function. There is no way how to make it valid again.

Remediation

There are multiple possible options, how to deal with this issue:

1. Consider revoking invalidation of a validator.
2. Add the ability to change JOIN_SPLIT_PROOF value.
3. Make JOIN_SPLIT_PROOF impossible to invalidate. That will also ensure the right of every user to withdraw their funds at any time.

3.6 The ValidatorAddress in the proof data is unchecked during deposit

Description

It appears that the ValidatorAddress field in proofdata isn't checked in some cases.

Examples

In any of the deposit proof constructions, such as code/packages/protocol/test/ZkAsset/ZkAsset.js:L241-L249

const depositProof = proof.joinSplit.encodeJoinSplitTransaction({
    inputNotes: [],
    outputNotes: [notes[0], notes[3]],
    senderAddress: accounts[0],
    inputNoteOwners: [],
    publicOwner: accounts[3],
    kPublic: -30,
    validatorAddress: aztecJoinSplit.address,
});

replace validatorAddress with any random hex string

validatorAddress: 0x9999999999999999999999999999999999999999,

Tests all complete successfully. Notably if you try to do this on other types of transfers, the test will revert.

Remediation

Verify that the ValidatorAddress is legitimate.

3.7 Multiple Integer Overflows in joinSplit.sol

Description

While fuzzing joinSplit.sol using Harvey, multiple integer overflows were detected. The impact of these overflows is likely minimal, since validators get called with staticcall, and don't have the ability to affect state.

Examples

One such example of an overflow is with the input of:

0x000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000012893657d8eb2efad4de0a91bcd0e39ad9837745dec3ea923737ea803fc8e3d0000000000000000000000000000000000000000000000000000000000000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff00000000000000000000000000000000000000000000000000000000000000000000000000000000000000

The string 0xfff... is at 0x184, which will instantly cause the notes variable to overflow. code/packages/protocol/contracts/ACE/validators/joinSplit/JoinSplit.sol:L67-L70

function validateJoinSplit() {
    mstore(0x80, calldataload(0x44))
    mstore(0xa0, calldataload(0x64))
    let notes := add(0x104, calldataload(0x184))

Remediation

Typically the solution to these issues is proper bounds checking. The overhead that this would require might not be worth the seemingly low risk. These overflows should be inspected further to ensure there aren't any unintended consequences. In some cases this will require a strong understanding of the cryptographic significance of the affected variables.

4 Threat Model

The creation of a threat model is beneficial when building smart contract systems as it helps to understand the potential security threats, assess risk, and identify appropriate mitigation strategies. This is especially useful during the design and development of a contract system as it allows to create a more resilient design which is more difficult to change post-development.

A threat model was created during the audit process in order to analyze the attack surface of the contract system and to focus review and testing efforts on key areas that a malicious actor would likely also attack. It consists of two parts a high level design diagram that helps to understand the attack surface and a list of threats that exist for the contract system.

4.1 Overview

The approach we take here is to take an adversarial threat model, which looks at the various attack surfaces of the system from the perspective of various hypothetical attackers.

4.2 Detail

Attacker #1 The Looter

The AZTEC Protocol, by design, holds a large amount of value for long periods of time, and even worse, it holds them all in the same contract (The AZTEC Cryptography Engine, ACE.sol). This typically represents a lot of risks, so let us take a look at the exposed attack surface.

Thankfully, the only exposed function that allows value extraction is NoteRegistry.updateNoteRegistry(), so the attack surface is incredibly narrow.

Most importantly, an attacker would have to bypass this hurdle: require(validateProofByHash(...)). This means they need a valid proof that the note transition is legitimate. If the cryptography holds, the only way someone could come up with a legitimate proof that would modify notes is if they had the private key material associated with the source notes. The proof also strongly binds the publicValue field, which is what would need to be manipulated to withdraw tokens from the NoteRegistries.

Attacker #2 The Malicious ACE Owner

This is probably the biggest threat to the system. Crucially, the people with ownership access to the ACE can add new validators. One trivial example of a malicious validator is one that would approve any proof with no input notes and no output notes, but with a positive publicValue. This would directly send tokens from the ACE to the attacker without touching any of the notes.

As the ACE grows in value, this attack is going to be more tempting. Legitimate owners may be tricked, bribed, or even coerced to hand over access. By limiting what an ACE owner is capable of, this threat can be greatly reduced. Many options are discussed in the recommendations section of this report.

Attacker #3 The Malicious ZkAsset Creator

Interestingly, when setting up a ZkAsset, you can link it to any ERC20 token you like, even tokens that already have a ZkAsset/NoteRegistry. Tokens are, however, compartmentalized per NoteRegistry, so even if a malicious asset for a target token was created, the asset would not be able to withdraw tokens that were deposited into other NoteRegistries.

The balance of the registries is controlled by a strong binding between registry's totalSupply and the balance of tokens controlled by the registry. Any time totalSupply is touched, it is accompanied by the corresponding linkedAsset.transfer function and the entire transaction reverts if either operations fail.

Even if a ZkAsset is completely malicious, and attempts to steal user funds after some period of regular use, nothing would let the malicious ZkAsset deployer touch the notes in the NoteRegistry without the private key material associated with those notes. Since the registries are controlled by the ACE, they are fundamentally safe from tampering so long as a legitimate ACE is being used. The worst the malicious asset creator would be able to do is lock up user funds in the associated registry by self destructing, or otherwise disabling user interaction. Perhaps they could extort the holder of locked funds into paying a fee to withdraw their assets.

For this reason, it is important that users have some level of trust in the ZkAssets they are interacting with. One possible option (explained more in the recommendation section) is to have a ZkAssetFactory which can only instantiate legitimate ZkAssets.

Attacker #4 The ZKAsetOwnable Owner

The owner of a ZkAsset really only has the ability to blacklist certain validators. As such, they may be able to block more complex operations on ZkNotes, but nothing that could cause ZkNote owners to lose value.

Attacker #5 The Griefer

Initially, clearProofByHashes() may seem like a good way to deny transactions for people, msg.sender gets hashed into validatedProofHash, so users can only clear proof hashes that they themselves set. There does not appear to be any exposed functionality that would allow a griefer to appreciably degrade the user experience of the ACTEC protocol.

Attacker #6 The Manipulator

Nearly every operation that can be done is strongly bound to the single user that is running the function. The opportunities for front-running and/or input tampering appear negligible.

Attacker #7 The Cryptographer

Last year there was a ZCash bug, which was known internally for nearly half a year before it could be patched. Zero Knowledge constructions are very new, and few can adequately find bugs in them. TODO more?

Attacker #8 The Observer and Anonymity-set

The owners of notes are public, so in general short term, transactions are fairly easy to correlate. If someone moves in 100 tokens, assigns them to a new note they control via a joinSplit, and withdraw to a new address, it's pretty obvious that those inputs and outputs are correlated. In general, leaving the value in ZkNotes for longer periods of time is going to increase privacy. Users should be mindful that it will be very obvious when funds they control move into these concealed assets, so the public pool of funds that they moved them from initially may come under expanded scrutiny.

Users should also be mindful that if they are using tools like Metamask, their real world IP address will be visible to the backend node provider (usually Infura) and could give that node operator information on the users real world location.

5 Tool-Based Analysis

Several tools were used to perform an automated analysis of the reviewed contracts. These issues were reviewed by the audit team, and relevant issues are listed in the Issue Details section.

5.1 Mythril

Mythril is a security analysis tool for Ethereum smart contracts. It uses concolic analysis to detect various types of issues. The tool was used for automated vulnerability discovery for all audited contracts and libraries. More details on Mythril's current vulnerability coverage can be found here.

The raw output of the Mythril vulnerability scan can be found here.

5.2 Harvey

Harvey is a greybox-fuzzer that uses light-weight program instrumentation to record the execution path for each tested input and compute its path identifier. The approach was developed by Valentin Wüstholz and Maria Christakis and published in their 2018 paper. The raw output files of the Harvey fuzzer can be found here.

5.3 Ethlint

Ethlint is an open source project for linting Solidity code. Only security-related issues were reviewed by the audit team.

The raw output of the Ethlint vulnerability scan can be found here.

5.4 Surya

Surya is an utility tool for smart contract systems. It provides a number of visual outputs and information about structure of smart contracts. It also supports querying the function call graph in multiple ways to aid in the manual inspection and control flow analysis of contracts.

A complete list of functions with their visibility and modifiers can be found here.

6 Test Coverage Measurement

The tests are implemented using either yarn, lerna or npm for different components in the main Aztec repository which makes it more complicated to test. However, we acknowledge that this issue is mainly due to the heavy development of these components.

In the Protocol component, there are 229 tests implemented and they all pass.

The code coverage of the tests covers 94.42% of the Statements, 95.08% of the Functions and 96.53% of the Lines.

The state of test coverage at the time of our review can be viewed by opening the index.html file from the coverage report directory in a browser.

Appendix 1 - File Hashes

The SHA1 hashes of the source code files in the scope of the audit are listed in the table below.

Appendix 2 - Severity

A.2.2 - Minor

Minor issues are generally subjective in nature, or potentially deal with topics like "best practices" or "readability". Minor issues in general will not indicate an actual problem or bug in code.

The maintainers should use their own judgment as to whether addressing these issues improves the codebase.

A.2.3 - Medium

Medium issues are generally objective in nature but do not represent actual bugs or security problems.

These issues should be addressed unless there is a clear reason not to.

A.2.4 - Major

Major issues will be things like bugs or security vulnerabilities. These issues may not be directly exploitable, or may require a certain condition to arise in order to be exploited.

Left unaddressed these issues are highly likely to cause problems with the operation of the contract or lead to a situation which allows the system to be exploited in some way.

A.2.5 - Critical

Critical issues are directly exploitable bugs or security vulnerabilities.

Left unaddressed these issues are highly likely or guaranteed to cause major problems or potentially a full failure in the operations of the contract.

Appendix 3 - Disclosure

ConsenSys Diligence (“CD”) typically receives compensation from one or more clients (the “Clients”) for performing the analysis contained in these reports (the “Reports”). The Reports may be distributed through other means, including via ConsenSys publications and other distributions.

The Reports are not an endorsement or indictment of any particular project or team, and the Reports do not guarantee the security of any particular project. This Report does not consider, and should not be interpreted as considering or having any bearing on, the potential economics of a token, token sale or any other product, service or other asset. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty. No Report provides any warranty or representation to any Third-Party in any respect, including regarding the bugfree nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third party should rely on the Reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this Report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. CD owes no duty to any Third-Party by virtue of publishing these Reports.

PURPOSE OF REPORTS The Reports and the analysis described therein are created solely for Clients and published with their consent. The scope of our review is limited to a review of Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty.

CD makes the Reports available to parties other than the Clients (i.e., “third parties”) -- on its GitHub account (https://github.com/ConsenSys). CD hopes that by making these analyses publicly available, it can help the blockchain ecosystem develop technical best practices in this rapidly evolving area of innovation.

LINKS TO OTHER WEB SITES FROM THIS WEB SITE You may, through hypertext or other computer links, gain access to web sites operated by persons other than ConsenSys and CD. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such web sites' owners. You agree that ConsenSys and CD are not responsible for the content or operation of such Web sites, and that ConsenSys and CD shall have no liability to you or any other person or entity for the use of third party Web sites. Except as described below, a hyperlink from this web Site to another web site does not imply or mean that ConsenSys and CD endorses the content on that Web site or the operator or operations of that site. You are solely responsible for determining the extent to which you may use any content at any other web sites to which you link from the Reports. ConsenSys and CD assumes no responsibility for the use of third party software on the Web Site and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.

TIMELINESS OF CONTENT The content contained in the Reports is current as of the date appearing on the Report and is subject to change without notice. Unless indicated otherwise, by ConsenSys and CD.