fix nullifier griefing bug #3859

Instead of short circuiting all checks in the spend circuit in the case of a dummy spend (a note with value 0), this PR changes the spend circuit so that it only skips the SCT inclusion proof check. This prevents a nullifier griefing bug reported by Zellic in #3859 where an attacker watching the mempool can race transactions with a 0-value transaction, copying the signatures, in order to prevent a that nullifier from ever being spent.

Cargo.toml

@@ -3,7 +3,7 @@
 # Set virtual workspace's resolver to v1, to support the "rust-docs" script.
 resolver = "1"
 
-exclude = ["tools/proto-compiler", "tools/parameter-setup"]
+exclude = ["tools/proto-compiler"]
 
 # Also remember to add to deployments/scripts/rust-docs
 members = [
@@ -50,6 +50,7 @@ members = [
   "crates/misc/tct-visualize",
   "crates/bench",
   "tools/summonerd", "crates/core/component/funding", "crates/core/genesis",
+  "tools/parameter-setup"
 ]
 
 # Config for 'cargo dist'

crates/core/app/tests/spend.rs

@@ -1,23 +1,27 @@
 mod common;
 
 use self::common::TempStorageExt;
+use ark_ff::{Fp, UniformRand};
 use cnidarium::{ArcStateDeltaExt, StateDelta, TempStorage};
 use cnidarium_component::{ActionHandler as _, Component};
-use decaf377_rdsa::SigningKey;
+use decaf377::{Fq, Fr};
+use decaf377_rdsa::{SigningKey, SpendAuth, VerificationKey};
 use penumbra_app::ActionHandler;
 use penumbra_asset::Value;
 use penumbra_compact_block::component::CompactBlockManager;
-use penumbra_keys::{test_keys, PayloadKey};
+use penumbra_keys::{keys::NullifierKey, test_keys, PayloadKey};
 use penumbra_mock_client::MockClient;
 use penumbra_num::Amount;
 use penumbra_sct::{
     component::{clock::EpochManager, source::SourceContext},
     epoch::Epoch,
 };
-use penumbra_shielded_pool::{component::ShieldedPool, SpendPlan};
+use penumbra_shielded_pool::{
+    component::ShieldedPool, Note, SpendPlan, SpendProof, SpendProofPrivate, SpendProofPublic,
+};
 use penumbra_transaction::{Transaction, TransactionBody, TransactionParameters};
 use penumbra_txhash::{AuthorizingData, EffectHash, TransactionContext};
-use rand_core::SeedableRng;
+use rand_core::{OsRng, SeedableRng};
 use std::{ops::Deref, sync::Arc};
 use tendermint::abci;
 
@@ -87,6 +91,120 @@ async fn spend_happy_path() -> anyhow::Result<()> {
     Ok(())
 }
 
+// PoC for issue surfaced in zellic audit: https://github.com/penumbra-zone/penumbra/issues/3859
+#[tokio::test]
+#[should_panic(expected = "assertion failed: cs.is_satisfied().unwrap()")]
+async fn invalid_dummy_spend() {
+    let mut rng = rand_chacha::ChaChaRng::seed_from_u64(1312);
+
+    let storage = TempStorage::new()
+        .await
+        .unwrap()
+        .apply_default_genesis()
+        .await
+        .unwrap();
+    let mut state = Arc::new(StateDelta::new(storage.latest_snapshot()));
+
+    let height = 1;
+
+    // Precondition: This test uses the default genesis which has existing notes for the test keys.
+    let mut client = MockClient::new(test_keys::FULL_VIEWING_KEY.clone());
+    let sk = test_keys::SPEND_KEY.clone();
+    client.sync_to(0, state.deref()).await.unwrap();
+    let note = client.notes.values().next().unwrap().clone();
+
+    let note_commitment = note.commit();
+    let proof = client.sct.witness(note_commitment).unwrap();
+    let root = client.sct.root();
+    let tct_position = proof.position();
+
+    // 1. Simulate BeginBlock
+    let mut state_tx = state.try_begin_transaction().unwrap();
+    state_tx.put_block_height(height);
+    state_tx.put_epoch_by_height(
+        height,
+        Epoch {
+            index: 0,
+            start_height: 0,
+        },
+    );
+    state_tx.apply();
+
+    // 2. Create a Spend action
+    let spend_plan = SpendPlan::new(&mut rng, note.clone(), tct_position);
+    let dummy_effect_hash = [0u8; 64];
+    let rsk = sk.spend_auth_key().randomize(&spend_plan.randomizer);
+    let auth_sig = rsk.sign(&mut rng, dummy_effect_hash.as_ref());
+    let mut spend = spend_plan.spend(&test_keys::FULL_VIEWING_KEY, auth_sig, proof.clone(), root);
+
+    let note_zero_value = Note::from_parts(
+        note.address(),
+        Value {
+            amount: Amount::from(0u64),
+            asset_id: note.asset_id(),
+        },
+        note.rseed(),
+    )
+    .unwrap();
+
+    let public = SpendProofPublic {
+        anchor: root,
+        balance_commitment: spend_plan.balance().commit(spend_plan.value_blinding),
+        nullifier: spend_plan.nullifier(&test_keys::FULL_VIEWING_KEY),
+        rk: spend_plan.rk(&test_keys::FULL_VIEWING_KEY),
+    };
+
+    // construct a proof for this spend using only public information, attempting to prove a spend
+    // of a dummy note.
+    let ak = VerificationKey::<SpendAuth>::try_from([0u8; 32]).unwrap();
+    let nk = NullifierKey(Fp::rand(&mut OsRng));
+
+    let private = SpendProofPrivate {
+        state_commitment_proof: proof,
+        note: note_zero_value,
+        v_blinding: Fr::rand(&mut OsRng),
+        spend_auth_randomizer: Fr::rand(&mut OsRng),
+        ak,
+        nk,
+    };
+    let bad_proof = SpendProof::prove(
+        Fq::rand(&mut OsRng),
+        Fq::rand(&mut OsRng),
+        &penumbra_proof_params::SPEND_PROOF_PROVING_KEY,
+        public,
+        private,
+    )
+    .expect("can generate ZKSpendProof");
+
+    spend.proof = bad_proof;
+
+    let transaction_context = TransactionContext {
+        anchor: root,
+        effect_hash: EffectHash(dummy_effect_hash),
+    };
+
+    // 3. Simulate execution of the Spend action
+    spend.check_stateless(transaction_context).await.unwrap();
+    spend.check_stateful(state.clone()).await.unwrap();
+    let mut state_tx = state.try_begin_transaction().unwrap();
+    state_tx.put_mock_source(1u8);
+    spend.execute(&mut state_tx).await.unwrap();
+    state_tx.apply();
+
+    // 4. Execute EndBlock
+
+    let end_block = abci::request::EndBlock {
+        height: height.try_into().unwrap(),
+    };
+    ShieldedPool::end_block(&mut state, &end_block).await;
+
+    let mut state_tx = state.try_begin_transaction().unwrap();
+    // ... and for the App, call `finish_block` to correctly write out the SCT with the data we'll use next.
+    state_tx.finish_block(false).await.unwrap();
+
+    state_tx.apply();
+}
+
 #[tokio::test]
 #[should_panic(expected = "was already spent")]
 async fn spend_duplicate_nullifier_previous_transaction() {

crates/core/component/shielded-pool/src/spend/proof.rs

@@ -72,11 +72,6 @@ pub struct SpendProofPrivate {
 fn check_satisfaction(public: &SpendProofPublic, private: &SpendProofPrivate) -> Result<()> {
     use penumbra_keys::keys::FullViewingKey;
 
-    let amount_u128: u128 = private.note.value().amount.into();
-    if amount_u128 == 0u128 {
-        return Ok(());
-    }
-
     let note_commitment = private.note.commit();
     if note_commitment != private.state_commitment_proof.commitment() {
         anyhow::bail!("note commitment did not match state commitment proof");
@@ -91,7 +86,10 @@ fn check_satisfaction(public: &SpendProofPublic, private: &SpendProofPrivate) ->
         anyhow::bail!("nullifier did not match public input");
     }
 
-    private.state_commitment_proof.verify(public.anchor)?;
+    let amount_u128: u128 = private.note.value().amount.into();
+    if amount_u128 != 0u128 {
+        private.state_commitment_proof.verify(public.anchor)?;
+    }
 
     let rk = private.ak.randomize(&private.spend_auth_randomizer);
     if rk != public.rk {
@@ -178,21 +176,19 @@ impl ConstraintSynthesizer<Fq> for SpendCircuit {
             NullifierVar::new_input(cs.clone(), || Ok(self.public.nullifier))?;
         let rk_var = RandomizedVerificationKey::new_input(cs.clone(), || Ok(self.public.rk))?;
 
-        // We short circuit to true if value released is 0. That means this is a _dummy_ spend.
-        let is_dummy = note_var.amount().is_eq(&FqVar::zero())?;
-        // We use a Boolean constraint to enforce the below constraints only if this is not a
-        // dummy spend.
-        let is_not_dummy = is_dummy.not();
-
         // Note commitment integrity.
         let note_commitment_var = note_var.commit()?;
-        note_commitment_var.conditional_enforce_equal(&claimed_note_commitment, &is_not_dummy)?;
+        note_commitment_var.enforce_equal(&claimed_note_commitment)?;
 
         // Nullifier integrity.
         let nullifier_var = NullifierVar::derive(&nk_var, &position_var, &claimed_note_commitment)?;
-        nullifier_var.conditional_enforce_equal(&claimed_nullifier_var, &is_not_dummy)?;
+        nullifier_var.enforce_equal(&claimed_nullifier_var)?;
 
         // Merkle auth path verification against the provided anchor.
+        //
+        // We short circuit the merkle path verification if the note is a _dummy_ spend (a spend
+        // with zero value), since these are never committed to the state commitment tree.
+        let is_not_dummy = note_var.amount().is_eq(&FqVar::zero())?.not();
         merkle_path_var.verify(
             cs.clone(),
             &is_not_dummy,
@@ -203,25 +199,23 @@ impl ConstraintSynthesizer<Fq> for SpendCircuit {
 
         // Check integrity of randomized verification key.
         let computed_rk_var = ak_element_var.randomize(&spend_auth_randomizer_var)?;
-        computed_rk_var.conditional_enforce_equal(&rk_var, &is_not_dummy)?;
+        computed_rk_var.enforce_equal(&rk_var)?;
 
         // Check integrity of diversified address.
         let ivk = IncomingViewingKeyVar::derive(&nk_var, &ak_element_var)?;
         let computed_transmission_key =
             ivk.diversified_public(&note_var.diversified_generator())?;
-        computed_transmission_key
-            .conditional_enforce_equal(&note_var.transmission_key(), &is_not_dummy)?;
+        computed_transmission_key.enforce_equal(&note_var.transmission_key())?;
 
         // Check integrity of balance commitment.
         let balance_commitment = note_var.value().commit(v_blinding_vars)?;
-        balance_commitment
-            .conditional_enforce_equal(&claimed_balance_commitment_var, &is_not_dummy)?;
+        balance_commitment.enforce_equal(&claimed_balance_commitment_var)?;
 
         // Check the diversified base is not identity.
         let identity = ElementVar::new_constant(cs, decaf377::Element::default())?;
-        identity.conditional_enforce_not_equal(&note_var.diversified_generator(), &is_not_dummy)?;
+        identity.enforce_not_equal(&note_var.diversified_generator())?;
         // Check the ak is not identity.
-        identity.conditional_enforce_not_equal(&ak_element_var.inner, &is_not_dummy)?;
+        identity.enforce_not_equal(&ak_element_var.inner)?;
 
         Ok(())
     }

crates/crypto/proof-params/src/gen/convert_id.rs

@@ -1,3 +1,3 @@
 
-pub const PROVING_KEY_ID: &'static str = "groth16pk1yn86gluhk3sy0lqhganhqzqlq9fnxx5dvay6296xeh3h6readefs8nzfsj";
-pub const VERIFICATION_KEY_ID: &'static str = "groth16vk17nws5qr9k79eqcrfvedahtlc8yk3cl2dnafy8dvu4qlqdmc0s2vqhehwu7";
+pub const PROVING_KEY_ID: &'static str = "groth16pk1x4m7qnm4czf3mtys3s8p8fnzpcz22fv26udz7cgql3x047rqn6nqquqh7t";
+pub const VERIFICATION_KEY_ID: &'static str = "groth16vk1eu7tznfakx6dh3kzfrtfzt034ypzc69eaqdmjptyayxjqp0jt84s94hy8m";

crates/crypto/proof-params/src/gen/delegator_vote_id.rs

@@ -1,3 +1,3 @@
 
-pub const PROVING_KEY_ID: &'static str = "groth16pk1enjmupx56ya5x640c8ux5z6uxed3epgaeacgtfvttnjw48hem9csau4dg5";
-pub const VERIFICATION_KEY_ID: &'static str = "groth16vk1lfaxqtdk4mja002dag6wsv422mydt2m5h0w5jct7w37972n58jaqpg3jnk";
+pub const PROVING_KEY_ID: &'static str = "groth16pk1wquddkhlkp3tj3fsjt7qtgaevfpndp2ejptvp7lxhafa0ddxqetqtp52ja";
+pub const VERIFICATION_KEY_ID: &'static str = "groth16vk1j8jt9leawttxhqdye26apdtfkju4nmhzf40y3339g774ca73qyusq7wpcn";

crates/crypto/proof-params/src/gen/nullifier_derivation_id.rs

@@ -1,3 +1,3 @@
 
-pub const PROVING_KEY_ID: &'static str = "groth16pk15nyrctlmt7tehtw5xse5qkxuduaryflwjgwqyxylnqgp3m66nlrq65q374";
-pub const VERIFICATION_KEY_ID: &'static str = "groth16vk16uhuvvk8hkh67kpcuxavksvu8p0gnzlhm54n22rlcvz2rvj82adsxqsjqf";
+pub const PROVING_KEY_ID: &'static str = "groth16pk1e79gm2pkg5rlkyeectfh4yualrjy9qdlez22l4f56g5tjqkqfv3syrrzsm";
+pub const VERIFICATION_KEY_ID: &'static str = "groth16vk1ylw2upegj9ppaz9s0hd0czh8yeqxvqc4s39468733cnfcch2cfwqnm4gqj";

crates/crypto/proof-params/src/gen/output_id.rs

@@ -1,3 +1,3 @@
 
-pub const PROVING_KEY_ID: &'static str = "groth16pk127jt39hfrz4xud8gdfknsrrmpj57ukpdecfntr5nvqrdar5aut7q2yuu60";
-pub const VERIFICATION_KEY_ID: &'static str = "groth16vk12kdj4y4ewuyuzjv50j394dngavrzwxjrdaz365hqjfs6xxcsnauqc0v2z3";
+pub const PROVING_KEY_ID: &'static str = "groth16pk1cq4kzdw0gksjch329u7dwznuutgswt084w259ef9aul3zpsnvmxqmjqwwu";
+pub const VERIFICATION_KEY_ID: &'static str = "groth16vk1clsj7lwhfqkvvuy6syd37a4ghnpgle67qefxtfm4fmkwf2yr6cmst2axts";

crates/crypto/proof-params/src/gen/spend_id.rs

@@ -1,3 +1,3 @@
 
-pub const PROVING_KEY_ID: &'static str = "groth16pk14tgw5vffk4gdyllc4hgvma4rhk75nufkyvf3kpsxv09wsyr9wx4qlcgxs9";
-pub const VERIFICATION_KEY_ID: &'static str = "groth16vk10w5undhz04tev857kk6047kk4ppywzale9vpf03g605anfucsp0q77ku8p";
+pub const PROVING_KEY_ID: &'static str = "groth16pk1q5g33sdz9u9thkpj67rzacrlashgumzjnzenu589tn3dx6798q2qwx3z59";
+pub const VERIFICATION_KEY_ID: &'static str = "groth16vk1v7rahtkzn3045auj3h24pf3du7dlhjyfwgvya4pwvtq4jhfmw7vqawpf33";

crates/crypto/proof-params/src/gen/swap_id.rs

@@ -1,3 +1,3 @@
 
-pub const PROVING_KEY_ID: &'static str = "groth16pk1j0zmn74aq9w0q2gu8jjf5wh93smv82l52lf7k4w528e6q36dcs6qsm06sc";
-pub const VERIFICATION_KEY_ID: &'static str = "groth16vk1wwr0yh4yjakrjzg2epv67al3ra8yp5hjvr8mvretflvn53lkgmsqf9p3rf";
+pub const PROVING_KEY_ID: &'static str = "groth16pk1uly6kxvkvmygl9n74zm27nujqj2deg793c6kcjwsmt43akt9jw3qulqxqc";
+pub const VERIFICATION_KEY_ID: &'static str = "groth16vk1pzv7jlra2tmkwxnchzqm2rd5pq8wvhkx5j3vpe5xspw3h6lyygqquqjjed";

crates/crypto/proof-params/src/gen/swapclaim_id.rs

@@ -1,3 +1,3 @@
 
-pub const PROVING_KEY_ID: &'static str = "groth16pk1v78n6c69ewu99ty4gmg58fxk82y3srqgl82czvpe2yfv0s00j53s476aqk";
-pub const VERIFICATION_KEY_ID: &'static str = "groth16vk1dp4m2nnc7lu7cqxp6lzaha67zqexcdp7ppzyvxy8kyhw5zq29zusrwxhvt";
+pub const PROVING_KEY_ID: &'static str = "groth16pk1myy8srsjzdkrgpzuflzlx2m9nharlyrv9cyrdxscfc45dpk088gs6jhtyf";
+pub const VERIFICATION_KEY_ID: &'static str = "groth16vk180e0rlmv4qds0jcuevkur9j5ykjrjyly0vqa3gnj6g6j77d94c6qtlwtnr";

docs/guide/src/dev/parameter_setup.md

@@ -11,8 +11,7 @@ For development purposes *only*, we have a crate in `tools/parameter-setup`
 that lets one generate the proving and verifying keys:
 
 ```shell
-cd tools/parameter-setup
-cargo run
+cargo run --release --bin penumbra-parameter-setup
 ```
 
 The verifying and proving keys for each circuit will be created in a serialized