McAfeeDEX security audit

Auditors

The security audit of McAfeeDEX smart-conract was performed by Callisto Network security department. 5 security auditors (including me) reviewed the contract independently. The security manager then compared our audit reports and summarized the results described.

Copies of each audit report and final summary can be found here. Security Audit summary can be found here.

Please note that our severity determination procedure implies that the contract developer is only responsible for the contract that we have audited. This means that some issues that are not directly related to the audited smart-contract are assigned “note” or “low” severity but these can still lead to financial losses of the DEX customers and it is strongly recommended that the developer of the contract fixed this issues before using the contract.

I will highlight the most important issues below.

Results

The main goal of the security audit is to ensure that the auditable software will work as intended and to verify that it can not harm end users.

During the security audit we found 2 issues that can result in financial losses for the users of McAfeeDEX and 18 issues that can not directly result in financial losses for the users.

The described problems which can result in financial losses are not directly exploitable bugs of the audited smart-contract. These problems are flaws of Ethereum architecture and ecosystem which affect the operability of McAfeeDEX.

The audited software must not be used because it can lead to financial losses for its customers. It is strongly recommended to apply the suggested fixes before deploying the production version of the smart-contract.

Findings

1. Problems with depositing/withdrawing ERC20 tokens. Lack of ERC20 extraction function.

This issue can cause financial losses for customers of McAfeeDEX.

This issue is not a bug of the McAfeeDEX smart-contract but it will still affect the users of the DEX. This issue is related to the problems of Ethereum’s token standard.

ERC20 token standard has a critical problem: it lacks event handling/emitting. The problem is described here and here.

There are two methods of transferring ERC20 tokens.

1. transfer function (used for normal transfers)
2. approve + transferFrom pattern

Whenever a user chooses a wrong function to deposit his tokens to the contract, the tokens get “stuck” without any possibility to recover them.

In a normal situation, an “event” (in this case, a transaction) is handled by the recipient (in this case, the smart-contract). This is how Ether (ETH) transfer works. This is how bank transfers work. Whenever an invalid transaction is sent, the receiver rejects it and nothing bad happens.

It is impossible to reject or handle a ERC20 token transaction because the ERC20 token standard lacks a critically important software design feature, which is called communication model.

It should be noted that “event handling” is a very common, widely adopted and widely used practice in programming. The lack of this feature is a serious flaw which can result in financial losses for end users. This issue has already resulted in serious financial losses for the whole Ethereum ecosystem. At 27 Dec, 2017 there were roughly $3,200,000 lost in top7 ERC20 tokens.

While this is mostly a problem of ERC20 token standard, the problem affect the McAfeeDEX smart-contract because it is intended to work with ERC20 tokens. For end users this means that the exchange will fail to credit the deposited tokens if users choose a wrong method of token depositing.

McAfeeDEX smart-contracts assume that users will only deposit tokens using the approve+transferFrom pattern. Depositing tokens using the transfer function will lead to the fact that the deposit will be sent to the contract, but will not be credited to the user’s balance. It should be noted that sending tokens into a certain address at your exchange is a standard procedure of making deposits in crypto industry thus it is an intuitively-clear method of depositing crypto assets. It is important that depositing ERC20 tokens to a normal centralized exchanges requires the use of transfer funciton that is intuitive for most users. Depositing tokens following an intuitive method of crypto asset depositing will result in loss of the deposited tokens for users of McAfeeDEX exchange.

Assuming that users will never make this mistake is wrong. Users will make this mistake and it will result in financial losses for them as it was shown with the $3M loss example above.

Assuming that users are responsible for this kind of decisions is also wrong and it is described at the Software Vulnerabilities paragraph at Vulnerabilities in computing page at Wikipedia as Blaming The Victim.

Common types of software flaws that lead to vulnerabilities include: … Blaming the Victim prompting a user to make a security decision without giving the user enough information to answer it.

Users must use the transfer function with most exchanges. However with McAfeeDEX they must never use this function and choose the approve+transferFrom method. A normal user is not capable of making this kind of decisions because a normal user may not have deep knowledge of Solidity and the inner logic of the smart-contract.

The audited contract is a financial system and it must be fault-tolerant. McAfeeDEX is designed to support multiple “portals” i.e. multiple variations of UI. John McAfee encourages people to create their own portals. This means that the developer of the smart-contract never knows what UI is and how it will look/work. There will be multiple variations of UI and the smart-contract developer cannot guarantee that UI developers will never make a mistake.

