[r2r] validate swap parameters #1475

borngraced · 2022-09-18T22:09:17Z

Validated maker_coin_htlc_pub and taker_coin_htlc_pub on Maker side
Validated maker_coin_htlc_pub and taker_coin_htlc_pub and secret_hash on Taker side

fixes: #1458

sergeyboyko0791

Good job!
Please fix a few moments

sergeyboyko0791 · 2022-09-23T11:06:20Z

mm2src/coins/eth.rs

+        }
+    }
+
+    fn validate_secret_hash(&self, secret_hash: &[u8], secret: &[u8]) -> bool { secret_hash == secret }


It can be simply done at the maker_swap.rs and taker_swap.rs modules

sergeyboyko0791 · 2022-09-23T11:09:03Z

mm2src/coins/eth/eth_tests.rs

@@ -1353,3 +1353,172 @@ fn test_sign_verify_message() {
        .unwrap();
    assert!(is_valid);
 }
+
+#[test]
+fn test_eth_extract_secret() {


Is this test related to validation of the other pubkey?

as there was not any test for extract_secret in eth module I decided to add one..should I remove?

No, let's keep it, thanks!

sergeyboyko0791 · 2022-09-23T11:09:23Z

mm2src/coins/eth/eth_tests.rs

+}
+
+#[test]
+fn test_eth_validate_valid_and_invalid_secret_hash() {


So this test can be removed :)

sergeyboyko0791 · 2022-09-23T11:10:14Z

mm2src/coins/qrc20/qrc20_tests.rs

+}
+
+#[test]
+fn test_qrc20_validate_valid_and_invalid_secret_hash() {


This is also can be removed

sergeyboyko0791 · 2022-09-23T11:13:58Z

mm2src/mm2_main/src/lp_swap/maker_swap.rs

+        // Validate maker_coin_htlc_pubkey realness
+        match self.taker_coin.validate_other_pubkey(taker_data.maker_coin_htlc_pub()) {
+            Ok(_) => (),
+            Err(err) => {


Please consider using if let Err(err) = ... statement here

sergeyboyko0791 · 2022-09-23T11:15:42Z

mm2src/mm2_main/src/lp_swap/maker_swap.rs

+        };
+
+        // Validate taker_coin_htlc_pubkey realness
+        match self.taker_coin.validate_other_pubkey(taker_data.taker_coin_htlc_pub()) {


Here's correct, our taker_coin should validate the other's taker_coin_htlc_pub.

sergeyboyko0791 · 2022-09-23T11:16:59Z

mm2src/mm2_main/src/lp_swap/maker_swap.rs

+        // Validate taker_coin_htlc_pubkey realness
+        match self.taker_coin.validate_other_pubkey(taker_data.taker_coin_htlc_pub()) {
+            Ok(_) => (),
+            Err(err) => {


Here the same please:

Please consider using if let Err(err) = ... statement here

sergeyboyko0791 · 2022-09-23T11:24:30Z

mm2src/mm2_main/src/lp_swap/taker_swap.rs

@@ -963,6 +963,15 @@ impl TakerSwap {
            )]));
        }

+        if !self
+            .maker_coin
+            .validate_secret_hash(maker_data.secret_hash(), self.r().secret.0.as_slice())


As I suggested above, we can simply check if maker_data.secret_hash() == self.r().secret.0

sergeyboyko0791 · 2022-09-23T11:25:03Z

mm2src/mm2_main/src/lp_swap/taker_swap.rs

+        match self.taker_coin.validate_other_pubkey(maker_data.maker_coin_htlc_pub()) {
+            Ok(_) => (),
+            Err(err) => {
+                return Ok((Some(TakerSwapCommand::Finish), vec![TakerSwapEvent::NegotiateFailed(
+                    ERRL!("!maker_data.maker_coin_htlc_pub {}", err).into(),
+                )]))
+            },
+        };
+
+        // Validate taker_coin_htlc_pubkey realness
+        match self.taker_coin.validate_other_pubkey(maker_data.taker_coin_htlc_pub()) {
+            Ok(_) => (),
+            Err(err) => {
+                return Ok((Some(TakerSwapCommand::Finish), vec![TakerSwapEvent::NegotiateFailed(
+                    ERRL!("!maker_data.taker_coin_htlc_pub {}", err).into(),
+                )]))
+            },
+        };


Same please:

Please consider using if let Err(err) = ... statement here

sergeyboyko0791 · 2022-09-23T11:25:33Z

mm2src/mm2_main/src/lp_swap/taker_swap.rs

@@ -986,6 +995,25 @@ impl TakerSwap {
                )]))
            },
        };
