diff --git a/website/static/audit_reports/20240223_PSE_Audit_audit_report.pdf b/website/static/audit_reports/20240223_PSE_Audit_audit_report.pdf new file mode 100644 index 0000000000..8b17a5ba7c Binary files /dev/null and b/website/static/audit_reports/20240223_PSE_Audit_audit_report.pdf differ diff --git a/website/versioned_docs/version-v1.x/audit.md b/website/versioned_docs/version-v1.x/audit.md index 0051fc9688..bdfa114350 100644 --- a/website/versioned_docs/version-v1.x/audit.md +++ b/website/versioned_docs/version-v1.x/audit.md @@ -1,112 +1,23 @@ --- title: MACI Security Audits -description: In the summer of 2022, MACI v1 was audited by HashCloak. The audit covered both the zk-SNARK circuits and the Solidity smart contracts. -sidebar_label: Security Assessments +description: Overview of MACI audit history with references to audit reports. +sidebar_label: Security audits sidebar_position: 12 --- -# Security Audits +# MACI Security Audits -## Links +## Full reports +- Audit by PSE Audit 2024/02 [report](/audit_reports/20240223_PSE_Audit_audit_report.pdf) - Audit by HashCloak 2022/09 [report](/audit_reports/202220930_Hashcloak_audit_report.pdf) - Audit by HashCloak 2021/09 [report](/audit_reports/20210922_Hashcloak_audit_report.pdf) -## HashCloak audit 2022 - -In the summer of 2022, MACI v1 was audited by HashCloak. The audit covered both the zk-SNARK circuits and the Solidity smart contracts. - -This audit revealed a number of high severity issues which have been remediated by the MACI development team. We will be looking at those in details in the following sections. - -## Data is not fully verified during a state update - -This issue could have allowed a malicious coordinator to change the MACI state arbitrarily, for instance by tampering with the voice credits and the voting public key of any user. - -In more details, the `processMessages.circom` circuit, did not fully verify that after a state update, the new state was the result of executing an arbitrary number of user messages on the previous state. `topupStateLeaves` and `topupStateLeavesPathElements` were never verified against the current state, and `topupStateIndexes` and `topupAmounts` were not verified against the message root. - -This was rectified with commit [6df6a4054da926b07f35c5befab4f1f8af33dcc6](https://github.com/privacy-scaling-explorations/maci/pull/522/commits/6df6a4054da926b07f35c5befab4f1f8af33dcc6) - -## Token for top-up is a free resource - -The provided `TopupCredit.sol` contract implemented unprotected `airdrop` and `airdropTo` functions, which could have allowed anyone to receive unlimited voice credits. While this contract was provided as a template, the issue has been rectified by adding the `onlyOwner` modifier to these two functions. - -```javascript -function airdropTo(address account, uint256 amount) public onlyOwner { - require(amount < MAXIMUM_AIRDROP_AMOUNT); - _mint(account, amount); -} - -function airdrop(uint256 amount) public onlyOwner { - require(amount < MAXIMUM_AIRDROP_AMOUNT, "amount exceed maximum limit"); - _mint(msg.sender, amount); -} -``` - -## Integer overflow problem and improper bit length restriction - -This issue within the `float.circom` circuit could have resulted in an overflow on the `IntegerDivision` template. This stemmed from the lack of validation of input size, as well as not preventing a division by zero. Furthemore, it was pointed out that using assert in circuits did not contribute to constraints verification, and could have been bypassed by a malicious coordinator. - -The issue was rectified with commit [efd4617724e956d2566062c6fe882e1d45cba7c4](https://github.com/privacy-scaling-explorations/maci/pull/523/commits/efd4617724e956d2566062c6fe882e1d45cba7c4) - -## MessageQueue in PollFactory is uninitialized - -MACI uses a message queue (a quinary merkle tree) to store all the messages to be processed for a single poll. When deploying a new poll, a corresponding message queue contract is deployed as well, however this was never initialized with a zero value. - -Should the queue never be initialized with the zero value, a malicious user could submit a message to initialize the queue with a value they know how to decrypt, which however would take a very long time to generate a proof for. This could result in a denial of service attack against the coordinator. - -The code was fixed by enqueuing a message containing the zero value `NOTHING_UP_MY_SLEEVE` which is the result of: - -`keccak256("Maci") % p` +## PSE audit 2024 -Translated into code, an `init` function was included in the Poll contract, with the following enqueuing of the placeholder leaf: +In February 2024 the PSE Audit team audited the MACI codebase with a focus on the smart contracts, TypeScript core, and Circom circuits Three critical bugs were found: two within the Circom circuits and one in the smart contracts. All three of these have been fixed. -```javascript -// init messageAq here by inserting placeholderLeaf -uint256[2] memory dat; -dat[0] = NOTHING_UP_MY_SLEEVE; -dat[1] = 0; -(Message memory _message, PubKey memory _padKey, uint256 placeholderLeaf) = padAndHashMessage(dat, 1); -extContracts.messageAq.enqueue(placeholderLeaf); -``` - -## Additional issues and improvements - -The rest of the issues were either low risk, informational or general optimizations. - -As an example, there were certain functions which did not enforce the checks-effets-interaction pattern, which could potentially have led to reentrancy attacks. While most of these have been fully remediated, the `deployPoll` function within MACI is not currently enforcing the pattern when deploying a new poll contract using the `PollFactory` factory contract. - -```javascript -function deployPoll( - uint256 _duration, - MaxValues memory _maxValues, - TreeDepths memory _treeDepths, - PubKey memory _coordinatorPubKey -) public afterInit { - uint256 pollId = nextPollId; - - [..snip] - - Poll p = pollFactory.deploy( - _duration, - _maxValues, - _treeDepths, - batchSizes, - _coordinatorPubKey, - vkRegistry, - this, - topupCredit, - owner() - ); - - polls[pollId] = p; - - emit DeployPoll(pollId, address(p), _coordinatorPubKey); -} -``` - -As seen above, an external call is made, before updating the state with the new poll. The issue is tracked [here](https://github.com/privacy-scaling-explorations/maci/pull/522#discussion_r981863147) and only left open as the code does not enforce best practices, however it does not pose any immediate risk. - -The rest of the issues were successfully fixed and reflected in the v1.1.1. For the full report, please refer to the `audit` folder inside the root of the repository. +Please see the [PSE Audit report](/audit_reports/20240223_PSE_Audit_audit_report.pdf) for details. ## Veridise disclosure 2023 @@ -116,8 +27,6 @@ Out of five issues disclosed, only three were relevant and have been since fixed We would like to thank you the Veridise team for their effort in keeping open source projects safe. -> Please note that at this time the fixed code is only present in the dev branch. This will be merged to the main branch in the next minor update. - ### Issue 1 **Description** @@ -225,3 +134,27 @@ for (var i = 0; i < levels; i++) { sum.nums[i] <== out[i] * (BASE ** i); } ``` + +## HashCloak audit 2022 + +In the summer of 2022, MACI v1 was audited by HashCloak. The audit covered both the zk-SNARK circuits and the Solidity smart contracts. + +This audit revealed a number of high severity issues which have been remediated by the MACI development team. All issues were successfully fixed and reflected in MACI v1.1.1. + +Please see the [HashCloak report](/audit_reports/202220930_Hashcloak_audit_report.pdf) for details. + +## HashCloak audit 2021 + +From July 5th, 2021 to August 2nd, 2021, the Ethereum Foundation’s Applied ZKPs team engaged HashCloak for an audit of the MACI protocol. The audit was conducted with 3 auditors over 15 person weeks. + +The following packages were in scope: + +- Circuits +- Contracts +- Core +- Crypto +- Domainobjs + +From August 18, 2021 to September 22, 2021, Hashcloak assisted the MACI team in resolving the issues. + +Please see the [HashCloak report](/audit_reports/20210922_Hashcloak_audit_report.pdf) for details.