interlay · gregdhill · Jun 1, 2022 · Jun 1, 2022
diff --git a/crates/bitcoin/src/address.rs b/crates/bitcoin/src/address.rs
@@ -101,6 +101,13 @@ impl Address {
     pub fn random() -> Self {
         Address::P2PKH(H160::random())
     }
+
+    pub fn is_zero(&self) -> bool {
+        match self {
+            Self::P2PKH(hash) | Self::P2SH(hash) | Self::P2WPKHv0(hash) => hash.is_zero(),
+            Self::P2WSHv0(hash) => hash.is_zero(),
+        }
+    }
 }
 
 impl Default for Address {

diff --git a/crates/issue/src/benchmarking.rs b/crates/issue/src/benchmarking.rs
@@ -13,7 +13,7 @@ use frame_support::assert_ok;
 use frame_system::RawOrigin;
 use orml_traits::MultiCurrency;
 use primitives::{CurrencyId, VaultId};
-use sp_core::{H160, H256, U256};
+use sp_core::{H256, U256};
 use sp_runtime::{
     traits::{One, Zero},
     FixedPointNumber,
@@ -84,7 +84,7 @@ fn mine_blocks<T: crate::Config>(end_height: u32) {
     let height = 0;
     let block = BlockBuilder::new()
         .with_version(4)
-        .with_coinbase(&BtcAddress::P2SH(H160::zero()), 50, 3)
+        .with_coinbase(&BtcAddress::random(), 50, 3)
         .with_timestamp(1588813835)
         .mine(U256::from(2).pow(254.into()))
         .unwrap();
@@ -118,7 +118,7 @@ fn mine_blocks<T: crate::Config>(end_height: u32) {
         let block = BlockBuilder::new()
             .with_previous_hash(prev_hash)
             .with_version(4)
-            .with_coinbase(&BtcAddress::P2SH(H160::zero()), 50, 3)
+            .with_coinbase(&BtcAddress::random(), 50, 3)
             .with_timestamp(1588813835)
             .add_transaction(transaction.clone())
             .mine(U256::from(2).pow(254.into()))
@@ -153,7 +153,7 @@ benchmarks! {
         let height = 0;
         let block = BlockBuilder::new()
             .with_version(4)
-            .with_coinbase(&BtcAddress::P2SH(H160::zero()), 50, 3)
+            .with_coinbase(&BtcAddress::random(), 50, 3)
             .with_timestamp(1588813835)
             .mine(U256::from(2).pow(254.into())).unwrap();
         let block_hash = block.header.hash;
@@ -163,7 +163,7 @@ benchmarks! {
         Security::<T>::set_active_block_number(1u32.into());
         BtcRelay::<T>::initialize(relayer_id.clone(), block_header, height).unwrap();
 
-        let vault_btc_address = BtcAddress::P2SH(H160::zero());
+        let vault_btc_address = BtcAddress::random();
 
         let transaction = TransactionBuilder::new()
         .with_version(2)
@@ -210,7 +210,7 @@ benchmarks! {
         mint_collateral::<T>(&vault_id.account_id.clone(), (1u32 << 31).into());
         mint_collateral::<T>(&relayer_id, (1u32 << 31).into());
 
-        let vault_btc_address = BtcAddress::P2SH(H160::zero());
+        let vault_btc_address = BtcAddress::random();
         let value: Amount<T> = Amount::new(2u32.into(), get_wrapped_currency_id::<T>());
 
         let issue_id = H256::zero();
@@ -300,7 +300,7 @@ benchmarks! {
         mint_collateral::<T>(&origin, (1u32 << 31).into());
         mint_collateral::<T>(&vault_id.account_id.clone(), (1u32 << 31).into());
 
-        let vault_btc_address = BtcAddress::P2SH(H160::zero());
+        let vault_btc_address = BtcAddress::random();
         let value = Amount::new(2u32.into(), get_wrapped_currency_id::<T>());
 
         let issue_id = H256::zero();

diff --git a/crates/issue/src/tests.rs b/crates/issue/src/tests.rs
@@ -6,7 +6,7 @@ use currency::Amount;
 use frame_support::{assert_noop, assert_ok, dispatch::DispatchError};
 use mocktopus::mocking::*;
 use sp_arithmetic::FixedU128;
-use sp_core::{H160, H256};
+use sp_core::H256;
 use sp_runtime::traits::One;
 use vault_registry::{DefaultVault, DefaultVaultId, Vault, VaultStatus, Wallet};
 
@@ -32,20 +32,31 @@ fn request_issue(origin: AccountId, amount: Balance, vault: DefaultVaultId<Test>
 
     ext::vault_registry::try_increase_to_be_issued_tokens::<Test>.mock_safe(|_, _| MockResult::Return(Ok(())));
     ext::vault_registry::register_deposit_address::<Test>
-        .mock_safe(|_, _| MockResult::Return(Ok(BtcAddress::default())));
+        .mock_safe(|_, _| MockResult::Return(Ok(BtcAddress::random())));
 
     Issue::_request_issue(origin, amount, vault)
 }
 
 fn request_issue_ok(origin: AccountId, amount: Balance, vault: DefaultVaultId<Test>) -> H256 {
+    request_issue_ok_with_address(origin, amount, vault, BtcAddress::random())
+}
+
+fn request_issue_ok_with_address(
+    origin: AccountId,
+    amount: Balance,
+    vault: DefaultVaultId<Test>,
+    address: BtcAddress,
+) -> H256 {
     ext::vault_registry::ensure_not_banned::<Test>.mock_safe(|_| MockResult::Return(Ok(())));
 
     ext::security::get_secure_id::<Test>.mock_safe(|_| MockResult::Return(get_dummy_request_id()));
 
     ext::vault_registry::try_increase_to_be_issued_tokens::<Test>.mock_safe(|_, _| MockResult::Return(Ok(())));
     ext::vault_registry::get_bitcoin_public_key::<Test>.mock_safe(|_| MockResult::Return(Ok(BtcPublicKey::default())));
-    ext::vault_registry::register_deposit_address::<Test>
-        .mock_safe(|_, _| MockResult::Return(Ok(BtcAddress::default())));
+
+    unsafe {
+        ext::vault_registry::register_deposit_address::<Test>.mock_raw(|_, _| MockResult::Return(Ok(address)));
+    }
 
     Issue::_request_issue(origin, amount, vault).unwrap()
 }
@@ -101,6 +112,7 @@ fn test_request_issue_succeeds() {
         let amount: Balance = 3;
         let issue_fee = 1;
         let issue_griefing_collateral = 20;
+        let address = BtcAddress::random();
 
         ext::vault_registry::get_active_vault_from_id::<Test>
             .mock_safe(|_| MockResult::Return(Ok(init_zero_vault(VAULT))));
@@ -110,7 +122,7 @@ fn test_request_issue_succeeds() {
         ext::fee::get_issue_griefing_collateral::<Test>
             .mock_safe(move |_| MockResult::Return(Ok(griefing(issue_griefing_collateral))));
 
-        let issue_id = request_issue_ok(origin, amount, vault.clone());
+        let issue_id = request_issue_ok_with_address(origin, amount, vault.clone(), address.clone());
 
         let request_issue_event = TestEvent::Issue(Event::RequestIssue {
             issue_id,
@@ -119,7 +131,7 @@ fn test_request_issue_succeeds() {
             fee: issue_fee,
             griefing_collateral: issue_griefing_collateral,
             vault_id: vault,
-            vault_address: BtcAddress::default(),
+            vault_address: address,
             vault_public_key: BtcPublicKey::default(),
         });
         assert!(System::events().iter().any(|a| a.event == request_issue_event));
@@ -152,7 +164,7 @@ fn test_execute_issue_succeeds() {
         ext::btc_relay::parse_merkle_proof::<Test>.mock_safe(|_| MockResult::Return(Ok(dummy_merkle_proof())));
         ext::btc_relay::parse_transaction::<Test>.mock_safe(|_| MockResult::Return(Ok(Transaction::default())));
         ext::btc_relay::get_and_verify_issue_payment::<Test, Balance>
-            .mock_safe(|_, _, _| MockResult::Return(Ok((Some(BtcAddress::P2SH(H160::zero())), 3))));
+            .mock_safe(|_, _, _| MockResult::Return(Ok((Some(BtcAddress::random()), 3))));
 
         assert_ok!(execute_issue(USER, &issue_id));
 
@@ -182,7 +194,7 @@ fn test_execute_issue_overpayment_succeeds() {
         ext::btc_relay::parse_merkle_proof::<Test>.mock_safe(|_| MockResult::Return(Ok(dummy_merkle_proof())));
         ext::btc_relay::parse_transaction::<Test>.mock_safe(|_| MockResult::Return(Ok(Transaction::default())));
         ext::btc_relay::get_and_verify_issue_payment::<Test, Balance>
-            .mock_safe(|_, _, _| MockResult::Return(Ok((Some(BtcAddress::P2SH(H160::zero())), 5))));
+            .mock_safe(|_, _, _| MockResult::Return(Ok((Some(BtcAddress::random()), 5))));
 
         ext::vault_registry::is_vault_liquidated::<Test>.mock_safe(|_| MockResult::Return(Ok(false)));
 
@@ -223,15 +235,15 @@ fn test_execute_issue_refund_succeeds() {
         ext::btc_relay::parse_merkle_proof::<Test>.mock_safe(|_| MockResult::Return(Ok(dummy_merkle_proof())));
         ext::btc_relay::parse_transaction::<Test>.mock_safe(|_| MockResult::Return(Ok(Transaction::default())));
         ext::btc_relay::get_and_verify_issue_payment::<Test, Balance>
-            .mock_safe(|_, _, _| MockResult::Return(Ok((Some(BtcAddress::P2SH(H160::zero())), 103))));
+            .mock_safe(|_, _, _| MockResult::Return(Ok((Some(BtcAddress::random()), 103))));
 
         // return some arbitrary error
         ext::vault_registry::try_increase_to_be_issued_tokens::<Test>.mock_safe(|_, amount| {
             assert_eq!(amount, &wrapped(100));
             MockResult::Return(Err(TestError::IssueCompleted.into()))
         });
         ext::vault_registry::register_deposit_address::<Test>
-            .mock_safe(|_, _| MockResult::Return(Ok(BtcAddress::default())));
+            .mock_safe(|_, _| MockResult::Return(Ok(BtcAddress::random())));
 
         ext::vault_registry::is_vault_liquidated::<Test>.mock_safe(|_| MockResult::Return(Ok(false)));
 

diff --git a/crates/redeem/src/benchmarking.rs b/crates/redeem/src/benchmarking.rs
@@ -13,7 +13,7 @@ use frame_support::assert_ok;
 use frame_system::RawOrigin;
 use orml_traits::MultiCurrency;
 use primitives::{CurrencyId, CurrencyId::Token, TokenSymbol::*, VaultCurrencyPair, VaultId};
-use sp_core::{H160, H256, U256};
+use sp_core::{H256, U256};
 use sp_runtime::traits::One;
 use sp_std::prelude::*;
 use vault_registry::types::{Vault, Wallet};
@@ -96,7 +96,7 @@ fn mine_blocks<T: crate::Config>(end_height: u32) {
     let height = 0;
     let block = BlockBuilder::new()
         .with_version(4)
-        .with_coinbase(&BtcAddress::P2SH(H160::zero()), 50, 3)
+        .with_coinbase(&BtcAddress::random(), 50, 3)
         .with_timestamp(1588813835)
         .mine(U256::from(2).pow(254.into()))
         .unwrap();
@@ -130,7 +130,7 @@ fn mine_blocks<T: crate::Config>(end_height: u32) {
         let block = BlockBuilder::new()
             .with_previous_hash(prev_hash)
             .with_version(4)
-            .with_coinbase(&BtcAddress::P2SH(H160::zero()), 50, 3)
+            .with_coinbase(&BtcAddress::random(), 50, 3)
             .with_timestamp(1588813835)
             .add_transaction(transaction.clone())
             .mine(U256::from(2).pow(254.into()))
@@ -173,7 +173,7 @@ benchmarks! {
         let origin: T::AccountId = account("Origin", 0, 0);
         let vault_id = get_vault_id::<T>();
         let amount = Redeem::<T>::redeem_btc_dust_value() + 1000u32.into();
-        let btc_address = BtcAddress::P2SH(H160::from([0; 20]));
+        let btc_address = BtcAddress::random();
 
         initialize_oracle::<T>();
 
@@ -236,7 +236,7 @@ benchmarks! {
 
         initialize_oracle::<T>();
 
-        let origin_btc_address = BtcAddress::P2PKH(H160::zero());
+        let origin_btc_address = BtcAddress::random();
 
         let redeem_id = H256::zero();
         let mut redeem_request = test_request::<T>(&vault_id);

diff --git a/crates/redeem/src/lib.rs b/crates/redeem/src/lib.rs
@@ -358,6 +358,10 @@ impl<T: Config> Pallet<T> {
             Error::<T>::AmountExceedsUserBalance
         );
 
+        // We saw a user lose bitcoin when he forgot to enter the address in the polkadotjs ui.
+        // Make sure this can't happen again.
+        ensure!(!btc_address.is_zero(), btc_relay::Error::<T>::InvalidBtcHash);
+
         // todo: currently allowed to redeem from one currency to the other for free - decide if this is desirable
         let fee_wrapped = if redeemer == vault_id.account_id {
             Amount::zero(vault_id.wrapped_currency())

diff --git a/crates/redeem/src/tests.rs b/crates/redeem/src/tests.rs
@@ -72,7 +72,7 @@ fn test_request_redeem_fails_with_amount_exceeds_user_balance() {
         amount.mint_to(&USER).unwrap();
         let amount = 10_000_000;
         assert_err!(
-            Redeem::request_redeem(Origin::signed(USER), amount, BtcAddress::default(), VAULT),
+            Redeem::request_redeem(Origin::signed(USER), amount, BtcAddress::random(), VAULT),
             TestError::AmountExceedsUserBalance
         );
     })
@@ -120,7 +120,7 @@ fn test_request_redeem_fails_with_amount_below_minimum() {
 fn test_request_redeem_fails_with_vault_not_found() {
     run_test(|| {
         assert_err!(
-            Redeem::request_redeem(Origin::signed(USER), 1500, BtcAddress::default(), VAULT),
+            Redeem::request_redeem(Origin::signed(USER), 1500, BtcAddress::random(), VAULT),
             VaultRegistryError::VaultNotFound
         );
     })
@@ -133,7 +133,7 @@ fn test_request_redeem_fails_with_vault_banned() {
             .mock_safe(|_| MockResult::Return(Err(VaultRegistryError::VaultBanned.into())));
 
         assert_err!(
-            Redeem::request_redeem(Origin::signed(USER), 1500, BtcAddress::default(), VAULT),
+            Redeem::request_redeem(Origin::signed(USER), 1500, BtcAddress::random(), VAULT),
             VaultRegistryError::VaultBanned
         );
     })
@@ -174,6 +174,7 @@ fn test_request_redeem_succeeds_with_normal_redeem() {
         let redeemer = USER;
         let amount = 90;
         let redeem_fee = 5;
+        let btc_address = BtcAddress::random();
 
         ext::vault_registry::try_increase_to_be_redeemed_tokens::<Test>.mock_safe(move |vault_id, amount_btc| {
             assert_eq!(vault_id, &VAULT);
@@ -197,7 +198,7 @@ fn test_request_redeem_succeeds_with_normal_redeem() {
         assert_ok!(Redeem::request_redeem(
             Origin::signed(redeemer),
             amount,
-            BtcAddress::P2PKH(H160::zero()),
+            btc_address,
             VAULT
         ));
 
@@ -208,7 +209,7 @@ fn test_request_redeem_succeeds_with_normal_redeem() {
             fee: redeem_fee,
             premium: 0,
             vault_id: VAULT,
-            btc_address: BtcAddress::P2PKH(H160::zero()),
+            btc_address,
             transfer_fee: Redeem::get_current_inclusion_fee(DEFAULT_WRAPPED_CURRENCY)
                 .unwrap()
                 .amount()
@@ -223,7 +224,7 @@ fn test_request_redeem_succeeds_with_normal_redeem() {
                 amount_btc: amount - redeem_fee - btc_fee.amount(),
                 premium: 0,
                 redeemer,
-                btc_address: BtcAddress::P2PKH(H160::zero()),
+                btc_address,
                 btc_height: 0,
                 status: RedeemRequestStatus::Pending,
                 transfer_fee_btc: Redeem::get_current_inclusion_fee(DEFAULT_WRAPPED_CURRENCY)
@@ -234,6 +235,21 @@ fn test_request_redeem_succeeds_with_normal_redeem() {
     })
 }
 
+#[test]
+fn test_request_redeem_fails_with_default_btc_address() {
+    run_test(|| {
+        let redeemer = USER;
+        let amount = 90;
+
+        ext::treasury::get_balance::<Test>.mock_safe(move |_, _| MockResult::Return(wrapped(100)));
+
+        assert_err!(
+            Redeem::request_redeem(Origin::signed(redeemer), amount, BtcAddress::P2PKH(H160::zero()), VAULT),
+            btc_relay::Error::<Test>::InvalidBtcHash
+        );
+    })
+}
+
 #[test]
 fn test_request_redeem_succeeds_with_self_redeem() {
     run_test(|| {
@@ -257,6 +273,7 @@ fn test_request_redeem_succeeds_with_self_redeem() {
 
         let redeemer = VAULT.account_id;
         let amount = 90;
+        let btc_address = BtcAddress::random();
 
         ext::vault_registry::try_increase_to_be_redeemed_tokens::<Test>.mock_safe(move |vault_id, amount_btc| {
             assert_eq!(vault_id, &VAULT);
@@ -279,7 +296,7 @@ fn test_request_redeem_succeeds_with_self_redeem() {
         assert_ok!(Redeem::request_redeem(
             Origin::signed(redeemer),
             amount,
-            BtcAddress::P2PKH(H160::zero()),
+            btc_address,
             VAULT
         ));
 
@@ -290,7 +307,7 @@ fn test_request_redeem_succeeds_with_self_redeem() {
             fee: 0,
             premium: 0,
             vault_id: VAULT,
-            btc_address: BtcAddress::P2PKH(H160::zero()),
+            btc_address,
             transfer_fee: Redeem::get_current_inclusion_fee(DEFAULT_WRAPPED_CURRENCY)
                 .unwrap()
                 .amount()
@@ -305,7 +322,7 @@ fn test_request_redeem_succeeds_with_self_redeem() {
                 amount_btc: amount - btc_fee.amount(),
                 premium: 0,
                 redeemer,
-                btc_address: BtcAddress::P2PKH(H160::zero()),
+                btc_address,
                 btc_height: 0,
                 status: RedeemRequestStatus::Pending,
                 transfer_fee_btc: Redeem::get_current_inclusion_fee(DEFAULT_WRAPPED_CURRENCY)

diff --git a/crates/refund/src/benchmarking.rs b/crates/refund/src/benchmarking.rs
@@ -12,7 +12,7 @@ use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite};
 use frame_support::assert_ok;
 use frame_system::RawOrigin;
 use primitives::VaultId;
-use sp_core::{H160, H256, U256};
+use sp_core::{H256, U256};
 use sp_runtime::traits::One;
 use sp_std::prelude::*;
 use vault_registry::types::Vault;
@@ -43,7 +43,7 @@ benchmarks! {
         );
         let relayer_id: T::AccountId = account("Relayer", 0, 0);
 
-        let origin_btc_address = BtcAddress::P2PKH(H160::zero());
+        let origin_btc_address = BtcAddress::random();
 
         let refund_id = H256::zero();
         let refund_request = RefundRequest {

diff --git a/crates/refund/src/tests.rs b/crates/refund/src/tests.rs
@@ -5,7 +5,7 @@ use currency::Amount;
 use frame_support::{assert_err, assert_ok};
 use mocktopus::mocking::*;
 use primitives::refund::RefundRequest;
-use sp_core::{H160, H256};
+use sp_core::H256;
 
 fn dummy_merkle_proof() -> MerkleProof {
     MerkleProof {
@@ -36,7 +36,7 @@ fn test_refund_succeeds() {
             &wrapped(1000),
             VAULT,
             USER,
-            BtcAddress::P2SH(H160::zero()),
+            BtcAddress::random(),
             issue_id
         ));