fix ife input spend challenge using wrong index #593

boolafish · 2020-03-04T09:03:18Z

Note

This is the fix for issue: omisego/security-issues#15

Add py tests to cover the scenerio
fix the contract using the right index

Test

The test in this commit would fail before making the contract change. VM Exception while processing transaction: revert Output index out of bounds

This is an effort to add tests to cover challenge IFE input spent on all output indexes. We found a bug that we are using wrong indexes in contract code. But, this test end up does not break the existing contract as although it is attracting wrong output, we are only using the output type with that output. With current system, we only have 1 output type for payment tx. As a result it failed to break with this test.. https://github.com/omisego/security-issues/issues/15

Test in previous commit failed to cover the situation that would fail the contract with wrong index. This one test the index from another side (challenge input index) and it works to break the contract. Without the fix, it would result in "VM Exception while processing transaction: revert Output index out of bounds" error from the EVM.

This commit is the fix on the contract for input spend challenge

boolafish · 2020-03-04T09:04:39Z

...amework/python_tests/tests/contracts/root_chain/test_challenge_in_flight_exit_input_spent.py

@@ -20,6 +21,97 @@ def test_challenge_in_flight_exit_input_spent_should_succeed(testlang, period):
    assert not in_flight_exit.input_piggybacked(0)


+
+@pytest.mark.parametrize("num_outputs", [1, 2, 3, 4])
+def test_challenge_in_flight_exit_should_succeed_for_all_output_indexes(testlang, num_outputs):


So this test (surprisingly) actually would not fail even without the fix

I think not that surprisingly:

the problem was with checking just the spending condition

the index of the double spend in the challenge tx is always 0, where owner[0] signs

this coincides with the input transaction having owner[0]'s output on position 0.

So even though the double spend would not necessarily be of owner[0] and at position 0, the spending condition would always pass.

this coincides with the input transaction having owner[0]'s output on position 0.

Yes, but other than 0, the index would mismatch. I was expecting this to break.

However because we are only using outputType from the output we retrieve, even the index mismatches, it would end up being same output type since we have only one type atm.

boolafish · 2020-03-04T09:04:49Z

...amework/python_tests/tests/contracts/root_chain/test_challenge_in_flight_exit_input_spent.py

+
+
+@pytest.mark.parametrize("challenge_index", [0, 1, 2, 3])
+def test_challenge_in_flight_exit_should_succeed_for_all_challenge_indexes(testlang, challenge_index):


this is the test that would actually fail

pik694

@pdobacz has also suggested to prepare a parametrizable test that would check all the possibilities - check each input, output index etc.

pik694 · 2020-03-04T09:46:55Z

...amework/python_tests/tests/contracts/root_chain/test_challenge_in_flight_exit_input_spent.py

-@pytest.mark.parametrize("period", [1, 2, 4])
-def test_challenge_in_flight_exit_input_spent_should_succeed(testlang, period):
+@pytest.mark.parametrize("period", [1, 2, 3, 4])
+def test_challenge_in_flight_exit_input_spent_should_succeed_in_all_periods(testlang, period):


Actually this test does not check whether we really can challenge in each period - it forwards just to the second and then challenges. Additionally, the comment above the test does not match now.

~~I updated the test name a bit, can you check again?~~

Actually, the name seems correct to me.

it forwards just to the second and then challenges

No, it actually forward to the period. Or at least, it forward to period * 1/2 MIN_EXIT_PERIOD

pik694 · 2020-03-04T09:49:56Z

...amework/python_tests/tests/contracts/root_chain/test_challenge_in_flight_exit_input_spent.py

+    for i in range(0, num_outputs):
+        owners.append(testlang.accounts[i])
+        outputs.append((testlang.accounts[i].address, NULL_ADDRESS, tx_output_amount))


could we extract it into a separate function? I know that such code is commonly used (which is bad), but this test is quite long and needs simplification so that it is easier to read.

I changed the way a bit, and also just realize that owners list is totally useless as the spend_utxo function would be only using the first owner when signing under the hood.

Please re-review whether your comment still hold with my change?

pdobacz · 2020-03-04T13:21:46Z

Suggesting a parametrized test for one of the other cases (ChallengeIFEOutputSpent) here #594, if this is useful

pdobacz · 2020-03-04T13:25:18Z

