Removes redundant partner argument from setTotalWithdraw. #251

err508 · 2018-08-22T13:41:38Z

Removes redundant partner argument from setTotalWithdraw, as described in #238. Additionally, the function was refactored to be more clear and readable, as discussed in #47. Closes #238.

as described in raiden-network#238. Additionally, the function was refactored to be more clear and readable, as discussed in raiden-network#47.

loredanacirstea · 2018-08-22T16:02:50Z

raiden_contracts/contracts/TokenNetwork.sol

        Channel storage channel = channels[channel_identifier];
        Participant storage participant_state = channel.participants[participant];
        Participant storage partner_state = channel.participants[partner];

        total_deposit = participant_state.deposit + partner_state.deposit;
-        // This should never happen, as we have an overflow check in setTotalDeposit
-        assert(total_deposit >= participant_state.deposit);
-        assert(total_deposit >= partner_state.deposit);


Why were these moved? These were placed here in #248, after reopening #47 (comment) - after the internal audit discussion

I moved them here as described in #47. It states "the code finishes with the asserts/invariants"

In this case, I considered readability to be more important. Why did you not comment on #248? It is very inefficient to change and revert code back and forth.
And this discussion is not in the scope of this PR, which should have only fixed the redundant partner argument.

If you wish to change the order again, please do it in another PR.

I guess readability is a little subjective, I'd prefer to stick to the ordering proposed in #47. I wasn't aware of #248 or requested for review.

tl;dr why are we discussing checks order in a PR that should have fixed something else?
Then I would rather have this discussed in a separate PR. Especially that this seems so important for you. The #248 was labeled for review. Anyone could have reviewed it and everyone is incentivized to review PRs regardless of assignment, especially if they have strong opinions.

loredanacirstea · 2018-08-22T16:13:45Z

raiden_contracts/contracts/TokenNetwork.sol


        // Entire withdrawn amount must not be bigger than the current channel deposit
-        require(total_withdraw <= (total_deposit - partner_state.withdrawn_amount));
+        require((total_withdraw + partner_state.withdrawn_amount) <= total_deposit);


This is actually better, but now I see that it misses an overflow check:

require(total_withdraw <= total_withdraw + partner_state.withdrawn_amount)

Overflows are out of the agreed upon contracts scope here (i.e. assuming one misbehaving client max.) - signatures from both clients are required, so they would have to agree upon a corrupted withdrawal request, where one party would loose tokens. I can add a check anyhow.

signatures from both clients are required, so they would have to agree upon a corrupted withdrawal request

Correct. However, the overflow check that I mentioned makes sure that a participant (with a collaborating partner) cannot withdraw more tokens from the TokenNetwork contract.

Issue: total_deposit is big enough to cause an overflow on total_withdraw + partner_state.withdrawn_amount & pass the check here: require((total_withdraw + partner_state.withdrawn_amount) <= total_deposit);

No other checks fail before require(token.transfer(participant, current_withdraw));. This can potentially transfer a lot of tokens to participant if the TokenNetwork contract has enough tokens or if the token contract is not properly implemented.

loredanacirstea · 2018-08-22T16:16:49Z

raiden_contracts/contracts/TokenNetwork.sol

        participant_state.withdrawn_amount = total_withdraw;
+        require(token.transfer(participant, current_withdraw));


Why did you change the order? It reverts changes done in #248 - e.g. d034f81

This triggered a warning when static analysis tools were used, because emitting an event is considered a state change.
The pattern that was checked for is: make sure all the state changes are made before any external calls (e.g. token transfers).

Made more sense to me to emit the event only after the require(transfer) did not throw. Otherwise if the event is emitted and code by a Node, it might be in an ill defined state no?

If the require(transfer) throws, then all changes are reverted, including the event.

To explain a bit more:
The entire transaction (including all the calls to external contracts done inside the transaction) is atomic.
Emitted events cannot be accessed from inside the EVM. They are for the outside world. Therefore, there cannot be a pattern where somehow, the emitted event can be used maliciously inside the EVM before the transaction is reverted due to an external call (it is not possible to have some kind of cross-function race condition with events)

ok thx for the explanation, I'll place it back to where it was.

Added overflow check to withdraw function.

`partner` argument was removed from `setTotalWithdraw` in raiden-network/raiden-contracts#251

err508 added the dev: Please Review label Aug 22, 2018

err508 added 2 commits August 22, 2018 15:42

Removes redundant partner argument from setTotalWithdraw,

afffe20

as described in raiden-network#238. Additionally, the function was refactored to be more clear and readable, as discussed in raiden-network#47.

Fix auxiliaries after withdraw refactor.

b8e2138

err508 force-pushed the simplify_withdraw branch from 9951d37 to d75821d Compare August 22, 2018 13:42

err508 closed this Aug 22, 2018

err508 reopened this Aug 22, 2018

err508 force-pushed the simplify_withdraw branch from d75821d to 18156df Compare August 22, 2018 14:59

loredanacirstea suggested changes Aug 22, 2018

View reviewed changes

err508 force-pushed the simplify_withdraw branch 4 times, most recently from ecd81a2 to ca005d0 Compare August 23, 2018 12:52

Adapt changes to test_cannel_withdraw.

6bb090d

Added overflow check to withdraw function.

err508 force-pushed the simplify_withdraw branch from ca005d0 to 6bb090d Compare August 23, 2018 14:13

err508 mentioned this pull request Aug 23, 2018

Remove partner argument from setTotalWithdraw. raiden-network/raiden#2240

Closed

loredanacirstea approved these changes Aug 23, 2018

View reviewed changes

loredanacirstea merged commit 15d7328 into raiden-network:master Aug 23, 2018

loredanacirstea added a commit to loredanacirstea/spec that referenced this pull request Aug 27, 2018

Fix setTotalWithdraw specs after removing partner argument

5d0fc4f

`partner` argument was removed from `setTotalWithdraw` in raiden-network/raiden-contracts#251

loredanacirstea mentioned this pull request Aug 27, 2018

Fix sc proof specs raiden-network/spec#113

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes redundant partner argument from setTotalWithdraw. #251

Removes redundant partner argument from setTotalWithdraw. #251

err508 commented Aug 22, 2018 •

edited

Loading

loredanacirstea Aug 22, 2018

err508 Aug 23, 2018

loredanacirstea Aug 23, 2018

err508 Aug 23, 2018 •

edited

Loading

loredanacirstea Aug 23, 2018 •

edited

Loading

loredanacirstea Aug 22, 2018

err508 Aug 23, 2018

loredanacirstea Aug 23, 2018

loredanacirstea Aug 22, 2018

err508 Aug 23, 2018

loredanacirstea Aug 23, 2018

loredanacirstea Aug 23, 2018

err508 Aug 23, 2018

		participant_state.withdrawn_amount = total_withdraw;
		require(token.transfer(participant, current_withdraw));

Removes redundant partner argument from setTotalWithdraw. #251

Removes redundant partner argument from setTotalWithdraw. #251

Conversation

err508 commented Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

err508 Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

loredanacirstea Aug 23, 2018 • 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

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

err508 commented Aug 22, 2018 •

edited

Loading

err508 Aug 23, 2018 •

edited

Loading

loredanacirstea Aug 23, 2018 •

edited

Loading