Without the orbital tool, code coverage is poor and the Withdraw tests aren't run. The reason for this is to make sure the Orbital tool works with the Mixer, but also because the Mixer uses globally unique ring IDs which will change every test run which makes using static test data a pain.

For example:

• Alice Deposits into a Ring of size 4 (A)
• All inputs are Withdrawn into 4 other Rings (B, C, D & E), each of size 4.
• This requires 12 additional Deposits.

When a Withdrawal is made from A, the probability that it is Alice is 1/4.

When a Withdrawal is made from B, C, D or E the probability that it is Alice is at least 1/16.

For more information, see:

• https://eprint.iacr.org/2017/338.pdf
• monero-project/monero#1673 (comment)
• https://lab.getmonero.org/pubs/MRL-0004.pdf
• http://monerolink.com/monerolink.pdf

The current mixer only supports ether deposits and withdrawals, however the deposit function does not check that the ether value passed matches the denomination in the function parameter. This could cause this ring or other rings withdrawals to fail, as insufficient funds would be held by the contract.

http://truffleframework.com/docs/getting_started/packages-ethpm

e.g.

 truffle install clearmatics-mobius

For functions which could be pure, e.g. all of the bn256g1, anything which doesn't reference state variables.

See:

• ethereum/solidity#3388
• ethereum/solidity#2966

There is no way to recover tokens from an unfilled Ring.

Some options are:

1. Make the remaining deposits to fill the ring and unlock funds
2. Allow removal of a specific participant (and their token) by providing their non-anonymous signature

The first is potentially expensive and an observer will see that the same gas payer has filled the ring.

The second will link the gas payer to the key owner.

Within the Mixer contracts Withdraw function the signature is checked for validity, including checking for the existence of the tag, then the 'tag' is added to ensure the same signer doesn't attempt to withdraw again.

Would this not be more readable if this were split into two checks, one where we require the signature tag not to already exist and the second to validate the signature.

require (AddTag)
require (SignatureValid)

The AddTag function would check for the existence of the tag, and return false if tag already exists or add and return true if it doesn't. This would also be consistent with the AddParticipant logic in the Deposit function.

Once solidity v0.4.22 is released, update all revert and require statements with a reason

If a user submits a Withdraw message there is nothing preventing others from replaying the same message.

This is a concern if:

1. The Withdraw fails due to lack of Gas
2. The replayed message to be processed before the real message (e.g. no guarantee of ordering)

Until this problem is solved it would be possible for an attacker to monitor all rings and retransmit Withdraw messages with no cost of failure.

The mixer contracts are short-lived and one-time-use contracts and thus there will be plenty of them deployed contiuously.

For that reason, I think it's a good idea to destroy the mixer contracts after use. The data in it is no longer useful after all withdrawals.

example/README.md still uses the old Orbital syntax.

example/example.json needs an update too.

It is possible to make the mixer support both ERC-223 and ERC-20 tokens as well as Ether in the Mixer contract. It's also possible to make it send Ether to smart contracts too, or send tokens then invoke a method on a smart contract.

In the ERC-223 and ERC-20 case the Mixer calls the transfer method of the token.

When the token address is 0 this is synonymous with Ether, if not zero the Mixer must validate the contract exists by calling extcodesize(addr).

ERC-223 tokens also support an optional arbitrary data field.

To make arbitrary smart contract calls the Withdraw caller must specify the msg.data field, contract address and Gas limit.

e.g. WithdrawToContract(ring_id, tx, ty, ctlist, address, gas, msg_data)

With this construct you make the Mixer call its self to Deposit again without needing a utility method for every conceivable thing the user may want to do, but it opens up a security hole for anything that relies on tx.origin rather than msg.sender

All of these parameters need to be included in the hash.

For now, I think it would be good to split the Mixer.Withdraw function into multiple variants to avoid convoluted code paths.

• WithdrawERC20
• WithdrawERC223
• WithdrawEther