+        // Validate maker_coin_htlc_pubkey realness
+        match self.taker_coin.validate_other_pubkey(maker_data.maker_coin_htlc_pub()) {


maker_coin should validate maker_coin_htlc_pub.
taker_coin should validate taker_coin_htlc_pub.

…alidate_swap_parameters

* enable retry if tx is not available onchain * fixed review notes * fixed failing tests * pr review fixes * set test timeout to 1

sergeyboyko0791

Great work! Only two comments

mm2src/coins/utxo/utxo_tests.rs

sergeyboyko0791 · 2022-10-04T09:34:10Z

mm2src/coins/qrc20/qrc20_tests.rs

@@ -1061,3 +1061,16 @@ fn test_send_contract_calls_recoverable_tx() {
        discriminant(&TransactionErr::TxRecoverable(TransactionEnum::from(tx), String::new()))
    );
 }
+
+#[test]
+fn test_qrc20_validate_valid_and_invalid_pubkey() {


Since Qrc20Coin uses utxo_common::validate_other_pubkey, we can remove this test.

sergeyboyko0791

LGTM!

sergeyboyko0791 · 2022-10-05T11:52:47Z

@shamardy could you please review?

artemii235

I also took a quick look at it and have one question.

artemii235 · 2022-10-06T15:15:42Z

mm2src/coins/eth/eth_tests.rs

+        erc20_tokens_infos: Default::default(),
+    }));
+
+    // raw transaction bytes of https://ropsten.etherscan.io/tx/0xe18bbca69dea9a4624e1f5b0b2021d5fe4c8daa03f36084a8ba011b08e5cd938


The mentioned transaction calls erc20Payment of swap contract, so extract_secret should not be applicable to it. Is there a mistake in the comment?

Nop, the comment is not a mistake. I actually made use of the TX to generate the secret used for the test. I didn't know extract_secret isn't compatible with swap contract. Will make changes, thanks

@artemii235 why do you think extract_secret isn't compatible with swap contract tx?

implementation of extract_secret for eth..

fn extract_secret(&self, _secret_hash: &[u8], spend_tx: &[u8]) -> Result<Vec<u8>, String> { let unverified: UnverifiedTransaction = try_s!(rlp::decode(spend_tx)); let function = try_s!(SWAP_CONTRACT.function("receiverSpend")); let tokens = try_s!(function.decode_input(&unverified.data)); if tokens.len() < 3 { return ERR!("Invalid arguments in 'receiverSpend' call: {:?}", tokens); } match &tokens[2] { Token::FixedBytes(secret) => Ok(secret.to_vec()), _ => ERR!( "Expected secret to be fixed bytes, decoded function data is {:?}", tokens ), } }

The mentioned transaction https://ropsten.etherscan.io/tx/0xe18bbca69dea9a4624e1f5b0b2021d5fe4c8daa03f36084a8ba011b08e5cd938 calls erc20Payment smart contract function.

