# Penumbra Security Review

Penumbra contracted me to review the groth16 circuits used inside of the Penumbra blockchain. This review is scoped to focus entirely on the integrity of the ZK proofs and may not cover issues related solely to the chain. The proof actions included are: spend, output, delegator vote, undelegate, swap, and swap claim. The audit is from commit with hash: de66328. The period of the review is two weeks.

### Findings

Summary: Critical: 1, High: 0, Medium: 0, Low: 0

Findings are presented chronologically:

* SwapClaim can be executed at multiple different batches allowing an attacker to create assets:
The SwapClaim proof takes both a private input which is a merkle proof to a commitment of a Swap hash and a public input of the BatchSwapOutputData. The chain checks that the merkle root of the swap claimed and BatchSwapOutData are both correct. The merkle proof contains a Position object which is a binary encoding of the merkle path to the global state root, the last bits of this path are called the block. Many blocks are gathered into epochs and then the block numbers repeat. The proof validates that the BatchSwapOutData.epochHeight + position.block = BatchSwapOutData.height, ensuring that within a single epoch the swap is claimed at the same relative position. However when a new epoch occurs this statement is also valid for a new BatchSwapOutData, so long as the absolute block is equal mod the epoch length. This enables a swap which occurred in epoc 1 to be executed in any following epoch at the rate of the batch auction in the same modulus by the epoch length.
This not only enables assets swaps to execute at future or past prices, but also allows new asset creation. The swap's private outputs are set as the percent of the global total in that epoch traded. So if an attacker creates an asset A and completely controls the supply they can first trade 1 billion asset A for 1 USDC, then manipulate the LP so in the next epoch they trade 1 asset A for 1 USDC. Using the bug to execute the first swap in the second Epoc the attacker will mint 1 Billion USDC in a private note.
* Severity Estimate: Critical [Likelihood: High, Impact: High]
* Remediation Recommendation: Enforce that the epoch is equal between the BatchSwapOutData and the SwapCommitment.

### Notes

Very low severity, performance notes, or other non-issue comments

* The lack of execution limits on the prices of private swaps is highly likely under some market conditions to result in user's swaps executing at prices substantially worse than they thought it would. This may enable some toxic LP strategies especially in coins with very few traders or created by the LP for this purpose. This is a design decision which should be clearly communicated to the users of the private swaps.
* `.conditional_enforce_not_equal(&note_var.diversified_generator(), &Boolean::TRUE)?;` on [this line](https://github.com/penumbra-zone/penumbra/blob/6c8d3e7ffb8ff01a12035454ccd97d1981553939/crates/core/component/shielded-pool/src/output/proof.rs#L129) should use just `.enforce_not_equal` as is used in all other locations.
* I recommend checks that the delta_0_i and delta_1_i are not greater than delta_0 and delta_1 to make the creation of assets harder if a batch swap price is assigned to the wrong input.