add state transition checks to challengeExit() function about contracts

[johannbarbie]

That way we can separate two things that will open up watcher market too

yeah, just need to find a solution for the validator dilemma : https://people.cs.uchicago.edu/~teutsch/papers/truebit.pdf

[jdkanani]

We have separate file for challenges - https://github.com/maticnetwork/contracts/blob/master/contracts/proofs/ExitValidator.sol

[johannbarbie]

i see in that function that you validate only require(exitId2 > exitId);, but not the transaction signature or if the state transition is valid.

Here is how an operator can attack you:

1. user registers transaction for exit
2. operator includes transfer to him with invalid signature (you don't check it)
3. operator cancel's user's exit.

[johannbarbie]

In plasma the signature checking is mandatory:

https://github.com/omisego/plasma-contracts/blob/master/contracts/RootChain.sol#L226-L229

it establishes the security of the chain, as it checks for a valid state transition.

[jdkanani]

This one? https://github.com/maticnetwork/contracts/blob/master/contracts/proofs/ExitValidator.sol#L45

[johannbarbie]

oh, nice :)
but let's dig deeper:

1. user A sends transaction to user B
2. operator C includes transfer, but modifies events to show that money went to C
3. user A notices the invalid state transition, and registers the previous transfer for exit
4. C slashes A's exit with transaction that A signed in 1.

where do you validate that based on the transaction content the right events have been emitted?

[johannbarbie]

i see that here you check if transaction root and receipt root included in same block:
https://github.com/maticnetwork/contracts/blob/master/contracts/mixin/RootChainValidator.sol#L44-L46
but i think you also need to check that from the transaction the receipt got created correctly.

[jdkanani]

Yeah definitely. That’s where file https://github.com/maticnetwork/contracts/blob/master/contracts/proofs/ERC20Validator.sol will be used

[johannbarbie]

ok, this file is not used anywhere in your current Plasma contract implementation, right?

as i understand you are comparing msg.data to receipt data here.

few questions come to mind:

• how do you know the user actually has the funds? wouldn't i be able to collude with the operator and spend more funds than i have?
• what if the contract implements any conditions? like a security token where only KYC'd users can receive funds, wouldn't you need to check such conditions?

[jdkanani]

Rootchain has addValidator method which will add validators as Rootchain was growing and gasLimit was the problem. In future, we will change owner to 0x after deployment or use governance. Though, in the case of governance, we haven't put any thoughts on this part. We are also looking towards other projects to see how plasma will upgrade or fix production bugs. Any help/advice would be appreciated.

How do you know the user actually has the funds? wouldn't I be able to collude with the operator and spend more funds than I have?

I am also not sure if UTXO based one also has a solution to this problem. A user has to watch chain and if they detect such fraud they start Exit process. Any suggestions?

what if the contract implements any conditions? like a security token where only KYC'd users can receive funds, wouldn't you need to check such conditions?

Only simple transfer with plasma exit/challenge for now. But we would love to add those things.

[jdkanani]

For KYC stuff, one thing just came in my mind is to add Merkle root of (token -> Merkle of Root ) and use that to challenge. (Again, haven't put many thoughts on this. Thought of it on the fly :)

[johannbarbie]

Rootchain was growing and gasLimit was the problem.

are you saying that the contract became too big? hence the transition checking has been taken out?

I am also not sure if UTXO based one also has a solution to this problem.

the difference is that in the UTXO model you compare specific outputs to inputs. in the account model it is not clear what i would need to check to see if the balance of an account is correct. have you thought about merkle sum trees or other ways to relate between the different transactions?

[johannbarbie]

Only simple transfer with plasma exit/challenge for now.

so on the matic sidechain, there can only be one certified ERC20 implementation where none of the token holders can be contracts?

why allow any other transactions than in these certified ERC20 contracts? wouldn't it just open a window for the operator to flood users with garbage transactions, until users can not keep up to validate everything, then the operator sneaks in invalid state transitions, based on which he can then steal funds from the users?

[jdkanani]

are you saying that the contract became too big? hence the transition checking has been taken out?

We created separated contracts and they will call each other.

the difference is that in the UTXO model you compare specific outputs to inputs

Are you talking about in validator node or in the Rootchain contract? I am not sure if PlasmaMVP checks UTXO's inputs' output when you exit.

[johannbarbie]

I am not sure if PlasmaMVP checks UTXO's previous inputs' output when you exit.

i think i'm talking about users, that need to check the chain all the time. in UTXO all the data they need is in the block. In the account model they need to know all the data of all accounts since the beginning of time. basically users will need to run full nodes in the browser, right?

[jdkanani]

why allow any other transactions than in these certified ERC20 contracts? wouldn't it just open a window for the operator to flood users with garbage transactions, until users cannot keep up to validate everything, then the operator sneaks in invalid state transitions, based on which he can then steal funds from the users?

A user can also validate tx which is relevant or I am missing something. The node should take care of it.

[jdkanani]

I think I'm talking about users, that need to check the chain all the time. in UTXO all the data they need is in the block. In the account model they need to know all the data of all accounts since the beginning of time. basically users will need to run full nodes in the browser, right?

I think the user can have simple latest state of (if users need it in any form example - Merkle sum as you mentioned) which they need and use it to get data instead of storing the whole chain. You can do same in UTXO (where you keep only unspent TXs).

[johannbarbie]

if you implement transition checking in challengeExit(), i see that you can do accounting for a specific, limited type of ERC20 contract.

I see a few disadvantages:

• cumbersome, expensive implementation
• potentially unbounded chain validation effort for users

but these disadvantages are not point of this issue. The issue can be closed once ERC20Validator.sol is implemented in challengeExit().

[jdkanani]

Yeah definitely. Alternatively, we can allow anyone to challenge ERC20 tx (if data is available or someone starts an exit). That way we can separate two things that will open up watcher market too - just a thought.

[atvanguard]

Hey @johannbarbie
Thanks for a detailed review! Since this discussion, we have made major changes to our plasma design. We are implementing the Plasma predicate pattern.

In this pattern, the state transition checks are delegated to the predicate that understands whether state transitions (for instance, funds were spent) are outdated. See verifyDeprecation in the ERC20Predicate.

The challengeExit function in withdrawManager calls verifyDeprecation in the correct predicate and cancels the exit if deemed to be outdated. See this.

The signature on the challenge tx is validated here and the fact that state transitions were correct is verified here - 3rd parameter (signer / participant) in the call to processReferenceTx is of essence. We are actively seeking review from the plasma community on the pull request as well.

Note: The above links will break once the pull request is merged and the branch is deleted.