fix ife input spend challenge using wrong index (#593)

* test: integ test for challenge IFE input spend for all output indexes

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: integ test for all challenge indexes on IFE input spend

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.

* refactor: uncomment tests

* fix: ife spend with wrong index

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

* style: fix linter

* fix: test description on period

* refactor: merge previous two tests into one with test parameters

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

* refactor: small changes on genreate list

plasma_framework/contracts/src/exits/payment/controllers/PaymentChallengeIFEInputSpent.sol

@@ -136,7 +136,8 @@ library PaymentChallengeIFEInputSpent {
         GenericTransaction.Transaction memory challengingTx = GenericTransaction.decode(data.args.challengingTx);
 
         GenericTransaction.Transaction memory inputTx = GenericTransaction.decode(data.args.inputTx);
-        GenericTransaction.Output memory output = GenericTransaction.getOutput(inputTx, data.args.challengingTxInputIndex);
+        PosLib.Position memory utxoPos = PosLib.decode(data.args.inputUtxoPos);
+        GenericTransaction.Output memory output = GenericTransaction.getOutput(inputTx, utxoPos.outputIndex);
 
         ISpendingCondition condition = data.controller.spendingConditionRegistry.spendingConditions(
             output.outputType, challengingTx.txType

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

@@ -1,11 +1,13 @@
 import pytest
 from eth_tester.exceptions import TransactionFailed
 from plasma_core.constants import NULL_ADDRESS
+from plasma_core.utils.transactions import decode_utxo_id, encode_utxo_id
+from tests_utils.constants import PAYMENT_TX_MAX_INPUT_SIZE, PAYMENT_TX_MAX_OUTPUT_SIZE
 
 
 # should succeed even when phase 2 of in-flight exit is over
-@pytest.mark.parametrize("period", [1, 2, 4])
-def test_challenge_in_flight_exit_input_spent_should_succeed(testlang, period):
+@pytest.mark.parametrize("period", [0, 1, 2, 3])
+def test_challenge_in_flight_exit_input_spent_should_succeed_for_all_periods(testlang, period):
     owner_1, owner_2, amount = testlang.accounts[0], testlang.accounts[1], 100
     deposit_id = testlang.deposit(owner_1, amount)
     spend_id = testlang.spend_utxo([deposit_id], [owner_1], [(owner_1.address, NULL_ADDRESS, 100)])
@@ -20,6 +22,56 @@ def test_challenge_in_flight_exit_input_spent_should_succeed(testlang, period):
     assert not in_flight_exit.input_piggybacked(0)
 
 
+@pytest.mark.parametrize(
+    "double_spend_output_index,challenge_input_index",
+    [(i, j) for i in range(PAYMENT_TX_MAX_OUTPUT_SIZE) for j in range(PAYMENT_TX_MAX_INPUT_SIZE)]
+)
+def test_challenge_in_flight_exit_input_spent_should_succeed_for_all_indices(
+    testlang, double_spend_output_index, challenge_input_index
+):
+    alice, bob, carol = testlang.accounts[0], testlang.accounts[1], testlang.accounts[2]
+    deposit_amount = 100
+    deposit_id = testlang.deposit(alice, deposit_amount)
+
+    tx_output_amount = deposit_amount // PAYMENT_TX_MAX_OUTPUT_SIZE
+    outputs = [(alice.address, NULL_ADDRESS, tx_output_amount)] * PAYMENT_TX_MAX_OUTPUT_SIZE
+
+    input_tx_id = testlang.spend_utxo([deposit_id], [alice], outputs=outputs)
+    blknum, tx_index, _ = decode_utxo_id(input_tx_id)
+    double_spend_utxo = encode_utxo_id(blknum, tx_index, double_spend_output_index)
+
+    ife_output_amount = tx_output_amount
+    ife_tx_id = testlang.spend_utxo(
+        [double_spend_utxo],
+        [alice],
+        [(bob.address, NULL_ADDRESS, ife_output_amount)]
+    )
+
+    inputs = []
+    for i in range(0, PAYMENT_TX_MAX_INPUT_SIZE):
+        if i == challenge_input_index:
+            inputs.append(double_spend_utxo)
+        else:
+            inputs.append(testlang.deposit(alice, tx_output_amount))
+
+    challenge_tx_id = testlang.spend_utxo(
+        inputs,
+        [alice] * PAYMENT_TX_MAX_INPUT_SIZE,
+        [
+            (carol.address, NULL_ADDRESS, tx_output_amount),
+        ],
+        force_invalid=True
+    )
+
+    testlang.start_in_flight_exit(ife_tx_id, sender=bob)
+    testlang.piggyback_in_flight_exit_input(ife_tx_id, 0, alice)
+    testlang.challenge_in_flight_exit_input_spent(ife_tx_id, challenge_tx_id, carol)
+
+    in_flight_exit = testlang.get_in_flight_exit(ife_tx_id)
+    for i in range(0, PAYMENT_TX_MAX_INPUT_SIZE):
+        assert not in_flight_exit.input_piggybacked(i)
+
+
 def test_challenge_in_flight_exit_input_spent_not_piggybacked_should_fail(testlang):
     owner_1, owner_2, amount = testlang.accounts[0], testlang.accounts[1], 100
     deposit_id = testlang.deposit(owner_1, amount)

plasma_framework/python_tests/tests/tests_utils/constants.py

@@ -11,3 +11,6 @@
 START_GAS = GAS_LIMIT - 1000000
 
 INITIAL_ETH = 10000 * 10 ** 18
+
+PAYMENT_TX_MAX_INPUT_SIZE = 4
+PAYMENT_TX_MAX_OUTPUT_SIZE = 4