Include Confirm sigs of Inputs in transaction

[AdityaSripal]

To solve the issue brought up here: https://ethresear.ch/t/plasma-vulnerabiltity-sybil-txs-drained-contract/1654

Confirm sigs of the inputs must be included in transaction. Transaction structure must now be modified

[Blknum1, TxIndex1, Oindex1, Amount1, ConfirmSig1, Blknum2, TxIndex2, Oindex2, Amount2, ConfirmSig2, NewOwner, Denom1, NewOwner, Denom2, Fee]

Here ConfirmSig1-1 is the first confirm sig from the first input, confirm sig is the second confirm sig from the first input, etc. If there is no confirm sig for the given position, replace with a zero-bytes.

We must also include the signatures since that is how it is stored in the merkle tree in child-chain blocks.

Changes to be made: Deposit, StartExit and ChallengeExit must be changed to handle new transaction structure

Note Possible Confusion: We are not including confirm sig for transaction itself, we are including confirm sigs of the INPUTS

Consider Transactions A -> B -> C: Only one input and output for each transaction for simplicity sake.

The Transaction B->C will include the confirm sig A sent to B for the transaction A->B.

[colin-axner]

Why do we need confirmSig1-1 and confirmSig1-2. Wouldn't blknum1, txindex1, and oindex1 all refer to one specific output that would need to be signed over. The other output of that transaction would be irrelevant right?

[hamdiallam]

I dont think we should change the transaction structure. The leaf can just be keccak(txBytes, sigs, confirmsigs)

confirmSigs of the inputs can just be passed into the function similar to how sigs is handled

[AdityaSripal]

@colin-axner You are right. Will change

@hamdiallam I think we need to change it because the confirm sigs are part of the message. In fact, because the entire transaction is included in the merkle tree, we actually will probably have to modify this further by including the signatures in the transaction structure as well.

[hamdiallam]

@AdityaSripal the txBytes is not included in the merkle tree. It is the hash of the bytes that is the leaf in the merkel tree

[colin-axner]

@hamdiallam Yeah but the txBytes will include sigs and confirmsig in the message of the transaction on the child chain.

[hamdiallam]

Exactly, I think we should always make the txByes as simple as we can on the rootchain. Playing around with the way the hash is generated is better than including those inputs in the txBytes. They can be included the child chain though.

Same reason as why omisego seperated sigs from txBytes on the rootchain

[colin-axner]

@hamdiallam I'm pretty sure we would need to fork tendermint and manually keep our forked version up to date (this may need to be done anyways to update the hash function). Also adjusting tendermint to exclude hashing some parts of the transaction could lead to further issues.

We can try changing the implementation in tendermint and adjust the decoding on the root contract as a backup option

@AdityaSripal thoughts?

[hamdiallam]

Ah, i see the issue. currently, the leaves of the merkle tree are 32byte hashes. We could change this though if tendermint cannot have the merkle tree leaves be hashes

[AdityaSripal]

@hamdiallam Doesn't seem like the sdk gives us that much flexibility. If we wanted to do that we would have to make major changes to the sdk. I would resist doing that. I think it's easier to add 4 extra fields to the txBytes.

[AdityaSripal]

We need to rlp encode the message and then attach the sigs at the end to make it easier to validate on the rootchain.

[AdityaSripal]

Also, we need to include the original denominations of the inputs. This is because we want to recover the original transaction bytes that created a deposit hash. This is only possible if we include the input denomination as part of the transaction.

[colin-axner]

@AdityaSripal I think you were originally correct about using confirmSig1-1, confirmSig1-2. While each output has separate confirmSigs, having two confirmSigs for one output is necessary when both inputs came from different addresses.

[hamdiallam]

Yeah, this can all be one sigs bytes variable that gets split up on the rootchain contract

[AdityaSripal]

Solved with #30