Following line stuck out on a quick browse-through:

https://github.com/omisego/plasma-contracts/blob/848e0399fdc0b059e151922baf95e58b00fe705c/contracts/RLP.sol#L203

It looks like it should be mstore(dest, mload(src))

It's inside the following function:
https://github.com/omisego/plasma-contracts/blob/848e0399fdc0b059e151922baf95e58b00fe705c/contracts/RLP.sol#L194-L217

What is upgrade by version

Upgrade by version means we do smart contract upgrade by deploying a new one, bumping the version on our either proxy contract or registry contract to change the pointer to new version.

With abstract layer design, it is possible to upgrade by "adding" new stuffs. For instance, new tx type and exit game. Now comes the question, do we want the ability to upgrade by version?

Why do we not need this

In production, we would probably prefer to limit ourselves to be able to upgrade only by "adding new things" + "exit and join" for security reason.

One possible way to do this securely enough is to add a limit of all upgrade by version need to wait two weeks (as plasma m(ore)vp security)

So, if not in prod, when do we need this?

I see potential value of this is to upgrade our pipeline test env using this method. In development, we push several changes and then audit once. Currently, those "several changes" can basically break our development and staging and need to cleanup our env. With upgradeability of abstract layer, though we can add new txs, but that means our watcher/operator codes needs to support such new tx types (and meanwhile maintain the old tx types). If we can just bump version, we only need to adjust our operator codes to support the code changes (before the changes go to audit/prod).

Side notes

There is two layers for this topic actually, the plasma framework layer that supports the basic feature and the abstracted layer for tx types.

1. Framework Layer: Without upgrade by version, there is no way to upgrade the framework actually. But fairly speaking, this part is more stable and rarely do feature land here.
2. Abstracted Layer: There is a possibility of isolation of predicate can make adding new stuffs out from operator and watcher. However, this is still remain a research topic. Some of our draft idea is here

Goal

Implement the followings

1. PlasmaFramework
2. ExitGame/ExitProcessor/Predicate contracts for payment with MoreVP (ETH/ERC20/broken ERC20)

Plan

Migration plan document: here

Reference

1. Design doc: here
2. Previous POC: omgnetwork/research#78

We have decided not to allow for reusable IFEs, so now once an IFE is flagged as finalised it cannot be reopened. Before we decided to do it, we had prepared the RootChain contract to be ready for such events.

Now we should clean up the code, so it does not make unecessary checks.

Example:

https://github.com/omisego/plasma-contracts/blob/0e77c7dd29378c3dd2410a7bc95c2e3fa24f2813/contracts/RootChain.sol#L567
The above cannot happen as IFE can either be in the first period or is finalised.

Note

Parent story: #106
We need to have a spec first before anything can starts on this.

Quite likely some of the previous work can be reused: https://hackmd.io/ox5XLjdPRK6N3Mp9T9eUZw?view

Successful Criteria

Has a spec out with details of the followings:

1. Private Deposits
2. Off-chain settlement validation
3. Withdrawals: standard and 2-phase withdraw

Quite likely this would need to define "order" submission and cancelation as well (though they do not need to be implemented).

Issue Type

[ ] bug report
[x] feature request

Current Behavior

Depositing DGX into the chain credits user with amount on plasma chain, while root contract balance is increased only by amount - fee. This causes chain to be insolvent for DGX token.

Expected Behavior

Account for the fact that token may extract such a fee and rely not on declared amount, but on actual change of balance of root chain contract.

It would be great if we could abstract away the fact that MVP transactions outputs must be accompanied by confsigs when spend (etc) into something smarter than an if statement.

Values other than 0 are interpreted as UTXO position of standard exit. This prevents user from targeting in-flight exit on top of the queue.

Solution: pass exit_id instead of UTXO position. It will work for both SE and IFE.

Issue Type

[x] bug report
[ ] feature request

Current Behavior

finalizeExits must be provided with enough gas to clear the queue. Queue can grow long enough so clearing it will not fit in block gas limit. This is an attack vector.

Expected Behavior

Being able to clear only part of the queue would be nice.