This is not secure to rely on an assumption that the problem can be solved at UI side because McAfeeDEX will have multiple variations of UI implemented by random developers and we can not guarantee that none of UI developers will make even a single mistake in the future.

Knowing that the problem exists and the fact that it will lead to huge financial losses for users of your software, smart-contract developers must implement a solution.

Real example of how this problem caused financial losses already

1. $3,200,000 losses are described in the “Motivation” section of ERC223 proposal.
2. $77,000 lost in GNOSIS smart-contract ($220,000 lost in other described contracts).
3. A user has unintentionally lost $130,000 with SALT and ZRX contracts.
4. ENS smart-contract caused financial losses for users due to lack of ERC20 transaction handling and extraction functions.
5. BNB token implementation caused a loss of $357,000 at Mar 22, 2019.
6. Now (Oct 29, 2019) there are $480,000 lost in BNB smart-contract.
7. Two users have lost USDC tokens at LINK smart-contract. The transfer occured at Oct 11, 2019 — the problem is extremely relevant.
8. MKR smart-contract owns $400,000 worth of tokens (Oct 29, 2019) while it is not intended to receive/own tokens. All these tokens are permanently lost.
9. $73,000 lost in BAT smart-contract (Oct 29, 2019).
10. $32,805 lost in Dai smart-contract (Oct 29, 2019).
11. $31,000 lost in MANA smart-contract (Oct 29, 2019).
12. ENJ smart contract owns 40 other tokens. All these tokens are lost.

Recommendation on resolving the issue #1

There are several possible solutions.

1. Use alternative token standards. ERC20 is widely adopted but it is not the only token standard on Ethereum. There is ERC223 tokens standard that is specially designed to resolve the described issue. If you don’t like ERC223 then ERC777 is also good.
2. You can’t handle ERC20 transfers properly but you can resolve the consequences. It is possible to implement an “emergency extraction function” that will serve to extract the “stuck” tokens from the contract. Then you can send this tokens back to users because it is relatively easy to recognize who this tokens belong to with a use of history of transactions.
3. Ethereum is not the only smart-contract development platform. If there would be an another platform that is not prone to the described issues then you can use it instead. It may turn impossible to build a SECURE and DECENTRALIZED exchange on Ethereum. The exchange is either insecure (i.e. prone to the ERC20 problem) or partially centralized if you implement the function that extracts stuck tokens (because you will own this tokens at some moment). However, this does not pose any risk of fund loss for customers of the exchange if they will use token functions properly.

2. Crosschain collision issue.

This issue can cause financial losses for customers of McAfeeDEX in theory.

This issue is not a bug of the McAfeeDEX smart-contract but it will still affect the users of the DEX. This issue is related to the architecture of Ethereum.

The address generation method of Ethereum assumes that an address is valid for every Ethereum-compatible chain. Currently, there are a large number of networks based on the Ethereum code, in which the address generation scheme is identical. The best example is Ethereum Classic.

This is not only important to handle transaction at the main network but also make sure that your contract can not cause any issues on any of the alternative networks. It is important to prevent users from depositing ETC into the address of your contract at ETH network. It is also important to make sure that you can extract ERC20 tokens from the address of your contract at any of the alternative networks.

This is not a direct problem of McAfeeDEX smart-contract but a flaw of Ethereum architecture. Ethereum was never intended to handle inter-blockchain interactions. However, this is a real problem that have to be addressed and only the creator of the contract can resolve this issue. We have a real example of how $58,000 were lost in GNT contract at ETC chain already.

While it is arguable if the contract developer is responsible for the consequences of this problem or Ethereum developers are — the fact is that it can lead to the loss of funds for those who attempt to use McAfeeDEX. It is important to address any issue that can lead to loss of funds for the smart-contract customers prior to pushing the contract for mass use. It is even more important to address the issues that already lead to the loss of funds in the past.

Recommendations on resolving issue #2

