raiden-network/raiden-contracts

Raiden Network Smart Contracts

raiden-contracts's Issues

Update Readme instructions

After removing populus, we have outdated instructions in the Development section.

E.g.

We use populus for development. At the moment, this library is incompatible with recent libraries that act as dependencies. Therefore, we have to use different environments here.

• add a link to the official smart contracts specification docs. - #300
• add disclaimer - #311
• maybe add a link to the Raiden Network gitter chat? - #300
  Make sure we have up to date instructions for:
• installation - #304
• usage - #304
• compilation - #304
• deployment - #307
• testing - #304

Implement withdraw functionality for TokenNetwork

Implement withdraw functionality. Described in raiden-network/spec#26

Add unit test for unlocking tokens after multiple close-reopen channel cycles

Make sure unlock can be called multiple times if multiple channels are open/closed - channel_identifier_to_unlock_data[channel_identifier] must not be deleted between channel settlements and channel (re)opening.

Triggered by #85 (comment).

Implement hashed balance proof changes in updateTransfer

As described in raiden-network/spec#25 (comment)

[MS] Make minimum_deposit modifiable according to token price

Problem definition

In the Monitoring Service smart contract we want a minimum deposit to be made in order to register as a Monitoring Service. Right now the value is not modifiable and is specified when the contract is created. We need to come up with a good way to be able to adjust the minimum_deposit without already registered MSs having to deposit more tokens.

Possible solution

Currently two approaches have been suggested:

1. Have an owner that can update the minimum_deposit on price fluctuation
2. Use an oracle to update the value when price is fluctuating

A third solution could also be to just hard code a value that reflects some fraction of the total token supply.

Next steps

• Discuss what the best solution is
• Implement solution
• Update spec

Use error strings in smart contracts

Description

Since solidity 0.4.22 error strings are supported for revert and require:

pragma solidity ^0.4.22;

contract VendingMachine {
    function buy(uint amount) payable {
        if (amount > msg.value / 2 ether)
            revert("Not enough Ether provided.");
        // Alternative way to do it:
        require(
            amount <= msg.value / 2 ether,
            "Not enough Ether provided."
        );
        // Perform the purchase.
    }
}

Adding this to the smart contract will make it easier to debug where things go wrong in a function. It also makes the terrible work flow of removing requires to figure out which require is failing obsolete.

Tasklist