Suggested Fix A (born in discussion with @pdobacz )

One idea is to have finalizeExits proceed the following way:
1/ finalize the first exit in the queue
2/ calculate the gas used for (1/)
3/ goto 1/ only if remaining gas allowance is > than result of (2/)

A (probably) necessary variation is to keep track of the maximum gas paid for a single exit finalized, and gracefully stop processing exits, in case when there are still exits in the queue, but there isn't enough gas for the pesimistic scenario.

Suggested Fix B (born in discussion with @pdobacz )

Alternatively this could be done as follows. We modify API:

finalizeExits(uint256 _top_utxoPos, uint256 _exits_to_process)

1/ finalizeExits checks if passed _top_utxoPos is the one on top of the queue
2/ loop processes only _exits_to_process exits
3/ sender does local gas estimation and attaches more than enough gas to execute those particular exits

_top_utxoPos check makes sure that in case of the tx race, gas losses will be minimal. Explicit number of exits to process simplifies gas estimation.

Drawback of this solution is that gas estimation is just that. Changed state of the token might enfluence amount of gas needed to perform the transfer (possible even for normal tokens because of gas refund mechanic), leading to loss of all of the gas attached to tx. This risk is minimisied by attaching more gas than needed to tx.

On the bright side - there are no reasons to suspect that finalizeExits will be a function called all of the time. Cost of not calling it is a time cost of money. Races should be rare and the one losing the race saves money, since somebody else paid for processing part of the queue.

Current stack

Our tests run under pytest testing framework. Each run of the tests we run an internal EVM provided by pyethereum. We compile and link our contracts using py-solc which wraps solc.

Motivation for changes

Our main motivation is the fact that py-solc does not work with solidity v0.5. One other is that pyethereum is deprecated.

Goal

We would like to have a testing framework that is capable of running solidity v0.5. The framework should also be future-proof (at least should look promising).

ref #103

Add CI to this project

Issue Type

[x ] bug report
[ ] feature request

Current Behavior

Transaction supplied to startInFlightExit can name a particular valid input several times. Inputs being mutually distinct isn't checked, transaction is deemed valid and as a result of this IFE one we'll be able to quadruple funds exited.

