Security/crypto review fixes - #80
Open
GeneralZero wants to merge 15 commits into
Open
Conversation
PoseidonDecrypt(l) pads the message up to a multiple of three, so the number of padding slots is decryptedLength - l: two when l % 3 == 1, one when l % 3 == 2. The two branch bodies were the other way round, which the comment directly above them already describes correctly as "(3 - (l mod 3))". Every PCT in the system is PoseidonDecrypt(1), so the live effect was the l % 3 == 1 branch checking one slot instead of two, leaving decrypted[1] unconstrained. The auth tag does not cover that gap: a prover chooses ciphertext[0..2] freely and then computes the tag over them, so an arbitrary field element could be smuggled through every transfer, mint, burn and withdraw PCT. Verified with a forged-but-valid-tag ciphertext, which the old code accepts and the new code rejects. The l % 3 == 2 branch was over-constrained for the same reason and would have rejected valid two-element ciphertexts. Latent, since nothing instantiates l=2.
Both randomness inputs carried only a range check, and zero passes it. CheckPublicKey already asserts privKey != 0; these bring the randomness up to the same standard. In ElGamalEncrypt, random = 0 makes c1 the identity and leaves the message unblinded, so the ciphertext is (identity, value*G) and anyone can recover the amount by a short discrete-log search. In CheckPCT, random = 0 makes both the auth key and the Poseidon encryption key the identity point, which is public, so anyone can decrypt the PCT. The sender is the only party who picks these values, so this is disclosure of their own transfer rather than theft - but it is silent, undetectable by the counterparty or the auditor, and a clean covert channel. There is no legitimate reason for either value to be zero.
…lic key register() combined its two guards with &&, so it reverted only when the registration hash was already used AND the account already held a key. A user re-registering with a different private key produces a different registration hash, so the first clause was false and the guard let the overwrite through. Nothing migrates the balance. The stored ciphertext stays encrypted under the old key while _validatePublicKey compares proofs against the new one, so every spend proof the account can build is rejected and the funds are unreachable, with no warning and no recovery path. The same overwrite silently staleness the eERC's auditorPublicKey if the auditor re-registers, leaving PCTs encrypted to a key the auditor may no longer hold. The circuit side matters too: register() reads the account as address(uint160(input[2])) while CheckRegistrationHash hashed the full field element, so senderAddress + k * 2^160 minted fresh hashes for the same account. Without the 160-bit bound the guard stays bypassable however the clauses combine. The existing "already registered user can not register again" test replays the same proof, for which both clauses are true either way, so it could not distinguish a correct guard from a broken one. The added test registers a second time with a freshly generated key, which is the case that actually fails against the old condition.
BabyJubJub.encrypt hardcodes random = 1, which is a reasonable choice given a contract has no source of randomness, but the consequences were undocumented. Deposit ciphertexts are deterministic: equal amounts to the same key produce identical ciphertexts, and the accumulated randomness of a deposit-only account is just its publicly observable operation count, so anyone can compute b*G = c2 - n*pk and confirm the balance. That leaks nothing beyond the deposit and withdrawal amounts, which are already public in the ERC20 transfers - but it does mean a converter balance carries no confidentiality at all until the account receives a private transfer with real randomness, and that is worth stating where someone reading the deposit path will see it.
decimals() was read live from the token on both legs of every conversion, and nothing in ERC20 requires it to stay constant. A token free to change what it reports could be scaled one way going in and another coming out: deposit while it reports 10 decimals and no scaling applies, flip it to 18, then withdraw the same encrypted units and _convertTo multiplies the payout by 10^8. The difference is paid out of the contract's holdings of that token, which is every other depositor of it. It does not even need malice - a legitimately upgradeable token that changes its reported decimals corrupts accounting for every holder. Reading it once in _addToken and storing it per token id makes the in-scaling and out-scaling agree by construction, whatever the token does afterwards. Two smaller things fall out: the two call sites no longer disagree on type (uint8 on deposit, uint256 on withdrawal), and a token with no decimals() now reverts when it is first registered rather than on every deposit attempt. _convertFrom had to be reordered to register before reading, since registration is what populates the cache. Note for anyone applying this to a live instance: tokens registered before this change would read 0 from the new mapping and need a backfill.
When the eERC has more decimals than the token, _convertTo divides, and an amount below the scaling factor floors to zero. _privateBurn has already debited the caller's encrypted balance in full by that point, so the call succeeded, emitted a Withdraw event for the full amount, and moved no tokens. The deposit leg handles the mirror case properly by returning the remainder as dust; this leg dropped it. The loss per call is bounded by scalingFactor - 1 encrypted units, always less than one indivisible unit of the underlying token, and it is never directable at anyone else - so the magnitude is small. What earns the fix is that the call reports success: a 10-decimal eERC over a 0-decimal token gives a 10^10 threshold, under which every withdrawal silently pays nothing. The test suite encoded the broken behaviour as correct. The 6-decimal block withdrew 1000 units through a 10^4 scaling factor - exactly zero tokens - and passed, because every deposit test checks the real ERC20 delta while none of the three withdraw blocks referenced the token at all. They now assert the balance moved and by how much, the amount is raised above the scaling factor, and a case below it asserts the revert. The assertions fail against the pre-fix contract.
…oken amountPCTs is an unbounded storage array. Every credit pushes an entry and every spend walks the whole array in _deleteUserHistory, popping matches. The asymmetry is the problem: inbound transfers grow the array, and the account pays to walk it. A third party could send transfers until the victim's next spend exceeded the block gas limit, at which point that balance is unspendable permanently, with no way to prune. One transfer per entry is expensive for the attacker but bounded; the victim's loss is total and irreversible. Capping the pending entries changes the failure mode from the victim's funds being frozen to the attacker's transfer reverting, and the account clears its history with a single spend. That is still a griefing vector - an attacker can block an account from receiving once it holds 300 unspent credits - but it is recoverable rather than permanent. 300 keeps the prune loop well inside the gas limit while leaving generous headroom for ordinary use.
_convertFrom reimplemented _addToUserBalance inline. The helper had existed for a week when the deposit path was written, and the copy then drifted from it, which is the usual fate of duplicated state-machine code. The consequence that matters is that the copy bypassed the MAX_PENDING_AMOUNT_PCTS ceiling, because that guard lives in _addToUserHistory and only the helper calls it. Nobody can weaponise that - _executeDeposit credits msg.sender only, so deposits cannot grow someone else's history - but an account could grow its own past the point where its prune loop fits in a block and then be unable to spend. Measured: with the inline copy the 301st deposit succeeds and the array reaches 301; via the helper it reverts with PendingHistoryLimitReached. The other drift goes away with it: deposits incremented transactionIndex twice, once directly and once inside _commitUserBalance, where every other credit increments it once. Deposits now advance it by one like the rest. Balance ciphertexts, decrypted balances, nonces and pruning behaviour are unchanged - verified by dumping every observable field across three deposits and a withdrawal under both versions - but the counter's absolute value is lower, so any off-chain consumer reading transactionIndex or an amount PCT index will see different numbers.
contributionSettings.contributions was 0, so hardhat-zkit emitted verification keys straight from the initial zkey, where the phase-2 trapdoor delta is still 1. All five generated verifiers had DELTA byte-identical to GAMMA, both equal to the BN254 G2 generator. Groth16 soundness rests entirely on delta being secret. With delta = 1 the verification equation collapses and a proof can be forged for any public input from public data alone: take A = alpha, B = beta + t*g2, C = t*alpha - L, and e(-A,B)*e(alpha,beta)*e(L,gamma)*e(C,delta) = 1 holds by construction. Confirmed against the committed constants for t = 1, 2 and 7, with no witness and no proving key. Against a deployment built from these scripts that means registering any key to any address, forging withdraw proofs to drain the converter's reserves, and forging transfers and mints. The keys here are regenerated with one contribution, which makes them unforgeable. It does NOT make them trustworthy: whoever ran the setup knows the toxic waste. Production keys must come from a multi-party ceremony ending in a public beacon, with the transcript published so the key can be reproduced from these circuits. The IC constants are unchanged by the contribution and reproduce exactly from the circuit source, so only DELTA moves here beyond the circuit fixes earlier in this series.
Two failure modes that have both occurred in this repository, neither of which any existing check would catch. delta == gamma in a verification key means the phase-2 trapdoor is 1 and the key accepts forged proofs for any statement. That is what a setup with zero contributions produces, so this makes the unsound configuration fail the build rather than ship quietly. Drift between the production keys and the circuits is the second. With gamma fixed to the G2 generator, the IC public-input commitments depend only on the phase-1 ptau and the compiled constraint system, not on the contributions - so the IC array is a fingerprint of the circuit. Comparing the production and generated verifiers per circuit detects a key that no longer describes the source it is committed alongside. This currently fails on all five production keys, which is correct: they predate the circuit changes in this series and cannot be used until the ceremony is re-run. Written as plain JS rather than TypeScript so it needs no ts-node, which the repo does not declare.
deployVerifiers took isProd as an optional flag defaulting to the generated verifiers, and both deploy scripts called it without passing anything. So the branch that deploys contracts/prod was unreachable and the scripts shipped the development keys - the ones with delta = 1 - purely by omission. A footgun that silent is worth removing even now that the generated keys are sound, because they are still not ceremony output. The parameter is now required, so every caller has to state which key set it means, and the deploy scripts pass true. Note that npm run check:verifiers currently reports the production keys as stale against these circuits, so the setup has to be re-run before either script is pointed at a live network.
Belongs with the padding-count fix earlier in this series; kept separate only to avoid rewriting signed history. Squash it into that commit if you prefer.
…ion keys" This reverts commit 5728cac.
* test(history): add pending PCT withdrawal gas coverage * chore: fix lint issues
BeratOz01
approved these changes
Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
List of security updates