fix(hd-wallet): enable/withdraw using any account'/change/address_index #1933

shamardy · 2023-08-04T22:46:45Z

Intermediate PR part of #1838

Global enabling of an account'/change/address_index path for all coins using hd_account_id config parameter is replaced by enable_hd which is a bool that defaults to false. I couldn't remove this parameter all together and make enabling of Iguana or HD mode at the coin level since the Iguana seed phrases which is passed to config at initiation will not always be BIP39 compliant which will result in HD context to fail in initializing. I made enable_hd default to false to not break backward compatibility. c.c. @KomodoPlatform/qa
HD withdrawal from any account'/change/address_index path is implemented for UTXO, EVM and Tendermint coins for now, other coins will be added in next PRs.
path_to_address parameter is added to coins activation requests to set the default account'/change/address_index path that will be used for swaps. If not provided, the default will be 0'/0/0.
from parameter in withdraw can be used to withdraw from any account'/change/address_index using
```
"from": {
    "account": integer,
    "is_change": bool,
    "address_index": integer
}
```
no need to document any of these changes for now since these parameters can be changed as this is is only an
Intermediate PR. For instance from.address_id that is used in HW wallets can be used instead since I found they are
the same, c.c. @KomodoPlatform/qa
Review process notes:
- @rozhkovdmitrii Please review the adex-cli changes related to replacing hd_account_id with enable_hd, activation scheme data doesn't support HD wallet, I should update how HD wallet enabling work with adex-cli when all the HD fixes are done in next PRs, as activation scheme data doesn't support HD wallet and will not be able to support it with the current changes.
- GUI authentication for EVM uses the activated key pair only (the ones used for swaps), I understand that this is not a problem for withdrawing using another keypair but I wanted to make sure that is the case @ozkanonur.
- @laruh please review changes to NFT code.
- @ozkanonur please review changes related to Tendermint.
- @artemii235 please review changes related to zcoin.
- @dimxy I added you to this review since it intersects with HW wallet task and signing RPC task too.
Todo in next PRs:
- Tests for Tendermint token withdraws.
- Withdraw from any account'/change/address_index for ARRR, NFTs, SLP and QRC20.
- Opening lightning channels from any account'/change/address_index and also Solana support can be postponed until these features are planned.
- Transaction history support.
- Displaying HD addresses and balances using account_balance RPC.
- Other SoW items.

… instead of hd_account_id in adex-cli,

…Policy for reusable code

…thdraw tests

ca333 · 2023-08-18T08:47:50Z

mm2src/crypto/src/global_hd_ctx.rs

+    drop_mutability!(priv_key);
+
+    let secret = *priv_key.private_key().as_ref();
+    Ok(Secp256k1Secret::from(secret))


we shouldn't store sensitive data longer in memory than needed. could use e.g. https://crates.io/crates/zeroize or similar approach. We ult. need the key only in case of signing requirement. Will open separate issue ticket since this affects multiple layers - but figured, maybe we want to start here / with HD withdrawl layer, since critical layer?

EDIT: given workload, lets keep on radar / handle in one of the next iterations.

could use e.g. https://crates.io/crates/zeroize or similar approach

Thanks for the suggestion. Using zeroize looks like a good approach to zeroing the memory region after the variable is dropped, we should also consider disabling swapping for this memory region to avoid the OS saving secrets to disk unintentionally. Please note that the master seed is kept in memory throughout the application runtime, we should probably add the option to keep it encrypted only in memory and decrypt it using the RPC password (the rpc password is also kept in memory, so maybe we should keep the hash of the password+salt instead to verify the password on each call) as part of this separate issue.

drop itself is enough for this purpose

For certain values, we have to keep them in memory constantly to operate mm2 without always asking users. Which might not be good for GUI clients and bots. To protect against memory attacks, we can change the memory addresses dynamically while the program is running so we don't have the important values on the same address all the time while the program is running. However, we should also make sure passphrases(and other informations) are safe, as they're stored in a plain text file right know (MM_CONF_PATH). If someone gets into the mm2 device, they can quickly get the passphrases from the text file without needing doing complicated hardware attack / reverse engineering stuff.

We talked about this topic in a public Mattermost channel with @gcharang. We sort of figured out a quite good solution for the issue, but I can't recall all the details since our discussion was very detailed and it was months ago.

Yes, I know that komodod use MemoryPageLocker object for locking privkeys in memory, I guess, to prevent sending virtual pages with sensitive data to swap file. I wonder if we have the same feature in rust

Yes, I know that komodod use MemoryPageLocker object for locking privkeys in memory, I guess, to prevent sending virtual pages with sensitive data to swap file. I wonder if we have the same feature in rust