On the fix itself: it looks on point, I'd however go for the other similar test cases, like the one I suggested (see #594). Such a test would fit for:

challenging IFE input (@pik694 working on this)
challenging IFE output add challenge output big test #594
challenging standard exits
challenging canonicity
responses to canonicity challenge

1. kudo Piotr's idea to parametize the inputs for indexes at once 1. rename the period test a bit as just realize that it can work with period 0 too Piotr's PR: #594

pdobacz

Looks good! As agreed, #594 will go separately, straight to master, same as other test expansions suggested!

kevsul

LGTM

boolafish added 4 commits March 4, 2020 17:59

refactor: uncomment tests

5a654fa

fix: ife spend with wrong index

62b7d32

This commit is the fix on the contract for input spend challenge

boolafish commented Mar 4, 2020

View reviewed changes

style: fix linter

4e1b643

boolafish requested review from pgebal, pdobacz, kevsul and pik694 March 4, 2020 09:25

pik694 suggested changes Mar 4, 2020

View reviewed changes

fix: test description on period

4167bcb

pdobacz mentioned this pull request Mar 4, 2020

add challenge output big test #594

Closed

1 task

refactor: merge previous two tests into one with test parameters

2517a88

1. kudo Piotr's idea to parametize the inputs for indexes at once 1. rename the period test a bit as just realize that it can work with period 0 too Piotr's PR: #594

boolafish force-pushed the boolafish/fix_ife branch from 137a29a to 2517a88 Compare March 5, 2020 06:33

refactor: small changes on genreate list

e9edf67

boolafish requested a review from pik694 March 5, 2020 06:58

boolafish mentioned this pull request Mar 5, 2020

test: challenge SE with all indexes #596

Closed

pdobacz approved these changes Mar 5, 2020

View reviewed changes

kevsul approved these changes Mar 5, 2020

View reviewed changes

pik694 approved these changes Mar 6, 2020

View reviewed changes

boolafish merged commit 8b408d9 into master Mar 6, 2020

boolafish deleted the boolafish/fix_ife branch March 6, 2020 00:52

pik694 mentioned this pull request Mar 6, 2020

Parametrize ife input challenge tests to cover all cases #599

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix ife input spend challenge using wrong index #593

fix ife input spend challenge using wrong index #593

boolafish commented Mar 4, 2020 •

edited

Loading

boolafish Mar 4, 2020 •

edited

Loading

pdobacz Mar 4, 2020

boolafish Mar 5, 2020

boolafish Mar 4, 2020

pik694 left a comment

pik694 Mar 4, 2020

boolafish Mar 5, 2020 •

edited

Loading

pik694 Mar 4, 2020

boolafish Mar 5, 2020 •

edited

Loading

pdobacz commented Mar 4, 2020

pdobacz commented Mar 4, 2020 •

edited

Loading

pdobacz left a comment

kevsul left a comment



		@pytest.mark.parametrize("challenge_index", [0, 1, 2, 3])
		def test_challenge_in_flight_exit_should_succeed_for_all_challenge_indexes(testlang, challenge_index):

fix ife input spend challenge using wrong index #593

fix ife input spend challenge using wrong index #593

Conversation

boolafish commented Mar 4, 2020 • edited Loading

Note

Test

boolafish Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

pdobacz Mar 4, 2020

Choose a reason for hiding this comment

boolafish Mar 5, 2020

Choose a reason for hiding this comment

boolafish Mar 4, 2020

Choose a reason for hiding this comment

pik694 left a comment

Choose a reason for hiding this comment

pik694 Mar 4, 2020

Choose a reason for hiding this comment

boolafish Mar 5, 2020 • edited Loading

Choose a reason for hiding this comment

pik694 Mar 4, 2020

Choose a reason for hiding this comment

boolafish Mar 5, 2020 • edited Loading

Choose a reason for hiding this comment

pdobacz commented Mar 4, 2020

pdobacz commented Mar 4, 2020 • edited Loading

pdobacz left a comment

Choose a reason for hiding this comment

kevsul left a comment

Choose a reason for hiding this comment

boolafish commented Mar 4, 2020 •

edited

Loading

boolafish Mar 4, 2020 •

edited

Loading

boolafish Mar 5, 2020 •

edited

Loading

boolafish Mar 5, 2020 •

edited

Loading

pdobacz commented Mar 4, 2020 •

edited

Loading