Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?

McAfeeDex Audit Report.

1. Summary

This document is a security audit report performed by RideSolo, where McAfeeDex has been reviewed.

2. In scope

  • contract.sol github commit hash 5db8389ce23ce11fb273c9573772287aa75ce43b.

Please note that no description has been provided for ReserveToken, therefore the severity of the related issues cannot be set correctly.

3. Findings

11 issues were reported including:

  • 0 high severity issue.
  • 2 medium severity issue.
  • 5 low severity issue.
  • 2 Owner privileges.
  • 3 notes.

3.1. Test Trade

Severity: medium

Description

When testTrade is used the tokens[tokenGet][sender] is checked to be higher or equal to amount which is wrong since taker fees are also substructed from msg.sender token balance or ether balance, meaning that for the test to be more correct takers fee should be calculated, added to amount and checked against tokens[tokenGet][sender].

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

Please note that this issue is applicable for users with accountLevels set to false. testTrade is used in the UI to prevent users from making wrong trade, however as explained it can allow users to take an offer and fail making them loosing all the transaction gas since revert isn't used adding it to a bad user experience .

Code snippet

https://github.com/RideSolo/MCAFEEDEXCONTRACT/blob/5db8389ce23ce11fb273c9573772287aa75ce43b/contract.sol#L278#L284

https://github.com/RideSolo/MCAFEEDEXCONTRACT/blob/5db8389ce23ce11fb273c9573772287aa75ce43b/contract.sol#L271

3.2. ERC20 Transfers (ERC20 Compliance)

Severity: low

Description

Both the implemented transfer and transferFrom functions returns false instead of throwing when the transfer conditions are not met.

Following ERC20 standard The transfer function SHOULD throw if the message caller’s account balance does not have enough tokens to spend. However it is also stipulated that "Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned".

If any third party contract does not handle the case that a transfer function can return false and expect a throw if the transfer condition are not met, the transaction can be handled as successful and the state of the caller contract can be set accordingly.

Please refere to the rfc definition of "MUST" and "SHOULD" to correctly implement ERC20 standard correctly.

code snippet

https://github.com/RideSolo/MCAFEEDEXCONTRACT/blob/5db8389ce23ce11fb273c9573772287aa75ce43b/contract.sol#L67

https://github.com/RideSolo/MCAFEEDEXCONTRACT/blob/5db8389ce23ce11fb273c9573772287aa75ce43b/contract.sol#L80

3.3. ERC20 Zero Value Transfers (ERC20 Compliance)

Severity: medium

Description

Both transfer and transferFrom do not handle transfers of zero value and return false since the transfer condition stipulate balances[_to] + _value > balances[_to] meaning that value cannot be equal to zero.

please note that "Transfers of 0 values MUST be treated as normal transfers and fire the Transfer event" following ERC20 standard. This issue can create compatibility issues with contracts that uses ERC20 standard.

Please refere to the rfc definition of "MUST" and "SHOULD" to correctly implement ERC20 standard correctly.

code snippet

https://github.com/RideSolo/MCAFEEDEXCONTRACT/blob/5db8389ce23ce11fb273c9573772287aa75ce43b/contract.sol#L67

https://github.com/RideSolo/MCAFEEDEXCONTRACT/blob/5db8389ce23ce11fb273c9573772287aa75ce43b/contract.sol#L80

3.4. Token Mint & Burn

Severity: low

Description

Following ERC20 "A token contract which creates new tokens SHOULD trigger a Transfer event with the _from address set to 0x0 when tokens are created". When create function is called to mint tokens the Transfer event is not emitted, in the opposite case when tokens are burned using destroy the transfer event can also be emitted but in this cas the _to address should be set to 0x0 address.

Code snippet

3.5. Owner privileges

Severity: High

Description

The owner allow himself to:

  • Create an almost unlimited token supply using create function.
  • Burn tokens directly from users balances without approval from them using destroy, meaning that the token holders do not have the full control over their assets.

3.6. Dex Admin Update Mechanism

Severity: low

Description

Admin address reset is handled by changeAdmin function, multiple issues can occure if the input address is wrong:

  • If admin_ address is set to zero accidentaly it will lock out the contract administrator forever from the contract. It is recommended to add a zero address check before setting the address.

  • A two step address verification should be used to avoid setting up a wrong address, meaning that in step two the address that will be set as admin should call a function confirming that it wasn't a wrong input.

Please note that setting a wrong admin address will result in disabling an important feature of McAfeeDex which eliminates the taker fees for future whitelisted users.

Code snippet

https://github.com/RideSolo/MCAFEEDEXCONTRACT/blob/5db8389ce23ce11fb273c9573772287aa75ce43b/contract.sol#L180#L183

3.7. Throw Keyword

Severity: low

Description

throw was used multiple times inside the audited contract please note that it is deprecated in favor of revert, require or assert from version 0.4.13.

Please note that for example using require in some specific cases will allow to return the users remaining gas after transaction execution plus a return message that provide readable reasons about the throw.

Referre to Solidity docs for more details.

Recommendation

Developers should use latest solidity version for contract developement and follow all needed requirement to build a safe smart contract.

code snippet

https://github.com/RideSolo/MCAFEEDEXCONTRACT/blob/master/contract.sol#L21#L23

https://github.com/RideSolo/MCAFEEDEXCONTRACT/blob/master/contract.sol#L119

https://github.com/RideSolo/MCAFEEDEXCONTRACT/blob/master/contract.sol#L124#L125

https://github.com/RideSolo/MCAFEEDEXCONTRACT/blob/master/contract.sol#L177

https://github.com/RideSolo/MCAFEEDEXCONTRACT/blob/master/contract.sol#L181

3.8. Wrong Token Deposit

Severity: note

Description

In case if tokens are directly deposited using normal ERC20 transfer function to SwitchDex contract, the admin will be unable to withdraw them from the contract since no function is intended for such issue.

Please referre to "Known vulnerabilities of ERC-20 token" to understand the lack of ERC20 transaction handling mechanism issue.

Recommendation

Keep track of the total token balances deposited through depositToken, any difference between the contract token balance and the total token deposited balance can be sent back to the users.

3.9. Unnecessary Extra Computation

Severity: note

Description

feeRebateXfer is set to zero and should be removed from tradeBalances function since it add extra complexity and gas usage.

Code snippet

https://github.com/RideSolo/MCAFEEDEXCONTRACT/blob/5db8389ce23ce11fb273c9573772287aa75ce43b/contract.sol#L267

https://github.com/RideSolo/MCAFEEDEXCONTRACT/blob/5db8389ce23ce11fb273c9573772287aa75ce43b/contract.sol#L272

https://github.com/RideSolo/MCAFEEDEXCONTRACT/blob/5db8389ce23ce11fb273c9573772287aa75ce43b/contract.sol#L273

3.10. Account Level Contract

Severity: note

Description

AccountLevels and AccountLevelsTest contracts can be removed since only two level user logic is implemented using accountLevels mapping instead.

Code snippet

https://github.com/RideSolo/MCAFEEDEXCONTRACT/blob/5db8389ce23ce11fb273c9573772287aa75ce43b/contract.sol#L131

https://github.com/RideSolo/MCAFEEDEXCONTRACT/blob/5db8389ce23ce11fb273c9573772287aa75ce43b/contract.sol#L139

3.11. Known vulnerabilities of ERC-20 token

Severity: low

Description

  1. It is possible to double withdrawal attack. More details here
  2. Lack of transaction handling mechanism issue. WARNING! This is a very common issue and it already caused millions of dollars losses for lots of token users! More details here

4. Conclusion

All highlighted issues should be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.