We can use mlock from libc, I don't know if there is any better way for that.

seems this would be platform-dependant as in komodod it is implemented like

bool MemoryPageLocker::Lock(const void* addr, size_t len) { #ifdef _WIN32 return VirtualLock(const_cast<void*>(addr), len) != 0; #else return mlock(addr, len) == 0; #endif }

Thanks for your input @dimxy @ozkanonur, I will look into it once the issue is opened and planned. We can discuss the details more then :)

Yes, we need to use winapi for doing this on Windows as libc doesn't support windows. Both winapi and libc are already present in our dependency tree, so we can do it easily.

ca333

secure code reviewed

feedback given on key handling / secure mem zeroing

should additionally ensure we don't accidentally create multiple mem copies of "sensitive" data due to (de)serialization and other relevant data ops.

smk762 · 2023-08-28T12:29:11Z

Can we please get an example of the new method structures (or a link to the same in code)?
I see this change touches a few places, so can you please confirm / add to the below so we can make sure we cover everything required.
- Tendermint activate / withdraw
- NFT activate / withdraw
- Trezor activate / withdraw
- BIP39 HD without trezor activate / withdraw
- zhtlc activate / withdraw
- evm activate withdraw
- utxo activate / withdraw
- Or just fix(hd-wallet): enable/withdraw using any account'/change/address_index #1933 (comment) ?
Are all tests ready for testing in wasm, or some are some ready in in CLI only?
Global enabling of an "account'/change/address_index" path for all coins using "hd_account_id" config parameter is replaced by "enable_hd" which is a bool that defaults to false.
- So enable_hd goes in MM2.json instead of hd_account_id?

shamardy · 2023-08-29T04:18:48Z

So enable_hd goes in MM2.json instead of hd_account_id?

Yes, it's better this way since we will not be tied to an address path (the account'/change/address_index part) for all coins when initializing mm2.

I see this change touches a few places, so can you please confirm / add to the below so we can make sure we cover everything required.
Tendermint activate / withdraw
NFT activate / withdraw
Trezor activate / withdraw
BIP39 HD without trezor activate / withdraw
zhtlc activate / withdraw
evm activate withdraw
utxo activate / withdraw
Or just #1933 (comment) ?

This is the most important #1933 (comment) as others can change. Everything else can change to make HD wallet feature better, but I will provide examples for everything else below:

Coins Activation:

path_to_address parameter is added to coins activation requests to set the default account'/change/address_index path that will be used for swaps. If not provided, the default will be 0'/0/0.
The below examples are for 7'/0/77 path, we can change the path_to_address as we want and enable different coins using different path_to_address which was the main purpose of this PR.
We can also disable a coin and re-enable it with different path_to_address, if a coin has ongoing swaps or matching orders it can't be disabled as before and if no matching orders, orders placed will be cancelled so it's ok to disable and enable with different path_to_address regarding swaps/orders.
The only problem is activating a coin after restarting mm2 with a different path_to_address than the last used one before stopping mm2 as swaps that were not finished will fail or worse things can happen. This problem will be solved in next PRs but if required, I can do a quick fix in this PR. GUIs can save the last used path_to_address for now and re-enable with it after restarting, but this is not the optimal solution as a seamless solution using API should be provided.

UTXO electrum:

{
    "userpass": "{{userpass}}",
    "method": "electrum",
    "coin": "RICK",
    "servers": [
        {
            "url": "electrum1.cipig.net:10017"
        },
        {
            "url": "electrum2.cipig.net:10017"
        },
        {
            "url": "electrum3.cipig.net:10017"
        }
    ],
    "path_to_address": {
        "account": 7,
        "is_change": false,
        "address_index": 77
    }
}

UTXO v2 (`task::enable_utxo::init`):

path_to_address is added to UtxoActivationParams

{
    "userpass": "{{userpass}}",
    "mmrpc": "2.0",
    "method": "task::enable_utxo::init",
    "params": {
        "ticker": "RICK",
        "activation_params": {
            "mode": {
                "rpc": "Electrum",
                "rpc_data": {
                    "servers": [
                        {
                            "url": "electrum1.cipig.net:10017"
                        },
                        {
                            "url": "electrum2.cipig.net:10017"
                        },
                        {
                            "url": "electrum3.cipig.net:10017"
                        }
                    ]
                }
            },
            "path_to_address": {
                "account": 7,
                "is_change": false,
                "address_index": 77
            }
        }
    }
}

EVM enable:

