search

clearmatics / mobius

Trustless Tumbling for Transaction Privacy
GNU Lesser General Public License v3.0
86 stars 23 forks source link

Deposit function in Mixer doesn't check denomination matches tx.value #41

Closed magooster closed 6 years ago

magooster commented 6 years ago

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.

HarryR commented 6 years ago

Good spot, this will be added to tests and fixed shortly.

HarryR commented 6 years ago

If the denomination / value is different it will be deposited into a different ring.

  1. https://github.com/clearmatics/mobius/blob/master/contracts/Mixer.sol#L161
  2. https://github.com/clearmatics/mobius/blob/master/contracts/Mixer.sol#L103

If I make two deposits, of 1 Wei and 2 Wei they will be placed in different rings.

magooster commented 6 years ago

Yes but i can deposit 1 wei, and set the denomination to 2 in the deposit function parameters. you need to check msg.value = denomination for ether deposits.

HarryR commented 6 years ago

Yup understood.

This code is half way between being ETH only and supporting ERC-223 tokens.

With ERC-223 the Deposit would be called from the tokenFallback function and the token and denomination values filled in using msg.sender and the passed value parameter.

For ETH the token becomes irrelevant and the value comes from msg.value.

magooster commented 6 years ago

If the Mixer contract is designed to support both ETH and ERC-223 based tokens then my comment is still valid, if its just ERC-223 tokens then its not and i'll hold off further review...

HarryR commented 6 years ago

@magooster For ERC-223 support see #22