search

clearmatics / mobius

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

Feature/erc 223 support #57

Closed AntoineRondelet closed 6 years ago

AntoineRondelet commented 6 years ago

This PR adds the support of all ERC20 compatible tokens (included ERC223 tokens) See: #22

AntoineRondelet commented 6 years ago

Just need to refactor the code a bit. I'll reopen it later

AntoineRondelet commented 6 years ago

I moved all the Deposit and Withdraw logics into their own functions DepositLogic and WithdrawLogic. The goal here was to avoid duplicate code.

Moreover, the call to the ERC20 compatible token is made via: UntrustedErc20Token, in order to illustrate the fact that we are doing a call to an external (and not trusted) contract (see: https://consensys.github.io/smart-contract-best-practices/recommendations/#mark-untrusted-contracts for further details).

Finally, in order to prevent any security breach to be used in case of malicious codes, the external calls are moved to the very end of each functions (see: https://consensys.github.io/smart-contract-best-practices/known_attacks/#reentrancy).

In order to do this, we introduced a variable entrySaved which is a memory variable (see: https://github.com/clearmatics/mobius/blob/feature/ERC-223-support/contracts/Mixer.sol#L282), This variable, is returned to the calling function via a return statement and aims to return a copy of the entry accessed in the storage (see: Data storage entry = m_rings[ring_id]; in: https://github.com/clearmatics/mobius/blob/feature/ERC-223-support/contracts/Mixer.sol#L262). In fact, when the condition if( ring.IsDead() ) is met, the entry m_rings[ring_id]; is deleted, thus returning a storage pointer to this entry triggers an undesirable behavior in the calling function. This is why we needed to include a memory variable as copy of this entry. This variable is removed once the calling function ends (see: https://solidity.readthedocs.io/en/latest/frequently-asked-questions.html#what-is-the-memory-keyword-what-does-it-do).

While adding this temporary entrySaved variable might not be optimal, this prevents from code duplication and makes possible to defer external calls to the end of the DepositERC20Compatible and WithdrawERC20Compatible functions.

AntoineRondelet commented 6 years ago

Some solidity syntax best practices might need to be added to Mobius, however, this should be part of another PR (to keep the history clean). I'm happy to do it, but since this PR includes breaking changes, I'd be happy to have a review @shapeshed and @zoenolan, so we can merge this PR first (if all is good for you), before I move on to other modifications