fix #112 - users were able to burn value during withdraw #114

snd · 2018-01-31T15:33:43Z

fixes #112 through solution laid out in issue

fix broke a lot of existing tests.
tests used a lot of raw data hex strings (#79) which were hard to adapt to changes.
therefore includes a pretty big refactoring to make it easy to adapt tests to changes like this.

still todo after opening PR:

more explicit, easier to test in isolation

… of strings

remove ignoring of messages with insufficient value because no longer needed. use user specified gas price on withdraw.

… value withdraws

…ailure first

…-types)

…by something easily modifiable (json, ethabi use)

debris · 2018-02-15T08:34:37Z

bridge/src/signature.rs

+
+/// an ECDSA signature consisting of `v`, `r` and `s`
+#[derive(PartialEq, Debug)]
+pub struct Signature {


hah... something like this should be migrated to ethereum-types at some point ;)

definitely : )

debris · 2018-02-15T08:38:15Z

bridge/src/bridge/withdraw_relay.rs

+							let payload: Bytes = app.home_bridge.functions().withdraw().input(
+								signatures.iter().map(|x| Signature::v_from_bytes(x.0.as_slice())),
+								signatures.iter().map(|x| Signature::r_from_bytes(x.0.as_slice()).0),
+								signatures.iter().map(|x| Signature::s_from_bytes(x.0.as_slice()).0),


v_from_bytes, r_from_bytes and s_from_bytes panic if bytes is not long enough

also, you don't have to call as_slice. & is shorter and commonly used ;)

i'll take more care here to handle the possible failures and either use Result or a panic with a proof (q.e.d.) it can't happen!

as_slice(x) == &x[..], as_slice(x) != &x, right?

https://doc.rust-lang.org/std/vec/struct.Vec.html#method.as_slice

usually i prefer &x[..] as well.
i prefer .as_slice(x) when it's part of a . chain since it's only requires a suffix and not also a prefix.

debris · 2018-02-15T08:38:45Z

bridge/src/bridge/withdraw_relay.rs

+								signatures.iter().map(|x| Signature::s_from_bytes(x.0.as_slice()).0),
+								message.clone().0).into();
+							let request = TransactionRequest {
+								from: app.config.home.account.clone(),


you don't need to clone() it. It implements Copy

debris · 2018-02-15T08:41:32Z

bridge/src/message_to_mainnet.rs

+	pub fn from_bytes(bytes: &[u8]) -> Self {
+		assert_eq!(bytes.len(), MESSAGE_LENGTH);
+
+		Self {


lol, this is new. It wasn't available in old versions of rust :)

debris · 2018-02-15T08:44:13Z

bridge/src/message_to_mainnet.rs

+		let hash = web3_log.transaction_hash
+			.ok_or(Error::from_kind(ErrorKind::Msg("`log` must be mined and contain `transaction_hash`".into())))?;
+		Ok(Self {
+			recipient: withdraw_log.recipient.0.into(),


this conversion is probably redundant since web3 and ethabi are using the same version of ethereum-types crate

debris · 2018-02-15T08:44:31Z

bridge/src/message_to_mainnet.rs

+			recipient: withdraw_log.recipient.0.into(),
+			value: U256(withdraw_log.value.0),
+			sidenet_transaction_hash: hash.to_vec().as_slice().into(),
+			mainnet_gas_price: U256(withdraw_log.home_gas_price.0),


all other coversion here as well

debris · 2018-02-15T08:46:19Z

bridge/src/message_to_mainnet.rs

+			mainnet_gas_price_raw: u64
+		) -> TestResult {
+			if recipient_raw.len() != 20 || sidenet_transaction_hash_raw.len() != 32 {
+				return TestResult::discard();


is it possible to specify the length of these vectors? If not, the chance that both of them will have desired len is pretty low...

it's possible but more work. one would have to create a new type that implements quickchecks Arbitrary and only generates vectors of a specific size.

quickcheck by default will fail if it doesn't generate at least 100 tests.
so we're guaranteed that it generates at least 100 vectors of the desired lengths.

this is not perfect but good enough in my opinion.

N.B. Since discarding a test means it neither passes nor fails, quickcheck will try to replace the discarded test with a fresh one. However, if your condition is seldom met, it's possible that quickcheck will have to settle for running fewer tests than usual. By default, if quickcheck can't find 100 valid tests after trying 10,000 times, then it will give up.

https://github.com/BurntSushi/quickcheck#discarding-test-results-or-properties-are-polymorphic

interesting. let's leave it as it is for now :)

now i remember the full context. this is just a workaround. the best solution would be to implement quickcheck::Arbitrary for the types in ethereum-types! ethereum-types could benefit from some quickchecking itself as well. i'll give it a shot next week.

yes. and let's hide it behind feature gate :)

added paritytech/primitives#21

debris · 2018-02-15T08:47:06Z

bridge/src/signature.rs

+
+	quickcheck! {
+		fn quickcheck_signature_roundtrips(v: u8, r_raw: Vec<u8>, s_raw: Vec<u8>) -> TestResult {
+			if r_raw.len() != 32 || s_raw.len() != 32 {


same question. How are these lengths defined?

debris · 2018-02-15T08:48:41Z

tests/tests/deposit_relay.rs

 		"eth_getLogs" =>
-			req => r#"[{"address":["0x0000000000000000000000000000000000000000"],"fromBlock":"0x1","limit":null,"toBlock":"0x1005","topics":[["0xe1fffcc4923d04b559f4d29a8bfc6cda04eb5b0d3c460751c2402c5c5cc9109c"],null,null,null]}]"#,
-			res => r#"[]"#;
+			req => json!([{


👍 👍 👍

resolves grumbles: #114 (comment) #114 (comment)

resolves grumble: #114 (comment)

debris · 2018-02-15T10:24:12Z

bridge/src/message_to_mainnet.rs

@@ -41,10 +41,10 @@ impl MessageToMainnet {
 		let hash = web3_log.transaction_hash
 			.ok_or(Error::from_kind(ErrorKind::Msg("`log` must be mined and contain `transaction_hash`".into())))?;


this could be also shortened to:

let hash = web3_log.transaction_hash.ok_or_else(|| "`log` must be mined and contain `transaction_hash`")?;

resolve grumble: #114 (comment)

resolves grumble: #114 (comment)

snd · 2018-02-15T12:06:20Z

@debris all grumbles addressed

debris · 2018-02-15T12:11:07Z

Cargo.lock

@@ -1026,6 +1051,7 @@ dependencies = [
 "checksum docopt 0.8.3 (registry+https://github.com/rust-lang/crates.io-index)" = "d8acd393692c503b168471874953a2531df0e9ab77d0b6bbc582395743300a4a"
 "checksum dtoa 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)" = "09c3753c3db574d215cba4ea76018483895d7bff25a31b49ba45db21c48e50ab"
 "checksum env_logger 0.3.5 (registry+https://github.com/rust-lang/crates.io-index)" = "15abd780e45b3ea4f76b4e9a26ff4843258dd8a3eed2775a0e7368c2e7936c2f"
+"checksum env_logger 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)" = "3ddf21e73e016298f5cb37d6ef8e8da8e39f91f9ec8b0df44b7deb16a9f8cd5b"


one more thing. Can you bump env_logger in cli/Cargo.toml:17:env_logger = "0.3" to 0.4 ? :)

so the library is not duplicated :)

seen in many other solidity projects with high code quality

addresses paritytech/parity-bridge#114 (comment)

resolves grumbles: paritytech/parity-bridge#114 (comment) paritytech/parity-bridge#114 (comment)

resolves grumble: paritytech/parity-bridge#114 (comment)

resolve grumble: paritytech/parity-bridge#114 (comment)

resolves grumble: paritytech/parity-bridge#114 (comment)

snd added the A3-inprogress label Jan 31, 2018

snd changed the title ~~fix #112 - users could burn value during withdraw~~ fix #112 - users were able to burn value during withdraw Jan 31, 2018

snd force-pushed the snd-issue-112 branch from ee4e687 to 2d16c5d Compare February 2, 2018 15:25

snd added 16 commits February 9, 2018 09:39

bridge.sol: improve docstring of withdraw

c7ef6bf

bridge.sol: improve param names

f758740

bridge.sol: move withdraw sig verify from modifier into function

36c0859

more explicit, easier to test in isolation

user explicitely specifies homeGasPrice on withdraw. resolves #112

41543ca

adapt integration tests to #112

8f1f8ca

make existing truffle tests run again after 112

f2e2976

magic numbers -> constants

c0d87f1

make rpc stream tests more readable/modifiable by using json! instead…

ef2a4bc

… of strings

tests/tests/deposit_relay.rs: &str -> json!

b166445

tests/tests/log_stream.rs: &str -> json!

7930299

extract code dealing with Withdraw message

5000043

remove ignoring of messages with insufficient value because no longer needed. use user specified gas price on withdraw.

remove contract code that dealt with removed ignoring of insufficient…

467c5dc

… value withdraws

fix test helpers.createMessage: allow setting of homeGasPrice

88a117a

truffle test for message: should extract homeGasPrice

e3bbe9e

tests/src/lib.rs: change order of assertions to see most meaningful f…

b3760ec

…ailure first

tests/Cargo.toml: add more required deps

9df5767

snd force-pushed the snd-issue-112 branch from 1484795 to 9df5767 Compare February 9, 2018 08:45

snd added 10 commits February 12, 2018 10:11

adapt to ethabi version bump

4299308

use ethereum_types 0.2 everywhere

999a4a7

update Cargo.lock

465e2d8

bridge/src/helpers.rs: no longer needed

78c6cf4

message_to_mainnet.rs: remove to_hex, add to_payload

ed060ae

message_to_mainnet.rs: fix from_bytes (respect little endian)

b289445

extract code dealing with signatures into module

87c57c9

remove conversion (web3 -> ethabi) that is no longer needed (ethereum…

be18c96

…-types)

tests/tests/withdraw_relay.rs: refactor: replace opaque data strings …

d7928c2

…by something easily modifiable (json, ethabi use)

remove test for function that doesn't exist anymore

4a3b4ba

proper error handling instead of expect

55192e9

snd added A0-pleasereview and removed A3-inprogress labels Feb 14, 2018

snd requested a review from debris February 14, 2018 11:19

debris reviewed Feb 15, 2018

View reviewed changes

debris mentioned this pull request Feb 15, 2018

Implement quickcheck::Arbitrary for ethereum-types paritytech/primitives#21

Closed

snd added 2 commits February 15, 2018 11:17

remove redundant conversions

189d5b2

resolves grumbles: #114 (comment) #114 (comment)

remove unnecessary .clone()

ca7f758

resolves grumble: #114 (comment)

debris reviewed Feb 15, 2018

View reviewed changes

snd added 5 commits February 15, 2018 11:33

shorten error handling

6f13eb4

resolve grumble: #114 (comment)

README.md: mention homeGasPrice

f437a2f

update readme

763e065

README.md: add missing word

4226a04

remove panics. improve error handling for signature.

0246119

resolves grumble: #114 (comment)

debris reviewed Feb 15, 2018

View reviewed changes

snd added 3 commits February 15, 2018 13:13

cli/Cargo.toml: bump env_logger to 0.4

9753db7

make ForeignBridge.submitSignature a bit easier to understand

033a923

bridge.sol: uint -> uint256: more explicit, clear.

586afe8

seen in many other solidity projects with high code quality

debris approved these changes Feb 15, 2018

View reviewed changes

snd merged commit 3961987 into master Feb 15, 2018

snd deleted the snd-issue-112 branch February 15, 2018 13:48

This was referenced Feb 15, 2018

replace json containing raw payloads in stream tests #79

Closed

Duplication of value extraction from the message #89

Closed

Rearrangement for configuration parameter estimated_gas_cost_of_withdraw #92

Closed

noot pushed a commit to noot/poa-bridge that referenced this pull request Jul 18, 2018

Cargo.lock: use newest cargo which doesn't add root

feb87f4

addresses paritytech/parity-bridge#114 (comment)

noot pushed a commit to noot/poa-bridge that referenced this pull request Jul 18, 2018

remove redundant conversions

c6e6252

resolves grumbles: paritytech/parity-bridge#114 (comment) paritytech/parity-bridge#114 (comment)

noot pushed a commit to noot/poa-bridge that referenced this pull request Jul 18, 2018

remove unnecessary .clone()

d44a280

resolves grumble: paritytech/parity-bridge#114 (comment)

noot pushed a commit to noot/poa-bridge that referenced this pull request Jul 18, 2018

shorten error handling

d808dc4

resolve grumble: paritytech/parity-bridge#114 (comment)

noot pushed a commit to noot/poa-bridge that referenced this pull request Jul 18, 2018

remove panics. improve error handling for signature.

1cce690

resolves grumble: paritytech/parity-bridge#114 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #112 - users were able to burn value during withdraw #114

fix #112 - users were able to burn value during withdraw #114

snd commented Jan 31, 2018 •

edited

Loading

debris Feb 15, 2018

snd Feb 15, 2018

debris Feb 15, 2018

debris Feb 15, 2018

snd Feb 15, 2018

snd Feb 15, 2018

debris Feb 15, 2018

snd Feb 15, 2018

debris Feb 15, 2018

debris Feb 15, 2018

snd Feb 15, 2018

debris Feb 15, 2018

debris Feb 15, 2018

snd Feb 15, 2018

debris Feb 15, 2018

snd Feb 15, 2018

debris Feb 15, 2018

debris Feb 15, 2018

debris Feb 15, 2018

debris Feb 15, 2018

debris Feb 15, 2018

snd Feb 15, 2018

snd commented Feb 15, 2018

debris Feb 15, 2018

debris Feb 15, 2018

snd Feb 15, 2018

		@@ -41,10 +41,10 @@ impl MessageToMainnet {
		let hash = web3_log.transaction_hash
		.ok_or(Error::from_kind(ErrorKind::Msg("`log` must be mined and contain `transaction_hash`".into())))?;

fix #112 - users were able to burn value during withdraw #114

fix #112 - users were able to burn value during withdraw #114

Conversation

snd commented Jan 31, 2018 • 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

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

snd commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snd commented Jan 31, 2018 •

edited

Loading