{
    "userpass": "{{userpass}}",
    "method": "enable",
    "coin": "ETH",
    "urls": [
        "http://eth1.cipig.net:8555",
        "http://eth2.cipig.net:8555",
        "http://eth3.cipig.net:8555"
    ],
    "swap_contract_address": "0x24ABE4c71FC658C91313b6552cd40cD808b3Ea80",
    "path_to_address": {
        "account": 7,
        "is_change": false,
        "address_index": 77
    }
}

EVM v2 (`enable_eth_with_tokens`/`enable_erc20`):

enable_erc20 is the same since it uses platform coin address and platform coin has to be activated for it to be used.

{
    "userpass": "{{userpass}}",
    "method": "enable_eth_with_tokens",
    "mmrpc": "2.0",
    "params": {
        "ticker": "ETH",
        "swap_contract_address": "0x24ABE4c71FC658C91313b6552cd40cD808b3Ea80",
        "fallback_swap_contract": "0x8500AFc0bc5214728082163326C2FF0C73f4a871",
        "nodes": [
            {
                "url": "http://eth1.cipig.net:8555",
                "gui_auth": false
            },
            {
                "url": "http://eth2.cipig.net:8555",
                "gui_auth": false
            },
            {
                "url": "http://eth3.cipig.net:8555",
                "gui_auth": false
            },
            {
                "url": "https://node.komodo.earth:8080/ethereum",
                "gui_auth": true
            }
        ],
        "erc20_tokens_requests": [
            {
                "ticker": "BUSD-ERC20"
            }
        ],
        "path_to_address": {
            "account": 7,
            "is_change": false,
            "address_index": 77
        }
    }
}

This comment is getting quite large so I will continue in separate comments :)

shamardy · 2023-08-29T04:25:44Z

Tendermint Activation (`enable_tendermint_with_assets`, `enable_tendermint_token`):

For enable_tendermint_token it's the same as enable_erc20 since it also uses platform coin address and platform coin has to be activated for it to be used.

{
    "userpass": "{{userpass}}",
    "method": "enable_tendermint_with_assets",
    "mmrpc": "2.0",
    "params": {
        "ticker": "IRIS",
        "tokens_params": [
            {
                "ticker": "ATOM-IBC_IRIS"
            }
        ],
        "rpc_urls": [
            "https://iris.komodo.earth/",
            "https://rpc.irishub-1.irisnet.org"
        ],
        "path_to_address": {
            "account": 7,
            "is_change": false,
            "address_index": 77
        }
    }
}

shamardy · 2023-08-29T04:55:50Z

`zhtlc`/`zcoin` Activation (`enable_z_coin::init`):

zcoin doesn't use path_to_address but uses only account field. ref #1933 (comment) we don't know yet if we will support multiple addresses per account or not in the future as I will open this for discussion when the time comes.

{
    "userpass": "{{userpass}}",
    "mmrpc": "2.0",
    "method": "task::enable_z_coin::init",
    "params": {
        "ticker": "ZOMBIE",
        "activation_params": {
            "mode": {
                "rpc": "Light",
                "rpc_data": {
                    "electrum_servers": [
                        {
                            "url": "zombie.sirseven.me:10033"
                        }
                    ],
                    "light_wallet_d_servers": [
                        "http://zombie.sirseven.me:443"
                    ]
                }
            },
            "account": 7
        }
    }
}

shamardy · 2023-08-29T05:01:29Z

NFT activate / withdraw:

If mm2 is started with enable_hd:true, NFT will use the default 0'/0/0 path for the activated address. HD wallet as in multiple accounts and addresses is not supported yet for NFT. It should be supported in future PRs.

Trezor activate / withdraw

This PR doesn't affect Trezor implementation at all and it should work as it has been before.

BIP39 HD without trezor activate / withdraw

@smk762 what do you mean with this? to use enable_hd:true, the passphrase has to be BIP39 compatible otherwise an error will be returned and mm2 will not be initialized.

shamardy · 2023-08-29T05:14:02Z

Withdraw methods (`withdraw` v1, `withdraw` v2, `ibc_withdraw`):

for the above methods from parameter can be used to withdraw from any account'/change/address_index using

"from": {
    "account": integer,
    "is_change": bool,
    "address_index": integer
}

This only works for UTXO,EVM and Tendermint for now, example for UTXO withdraw:

{
    "userpass": "{{userpass}}",
    "mmrpc": "2.0",
    "method": "withdraw",
    "params": {
        "coin": "RICK",
        "to": "RNBA756iaFCx2Uhya3pvCufbeyovAaknJL",
        "amount": 1.025,
        "from": {
            "account": 3,
            "is_change": 0,
            "address_index": 6
        }
    }
}

