ETH_MCAFEEDEX_audit_report.md

McAfeeDEX audit report

1. Summary

This document is a security audit of the smart-contract of McAfeeDEX performed by Dexaran for Callisto Network.

2. In scope

• MCAFEEDEXCONTRACT

3. Findings

In total, X issues were reported including:

• 1 high severity issue
• 0 medium severity issues
• 1 low severity issue
• 1 note

3.1 Lack of ERC20 extraction functions

Severity: high

Description

Warning! This problem will lead to huge financial losses for McAfeeDEX customers.

There is a critical flaw in the design of ERC20 token standard. ERC20 tokens lack event emitting/handling.

This makes it impossible for the recipient contract to reject incorrect transactions of ERC20 tokens. For the McAfeeDEX contract it means that users can (and will) deposit their tokens via the transfer function and the transaction will be completed successfully. Tokens will not be credited to the customer's account. These tokens will be trapped inside the contract forever without the possibility of their recovery.

Knowing this, it is necessary to implement a function that will allow to withdraw "trapped" tokens from the contract and send them back to users.

Recommendation

It is necessary to track the amount of tokens that were deposited properly. In this case, the developer can compare the amount of tokens that the McAfeeDEX contract owns to the amount of tokens that were recorded as "properly deposited". Then the developer can extract the excess of tokens to send them back to users manually.

contract SwitchDex is SafeMath {

  mapping (address => uint256) public total_deposited;
  
  ...
  
    function depositToken(address token, uint amount) {
    //remember to call Token(address).approve(this, amount) or this contract will not be able to do the transfer on your behalf.
    if (token==0) throw;
    if (!Token(token).transferFrom(msg.sender, this, amount)) throw;
    tokens[token][msg.sender] = safeAdd(tokens[token][msg.sender], amount);
    Deposit(token, msg.sender, amount, tokens[token][msg.sender]);
    
    // Record tokens as properly deposited
    total_deposited[token] = safeAdd( total_deposited[token], amount );
  }
  
  function extractERC20(address _token) {
    if (msg.sender != admin) throw;
    
    uint256 actual_balance = StandardToken( _token ).balanceOf( this );
    StandardToken( _token ).transfer( msg.sender, safeSub( actual_balance, total_deposited[_token] );
  }
}

You could also use an alternative smart-contract development platform that is not prone to these problems to help your users.

3.2 Interchain collision

Severity: note

Description

This is not a problem with this contract, but it can still lead to financial losses for its customers.

Ethereum addresses are not confirmed on-chain. This means that an abstract address is valid for any Ethereum sisterchain. For the McAfeeDEX contract this means that users can successfully send their funds into the address of the contract at any sisterchain (for example at ETC chain) and the transaction will succeed because there is no contract at ETC chain at the same address and the transaction will not be reverted.

While this is not an issue of this contract, I would recommend to deploy a dummy contract (with ERC20 extraction function) at the most popular Ethereum forks to save users from possible mistakes.

This is not just a theoretical issue! Here is the real example of fund loss.

Recommendation

I would recommend to deploy a dummy contract at the same address at Ethereum fork-chains. The address of the contract is determined by the creator's address and the transaction nonce. So, a developer can use the same address that was used to create the contract at the Ethereum mainnet. It is necessary to increase the nonce at the forkchain to (deployment nonce - 1).

Then you need to deploy the dummy contract. This will cause the dummy contract to be deployed to the same address as the main McAfeeDEX contract.

3.3 Lack of upgrading functions

Severity: low

Description

It is advised to design contracts so that it is possible to migrate/upgrade them in future.

In some specific circumstances it may be worth to stop users from using the previous version of the contract if some issues were detected. I would recommend to implement a freeze function that can stop users from depositing their funds into the contract.

This is possible to implement it so that the function will be disabled after a certain period of time, for example 1 year.

4. Conclusion

The contract contains a high severity issue. It is strongly recommended to apply the suggested fixes before using the contract on the mainnet.