And maybe functions like:

• WithdrawEtherTo (recipient address)

As of solidity 0.4.17, the constant modifier for functions has been deprecated in favour of the pure or view modifiers. The contracts still contain a number of functions using the deprecated constant modifier.

Within some of the contracts I still see a mix of uint and uint256 being used (uint is an alias for uint256). IMHO best practice is to specify the size explicitly.

It might be worth fixing the version of solidity to ensure everyone is running the same tested bytecode. The current version pragma using the caret syntax allows for solidity versions >=0.4.18 <0.5.0.

In Mixer.sol the types of variables are not set explicitly (using var), lines 110,111,180.

As with all Ethereum transactions the address submitting the transaction can be used to trace origins and destinations of payments, however the ring signature will still reduce linkability to a 1/N probability between each sender and receiver.

With the account abstraction improvements expected in Serenity it will be possible to sever this link and enjoy much stronger practical anonymity guarantees, but this isn't ready yet.

With incentivised / proxy withdraw you can let somebody else submit the Withdraw message in return for an amount enough to cover the Gas costs and their time.

The problem is a fluctuation in the Gas price could make it cost more for the submitter than the incentive is worth. The tx.gasprice global variable exists, but the sender needs to verify the transaction can be completed at the current gas price and doesn't revert() (which will cost them)

The combined Withdraw and Deposit function is used to transfer a token from one ring to another in a single operation.

This could be 'churning' a token, or it could be transferring the token to a new owner.

For example:

• Alice deposits 1 Token into Ring, using pubx,puby that she controls
• Alice must wait for the ring to fill up before it can be moved
• Alice performs WithdrawDeposit, depositing the token into another Ring with another pubx,puby

To do this Alice generates the Withdraw parameters, and passes the additional Ring address and pubx,puby.

This relies on the replay protection modification from #12 to bind the signature to a specific set of parameters.

The only thing necessary for this too be possible is to extend the 'Withdraw' function to include target address and msg.data, this can be used to implement arbitrary smart contract calls.

This requires additional parameters to the Withdraw function, in the case of sending ETH it would be:

• target address
• message data

Care needs to be taken to ensure that this functionality cannot be used to perform internal calls or abuse trust relationships, that's a significant problem - if there is any trust relationship between the Ring and a Parent contract the withdraw function must be barred from using the Parent as the Target.

However, with ERC20 the payable function attribute isn't really applicable - e.g. you can send tokens to a contract and all it does is modify a counter in the Token - but it doesn't invoke the target method on the third-party contract.

output = Token.transferFrom(this, msg.sender, PaymentAmount);

The code would have to be changed to something like:

            if (UsingToken) {            
                if( ! Token.transferFrom(this, Target, PaymentAmount) ) {
                    revert();
                }
            }
            output = Target.call.gas(X).value(PaymentAmount)(TargetData)

Where Target is the contract address, and TargetData is the arbitrary msg.data to be passed.

For more information about handling transfer of Gas from one call to another, see: ethereum/solidity#2999

The problem is - the call needs to be performed last, and we may need to save some gas for a handful of cleanup operations.

The second problem is, if you just want to send a token to a target address, it's not necessary to perform the second call() on the target afterwards.

Need to handle the following conditions:

• Perform token transfer to non-contract address
• Perform ETH transfer to non-contract address
• Perform token transfer to contract (without call)
• Perform ETH transfer to contract (with call)
• Perform token transfer to contract (with call)

Lets call these two functions:

• Withdraw (compatible with current function)
• ExWithdraw (includes additional parameters)

Third problem:

In Deposit, if UsingToken it performs Token.transferFrom from msg.sender, this doesn't work if the function has been chained from the ExWithdraw function.

When using ERC223 this doesn't seem to be a problem because it supports the tokenFallback function. The from parameter of tokenFallback must be the address of the Ring or Parent.

