diff --git a/libwallet/src/contract/slate.rs b/libwallet/src/contract/slate.rs index 4cc65fe..6bc33aa 100644 --- a/libwallet/src/contract/slate.rs +++ b/libwallet/src/contract/slate.rs @@ -26,11 +26,6 @@ use super::types::ProofArgs; use crate::contract::proofs::InvoiceProof; use ed25519_dalek::PublicKey as DalekPublicKey; -/// TODO: Removed for now, consider secp error in sign function -/// The secret key we replace the actual key with after we have signed with the Context keys. This is -/// to prevent possibility of signing with the same key twice. -/// pub const SEC_KEY_FAKE: [u8; 32] = [0; 32]; - /// Add payment proof data to slate, noop for sender pub fn add_payment_proof<'a, T: ?Sized, C, K>( w: &mut T, @@ -192,7 +187,10 @@ pub fn transition_state(slate: &mut Slate) -> Result<(), Error> { } /// Add partial signature to the slate. -// TODO: Should be a sign & forget pubkey+nonce implementation. +// Nonce reuse is prevented by forgetting the signing context after a signed step: +// save_step deletes the private context (which holds sec_key/sec_nonce) once is_signed +// (except the deliberately-retained step2 context used for safe cancel). The context is +// keyed by slate.id, so a given nonce is only ever used for one message. pub fn sign<'a, T: ?Sized, C, K>( w: &mut T, keychain_mask: Option<&SecretKey>, @@ -213,13 +211,6 @@ where ); debug!("contract::slate::sign => done"); - // TODO: This produces a secp error, probably need a valid key. Verify that this is what we want to do. - // let fake_key = SecretKey::from_slice(keychain.secp(), &SEC_KEY_FAKE)?; - // context.sec_key = fake_key.clone(); - // context.sec_nonce = fake_key.clone(); - // context.initial_sec_key = fake_key.clone(); - // context.initial_sec_nonce = fake_key.clone(); - Ok(()) } diff --git a/libwallet/src/contract/utils.rs b/libwallet/src/contract/utils.rs index 2499689..3e81d26 100644 --- a/libwallet/src/contract/utils.rs +++ b/libwallet/src/contract/utils.rs @@ -285,8 +285,22 @@ where } batch.commit()?; + drop(batch); - // TODO: Assert we don't have the context to avoid potentially leaking it! Also write tests around this. + // Defense in depth: once we have signed (and are past the step2 context that is kept + // deliberately for safe cancel), the signing context holding sec_key/sec_nonce must be + // gone so the nonce can never be reused. It is deleted in the is_signed && !is_step2 + // branch above; verify the deletion actually took effect. + if is_signed && !is_step2 { + if w + .get_private_context(keychain_mask, slate.id.as_bytes()) + .is_ok() + { + return Err(Error::GenericError( + "signing context was not removed after signing".into(), + )); + } + } debug!("contract::utils::save_step => Atomic updated done"); Ok(())