1. Address of the newly deployed contract is assigned based on the address of the contract creator and the nonce of the transaction. This means that the creator of the contract can deploy the a contract at the same address at any of the alternative Ethereum-based chains. It is recommended to deploy a version of your exchange or a dummy contract that will prevent accidental deposits (keep in mind that you need to make sure that you can extract stuck ERC20 tokens from the dummy contract as it is described at issue #1) on any of the alternative chains.
2. Consider using a smart-contract development platform that was designed with the inter-blockchain operability in mind.

3. Lack of upgradeability and freezing functions.

Sometimes it may be necessary to update or debug the contract. It is even more important because the issue that requires the upgrade of the contract is already discovered (see finding #1: lack of ERC20 extraction function).

Recommendation on resolving issue #3

1. It is recommended to implement a function that will allow to freeze the contract to make sure that users can no longer utilize the “deprecated” version of the contract in case of emergency upgrade. If this proposal hurts decentralization, then it is possible to implement this “freeze” function so that it can be disabled in future. Once the contract will be time-tested and it will be decided that it will not require any updates then the contract creator can disable the special functions and leave the contract in immutable state.

2. Consider using a smart-contract development platform that has a built-in smart-contract upgrading features.

4. ERC20 compliance issues.

This issue can confuse third-party smart-contract developers who work with the McAfeeDEX smart-contract.

There are token codes presented in the source code of the audited smart-contract.

The described exchange is intended to work with ESH tokens as well.

Token, the code of which is presented in the audited contract, has some mismatches with the definition of the ERC20 token standard. While it can not cause any problems to the McAfeeDEX smart-contract, it can still cause issues for 3d party contracts that will rely on proper work of McAfeeDEX smart-contract. It is recommended to adhere to the definition of the ERC20 standard to prevent this type of issues.

5. Test Trade does not take fees into account.

This issue can confuse third-party interface developers.

This issue will lead user’s transaction that wants to trade the full balance of one asset against another to throw if the amount is set to be equal to their total balance since they won’t have enough of tokens to cover the exchange cost.

Appeal to McAfeeDEX team [subjective opinion]

Everything described below is a personal opinion of Dexaran and does not reflect the official position of the Callisto Network or the objective results of a security audit.

Ethereum does not fit your needs to build a SECURE decentralized exchange. You must consider using EOS as a better smart-contract development platform.

The issues described are mainly issues of the Ethereum ecosystem and architecture. You can argue that the design flaws of the ERC20 are not your problem. You can argue that Ethereum design flaws are not your problem. However, the McAfeeDEX team is fully responsible for choosing Ethereum as the base platform for the development of this exchange. McAfeeDEX is solely responsible for choosing the ERC20 standard for working with tokens.

In crypto- we claim the title of the financial system but we complain that the adoption of DEX’es is slow. Banks handle thousands or millions of user mistakes every day. The adoption will not come to DEX’es if we fail to do the same. The adoption will not come if we let our customers lose their funds when they attempt to use our software.

It is stated that the main goal of McAfeeDEX is to help the crypto industry to grow and develop. Centralized fiat-to-crypto exchanges are one of the most important problems — and I agree with this point.

Any system that involves humans is prone to human mistakes and we must understand it and take it seriously. There are thousands of mistakes that are handled by banks and banking transfers every day. Using ERC20 tokens assumes no error handling — all these funds are simply burned instead. How can we build a better system if we can not serve real humans? Even if your software is secure on its own it can still lead to the loss of funds for your customers because you have chosen a platform that has serious security shortcomings.

While you CAN solve the described problems with the implementation of some fixes that I described I would strongly advise not to promote ERC20 tokens. By supporting ERC20 tokens you promote the development of insecure standards. This encourages token developers to use this standard for the development of their tokens. Even if you can implement the solution for your exchange — it is far from guaranteed that every other developer of every other software will do the same.

Here is an example of how token developers behave. Let me quote STORJ co-founder here:

We decided to err on the side of well-tested code.

© STORJ co-founder [knowing that this will result in financial losses for his customers]

They know that their implementation contains critical flaws. They know that this WILL result in financial losses for their customers. They know that they can fix it but they are still using the ERC20 standard.

ERC20 tokens are resulting in huge financial losses for the whole Ethereum ecosystem and this is a great, terrific and disastrous problem and it is not beneficial for the crypto industry if someone will facilitate the growth of this problem.

I strongly support your idea of ​​building a secure, decentralized exchange, and I am ready to take the time and effort to help this. I highly recommend using EOS as a platform to build the decentralized exchange. Using this platform, you will automatically solve all the problems described above. I can personally port the described exchange and implement it in C++ for deploying on EOS.

There is always an issue of trust. You either trust the owner of the centralized exchange (you believe that he will not steal your funds), or you trust the developer of the smart contract (you believe that he did not make any mistakes and chose the appropriate platform for deployment).