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

docs: add Feb 2024 PSE audit report #1241

Merged
merged 4 commits into from
Feb 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Binary file not shown.
131 changes: 32 additions & 99 deletions website/versioned_docs/version-v1.x/audit.md
Original file line number Diff line number Diff line change
@@ -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

Expand All @@ -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**
Expand Down Expand Up @@ -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.
Loading