(It seems this can't be circumvented by challenging canonicity or piggybacking. Even if, such way of handling would seem dangerous.)

Expected Behavior

Transaction should be valid for being made into an IFE, because it's de facto invalid (sum(inputs)<sum(outputs))

Steps to Reproduce

NA

Suggested Fix

add a check which rejects transactions that have nonzero repeating inputs

parent story: #105

Issue Type

[x] bug report
[ ] feature request

Note

After a transaction is submitted, for unknown reason operator does not submit that tx into a block.
To prevent operator withholding, the output owner starts an IFE and piggybacked that.
All the sudden operator put that tx into block after the piggyback.
Now, input owner can piggyback IFE input, and simply create another IFE to make such IFE non-conical so that they can exit the input.
And now we are left with a situation where in the plasma chain a officially "used tx" is allowed to exit.

Issue Type

[x] bug report
[ ] feature request

Current Behavior

When there is an in-flight exit that is not piggybacked by anyone, processExits function call returns with error.

Steps to Reproduce

1. Checkout ce4a23d
2. Comment out line 200 in test_start_in_flight_exit.py
3. Run tests

Issue Type

As OMG contract is implemented in a way that does not return "true" when transfer, as we want to be able to support OMG token, our contract need to support ERC20 transferFrom function without a return value.

For details, see this: https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca

[ ] bug report
[x] feature request

Current Behavior

In function depositFrom(bytes _depositTx)

// Warning, check your ERC20 implementation. TransferFrom should return bool
require(ERC20(decodedTx.outputs[0].token).transferFrom(msg.sender, address(this), decodedTx.outputs[0].amount));

Expected Behavior

Does not need the transferFrom to return bool.

Suggested Fix

Perhaps use address.call directly, and in solidity 0.5, it returns success or failed + return value. We can rely on the result of "success" or "failed' directly instead.

Or the article above (in Issue Type) do provide some wrapper example code as well.

Motivation for Change

To support OMG token

Potentially we can have a predicate for the re-org protection logic/state. So we can even in the future switch our protection mechanism.

doc: https://gist.github.com/paulperegud/3fb4bb96693abe2e0b175f7b20e64362

Extend ECRecover lib to support chain_id in signature verification. Also try to pass its value at deployment time.
Needed to fully support EIP-712 signing.

This is described in EIP-155 Simple replay attack prevention

Why

For plasma framework, the underneath layer should be agnostic and decoupled from the logic on top of it. "Which Token" is more or less a business logic of the transaction or the output/state. Especially we are able to abstract our contract to deposit, it is potentially possible to "deposit" something that is not even token based into plasma now!

The reason of priority per queue is because we want to make sure a bad external code (token contract) cannot break the "processExit" function for others.

Proposal

Use pull over push to secure the transfer. So we do not transfer while "processExit", instead, we just mark things as "transferable" and let user to do the real transfer in another call.

What to do

• Provide a pseudo code/POC for the pull over push solution and discuss whether this is applicable.

parent story: #105

Personally I'd love to have truffle on waffle~

Background

It seems like py-solc is not really updating to support latest solidity. We can update that but whether we want to maintain that ourselves is a question. It seems like a good timing to move away.

Also, related to previous audit recommendation: https://www.pivotaltracker.com/story/show/161138286

So one obvious option is truffle. However, Pepesza points out there's another framework called Waffle might be an alternative.

Some links for Waffle:
https://github.com/EthWorks/Waffle
https://medium.com/ethworks/meet-waffle-8e630e5e9e91

What to do here

Let's put some reasoning, pros and cons on the two choices. Ideally make decision.

Issue Type

[x] bug report
[ ] feature request

Current Behavior

finalizeExits will send money from challenged exit to 0x0.

Expected Behavior

Successfully challenged exits should not lead to any money movements.

Suggested Fix A

Add a check in finalizeExits loop which will make sure that if owner == 0, than no money is being moved.

Pro: trivial implementation; challengeExit stays cheap
Con: challenged exits stay in the queue, bloating it, increasing gas needed to operate on queue

Suggested Fix B

Alternatively, we can remove exit from the queue on successful challengeExit.

Pro: decreases size of the queue early, preserving everyone's gas
Con: challengeExit gas cost is no longer constant; needs careful implementation of random access remove for PriorityQueue structure.

After rc deposit, user can trade on dex without waiting for block.

1. User make rc deposit
2. Operator give a signature to dex,
3. User can trade without waiting

Dex can use signature from Operator to start mvp chain transaction.
No need to freeze utxo in dex accounts.

From @pdobacz:

a batch of longer-running tests that test whether gas is always under control for submitBlock, startExit, challengeExit and finalizeExit, no matter how much we put into the contract, like we did for HonteStaking.sol

Another, slightly paranoid thing I'd consider is adding assertions on pre/post-conditions to actions executed using testlang, like "if a successful deposit is done, then contract's balance increases, depositor's - decreases", "if a successful finalizeExit is done, then balances are updated properly". Likewise, I'd assert proper event firing in post conditions. This is something we did for HonteStaking too.

From @pgebal:

fix leaves=[] in FixedMerkle
add a test for a failure of exiting utxo that is already exited (finalized) will do in #19

Why

Abstract design requires each exit games to be as independent with each others as possible. Current solution for the collision requires the knowledge of "exit id" of input transaction. However, the input transaction's "exit id" would be decided by its own game. We do not want to be in the situation while designing exit game for new tx we need to take care of all input exit games' logic.

previous discussion: omgnetwork/research#78 (comment)

This is a blocker for abstract layer implementation, we need decision on this

Proposal

In non-canonical challenge, an IFE can be challenged by input that is already finalized, and this would have highest priority so that nothing can respond this.

Alternatives, Kelvin's solution for #102 might just as well solve this?

What to do

• Basically, there must have dependency somewhere in MoreVP. Currently it is linked by exit id. We should also analyze is it possible to use utxo pos instead of exit id?
• Check whether Kelvin's solution for #102 would just as well solve this

parent story: #105.
Previous POC code: https://github.com/omisego/plasma-contracts/blob/abstract_poc/abstract_poc/contracts/src/framework/ExitGameController.sol

Watcher nodes will not be able to challenge one type of exit with other type of exit.

parent story: #105
POC code: https://github.com/omisego/plasma-contracts/blob/abstract_poc/abstract_poc/contracts/src/framework/PlasmaBlockController.sol

Note

As an exchange
I can use Restricted Custody exchange on OmiseGO Network (elixir-omg)
So that my user's funds are safe

As an exchange
I can exit funds on behalf of users only
So that users don't lose their money if the chain goes byzantine

As a regulator
I can validate the venue's transaction retrospectively
So that exchange has less motivation to act fraud and user is more safe.

As a user
I can state which venue to use my custody
So that my funds can be used in settlement transaction

As a user
I can withdraw my funds back from the venue
So that my funds can be safe under full plasma security promise instead of the DEX security promise.

Successful Criteria

From the priority doc

Required implementation:

1. Private Deposits
2. Off-chain settlement validation
3. Withdrawals: standard and 2-phase withdraw

For details, please check the doc.

When processing a deposit we check if only the first output is set, but we do not check inputs at all.

Not checking inputs (asserting all are zeroed) of the deposit may lead to some unexpected behaviour, ex. making an IFE canonical by creating the deposit tx that looks the same as an IFE transaction.

Things that can't be done using this potential vulnerability are:

1. preventing the exit by creating deposit that "spends" exited utxo <-> we are checking the signature, not the inclusion proof
2. ...

Issue Type

[ ] bug report
[0] feature request

Current Behavior

In the current contract design, for a non-ETH token to exit (SE or IFE) need to make sure it calls addToken beforehand. If a mass exit happens, this might lead to users fail on their attempt to exit.

related PR: #76 (The PR added doc, but probably better to have a bit more action)

Expected Behavior

Either let users clearly understand this or not make this an issue at first hand.

Steps to Reproduce

N/A

Suggested Fix

Some possible solutions:

1. Provide warning, eg. on watcher status.get -> "event": "your_utxo_uses_unadded_token" So users could be aware of it.
2. Force a deposit to fail if not addToken already
3. Deposit magically addToken if not added already
4. Reimplement PriorityQueue that accepts multiple coins, then no more issue. (AddToken basically creates an empty queue for that token)

Motivation for Change

as explained in Current Behavior

System Specs

N/A

Why

There are some custom rules on deposit txs.

1. The exit priority formula is slightly different
2. currently they are using different way to compute the exit id.
3. the correctness of this tx would be used and checked on-chain directly when deposit
4. Deposit tx do not need IFE
5. In the future, as we abstract wallet now, different ERC use cases would have different output type, they might need new tx type to support new output anyway (?)

In general, I think deposit is different enough to have its own type. And the exit game of it is fairly simple (standard exit only)

Discussion

Do we want each ERC to have its own type? eg. separate ERC721 and ERC20?

https://github.com/ethereum/solidity/blob/v0.5.0/Changelog.md

contracts need to be updated, some syntax breaking change for ex,

StandardToken.sol:60:63: ParserError: The state mutability modifier "constant" was removed in version 0.5.0. Use "view" or "pure" instead.
  function allowance(address _owner, address _spender) public constant returns (uint256) {

Issue Type

[ ] bug report
[ X] feature request

Current Behavior

Currently we use elixir-omg dev tooling to deploy contracts to public networks.

Expected Behavior

We would like to use Truffle as per #103

Suggested Fix

Truffle, truffle, truffle

Motivation for Change

Ease of deployment.

System Specs

/

See discussion here:
https://github.com/omisego/devops/issues/87#issuecomment-501452334

The followings are using default getter from public variable. (all using mapping)

- exits
- inflightExits
- blocks

We could try out some experiment by write some simple batch getter by taking an array of ids/kets and return the value.

Issue Type

[*] bug report
[ ] feature request

Current Behavior

Operator can steal all the funds in root chain account.

Steps to Reproduce

1. Operator creates a block that includes tx1 and tx2 double-spending shared input a and withholds it.
2. Next, she creates IFEs from transactions tx1 and tx2.
3. During period 1 other players will challenge canonicity of tx1 with tx2 and vice versa.
4. Operator waits for period 2, and responds to both challenges by providing inclusion proofs.

This creates double-spend where both transactions are canonical (exiting from outputs b and c) and are exiting with high priority - priority of input a. Since operator chooses the moment when he creates IFEs, he exits first, potentially stealing all the funds from the chain.

Why external observers can't do anything about it? Because challenges are forbidden in period 2. Otherwise they could take revealed merkle proof for transaction tx1 and show that it precedes tx2, making tx2 non-canonical.

Suggested Fix

Challenge should be possible in both periods 1 and 2. Response - only in period 1.

This allows attacker to prevent all exits.

Context: dev-morevp and rebased-dev-morevp branches.

Discovered bug in the way we mark in-flight exits. Setting most-significant bit of priority to 1 for in-flight exits creates a situation where any standard exit jumps before any in-flight exit in finalization queue. This also means that non-mature standard exit blocks mature in-flight exits.

Currently reworking whole thing to have following setup of bits (starting with most-significant bits):

[255-192] <- exitable timestamp
[191-1] <- unique id that represents particular exit (utxo pos or hash of tx)
[0-0] <- in-flight exit flag

Issue Type

[0] bug report
[ ] feature request

Current Behavior

Create two deposit with this same data (owner, token, amount) generate the same deposit bytes.
Then getStandardExitId isn't unique, preventing exits

Expected Behavior

Change processDeposit to create unique deposit bytes.

• Command that caused error: function getStandardExitId(bytes32 _txhash, uint8 _oindex) and function _processDeposit(bytes _depositTx, PlasmaCore.Transaction memory decodedTx)

Why

Current interaction logic between SE and IFE is complex, this make the code of IFE and SE really complex and highly coupled, and any security bug would be really hard to know.

Proposal

Instead of blocking IFE or SE from starting, we make sure an output can only exit once during "process exit". In other words, we allow co-existing SE and IFE with same output, just only the first one would really transfers the fund. The later ones would just check already finalized and do nothing during "process exit".

previous discussion: here

**This does not solve the issue of IFE collides with SE from input, see #110 **

What we need to make decision

• analyze the gas cost
• Check does it worth to decrease the complexity in contract but add complexity in watcher and child-chain.

As a trader, I can exit from deposit (to venue) transaction from root chain.

• spec doc: here
• https://omisego.atlassian.net/browse/DEX-65

Issue Type

[x] bug report
[ ] feature request

Current Behavior

From the code in _processDeposit(...), there's the following comment:

// Check that all but first inputs are 0.

but the implementation checks output instead of input:

require(decodedTx.outputs[i].amount == 0);

code link: here

Expected Behavior

Comment and implementation should be consistent.

Steps to Reproduce

N/A

Suggested Fix

Motivation for Change

System Specs

Issue Type

[x] bug report
[ ] feature request

Current Behavior

Operator can steal funds from root chain account.

Steps to Reproduce

1. Operator creates two transactions tx1 and tx2 double-spending shared input a
2. Operator create IFE from transactiontx1 and doesn't include it in block
3. IFE finalizes
4. Operator creates a block that includes tx2
5. Operator create IFE from transaction tx2
6. During period 1 other player challenges canonicity of tx2
7. Operator responds to challege by providing inclusion proofs.

Second double spend IFE exiting with high priority (priority of input a)

Suggested Fix

in function _processInFlightExit check inputs of canonical IFE transaction in exits (mapping from contract)

1. if all inputs aren't in exits, add inputs to exits and IFE stay canonical
2. otherwise IFE transactions become non-canonical

This introduces malleability and makes parsing of tx on client side harder.

Successfully challenged exits stay in priority queue (since it does not support random access deletion) and are cleared in finalizeExits loop. Event should be emitted if and only if funds are being transferred.

Implement the registry code for plasma framework

function 'respondToNonCanonicalChallenge' should emit an event of some type so observing processes could react