Ethereum Settlement Engine improvements #150

gakonst · 2019-07-22T09:15:29Z

Fixes most of #149

TODO: Add more tests for SPSP payments in interledger similar to three_nodes.rs

gakonst · 2019-07-22T15:34:39Z

#!/bin/bash -eo pipefail
cargo audit
    Fetching advisory database from `https://github.com/RustSec/advisory-db.git`
      Loaded 33 security advisories (from /home/circleci/.cargo/advisory-db)
    Scanning Cargo.lock for vulnerabilities (346 crate dependencies)
error: Vulnerable crates found!

ID:	 RUSTSEC-2019-0011
Crate:	 memoffset
Version: 0.2.1
Date:	 2019-07-16
URL:	 https://github.com/Gilnaa/memoffset/issues/9#issuecomment-505461490
Title:	 Flaw in offset_of and span_of causes SIGILL, drops uninitialized memory of arbitrary type on panic in client code
Solution: upgrade to: >= 0.5.0

This is not part of our deps

tarcieri · 2019-07-22T15:36:24Z

@gakonst what version of cargo-audit is installed? You may be hitting an old bug: rustsec/rustsec#88

crates/interledger-settlement-engines/src/engines/ethereum_ledger/types.rs

crates/interledger-settlement-engines/src/engines/ethereum_ledger/utils.rs

crates/interledger-settlement-engines/src/stores/redis_ethereum_ledger/store.rs

emschwartz · 2019-07-23T03:15:14Z

crates/interledger-settlement-engines/src/stores/redis_ethereum_ledger/store.rs

