Rlp meltdown #476

kevsul · 2019-11-22T17:22:34Z

Extra restrictions to RLP and tx decoding:

refactored RLPReader.sol yo remove duplicate code
RLPReader.toUint() treats leading zeros as invalid
RLPReader.toAddress() requires the item data to be exactly 20 bytes.
RLPReader.toBytes32() requires the item data to be exactly 32 bytes.
Transaction inputs must be bytes32
Transaction inputs cannot be empty (all zeros)
Transaction outputs cannot be empty (outputGuard == 0 AND amount == 0)
Transaction metadata must be bytes32

plasma_framework/contracts/src/transactions/PaymentTransactionModel.sol

boolafish · 2019-11-25T02:03:19Z

plasma_framework/contracts/src/transactions/PaymentTransactionModel.sol


        return Transaction({txType: txType, inputs: inputs, outputs: outputs, metaData: metaData});
    }
+
+    function isOutputEmpty(PaymentOutputModel.Output memory output) internal pure returns (bool) {


We have some places that was treating amount == 0 as empty. Might worth take a looks and see if those places should change to be using this or not. Probably does not make sense to change but let double check.

https://github.com/omisego/plasma-contracts/blob/master/plasma_framework/contracts/src/exits/payment/controllers/PaymentStartStandardExit.sol#L168

https://github.com/omisego/plasma-contracts/blob/master/plasma_framework/contracts/src/exits/payment/controllers/PaymentProcessInFlightExit.sol#L208-L224

plasma_framework/contracts/src/utils/RLPReader.sol

boolafish · 2019-11-25T04:59:18Z

plasma_framework/contracts/src/utils/RLPReader.sol

     * @dev This function is dangerous !!! Ensure that the returned length is within the bounds of where memPtr points to.
     * @param memPtr Pointer to the dynamic bytes array in memory
-     * @return The encoded RLPItem length
+     * @return The length of the RLPItem (including the length field) and the offset of the payload


should it be "including the offset"?

I mean the total length - length of the length field and the data

ah...just see offset is in the later part of the sentence

plasma_framework/contracts/src/utils/RLPReader.sol

boolafish · 2019-11-25T05:03:46Z

plasma_framework/contracts/src/utils/RLPReader.sol

     */
-    function decodeItemLengthUnsafe(uint256 memPtr) internal pure returns (uint256) {
-        uint256 decodedItemLengthUnsafe;
+    function decodeLengthAndOffset(uint256 memPtr) internal pure returns (uint256, uint256) {


Can we just pass in the RLPItem and also do the length check done in the RLP spec python example?

https://github.com/ethereum/wiki/wiki/RLP

I don't think it easy to do it like that. In the python example they use string manipulation with substr and concatenation, but this isn't so easy in Solidity. Memory pointers seem to be a better fit.

oh, I am not suggesting about the memory pointer part. But if just compare the two functions, the python example also has conditions like length > prefix - 0x80 in the branchings. If we pass in the RLPItem, I think we can use the item.len to match the logic in python.

Or are they actually different things?

They're different. In python length = len(input), but in Solidity you only have the length of the overall buffer. You don't get the length of each item item.len (until you decode it)

I mean, in python length is the length of the remainder of the buffer, which is not the same as the item.len.

okay, let me try to understand the assemblies again 😅

boolafish · 2019-11-25T05:17:43Z

lol, it is so much "fun" to read the assemblies, ewwww

…sma-contracts into rlp-meltdown-gerhard

Rlp meltdown gerhard

boolafish · 2019-11-27T09:16:12Z

plasma_framework/contracts/src/transactions/PaymentTransactionModel.sol

+     * @param tx An RLP-encoded transaction
+     * @return A decoded PaymentTransaction struct
+     */
+    function decode(bytes memory tx) internal pure returns (PaymentTransactionModel.Transaction memory) {


my IDE argues about the tx variable

This declaration shadows a builtin symbol.

Nice catch, I wonder why sol linter isn't picking these up?

thec00n · 2019-11-28T04:12:21Z

plasma_framework/contracts/src/transactions/PaymentTransactionModel.sol

            inputs[i] = input;
        }

        PaymentOutputModel.Output[] memory outputs = new PaymentOutputModel.Output[](rlpOutputs.length);
        for (uint i = 0; i < rlpOutputs.length; i++) {
            PaymentOutputModel.Output memory output = PaymentOutputModel.decode(rlpOutputs[i]);
+            require(output.outputType > 0, "Output type must not be 0");


1.) Why are the checks related to PaymentOutputModel not in decode?
2.) I think we concluded that the amount has be greater than 0?

1.) Why are the checks related to PaymentOutputModel not in decode?

PaymentOutputModel is going to be removed as part of refactor, will be in the next PR

2.) I think we concluded that the amount has be greater than 0?

OK

thec00n · 2019-11-28T04:15:37Z

Can we add tests that cover all loc/branches in https://coveralls.io/builds/27286020/source?filename=contracts/src/utils/RLPReader.sol
Is the plan to remove https://github.com/omisego/plasma-contracts/blob/11a731267140cb8f54084f456c08f97ada8d0206/plasma_framework/contracts/src/transactions/WireTransaction.sol? If not than we need to add checks there as well.

kevsul · 2019-11-28T08:49:56Z