• SecretRegistry - no requires
• TokenNetworkRegistry (#1498)
• TokenNetwork (#1499)
• Controllable (#1492)
• MonitoringService
• OneToN
• ServiceRegistry
• UserDeposit

Assess keeping invariants grouped

(must be updated with a proper description)

As mentioned in this thread: #39 (comment)

keeping the invariants grouped? I think it is easier to verify them if we don't need to jump around in the contract code.
IMO this should not take priority over clarity if the input is invalid, only in cases where the input is valid but a race happened (e.g. two monitoring services called update at the same time, only one of the transactions will work)

Cannot call settleChannel without updateNonClosingBalanceProof

One should be able to settle a channel without needing to provide balance proofs.
This can happen when:

• no off-chain transfers were done
• updateNonClosingBalanceProof was not called

settleChannel currently fails when balance proof values are 0.

Group requires in the MS smart contract

Make the MonitoringService smart contract comply with this suggested style change: #47

Documenting smart contracts refactoring

The scope of this issue is to document the initial smart contracts refactoring discussed here: raiden-network/raiden#1286.
The output of this discussion is the first repository commit 921c006 .

Check if order is important in settleChannel calculations for owed tokens

raiden-network/raiden#1286 (comment)

This changed the semantics of the code, the order of payments is not arbitrary, the node that did not close the channel must be paid first.

Old code: counter_net (non-closing participant settlement token amount) was calculated first
New code: order does not matter

Why would the order matter?

Use idempotent deposit function in MS contract?

Comment from Loredana:

In the TokenNetwork contract we have decided to use an idempotent setDeposit function, which receives the total amount of tokens that should end up as deposited in the contract, instead of the amount that is added when calling the function. The main reason for this was to ensure that you don't deposit tokens by mistake, due to an additional API call.
I think we should be consistent and also use this approach here.

See how it's done in the TokenNetwork contract and implement similar functionality in the MS contract.

---

edit by @pirapira: In MSC and UDC, currently, deposit(totalDeposit) can cause more-than-expected deposits. This totalDeposit pattern works in TokenNetwork because it has only-ever-increasing totalDeposit. In MSC and UDC, balance[user] might decrease as well.

Is it possible Recreate a channel to prevent parnter to unlock?

1.A create a channel with B on RDN
2.A send 100 RDN to B, but without sending B the Unlock Message.
3. B call SecretRegistry, register the secret.
3. A close channel
4. settle channel
5. re create a channel with B with very small settle time ,like 6 blocks
6. close and settle the new channel.

if B doesn't monitoring the channel event between step 4 and step 6(that is about 7 blocks time).
then B will lose the 100 RDN.

A short time DoS attach to B, only less than 2 minutes, will make B lose 100 RDN.
am I right?

Make contracts installable as pip package

To integrate this repo with other projects, we'll need a working setup.py that allows us to install the contracts as a pip package.

Goals

Installable setup.py

working with pip install -e . and py setup.py build targets

utility library

setup.py should also have a contract compilation target that'll build the ABI & bytecode. These should be available with a helper library to other projects. The idea is to get rid of solc dependency for all projects except this one.

Quick & dirty example

import raiden_contracts

registry_abi = raiden_contracts.get_abi('RegistryContract')
event_abi = raiden_contracts.get_event_abi('RegistryContract', ''SomethingHappenedEvent)

Add proper tests for unlock and updateTransfer

Especially tests for computing the merkle root and verifying that we handle hashlocks correctly.

Assess using a modifier for checking channel participant addresses

require(channel.participants[participant].initialized);

As suggested in 921c006#r28362340

This check is extremely important, and it's only valid if the participant address is taken from a recoverable signature.
It seems the function is being used correctly in all call sites, but it's a bit convoluted to review, could we please improve the locality of this? (I suggested a fail-fast approach, with all invariants checked in the public interfaces)

Fix `keccak256` and fallback function breaking changes for solc 0.5.0

The following 0.5.0 breaking changes currently affect the contracts:

• fallback functions must be external (https://github.com/ethereum/solidity/blob/develop/Changelog.md#0417-2017-09-21)
• keccak256 can only be used with a single bytes argument; abi.encodePacked can be used to tightly pack arguments (https://github.com/ethereum/solidity/blob/develop/Changelog.md#0424-2018-05-16)

List of tests that should be added

• updateNonClosingBalanceProof with different nonces - success on bigger nonce, fail on smaller nonce
• add more cases to channel_test_values.py
• make sure unlock can be called multiple times if multiple channels are open/closed (channel_identifier_to_unlock_data[channel_identifier] must not be deleted between channel close and channel (re)open

EndpointRegistry - add basic unit tests

We decided to keep the EndpointRegistry contract for now - #82, we need to add some basic unit tests to make sure it works.

TokenNetwork ChannelSettled event - add the transferred amounts as arguments

event ChannelSettled(uint256 channel_identifier, uint256 participant1_amount, uint256 participant2_amount);

As discussed in raiden-network/spec#70

Asses using Enum for channel states in TokenNetwork

As mentioned in #39 (comment) and #39 (comment):

Channel state should be of Enum type for making the code clearer.
Gas costs must be assessed in order to make sure that Enums can also be packed to 32 bytes, as the current uint8 variable. Enums should be equivalent to bytes1, but this should be checked.

Add overflow and underflow checks for all operations

Make sure we have asserts for all smart contracts operations (e.g. +, -, *)

E.g. for settleChannel , getSettleTransferAmounts

        total_deposit_available = (
            participant1_deposit +
            participant2_deposit -
            participant1_locked_amount -
            participant2_locked_amount
        );

        // Overflow and underflow checks
        require(total_deposit_available <= (participant1_deposit + participant2_deposit));
        require(total_deposit_available > 0);

        participant1_amount = (
            participant1_deposit +
            participant2_transferred_amount -
            participant1_transferred_amount
        );

        // Overflow and underflow checks
        require(participant1_amount <= participant1_deposit + participant2_transferred_amount);
        require(participant1_amount > ?);

Consider using raiden-libs as a dependency

Components that can be reused:

• merkle.py

Fix solc warnings

When using the ContractManager to compile the onctracts I get shown the following warnings. These should be fixed.

Token.sol:6:5: Warning: Functions in interfaces should be declared external.
    function totalSupply() public constant returns (uint256 supply);
    ^--------------------------------------------------------------^
Token.sol:10:5: Warning: Functions in interfaces should be declared external.
    function balanceOf(address _owner) public constant returns (uint256 balance);
    ^---------------------------------------------------------------------------^
Token.sol:16:5: Warning: Functions in interfaces should be declared external.
    function transfer(address _to, uint256 _value) public returns (bool success);
    ^---------------------------------------------------------------------------^
Token.sol:23:5: Warning: Functions in interfaces should be declared external.
    function transferFrom(address _from, address _to, uint256 _value) public returns (bool success);
    ^----------------------------------------------------------------------------------------------^
Token.sol:29:5: Warning: Functions in interfaces should be declared external.
    function approve(address _spender, uint256 _value) public returns (bool success);
    ^-------------------------------------------------------------------------------^
Token.sol:34:5: Warning: Functions in interfaces should be declared external.
    function allowance(address _owner, address _spender) public constant returns (uint256 remaining);
    ^-----------------------------------------------------------------------------------------------^
TokenNetwork.sol:668:14: Warning: Use of the "var" keyword is deprecated.
        var (r, s, v) = signatureSplit(signature);
             ^
TokenNetwork.sol:668:17: Warning: Use of the "var" keyword is deprecated.
        var (r, s, v) = signatureSplit(signature);
                ^
TokenNetwork.sol:668:20: Warning: Use of the "var" keyword is deprecated.
        var (r, s, v) = signatureSplit(signature);
                   ^
Token.sol:6:5: Warning: Functions in interfaces should be declared external.
    function totalSupply() public constant returns (uint256 supply);
    ^--------------------------------------------------------------^
Token.sol:10:5: Warning: Functions in interfaces should be declared external.
    function balanceOf(address _owner) public constant returns (uint256 balance);
    ^---------------------------------------------------------------------------^
Token.sol:16:5: Warning: Functions in interfaces should be declared external.
    function transfer(address _to, uint256 _value) public returns (bool success);
    ^---------------------------------------------------------------------------^
Token.sol:23:5: Warning: Functions in interfaces should be declared external.
    function transferFrom(address _from, address _to, uint256 _value) public returns (bool success);
    ^----------------------------------------------------------------------------------------------^
Token.sol:29:5: Warning: Functions in interfaces should be declared external.
    function approve(address _spender, uint256 _value) public returns (bool success);
    ^-------------------------------------------------------------------------------^
Token.sol:34:5: Warning: Functions in interfaces should be declared external.
    function allowance(address _owner, address _spender) public constant returns (uint256 remaining);
    ^-----------------------------------------------------------------------------------------------^
TokenNetwork.sol:668:14: Warning: Use of the "var" keyword is deprecated.
        var (r, s, v) = signatureSplit(signature);
             ^
TokenNetwork.sol:668:17: Warning: Use of the "var" keyword is deprecated.
        var (r, s, v) = signatureSplit(signature);
                ^
TokenNetwork.sol:668:20: Warning: Use of the "var" keyword is deprecated.
        var (r, s, v) = signatureSplit(signature);
                   ^
Token.sol:6:5: Warning: Functions in interfaces should be declared external.
    function totalSupply() public constant returns (uint256 supply);
    ^--------------------------------------------------------------^
Token.sol:10:5: Warning: Functions in interfaces should be declared external.
    function balanceOf(address _owner) public constant returns (uint256 balance);
    ^---------------------------------------------------------------------------^
Token.sol:16:5: Warning: Functions in interfaces should be declared external.
    function transfer(address _to, uint256 _value) public returns (bool success);
    ^---------------------------------------------------------------------------^
Token.sol:23:5: Warning: Functions in interfaces should be declared external.
    function transferFrom(address _from, address _to, uint256 _value) public returns (bool success);
    ^----------------------------------------------------------------------------------------------^
Token.sol:29:5: Warning: Functions in interfaces should be declared external.
    function approve(address _spender, uint256 _value) public returns (bool success);
    ^-------------------------------------------------------------------------------^
Token.sol:34:5: Warning: Functions in interfaces should be declared external.
    function allowance(address _owner, address _spender) public constant returns (uint256 remaining);
    ^-----------------------------------------------------------------------------------------------^
/Users/paul/Projects/raiden-contracts/raiden_contracts/contracts/Token.sol:6:5: Warning: Functions in interfaces should be declared external.
    function totalSupply() public constant returns (uint256 supply);
    ^--------------------------------------------------------------^
/Users/paul/Projects/raiden-contracts/raiden_contracts/contracts/Token.sol:10:5: Warning: Functions in interfaces should be declared external.
    function balanceOf(address _owner) public constant returns (uint256 balance);
    ^---------------------------------------------------------------------------^
/Users/paul/Projects/raiden-contracts/raiden_contracts/contracts/Token.sol:16:5: Warning: Functions in interfaces should be declared external.
    function transfer(address _to, uint256 _value) public returns (bool success);
    ^---------------------------------------------------------------------------^
/Users/paul/Projects/raiden-contracts/raiden_contracts/contracts/Token.sol:23:5: Warning: Functions in interfaces should be declared external.
    function transferFrom(address _from, address _to, uint256 _value) public returns (bool success);
    ^----------------------------------------------------------------------------------------------^
/Users/paul/Projects/raiden-contracts/raiden_contracts/contracts/Token.sol:29:5: Warning: Functions in interfaces should be declared external.
    function approve(address _spender, uint256 _value) public returns (bool success);
    ^-------------------------------------------------------------------------------^
/Users/paul/Projects/raiden-contracts/raiden_contracts/contracts/Token.sol:34:5: Warning: Functions in interfaces should be declared external.
    function allowance(address _owner, address _spender) public constant returns (uint256 remaining);
    ^-----------------------------------------------------------------------------------------------^
/Users/paul/Projects/raiden-contracts/raiden_contracts/contracts/Token.sol:6:5: Warning: Functions in interfaces should be declared external.
    function totalSupply() public constant returns (uint256 supply);
    ^--------------------------------------------------------------^
/Users/paul/Projects/raiden-contracts/raiden_contracts/contracts/Token.sol:10:5: Warning: Functions in interfaces should be declared external.
    function balanceOf(address _owner) public constant returns (uint256 balance);
    ^---------------------------------------------------------------------------^
/Users/paul/Projects/raiden-contracts/raiden_contracts/contracts/Token.sol:16:5: Warning: Functions in interfaces should be declared external.
    function transfer(address _to, uint256 _value) public returns (bool success);
    ^---------------------------------------------------------------------------^
/Users/paul/Projects/raiden-contracts/raiden_contracts/contracts/Token.sol:23:5: Warning: Functions in interfaces should be declared external.
    function transferFrom(address _from, address _to, uint256 _value) public returns (bool success);
    ^----------------------------------------------------------------------------------------------^
/Users/paul/Projects/raiden-contracts/raiden_contracts/contracts/Token.sol:29:5: Warning: Functions in interfaces should be declared external.
    function approve(address _spender, uint256 _value) public returns (bool success);
    ^-------------------------------------------------------------------------------^
/Users/paul/Projects/raiden-contracts/raiden_contracts/contracts/Token.sol:34:5: Warning: Functions in interfaces should be declared external.
    function allowance(address _owner, address _spender) public constant returns (uint256 remaining);
    ^-----------------------------------------------------------------------------------------------^
/Users/paul/Projects/raiden-contracts/raiden_contracts/contracts/Token.sol:6:5: Warning: Functions in interfaces should be declared external.
    function totalSupply() public constant returns (uint256 supply);
    ^--------------------------------------------------------------^
/Users/paul/Projects/raiden-contracts/raiden_contracts/contracts/Token.sol:10:5: Warning: Functions in interfaces should be declared external.
    function balanceOf(address _owner) public constant returns (uint256 balance);
    ^---------------------------------------------------------------------------^
/Users/paul/Projects/raiden-contracts/raiden_contracts/contracts/Token.sol:16:5: Warning: Functions in interfaces should be declared external.
    function transfer(address _to, uint256 _value) public returns (bool success);
    ^---------------------------------------------------------------------------^
/Users/paul/Projects/raiden-contracts/raiden_contracts/contracts/Token.sol:23:5: Warning: Functions in interfaces should be declared external.
    function transferFrom(address _from, address _to, uint256 _value) public returns (bool success);
    ^----------------------------------------------------------------------------------------------^
/Users/paul/Projects/raiden-contracts/raiden_contracts/contracts/Token.sol:29:5: Warning: Functions in interfaces should be declared external.
    function approve(address _spender, uint256 _value) public returns (bool success);
    ^-------------------------------------------------------------------------------^
/Users/paul/Projects/raiden-contracts/raiden_contracts/contracts/Token.sol:34:5: Warning: Functions in interfaces should be declared external.
    function allowance(address _owner, address _spender) public constant returns (uint256 remaining);
    ^-----------------------------------------------------------------------------------------------^

Write Monitoring Service smart contract

Implement Monitoring Service smart contract based on this.

Review token contracts used in tests

These contracts were mostly ported from the old raiden contracts, they are only used in testing.

However, there are some weird implementations in there. E.g.:

raiden-contracts/raiden_contracts/contracts/test/StandardToken.sol

Line 19 in 81698bb

//If your token leaves out totalSupply and can issue more tokens as time goes on, you need to check if it doesn't wrap.

raiden-contracts/raiden_contracts/contracts/test/StandardToken.sol

Line 31 in 81698bb

//same as above. Replace this line with the following if you want to protect against wrapping uints.

Replace hashlock with secrethash

As changed in the terminology specs: raiden-network/spec#40

Change TokenNetwork to TokenNetworkERC20

We will have a TokenNetwork library with the core code and multiple contracts that use TokenNetwork but implement the specified token interface. E.g. TokenNetworkERC20

Disallow multiple channels between two nodes

In a discussion we concluded to disallow having multiple channels between a pair of nodes for now. A check for this should be added to the contracts.

Add tests for MS smart contract

Currently we only have one test that tests the happy case.

More tests must be added to test edge cases and scenarios where the contract should fail.

why not make channel_identifier be unique ?

why not make channel_identifier=sha3(participant1,participant2,address(tokenNetwork) ?
so channel identidifier can be used to as the id of a channel. I thinks is useful for implementation of raiden network.

if channel_identifier=sha3(participant1,participant2), there will be many channels with the same identifier, so we have to use channel_identifier and tokenNetwork as the id of a channel.

Check if we save gas cost in using a separate ClosingRequest struct

Using a ClosingRequest struct vs. storing all the channel data in the Channel struct.

Needs to be done:

• gas cost comparison between implementations for openChannel and closeChannel

raiden-network/raiden#1286 (comment)

Remove populus

Remove Populus dependency and use a shared fixtures set for tests in PFS, MS and contracts.

Reason: Populus being outdated makes it increasingly harder to maintain the dependencies with each subsequent update in the upstream libraries.

PR #59 fixes this issue.

unlocked_amount may be 0 too.

function unlock(
        uint256 channel_identifier,
        address participant,
        address partner,
        bytes merkle_tree_leaves)
        public

        // Transfer the unlocked tokens to the participant
        require(token.transfer(participant, unlocked_amount));

        // Transfer the rest of the tokens back to the partner
        if (returned_tokens > 0) {
            require(token.transfer(partner, returned_tokens));
        }

this code assumes that returned_tokens maybe 0,and unlocked_amount must greater than 0。
for example ,the caller (msg.sender) maybe partner(argument), and all locks are locked.
in this condition, returned_tokens equals nonce_or_locked_amount。

and some ERC20 token doesn't allow transfer 0 token.

Change closing parenthesis style for Solidity

As mentioned here #39 (comment)
and changed here raiden-network/raiden#1380,

this is the style that should be used:

    function recoverAddressFromSignature(
        uint64 nonce,
        uint256 transferred_amount,
        bytes32 locksroot,
        bytes32 extra_hash,
        bytes signature
    )
        constant
        internal
        returns (address)
    {
        ...

Use new ABI encoder in contract tests

Soon (in ~2 versions is what Lefteris heard was told during Edcon) the old ABI encoder will be removed from solidity. Thus we should already start testing the new ABI encoder in our tests to see if there are any problems with it before we decide to officially switch to using it.

Move EndpointRegistry from raiden to raiden-contracts

EndpointRegistry contract will be kept at least temporarily. This is used with the UDP transport protocol. It is not required by the matrix-based transport.

Providing a balance proof with a transferred_amount bigger than the deposit should fail

At the moment, providing a transferred_amount > participant's deposit does not fail.
This should fail and the settlement logic should be reviewed to make sure it is correct.

Create script to save transaction gas cost estimations to doc

tl;dr
Gas cost estimations are calculated in test_print_gas.py. This should not be ran as a test, but should be a script that generates a file with the information, which should be added in the docs.

Problem Definition

There is no way for (advanced developer) users to easily see a rundown of how much it would cost at any verion of raiden to perform common channel operations.

Solution

Add a table, probably in spec.rst with gas costs of all raiden common channel operations which should be updated every time we change the logic of the contracts or if there is any considerable contract revamp.

This way we will be able to efficiently have a versioned tracking of gas costs optimizations and also a way to identify bottlenecks that require optimization.

Discuss saving gas by not having 0 value token transfers in settleChannel

Add a conditional check such as: if (token_amount > 0) { // do the token transfer }

PROs:

• smaller gas cost for settleChannel by not doing 0 value token transfers - save ~8500 gas / transfer

CONs:

• Note Transfers of 0 values MUST be treated as normal transfers and fire the Transfer event.

https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20.md#transfer

• It may be possible to do an attack where 1 of the transfers does not go through. The safe approach would be to register the effects on the blockchain as default. Then, anything not registered would be proof of an attack.

Channel lifecycle specification

As mentioned in #39 (comment)

Main point:

It would be good to have information about the channel lifecycle until it has ended.

Add `chain_id` in the balance proof signature

Following the agreed upon specs (https://github.com/raiden-network/spec/blob/master/smart_contracts.rst#balance-proof), chain_id must be added to the balance proof.

As mentioned also in raiden-network/raiden#1286 (comment)

Changes needed to:

• closeChannel
• updateTransfer & related functions
• recoverAddressFromSignature

Gas cost optimization - storage pointer computation for mappings

Not urgent now, but should be considered after we are completely settled on the contracts ABIs.
Any other gas cost optimization suggestions or issues should be gathered here for now.

• 921c006#r28074132

How clever is the solidity compiler for repeated expressions like channels[last_channel_index]? Will it notice that this is the same storage location and compute the pointer only once?

Fix nonce check in closeChannel

As mentioned in 0634fa0#r28362284, nonce check must be done before trying to recover partner_address. Otherwise, the transaction will fail.

raiden-contracts/raiden_contracts/contracts/TokenNetwork.sol

Line 276 in 3bcb0e4

if (nonce > 0) {

See if uint64 for block numbers should be changed to uint256

SecretRegistry & TokenNetwork

raiden-network/raiden#1286 (comment)

Maybe we should upcast the block number? My idea is to match the block.number type.

Needs to be done:

• see if there are any storage gas cost optimizations from uint64. This is the main reason to downcast.

Add a batch version of unlock

As described here: raiden-network/spec#35

Compile contracts data automatically before package release

TokenNetwork data structures - assess if they are optimal

Rethink data structures and assess if they are optimal:

• clear code
• gas cost efficient

This should happen after all agreed upon features are implemented. Functionality first, then optimizations.

Implement cooperativeSettle as per specifications

https://github.com/raiden-network/spec/blob/master/smart_contracts.rst

function cooperativeSettle(
    uint channel_identifier,
    address participant1,
    address participant2,
    uint256 balance1,
    uint256 balance2,
    bytes signature1,
    bytes signature2)
    public

Set Travis to actually run tests