diff --git a/bip39bug.md b/bip39bug.md new file mode 100644 index 0000000..bb62319 --- /dev/null +++ b/bip39bug.md @@ -0,0 +1,13 @@ +## Zecwallet-cli BIP39 derivation bug + +In v1.0 of zecwallet-cli, there was a bug that incorrectly derived HD wallet keys after the first key. That is, the first key, address was correct, but subsequent ones were not. + +The issue was that the 32-byte seed was directly being used to derive then subsequent addresses instead of the 64-byte pkdf2(seed). The issue affected both t and z addresses. + +Note that no funds are at risk. The issue is that, if in the future, you import the seed into a different wallet, you might not see all your addresses in the new wallet, so it's better to fix it now. + +## Fix +If you start a wallet that has this bug, you'll be notified. +The bug can be automatically fixed by the wallet by running the `fixbip39bug` command. Just start `zecwallet-cli` and type `fixbip39bug`. + +If you have any funds in the incorrect addresses, they'll be sent to yourself, and the correct addresses re-derived. \ No newline at end of file diff --git a/lib/src/commands.rs b/lib/src/commands.rs index 28bb26b..721315b 100644 --- a/lib/src/commands.rs +++ b/lib/src/commands.rs @@ -196,47 +196,9 @@ impl Command for ExportCommand { } let address = if args.is_empty() { None } else { Some(args[0].to_string()) }; - - format!("{}", lightclient.do_export(address).pretty(2)) - } -} - -struct EncryptCommand {} -impl Command for EncryptCommand { - fn help(&self) -> String { - let mut h = vec![]; - h.push("Encrypt the wallet with a password"); - h.push("Note 1: This will encrypt the seed and the sapling and transparent private keys."); - h.push(" Use 'unlock' to temporarily unlock the wallet for spending or 'decrypt' "); - h.push(" to permanatly remove the encryption"); - h.push("Note 2: If you forget the password, the only way to recover the wallet is to restore"); - h.push(" from the seed phrase."); - h.push("Usage:"); - h.push("encrypt password"); - h.push(""); - h.push("Example:"); - h.push("encrypt my_strong_password"); - - h.join("\n") - } - - fn short_help(&self) -> String { - "Encrypt the wallet with a password".to_string() - } - - fn exec(&self, args: &[&str], lightclient: &LightClient) -> String { - if args.len() != 1 { - return self.help(); - } - - let passwd = args[0].to_string(); - - match lightclient.wallet.write().unwrap().encrypt(passwd) { - Ok(_) => object!{ "result" => "success" }, - Err(e) => object!{ - "result" => "error", - "error" => e.to_string() - } + match lightclient.do_export(address) { + Ok(j) => j, + Err(e) => object!{ "error" => e } }.pretty(2) } } @@ -347,11 +309,11 @@ impl Command for SendCommand { // 2 - A single argument in the form of a JSON string that is "[{address: address, value: value, memo: memo},...]" // 1 - Destination address. T or Z address - if args.len() < 1 || args.len() > 3 { + if args.len() < 1 || args.len() > 3 { return self.help(); } - // Check for a single argument that can be parsed as JSON + // Check for a single argument that can be parsed as JSON if args.len() == 1 { // Sometimes on the command line, people use "'" for the quotes, which json::parse doesn't // understand. So replace it with double-quotes @@ -365,11 +327,11 @@ impl Command for SendCommand { } }; - if !json_args.is_array() { + if !json_args.is_array() { return format!("Couldn't parse argument as array\n{}", self.help()); } - let maybe_send_args = json_args.members().map( |j| { + let maybe_send_args = json_args.members().map( |j| { if !j.has_key("address") || !j.has_key("amount") { Err(format!("Need 'address' and 'amount'\n")) } else { @@ -397,7 +359,7 @@ impl Command for SendCommand { }; let memo = if args.len() == 3 { Some(args[2].to_string()) } else {None}; - + lightclient.do_sync(true); match lightclient.do_send(vec!((args[0], value, memo))) { Ok(txid) => { object!{ "txid" => txid } }, @@ -428,7 +390,7 @@ impl Command for SaveCommand { } fn exec(&self, _args: &[&str], lightclient: &LightClient) -> String { - match lightclient.do_save() { + match lightclient.do_save() { Ok(_) => { let r = object!{ "result" => "success" }; r.pretty(2) @@ -462,7 +424,10 @@ impl Command for SeedCommand { } fn exec(&self, _args: &[&str], lightclient: &LightClient) -> String { - format!("{}", lightclient.do_seed_phrase().pretty(2)) + match lightclient.do_seed_phrase() { + Ok(j) => j, + Err(e) => object!{ "error" => e } + }.pretty(2) } } @@ -537,7 +502,10 @@ impl Command for NewAddressCommand { return format!("No address type specified\n{}", self.help()); } - format!("{}", lightclient.do_new_address(args[0]).pretty(2)) + match lightclient.do_new_address(args[0]) { + Ok(j) => j, + Err(e) => object!{ "error" => e } + }.pretty(2) } } @@ -620,7 +588,7 @@ impl Command for QuitCommand { } fn exec(&self, _args: &[&str], lightclient: &LightClient) -> String { - match lightclient.do_save() { + match lightclient.do_save() { Ok(_) => {"".to_string()}, Err(e) => e } diff --git a/lib/src/grpcconnector.rs b/lib/src/grpcconnector.rs index 10d0d88..3a63b4e 100644 --- a/lib/src/grpcconnector.rs +++ b/lib/src/grpcconnector.rs @@ -284,7 +284,7 @@ pub fn broadcast_raw_tx(uri: &http::Uri, no_cert: bool, tx_bytes: Box<[u8]>) -> txid = txid[1..txid.len()-1].to_string(); } - Ok(txid) + Ok(txid) } else { Err(format!("Error: {:?}", sendresponse)) } diff --git a/lib/src/lightclient.rs b/lib/src/lightclient.rs index d4325d2..69d47fc 100644 --- a/lib/src/lightclient.rs +++ b/lib/src/lightclient.rs @@ -168,7 +168,8 @@ impl LightClientConfig { pub fn base58_pubkey_address(&self) -> [u8; 1] { match &self.chain_name[..] { "main" => mainnet::B58_PUBKEY_ADDRESS_PREFIX, - + + c => panic!("Unknown chain {}", c) } } @@ -177,7 +178,8 @@ impl LightClientConfig { pub fn base58_script_address(&self) -> [u8; 1] { match &self.chain_name[..] { "main" => mainnet::B58_SCRIPT_ADDRESS_PREFIX, - + + c => panic!("Unknown chain {}", c) } } @@ -222,6 +224,26 @@ impl LightClient { } + /// Method to create a test-only version of the LightClient + #[allow(dead_code)] + fn unconnected(seed_phrase: String) -> io::Result { + let config = LightClientConfig::create_unconnected("test".to_string()); + let mut l = LightClient { + wallet : Arc::new(RwLock::new(LightWallet::new(Some(seed_phrase), &config, 0)?)), + config : config.clone(), + sapling_output : vec![], + sapling_spend : vec![] + }; + + l.set_wallet_initial_state(); + l.read_sapling_params(); + + info!("Created new wallet!"); + info!("Created LightClient to {}", &config.server); + + Ok(l) + } + pub fn new_from_phrase(seed_phrase: String, config: &LightClientConfig, latest_block: u64) -> io::Result { if config.get_wallet_path().exists() { return Err(Error::new(ErrorKind::AlreadyExists, @@ -279,12 +301,10 @@ impl LightClient { } // Export private keys - pub fn do_export(&self, addr: Option) -> JsonValue { + pub fn do_export(&self, addr: Option) -> Result { if !self.wallet.read().unwrap().is_unlocked_for_spending() { error!("Wallet is locked"); - return object!{ - "error" => "Wallet is locked" - }; + return Err("Wallet is locked"); } // Clone address so it can be moved into the closure @@ -317,11 +337,12 @@ impl LightClient { all_keys.extend_from_slice(&z_keys); all_keys.extend_from_slice(&t_keys); - all_keys.into() + Ok(all_keys.into()) } pub fn do_address(&self) -> JsonValue { let wallet = self.wallet.read().unwrap(); + // Collect z addresses let z_addresses = wallet.zaddress.read().unwrap().iter().map( |ad| { encode_payment_address(self.config.hrp_sapling_address(), &ad) @@ -370,8 +391,7 @@ impl LightClient { } } - pub fn do_save(&self) -> Result<(), String> { - + pub fn do_save(&self) -> Result<(), String> { // If the wallet is encrypted but unlocked, lock it again. { let mut wallet = self.wallet.write().unwrap(); @@ -423,19 +443,17 @@ impl LightClient { } } - pub fn do_seed_phrase(&self) -> JsonValue { + pub fn do_seed_phrase(&self) -> Result { if !self.wallet.read().unwrap().is_unlocked_for_spending() { error!("Wallet is locked"); - return object!{ - "error" => "Wallet is locked" - }; + return Err("Wallet is locked"); } let wallet = self.wallet.read().unwrap(); - object!{ + Ok(object!{ "seed" => wallet.get_seed_phrase(), "birthday" => wallet.get_birthday() - } + }) } // Return a list of all notes, spent and unspent @@ -610,12 +628,10 @@ impl LightClient { } /// Create a new address, deriving it from the seed. - pub fn do_new_address(&self, addr_type: &str) -> JsonValue { + pub fn do_new_address(&self, addr_type: &str) -> Result { if !self.wallet.read().unwrap().is_unlocked_for_spending() { error!("Wallet is locked"); - return object!{ - "error" => "Wallet is locked" - }; + return Err("Wallet is locked".to_string()); } let wallet = self.wallet.write().unwrap(); @@ -626,13 +642,11 @@ impl LightClient { _ => { let e = format!("Unrecognized address type: {}", addr_type); error!("{}", e); - return object!{ - "error" => e - }; + return Err(e); } }; - array![new_address] + Ok(array![new_address]) } pub fn do_rescan(&self) -> String { @@ -872,3 +886,61 @@ impl LightClient { } } } + + +pub mod tests { + use lazy_static::lazy_static; + //use super::LightClient; + + lazy_static!{ + static ref TEST_SEED: String = "youth strong sweet gorilla hammer unhappy congress stamp left stereo riot salute road tag clean toilet artefact fork certain leopard entire civil degree wonder".to_string(); + } + + #[test] + pub fn test_encrypt_decrypt() { + let lc = super::LightClient::unconnected(TEST_SEED.to_string()).unwrap(); + + assert!(!lc.do_export(None).is_err()); + assert!(!lc.do_new_address("z").is_err()); + assert!(!lc.do_new_address("t").is_err()); + assert_eq!(lc.do_seed_phrase().unwrap()["seed"], TEST_SEED.to_string()); + + // Encrypt and Lock the wallet + lc.wallet.write().unwrap().encrypt("password".to_string()).unwrap(); + assert!(lc.do_export(None).is_err()); + assert!(lc.do_seed_phrase().is_err()); + assert!(lc.do_new_address("t").is_err()); + assert!(lc.do_new_address("z").is_err()); + assert!(lc.do_send(vec![("z", 0, None)]).is_err()); + + // Do a unlock, and make sure it all works now + lc.wallet.write().unwrap().unlock("password".to_string()).unwrap(); + assert!(!lc.do_export(None).is_err()); + assert!(!lc.do_seed_phrase().is_err()); + assert!(!lc.do_new_address("t").is_err()); + assert!(!lc.do_new_address("z").is_err()); + } + + #[test] + pub fn test_addresses() { + let lc = super::LightClient::unconnected(TEST_SEED.to_string()).unwrap(); + + // Add new z and t addresses + + let taddr1 = lc.do_new_address("t").unwrap()[0].as_str().unwrap().to_string(); + let taddr2 = lc.do_new_address("t").unwrap()[0].as_str().unwrap().to_string(); + let zaddr1 = lc.do_new_address("z").unwrap()[0].as_str().unwrap().to_string(); + let zaddr2 = lc.do_new_address("z").unwrap()[0].as_str().unwrap().to_string(); + + let addresses = lc.do_address(); + assert_eq!(addresses["z_addresses"].len(), 3); + assert_eq!(addresses["z_addresses"][1], zaddr1); + assert_eq!(addresses["z_addresses"][2], zaddr2); + + assert_eq!(addresses["t_addresses"].len(), 3); + assert_eq!(addresses["t_addresses"][1], taddr1); + assert_eq!(addresses["t_addresses"][2], taddr2); + } + + +} \ No newline at end of file diff --git a/lib/src/lightwallet/bugs.rs b/lib/src/lightwallet/bugs.rs index 60f237f..48eeba7 100644 --- a/lib/src/lightwallet/bugs.rs +++ b/lib/src/lightwallet/bugs.rs @@ -70,6 +70,7 @@ impl BugBip39Derivation { // Tranfer money // 1. The desination is z address #0 + println!("Sending funds to ourself."); let zaddr = client.do_address()["z_addresses"][0].as_str().unwrap().to_string(); let balance_json = client.do_balance(); let fee: u64 = DEFAULT_FEE.try_into().unwrap(); diff --git a/lib/src/startup_helpers.rs b/lib/src/startup_helpers.rs index 0b5ac59..c597c5d 100644 --- a/lib/src/startup_helpers.rs +++ b/lib/src/startup_helpers.rs @@ -17,4 +17,4 @@ pub fn report_permission_error() { user, home); } -} \ No newline at end of file +} diff --git a/src/main.rs b/src/main.rs index 0db96ad..c2ab390 100644 --- a/src/main.rs +++ b/src/main.rs @@ -160,7 +160,7 @@ fn startup(server: http::Uri, dangerous: bool, seed: Option, first_sync: std::io::Error::new(ErrorKind::Other, e) })?; - let lightclient = match seed { + let lightclient = match seed { Some(phrase) => Arc::new(LightClient::new_from_phrase(phrase, &config, latest_block_height)?), None => Arc::new(LightClient::read_from_disk(&config)?) };