Unfortunately, there is no current way to see balances and addresses for an account to make sure withdraw worked. I used this https://iancoleman.io/bip39/ for addresses and checked chain explorers to make sure it worked fine. Account addresses and balances methods should be provided in the next PR but it might require some changes in activation and withdraw methods too.

shamardy · 2023-08-29T05:17:22Z

Zcoin withdraw:

We can't withdraw from any account for now, it will be added in next PRs too. Only enabled account with coin activation is used using task::withdraw::init.

Are all tests ready for testing in wasm, or some are some ready in in CLI only?

All are working for wasm except zcoin since it's not supported in wasm yet.

smk762 · 2023-08-30T12:54:46Z

Thanks for the examples and clarifications

BIP39 HD without trezor activate / withdraw

@smk762 what do you mean with this? to use enable_hd:true, the passphrase has to be BIP39 compatible otherwise an error will be returned and mm2 will not be initialized.

Basically meaning to test the activation / withdraw without using a hardware wallet

shamardy · 2023-08-30T12:57:37Z

Basically meaning to test the activation / withdraw without using a hardware wallet

Yeah, this whole PR is for this. Not related to hardware wallet at all.

smk762

LGTM, thanks.
The only issues errors encountered are TODO in subsequent PRs, so approving this one.

For reference, these were the issues I encountered:

unsure of derivation paths for zhtlc, and the ones attempted returned an address which would not receive funds sent from a legacy desktop address.
SLP activates, but there was an infra issue with withdrawals unrelated to this PR "Internal error: Error PayloadTooShort on request to the url https://bchd.dragonhound.info/pb.bchrpc/GetSlpTrustedValidation" I will check the node.
Tendermint activates, but withdraw not tested as docs for [r2r] cosmos ibc transfer implementation #1636 have not yet been drafted. I will revisit these tests when drafting the docs and/or future related PRs.

shamardy · 2023-08-31T13:44:06Z

Thank you for the testing @smk762, I added this comment to the issue checklist #1838 (comment) to not forget about them :)

shamardy added 21 commits July 19, 2023 06:00

choose path from account to address when enabling a coin in hd mode

8963ff3

fix eth_coin_from_conf_and_request path_to_address

6dfe271

Merge remote-tracking branch 'origin/dev' into fix-hd-acc-activation

9e5d92e

add more test cases for /account'/change/address_index, use enable_hd…

8ac79d7

… instead of hd_account_id in adex-cli,

remove unneeded todos after checking them

5581805

wip: withdraw from any hd /account'/change/address_index for utxo done

abaf8a5

test withdraw from any hd address for segwit

77f32b1

withdraw from any hd /account'/change/address_index for EVM

b66cf69

fix wasm-lint

40b5baa

fix wasm build

28e0fc4

wip: withdraw from any hd /account'/change/address_index for tendermint

e94dcb5

refactor: add bip39_secp_priv_key_or_err where it was missing and use it

0db8ba4

refactor: make derivation_path part of priv_key_policy

7a95512

refactor: use a common PrivKeyPolicy enum for all coins

b7d4e44

refactor: create hd_wallet_derived_priv_key_or_err method for PrivKey…

5143cfc

…Policy for reusable code

refactor: remove duplicated tendermint methods that I created

80a0b6d

remove todos unrelated to this PR

53d5e74

Merge remote-tracking branch 'origin/dev' into fix-hd-acc-activation

9265cfa

HD withdraw for tendermint token and IBC, add some tendermint coin wi…

8016af1

…thdraw tests

refactor: remove path_to_address from UtxoCoinConf

afd268e

refactor: StandardUtxoWithdraw::new()

b6a465b

shamardy added the in progress Changes will be made from the author label Aug 4, 2023

shamardy self-assigned this Aug 4, 2023

shamardy added 1.0.7-beta under review and removed in progress Changes will be made from the author labels Aug 4, 2023

shamardy requested review from rozhkovdmitrii, artemii235, dimxy and onur-ozkan August 7, 2023 03:24

shamardy requested review from ca333, DeckerSU and Alrighttt August 17, 2023 10:27

ca333 reviewed Aug 18, 2023

View reviewed changes

ca333 previously approved these changes Aug 18, 2023

View reviewed changes

smk762 requested a review from kivqa August 21, 2023 05:56

smk762 added the QA label Aug 21, 2023

shamardy removed testing sec-review no-deps labels Aug 21, 2023

Merge remote-tracking branch 'origin/dev' into fix-hd-acc-activation

afe2e08

shamardy dismissed ca333’s stale review via afe2e08 August 23, 2023 12:08

