overflow/underflow mitigation

[err508]

When reviewing TokenNetwork.sol, some overflow/underflow issues arose. In this issue we should discuss how to adapt and fix, w.r.t to typing and testing. Potential mitigations paths are:

i) changing from UINT to INT, as suggested by @hackaugusto This would allow to do something like require(x>=0), helping to mitigate the underflow vectors.

ii) Use SafeMaths add and sub: https://github.com/OpenZeppelin/openzeppelin-solidity/blob/6e66ba321e545c7840f51ba978a0dd9aaad3ad99/contracts/math/SafeMath.sol#L27-L31

ii)Decrease contract internal type_size to UINT128 to do the computations in. Add a modulo logic in INT256 to give the contract the space to represent/deal with overflows/underflows. This would also decrease gas cost for storage but must be evaluated and formalized first.

iv) Add a corresponding testing environment that mimics the contract behaviour, to check that local and contract-internal computation is identical.

[err508]

114 #116 # raiden/#1529

[hackaugusto]

i) changing from UINT to INT, as suggested by @hackaugusto

This is not necessary, we are using uint256 because that is what is used by the ERC20 token standard, converting back and forth from these two types would probably be a bad idea.

[loredanacirstea]

i) changing from UINT to INT

Same - opens a whole other set of issues - compatibility with token standards, conversion issues between uint and int, max int value lower than max uint value etc. http://solidity.readthedocs.io/en/v0.4.24/types.html#conversions-between-elementary-types

ii) Use SafeMath

SafeMath would only force the transaction to fail. We don't want that in settleChannel, for example - this is where we currently have issues. In other functions, we can have proper checks in place (like we have now). I'm not against using SafeMath for all other functions except settleChannel, but last time this was discussed for uRaiden, we settled on not using additional libraries. We can reopen this discussion.

ii)Decrease contract internal type_size to UINT128 to do the computations in. Add a modulo logic in INT256

• decrease size for what variables? It would help to have a proposed implementation, to see exactly what you have in mind.

[LefterisJP]

Why not add the checks ourselves where needed like we did in uRaiden and in the token sale contract where under/overflow checks are needed?

[err508]

Same - opens a whole other set of issues - compatibility with token standards, conversion issues between uint and int, max int value lower than max uint value etc.

I don't see why would have to do conversions when using INT, I'm not aware of a place in the repo where we want to use and convert a negative number. It would only be an easy require, where we now have to think about and check for underflows.

decrease size for what variables? It would help to have a proposed implementation, to see exactly what you have in mind.

My idea was to use the space between UINT128 and UINT256 to check for flow_errors and safe gas. But that is not necessary if we settle for SafeMath.

SafeMath would only force the transaction to fail. We don't want that in settleChannel, for example - this is where we currently have issues.

I think some transactions must fail. We do computations over thefinite ring ℤ mod 256, but actually want to do token transfers, which we think of in ℤ. This model will always break when computing crosses its upper or lower boundaries. In fact the cases where this works are finite, where the cases when it fails are infinite. I would therefore not recommend to try to handle these cases and instead prefer to fail safely.

[hackaugusto]

I think some transactions must fail.

As loredana said We don't want that in settleChannel, the smart contract must always settle the channel, otherwise a participant can provide an old balance proof and funds will be locked. So, for the settleChannel function safe math is not the correct approach, since it uses asserts

[loredanacirstea]

Was thinking a bit about this and I think we should consider:

• indeed failing the settleChannel transaction ONLY the issue raised about very big transferred_amounts, with an overflow risk.
• considering https://github.com/raiden-network/spec/issues/77#issuecomment-395014293 - allowing cooperativeSettle during/after settlement window to get back locked tokens.

Notes:

• also handling the overflow on a big transferred_amount would complicate things if you want to somehow solve it in settleChannel (e.g. take the biggest possible transferred_amount that does not trigger an overflow). I don't like to complicate the smart contracts code.
• cooperativeSettle will be the default settlement method. The uncooperative settle process will happen if one of the nodes is offline. Therefore, even if cooperativeSettle is an option, it may not be an option for some cases where the partner will not return online. Granted, the overflow issue described would also be very rare in my opinion.

[hackaugusto]

considering raiden-network/spec#77 (comment) - allowing cooperativeSettle during/after settlement window to get back locked tokens.