While extract_secret should be called on the transaction calling receiverSpend (e.g. https://ropsten.etherscan.io/tx/0x432935be34424a0aa0403061652be8522da5b8637a6e499325ea910c430cd97d) - the name is actually hard-coded in extract_secret. It shouldn't have worked at all, but it somehow decodes the function input bytes and returns the token address padded with 12 zero bytes. There should be a bug in decode_input function - I added it in my fork of ethabi crate.

cc @sergeyboyko0791

Oh great, thanks 👍

What's interesting: even the most recent official ethabi release doesn't seem to perform any validation that the input is actually from the original fn: https://github.com/rust-ethereum/ethabi/blob/6f18e11621d29b6cd978ef39b48fa572b823bb80/ethabi/src/function.rs#L78. The code expects input without 4 bytes of short signature (partial hash of the function name). So we have to implement additional validation in our code. #1494

borngraced · 2022-10-07T13:23:19Z

@artemii235, I have made changes to this PR. You can now review again. thanks

artemii235

Great work, thanks!

sergeyboyko0791

Re-approve!

sergeyboyko0791 · 2022-10-10T10:49:33Z

@borngraced I'm currently checking how to fix ETH tests. Will let you know when I get an answer.

sergeyboyko0791 · 2022-10-10T17:21:36Z

@borngraced let's wait for #1495 is merged

borngraced added 3 commits September 16, 2022 16:55

pubkey and secret validation done

2239329

Validate taker and maker coin contract

ca04bfd

impl scret_hash validation

967ed54

borngraced requested review from shamardy and sergeyboyko0791 September 18, 2022 22:09

borngraced self-assigned this Sep 18, 2022

borngraced added 5 commits September 19, 2022 00:24

optimized swap_contract_addre validation

e99754c

unimplement negotiate_coin_contract_address for slp.rs

5b60cb4

removed coin contract addr validation — Postponed

05faf8b

added unit test for parameters

bd8b162

remove unused

39a69ea

borngraced changed the title ~~[wip] validate swap parameters~~ [r2r] validate swap parameters Sep 20, 2022

borngraced added 2 commits September 20, 2022 15:38

Merge remote-tracking branch 'origin/dev' into validate_swap_parameters

28e878c

Merge remote-tracking branch 'origin/dev' into validate_swap_parameters

9d698c3

sergeyboyko0791 suggested changes Sep 23, 2022

View reviewed changes

borngraced added 2 commits September 23, 2022 16:07

PR review fixes

a65d348

removed validate_secret_hash method from trait

3bea7a1

borngraced force-pushed the validate_swap_parameters branch from 45a562f to 3bea7a1 Compare September 26, 2022 15:32

borngraced added 4 commits September 26, 2022 16:36

removed validate_secret_hash method from trait

f972bb8

Merge remote-tracking branch 'origin/validate_swap_parameters' into v…

b883e1a

…alidate_swap_parameters

merge dev branch

703c646

Merge remote-tracking branch 'origin/validate_swap_parameters' into v…

b2c2fef

…alidate_swap_parameters

borngraced changed the title ~~[r2r] validate swap parameters~~ [wip] validate swap parameters Sep 27, 2022

borngraced force-pushed the validate_swap_parameters branch 2 times, most recently from fbba2b7 to b2c2fef Compare September 30, 2022 11:46

borngraced added 3 commits September 30, 2022 13:38

[r2r] Retry tx wait confirmation if not on chain (#1474)

4858ac9

* enable retry if tx is not available onchain * fixed review notes * fixed failing tests * pr review fixes * set test timeout to 1

Merge branch 'dev' into validate_swap_parameters

007c9bf

minor fixes

f98bde3

borngraced changed the title ~~[wip] validate swap parameters~~ [r2r] validate swap parameters Oct 3, 2022

fix tests

0f4c5bc

borngraced added 2 commits October 4, 2022 05:09

fixed secret_hash check

42fdbf3

cargo fmt

d3d8877

sergeyboyko0791 reviewed Oct 4, 2022

View reviewed changes

borngraced added 2 commits October 4, 2022 11:57

pr riview fix

68cd674

use real pubkey for test case

c9082ff

sergeyboyko0791 previously approved these changes Oct 5, 2022

View reviewed changes

artemii235 reviewed Oct 6, 2022

View reviewed changes

use correct raw tx bytes for extract_secret test

a610f57

borngraced dismissed sergeyboyko0791’s stale review via a610f57 October 7, 2022 13:22

artemii235 mentioned this pull request Oct 7, 2022

Check that input transaction is calling ReceiverSpend in eth::extract_secret. #1494

Closed

artemii235 approved these changes Oct 7, 2022

View reviewed changes

sergeyboyko0791 approved these changes Oct 10, 2022

View reviewed changes

borngraced added 2 commits October 11, 2022 09:12

Merge remote-tracking branch 'origin/dev' into validate_swap_parameters

af26a29

Merge remote-tracking branch 'origin/dev' into validate_swap_parameters

ca4ec9c

sergeyboyko0791 merged commit 8e92b9d into dev Oct 11, 2022

sergeyboyko0791 deleted the validate_swap_parameters branch October 11, 2022 10:58

borngraced mentioned this pull request Feb 17, 2023

[release] 1.0.0-beta #1576

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[r2r] validate swap parameters #1475

[r2r] validate swap parameters #1475

borngraced commented Sep 18, 2022

sergeyboyko0791 left a comment

sergeyboyko0791 Sep 23, 2022

sergeyboyko0791 Sep 23, 2022

borngraced Sep 23, 2022 •

edited

Loading

sergeyboyko0791 Sep 23, 2022

sergeyboyko0791 Sep 23, 2022

sergeyboyko0791 Sep 23, 2022

sergeyboyko0791 Sep 23, 2022

borngraced Sep 23, 2022

sergeyboyko0791 Sep 23, 2022

sergeyboyko0791 Sep 23, 2022

sergeyboyko0791 Sep 23, 2022

borngraced Sep 23, 2022

sergeyboyko0791 Sep 23, 2022

sergeyboyko0791 Sep 23, 2022

borngraced Sep 23, 2022

sergeyboyko0791 left a comment

sergeyboyko0791 Oct 4, 2022

borngraced Oct 4, 2022

sergeyboyko0791 left a comment

sergeyboyko0791 commented Oct 5, 2022

artemii235 left a comment

artemii235 Oct 6, 2022

borngraced Oct 6, 2022 •

edited

Loading

borngraced Oct 6, 2022

artemii235 Oct 7, 2022 •

edited

Loading

borngraced Oct 7, 2022

artemii235 Oct 7, 2022 •

edited

Loading

borngraced commented Oct 7, 2022

artemii235 left a comment

sergeyboyko0791 left a comment

sergeyboyko0791 commented Oct 10, 2022

sergeyboyko0791 commented Oct 10, 2022

[r2r] validate swap parameters #1475

[r2r] validate swap parameters #1475

Conversation

borngraced commented Sep 18, 2022

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borngraced Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

sergeyboyko0791 commented Oct 5, 2022

artemii235 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borngraced Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artemii235 Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artemii235 Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

borngraced commented Oct 7, 2022

artemii235 left a comment

Choose a reason for hiding this comment

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

sergeyboyko0791 commented Oct 10, 2022

sergeyboyko0791 commented Oct 10, 2022

borngraced Sep 23, 2022 •

edited

Loading

borngraced Oct 6, 2022 •

edited

Loading

artemii235 Oct 7, 2022 •

edited

Loading

artemii235 Oct 7, 2022 •

edited

Loading