smk762 mentioned this pull request Aug 31, 2023

Removes obsolete zhtlc fields, adds zhtlc derivation paths KomodoPlatform/coins#809

Draft

smk762 approved these changes Aug 31, 2023

View reviewed changes

shamardy merged commit 51c44f6 into dev Aug 31, 2023
21 of 30 checks passed

shamardy deleted the fix-hd-acc-activation branch August 31, 2023 13:45

shamardy mentioned this pull request Sep 7, 2023

chore(release): v1.0.7-beta #1936

Merged

shamardy mentioned this pull request Oct 4, 2023

feat(hd_wallet): evm hd wallet and trezor #1979

Merged

8 tasks

shamardy mentioned this pull request Nov 28, 2023

feat(crypto): mnemonic generation/encryption/decryption/storage #2014

Merged

17 tasks

shamardy mentioned this pull request Apr 8, 2024

Seed generation, encryption and storage support #1939

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(hd-wallet): enable/withdraw using any account'/change/address_index #1933

fix(hd-wallet): enable/withdraw using any account'/change/address_index #1933

shamardy commented Aug 4, 2023 •

edited

Loading

ca333 Aug 18, 2023 •

edited

Loading

shamardy Aug 21, 2023

onur-ozkan Aug 21, 2023

onur-ozkan Aug 21, 2023 •

edited

Loading

dimxy Aug 21, 2023

onur-ozkan Aug 21, 2023

dimxy Aug 21, 2023

shamardy Aug 21, 2023

onur-ozkan Aug 21, 2023

ca333 left a comment

smk762 commented Aug 28, 2023

shamardy commented Aug 29, 2023 •

edited

Loading

shamardy commented Aug 29, 2023

shamardy commented Aug 29, 2023

shamardy commented Aug 29, 2023

shamardy commented Aug 29, 2023

shamardy commented Aug 29, 2023

smk762 commented Aug 30, 2023

BIP39 HD without trezor activate / withdraw

shamardy commented Aug 30, 2023

smk762 left a comment

shamardy commented Aug 31, 2023

fix(hd-wallet): enable/withdraw using any account'/change/address_index #1933

fix(hd-wallet): enable/withdraw using any account'/change/address_index #1933

Conversation

shamardy commented Aug 4, 2023 • edited Loading

ca333 Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

shamardy Aug 21, 2023

Choose a reason for hiding this comment

onur-ozkan Aug 21, 2023

Choose a reason for hiding this comment

onur-ozkan Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

dimxy Aug 21, 2023

Choose a reason for hiding this comment

onur-ozkan Aug 21, 2023

Choose a reason for hiding this comment

dimxy Aug 21, 2023

Choose a reason for hiding this comment

shamardy Aug 21, 2023

Choose a reason for hiding this comment

onur-ozkan Aug 21, 2023

Choose a reason for hiding this comment

ca333 left a comment

Choose a reason for hiding this comment

smk762 commented Aug 28, 2023

shamardy commented Aug 29, 2023 • edited Loading

Coins Activation:

UTXO electrum:

UTXO v2 (task::enable_utxo::init):

EVM enable:

EVM v2 (enable_eth_with_tokens/enable_erc20):

shamardy commented Aug 29, 2023

Tendermint Activation (enable_tendermint_with_assets, enable_tendermint_token):

shamardy commented Aug 29, 2023

zhtlc/zcoin Activation (enable_z_coin::init):

shamardy commented Aug 29, 2023

NFT activate / withdraw:

Trezor activate / withdraw

BIP39 HD without trezor activate / withdraw

shamardy commented Aug 29, 2023

Withdraw methods (withdraw v1, withdraw v2, ibc_withdraw):

shamardy commented Aug 29, 2023

Zcoin withdraw:

smk762 commented Aug 30, 2023

BIP39 HD without trezor activate / withdraw

shamardy commented Aug 30, 2023

smk762 left a comment

Choose a reason for hiding this comment

shamardy commented Aug 31, 2023

shamardy commented Aug 4, 2023 •

edited

Loading

ca333 Aug 18, 2023 •

edited

Loading

onur-ozkan Aug 21, 2023 •

edited

Loading

shamardy commented Aug 29, 2023 •

edited

Loading

UTXO v2 (`task::enable_utxo::init`):

EVM v2 (`enable_eth_with_tokens`/`enable_erc20`):

Tendermint Activation (`enable_tendermint_with_assets`, `enable_tendermint_token`):

`zhtlc`/`zcoin` Activation (`enable_z_coin::init`):

Withdraw methods (`withdraw` v1, `withdraw` v2, `ibc_withdraw`):