This document is a security audit report performed by RideSolo, where McAfeeDex has been reviewed.
- 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.
11 issues were reported including:
- 0 high severity issue.
- 2 medium severity issue.
- 5 low severity issue.
- 2 Owner privileges.
- 3 notes.
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 .
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.
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.
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.
The owner allow himself to:
- Create an almost unlimited token supply using
createfunction. - 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.
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.
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.
Developers should use latest solidity version for contract developement and follow all needed requirement to build a safe smart contract.
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
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.
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.
feeRebateXfer is set to zero and should be removed from tradeBalances function since it add extra complexity and gas usage.
AccountLevels and AccountLevelsTest contracts can be removed since only two level user logic is implemented using accountLevels mapping instead.
- It is possible to double withdrawal attack. More details here
- 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
All highlighted issues should be fixed.