uncoopSettle is the fallback to the coopSettle. Why do you expect it to work after a failure? IMO failing in uncoopSettle is not acceptable.

TL;DR a variation of suggestion iii from @err508 with a tighter representation: Reserve the highest order bit for addition overflow, define the valid values for deposit, withdraws, and transferred amounts to be in the range [0,2**255) and use uint256 to represent it, this allows for one addition without having to worry about overflows.

So, here are the expressions that can overflow (Ignoring setTotalDeposit and setTotalWithdraw which IMO are handling it correctly as of 65774576b77f4614d87c024521eea079ebcd4076, an addition of settle_timeout and current block number, and the merkle tree unlock code):

https://github.com/raiden-network/raiden-contracts/blob/65774576b77f4614d87c024521eea079ebcd4076/raiden_contracts/contracts/TokenNetwork.sol#L777-L782

To fix this case, we can use the uint256 to represent the deposit and withdraw values, but limit the values to be at most uint255 in the setTotalDeposit and setTotalWithdraw functions (IOW the highest bit must not be set). The addition would not overflow in the uint256 representation, we know the sum of the withdraws is always smaller than the sum of the deposits, so subtracting the withdraws after the deposit are summed wont underflow, and we don't need to add logic to handle either case.

https://github.com/raiden-network/raiden-contracts/blob/65774576b77f4614d87c024521eea079ebcd4076/raiden_contracts/contracts/TokenNetwork.sol#L538-L543

For this snippet we can do the same, since we only have one addition only one bit is required.

The down side is that we need to always keep track of how many bits are required for the overflow, and define the valid range accordingly, but I think it's worth it since the valid ranges will still be quite large and we don't have to add code to handle the overflows (underflow for old balance proofs needs to be handled though)

[loredanacirstea]

To fix this case, we can use the uint256 to represent the deposit and withdraw values, but limit the values to be at most uint255 in the setTotalDeposit and setTotalWithdraw functions (IOW the highest bit must not be set).

This solves an issue that is already solved. (IMO are handling it correctly as of 6577457)

The issue would still be enforcing transferred_amount < 2**255. It is EASY to limit the transferred_amount in the Raiden client. If you want to make settleChannel bullet proof for any other client, who can effectively put any transferred_amount there (he can put anything there actually, contract does not check the balance_hash), then:

transferred_amount can only be controlled in settleChannel and you either:

• don't allow a big value (enforce a smaller uint size for it, enforce a smaller uint size for deposits, withdraws as suggested) -> transaction can still fail if there was a big value set in balance_hash
• you allow a big value and try to compute the most fair settlement amounts that you can (e.g. take the max transferred_amount that does not trigger an overflow; very tricky to solve correctly)

[hackaugusto]

you allow a big value and try to compute the most fair settlement amounts that you can (e.g. take the max transferred_amount that does not trigger an overflow; very tricky to solve correctly)

Having the highest bit set would be invalid by the protocol, two honest parties would not do it, two dishonest parties can do it and the results are undefined. For the important case, one honest + dishonest party, the honest party would be favored as we do it right now (if overflow, set it to UINT256MAX).

[loredanacirstea]

The overflows and underflows are now handled correctly (until proven wrong) in the smart contracts:

• settleChannel handles them without throwing - in other words, the issue discussed above was fixed in https://github.com/raiden-network/raiden-contracts/pull/123
• all the rest of the functions have checks in place and throw using require

ii) Use SafeMaths add and sub

You can open a separate issue for this, to track conclusions better. If you want to do this, consider to also add a test counting the number of + & - , so that no additional functionality is added in the future that would use -, + instead of add, sub.

iv) Add a corresponding testing environment that mimics the contract behaviour

Yes, part of testing. This should be a separate issue for each function that needs this. @err508 's current PR https://github.com/raiden-network/raiden-contracts/pull/128 already contributes to this.

What else is there to solve/discuss here @err508? Can we close the issue?

[err508]

You can open a separate issue for this, to track conclusions better.

I would suggest to keep this issue open to keep track of the arguments put forward and decisions being made, also to smooth on-boarding for future collaborators. The title fits all problems in the repo regarding vectors due to modulo arithmetic in the contracts.

[pirapira]

No, this repo doesn't have an issue that is not closable.