+                .query_async(self.connection.clone())
+                .map_err(move |err| error!("Error loading account data: {:?}", err))
+                .and_then(
+                    move |(_conn, ret): (_, u64)| {


Suggested change

move |(_conn, ret): (_, u64)| {

move |(_conn, ret): (_, bool)| {

crates/interledger-settlement-engines/src/stores/redis_ethereum_ledger/store.rs

emschwartz · 2019-07-23T03:17:02Z

crates/interledger-settlement-engines/src/stores/redis_ethereum_ledger/store.rs

+        )
+    }
+
+    fn credit_tx(&self, tx_hash: H256) -> Box<dyn Future<Item = (), Error = ()> + Send> {


Is there a possibility that separating credit_tx from check_tx_credited could cause a race condition? Is it possible that two threads or tasks could call check_tx_credited at the same time, both get a return value that indicates that it hasn't been credited, and both credit the balance accordingly?

It does not make sense to set poll_frequency to less than 60 seconds in Ethereum, since block time is targeted at 15 sec, and you want a buffer of some blocks to avoid reorgs. So I doubt it can happen in that case.

I think you are right though that if the function gets called frequently enough for the same range of blocks this can happen.

tarcieri · 2019-07-23T05:23:33Z

Re: RUSTSEC-2019-0011

What's actually happening is we retroactively updated the advisory to note that the older versions of memoffset are actually vulnerable too. It's been updated to note the older versions are also vulnerable now:

https://rustsec.org/advisories/RUSTSEC-2019-0011.html

It's been one of the more controversial vulns we've added to the database in that it falls into the "high severity / low exploitability" category. However, I would say the more we investigated what could go wrong based on particular patterns of misuse, the severity of the vulnerability continued to escalate, to first memory exposure and then to remote code execution.

If you feel comfortable with the current usage patterns, note that cargo-audit v0.7.0 added a --ignore option you can use to ignore particular advisories. This one is a bit annoying in that it has impacted a lot of people, and there hasn't been a particularly good resolution path, and for many the advisory wasn't applicable to their particular usage patterns.

…ved data We cannot assume that the operator's balance will keep increasing, since they may transfer funds to another address

- use join_all to avoid callback hell - avoid using spawn's - mark a transaction as credited only after the call to the connector was successful - save the last observed block only after all calls in that block range has been successfully credited

…tions which are already in the mempool Allows the removal of the nonce middleware

…ction

Otherwise we would be signing any message that gets sent to us and that is insecure

crates/interledger-settlement-engines/src/engines/ethereum_ledger/eth_engine.rs

gakonst · 2019-07-24T07:31:11Z

re security advisory: tokio-rs/tokio#1345

…ccount Otherwise it may take too long for the connector to ping its engine to get the address from its peer

crates/interledger-settlement-engines/src/engines/ethereum_ledger/eth_engine.rs

This ensures the same transaction is never processed twice by the connector (also minor improvements in variable/function naming

also more verbose failure log when max retries get exceeded to notify connector about an incoming settlement

Adding it resulted in incorerct behavior the balance is already lowered in the PROCESS_FULFILL script. With it, the balance was lowered by 2x the amount it should. Note that the reason the balance is lowered first is because we don't want two packets fulfilled simultaneously to trigger settlements for the same amount_to_settle. By reducing the balance by the amount_to_settle before making the settlement client send the request to the settlement engine, this ensures that the settlement will only happen once. The refund_settlement is there in case that call to the SE fails and we need to revert the change to the balance.

gakonst requested a review from emschwartz as a code owner July 22, 2019 09:15

gakonst mentioned this pull request Jul 22, 2019

Settlement Engine Framework and Ethereum Ledger Settlement Engine #125

Merged

gakonst force-pushed the eth-se-improvements branch from 27a837d to cd767e0 Compare July 22, 2019 09:32

emschwartz reviewed Jul 23, 2019

View reviewed changes

gakonst force-pushed the eth-se-improvements branch from a81f828 to 331e2c6 Compare July 23, 2019 09:31

gakonst added 10 commits July 23, 2019 13:21

fix(store): only save the Ethereum block number in the recently obser…

c620039

…ved data We cannot assume that the operator's balance will keep increasing, since they may transfer funds to another address

Use pending in eth_TransactionCount call to take into account transac…

df2a142

…tions which are already in the mempool Allows the removal of the nonce middleware

improvement(eth-se): Estimate gas and gas price before sending transa…

e3f465d

…ction

improve redis keys naming

37c4d1e

feat(eth-se): Add filter for ERC20 transactions

3e88b78

add ERC20Transfer struct

8c107e6

add option to credit incoming erc20 transfers

991bb5b

fix review comments

fe56c41

fix(eth-se): Sign the address along with the challenge

5047462

Otherwise we would be signing any message that gets sent to us and that is insecure

gakonst force-pushed the eth-se-improvements branch from 331e2c6 to 5047462 Compare July 23, 2019 11:15

no single variable names

69b0e15

gakonst force-pushed the eth-se-improvements branch from 92ee904 to 69b0e15 Compare July 23, 2019 11:58

emschwartz requested changes Jul 23, 2019

View reviewed changes

crates/interledger-settlement-engines/src/engines/ethereum_ledger/eth_engine.rs Outdated Show resolved Hide resolved

crates/interledger-settlement-engines/src/engines/ethereum_ledger/eth_engine.rs Outdated Show resolved Hide resolved

Use notify instead of submit as function names for clarity

8f362c8

improve example and switch to FixedInterval retry when creating new a…

674e6b2

…ccount Otherwise it may take too long for the connector to ping its engine to get the address from its peer

emschwartz reviewed Jul 24, 2019

View reviewed changes

gakonst added 3 commits July 24, 2019 20:12

improvement(eth-se): Use tx hash as an idempotency key

713700d

This ensures the same transaction is never processed twice by the connector (also minor improvements in variable/function naming

do not sign address when authenticating

1f0d86c

also more verbose failure log when max retries get exceeded to notify connector about an incoming settlement

gakonst force-pushed the eth-se-improvements branch from bd6e4c3 to ee965bd Compare July 25, 2019 05:32

emschwartz approved these changes Jul 25, 2019

View reviewed changes

gakonst merged commit 5ba5e80 into gakonst-eth-se Jul 25, 2019

gakonst deleted the eth-se-improvements branch July 25, 2019 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ethereum Settlement Engine improvements #150

Ethereum Settlement Engine improvements #150

gakonst commented Jul 22, 2019 •

edited

Loading

gakonst commented Jul 22, 2019

tarcieri commented Jul 22, 2019

emschwartz Jul 23, 2019

emschwartz Jul 23, 2019

gakonst Jul 23, 2019

tarcieri commented Jul 23, 2019 •

edited

Loading

gakonst commented Jul 24, 2019

	move \|(_conn, ret): (_, u64)\| {
	move \|(_conn, ret): (_, bool)\| {

Ethereum Settlement Engine improvements #150

Ethereum Settlement Engine improvements #150

Conversation

gakonst commented Jul 22, 2019 • edited Loading

gakonst commented Jul 22, 2019

tarcieri commented Jul 22, 2019

emschwartz Jul 23, 2019

Choose a reason for hiding this comment

emschwartz Jul 23, 2019

Choose a reason for hiding this comment

gakonst Jul 23, 2019

Choose a reason for hiding this comment

tarcieri commented Jul 23, 2019 • edited Loading

gakonst commented Jul 24, 2019

gakonst commented Jul 22, 2019 •

edited

Loading

tarcieri commented Jul 23, 2019 •

edited

Loading