With ERC20 and ERC223 tokens, it seems that it's not immediately possible to perform arbitrary calls and a commbined WithdrawDeposit method must be used to ensure the right actions are performed.

For the sake of simplicity we are making an assumption that, for now at least, we will not do anything special when withdrawing tokens?

This rule of thumb should be followed:

Anybody can send the message, but only the owner can decide what it does.

This means that msg.sender should be ignored, the owner should instead specify who the token/eth should be transferred too. etc.

ERC223 EIP: ethereum/EIPs#223
ERC223 reference implementation: https://github.com/Dexaran/ERC223-token-standard

I am wondering whether it would be cleaner to add all community related files (contribution, issue/PR templates and so on) under .github like https://github.com/docker/cli/tree/master/.github or https://github.com/ethereum/go-ethereum/tree/master/.github for instance.

Looking at the current state of the repo, it looks like many images are in this .github folder. We could do something like .github/images and put these images into it, and have the contribution files under .github directly.

What do you think @zoenolan ?

Description

As per discussion with @zoenolan in the #dev channel in Slack, this repository is to be archived as it is now legacy and work is now carried on in the Zeth repository.

Acceptance criteria

• Link from this repository to the clearmatics/zeth repository
• Archive repository
• Archive Orbital repository

As per:
https://github.com/mimblewimble/grin/blob/master/doc/intro.md
https://fc17.ifca.ai/bitcoin/papers/bitcoin17-final41.pdf

The components necessary would be:

• Range proof (base 4? e.g. Back-Maxwell range proof) to verify the value
• Pedersen commitment to amount
• Signature provided by the blinding key

README.md still uses the old orbital syntax.

The Withdraw function in the Mixer contract (Mixer.sol), includes a require condition for a full ring (Line 199). This require condition is also included in the LinkableRing solidity library (LinkableRing.sol) function SignatureValid (Line 255).

It is possible to derive the X/Y coords of a point from a compressed X coordinate which includes a sign bit. This halves the size of every point that's passed as an argument or stored.

At the moment the Gas cost of decompressing the point is more expensive than the storage costs.

The following changes are necessary:

1. remove eclib code
2. add test data generated by orbital for bn256.g1 curve (keep secp256k1 data for reference)
3. update deposit() to validate points for bn256.g1 [1]
4. update 'ring full' branch in deposit() to calculate hashx for bn256.g1 [2]
5. in withdraw() update ring signature calculation [3]
6. update tests
7. check if coverage runs without timing out

[1] https://github.com/clearmatics/mobius/blob/master/contracts/Ring.sol#L81
[2] https://github.com/clearmatics/mobius/blob/master/contracts/Ring.sol#L118
[3] https://github.com/clearmatics/mobius/blob/master/contracts/Ring.sol#L162

Recently Vitalik Buterin proposed to abuse ecrecover to compute hashed result of ecmul:
https://ethresear.ch/t/you-can-kinda-abuse-ecrecover-to-do-ecmul-in-secp256k1-today/2384

I had implemented his idea: https://github.com/1Address/ecsol/blob/master/contracts/EC.sol

It uses about 32K gas to call ecmulVerify.

The following conditions need to be tested:

• submitting a deposit with a bad point
• cannot add same public key twice to any ring
• submitting a transaction with a bad transaction value (e.g. submitting a payment of 1 when the ring size is 5)
• submitting a withdrawal twice (e.g. duplicate tag check)
• submitting a deposit twice to the same withdraw address

See code coverage, the require() statements are passing for successful conditions, but need to test the edge cases.

The members of the struct Data in Mixer.sol could be reordered to enable tighter packing in storgae by the EVM.

Currently there is a requirement that any deposit must be an exact power of two, this was enforced for efficiency - making it more likely for rings to be filled.

However, many people may want to tumble tokens in decimal units, e.g. 10**5 or 100000.

Should the power of 2 requirement be removed?