Can we add tests that cover all loc/branches in https://coveralls.io/builds/27286020/source?filename=contracts/src/utils/RLPReader.sol

Yep

Is the plan to remove https://github.com/omisego/plasma-contracts/blob/11a731267140cb8f54084f456c08f97ada8d0206/plasma_framework/contracts/src/transactions/WireTransaction.sol? If not than we need to add checks there as well.

Will be refactored.

…racts into rlp-meltdown

thec00n

Made a few minor doc improvements: #490
Otherwise LGTM.

RLP meltdown: some minor documentation improvements

kevsul added 3 commits November 22, 2019 17:49

fix: rlp catch invalid leading zeros

237a6e2

fix: rlp meltdown fixes

531fdd8

fix: merge conflict

6f7564c

kevsul requested review from boolafish, pgebal and thec00n November 22, 2019 17:22

boolafish reviewed Nov 25, 2019

View reviewed changes

thec00n and others added 7 commits November 25, 2019 14:52

chore: review, add tests, code improvements

fcd91a7

fix: sol linter errors

d5570d7

chore: remove unnecessary check in toList

192a096

chore: add negative test case for toAddress

5cd343c

chore: add negative test case for toAddress

3833b1c

Merge branch 'rlp-meltdown-gerhard' of https://github.com/omisego/pla…

6854c90

…sma-contracts into rlp-meltdown-gerhard

Merge pull request #479 from omisego/rlp-meltdown-gerhard

034ed4e

Rlp meltdown gerhard

kevsul mentioned this pull request Nov 25, 2019

fix: rlp catch invalid leading zeros #444

Closed

kevsul added 6 commits November 25, 2019 15:46

refactor: consistent var name changes

89a0c2c

fix: '0x00' should not be decoded as uint

911eb35

update with CR comments

a449343

fix: Check for 0 transactionType or outputType

5e84268

fix: remove isOutputEmpty check

7646f2b

Merge branch 'master' into rlp-meltdown

04014b3

boolafish reviewed Nov 27, 2019

View reviewed changes

kevsul added 3 commits November 27, 2019 11:11

lint: lint fix

d516ef9

lint: more lint

9ae9b29

Merge branch 'master' into rlp-meltdown

11a7312

thec00n reviewed Nov 28, 2019

View reviewed changes

thec00n self-requested a review November 28, 2019 04:13

kevsul and others added 8 commits November 28, 2019 14:55

fix: disallow zero amount in outputs

fdd7a84

test: more invalid rlp tests

8a37326

fix: remove unnecessary rlp length of length checks

0f19173

Merge branch 'master' into rlp-meltdown

fbdbfdd

fix: python deposit test

0d26d60

Merge branch 'rlp-meltdown' of https://github.com/omisego/plasma-cont…

041ad08

…racts into rlp-meltdown

Merge branch 'master' into rlp-meltdown

b201003

some minor documentation improvements

aeb1eac

thec00n approved these changes Nov 29, 2019

View reviewed changes

kevsul added 2 commits November 29, 2019 09:28

Merge pull request #490 from omisego/rlp-meltdown-doc-improvements

8109143

RLP meltdown: some minor documentation improvements

Merge branch 'master' into rlp-meltdown

afe5f8c

kevsul merged commit d65293d into master Nov 29, 2019

kevsul deleted the rlp-meltdown branch November 29, 2019 09:10

thec00n mentioned this pull request Dec 2, 2019

Strict RLP decoding / Enforce PaymentTransaction according to spec omgnetwork/elixir-omg#1176

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rlp meltdown #476

Rlp meltdown #476

kevsul commented Nov 22, 2019

boolafish Nov 25, 2019

boolafish Nov 25, 2019 •

edited

Loading

kevsul Nov 27, 2019

boolafish Nov 27, 2019

boolafish Nov 25, 2019

kevsul Nov 27, 2019

boolafish Nov 27, 2019 •

edited

Loading

kevsul Nov 27, 2019 •

edited

Loading

kevsul Nov 27, 2019

boolafish Nov 27, 2019

boolafish commented Nov 25, 2019

boolafish Nov 27, 2019

kevsul Nov 27, 2019 •

edited

Loading

thec00n Nov 28, 2019

kevsul Nov 28, 2019 •

edited

Loading

kevsul Nov 28, 2019 •

edited

Loading

thec00n commented Nov 28, 2019

kevsul commented Nov 28, 2019

thec00n left a comment

Rlp meltdown #476

Rlp meltdown #476

Conversation

kevsul commented Nov 22, 2019

Choose a reason for hiding this comment

boolafish Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boolafish Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

kevsul Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boolafish commented Nov 25, 2019

Choose a reason for hiding this comment

kevsul Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevsul Nov 28, 2019 • edited Loading

Choose a reason for hiding this comment

kevsul Nov 28, 2019 • edited Loading

Choose a reason for hiding this comment

thec00n commented Nov 28, 2019

kevsul commented Nov 28, 2019

thec00n left a comment

Choose a reason for hiding this comment

boolafish Nov 25, 2019 •

edited

Loading

boolafish Nov 27, 2019 •

edited

Loading

kevsul Nov 27, 2019 •

edited

Loading

kevsul Nov 27, 2019 •

edited

Loading

kevsul Nov 28, 2019 •

edited

Loading

kevsul Nov 28, 2019 •

edited

Loading