Deposit function in Mixer doesn't check denomination matches tx.value
magooster opened this issue · 6 comments
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.
Good spot, this will be added to tests and fixed shortly.
If the denomination / value is different it will be deposited into a different ring.
- https://github.com/clearmatics/mobius/blob/master/contracts/Mixer.sol#L161
- 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.
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.
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.
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...
@magooster For ERC-223 support see #22