This document is a security audit report performed by danbogd, where McAfeeDex has been reviewed.
Сommit hash 5db8389ce23ce11fb273c9573772287aa75ce43b.
In total, 19 issues were reported including:
- 3 medium severity issues
- 6 low severity issues
- 3 owner privileges (ability of owner to manipulate contract, may be risky for investors).
- 7 notes.
No critical security issues were found.
-
Double withdrawal attack is possible. More details here.
-
Lack of transaction handling mechanism. WARNING! This is a very common issue, and it already caused millions of dollars losses for lots of token users! More details here.
Add the following code to the transfer(_to address, ...) function:
require( _to != address(this) );From ERC-20 specification:
The function SHOULD throw if the _from account balance does not have enough tokens to spend.
In the implementation of McAfeeDEX a function returns false instead. This can lead to serious consequences for 3d party developers who work with this contract.
For example, an external contract may use this token contract as:
AdChainToken.transferFrom(recipient, this, value);
points[recipient] += value;In this case, the recipient will get the increase of points, but the transfer of tokens will not happen. According to the definition of ERC20 standard the transaction must be interrupted by the call of throw at the token contract, however in case of the audited smart-contract the code will successfully complete the transaction.
function transfer(address _to, uint256 _value) returns (bool success) {
//Default assumes totalSupply can't be over max (2^256 - 1).
//If your token leaves out totalSupply and can issue more tokens as time goes on, you need to check if it doesn't wrap.
//Replace the if with this one instead.
if (balances[msg.sender] >= _value && balances[_to] + _value > balances[_to]) {
//if (balances[msg.sender] >= _value && _value > 0) {
balances[msg.sender] -= _value;
balances[_to] += _value;
Transfer(msg.sender, _to, _value);
return true;
} else { return false; }
}
function transferFrom(address _from, address _to, uint256 _value) returns (bool success) {
//same as above. Replace this line with the following if you want to protect against wrapping uints.
if (balances[_from] >= _value && allowed[_from][msg.sender] >= _value && balances[_to] + _value > balances[_to]) {
//if (balances[_from] >= _value && allowed[_from][msg.sender] >= _value && _value > 0) {
balances[_to] += _value;
balances[_from] -= _value;
allowed[_from][msg.sender] -= _value;
Transfer(_from, _to, _value);
return true;
} else { return false; }
}According to the ERC20 standard, a transfer event must be generated when the token contract is initialized, if any token value is set to any given address.
function create(address account, uint amount) {
if (msg.sender != minter) throw;
balances[account] = safeAdd(balances[account], amount);
totalSupply = safeAdd(totalSupply, amount);
}
function destroy(address account, uint amount) {
if (msg.sender != minter) throw;
if (balances[account] < amount) throw;
balances[account] = safeSub(balances[account], amount);
totalSupply = safeSub(totalSupply, amount);
}Transfer & transferFrom functions do not prevent from sending tokens to address 0x0.
Add zero address checking.
require(to != address(0));Both transfer and transferFrom can not process transfers of 0 tokens.
The condition balances[_to] + _value > balances[_to] means 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 rely on ERC20 standard definition.
Please refer to the rfc definition of "MUST" and "SHOULD" to correctly implement ERC20.
function transfer(address _to, uint256 _value) returns (bool success) {
//Default assumes totalSupply can't be over max (2^256 - 1).
//If your token leaves out totalSupply and can issue more tokens as time goes on, you need to check if it doesn't wrap.
//Replace the if with this one instead.
if (balances[msg.sender] >= _value && balances[_to] + _value > balances[_to]) {
//if (balances[msg.sender] >= _value && _value > 0) {
balances[msg.sender] -= _value;
balances[_to] += _value;
Transfer(msg.sender, _to, _value);
return true;
} else { return false; }
}
function transferFrom(address _from, address _to, uint256 _value) returns (bool success) {
//same as above. Replace this line with the following if you want to protect against wrapping uints.
if (balances[_from] >= _value && allowed[_from][msg.sender] >= _value && balances[_to] + _value > balances[_to]) {
//if (balances[_from] >= _value && allowed[_from][msg.sender] >= _value && _value > 0) {
balances[_to] += _value;
balances[_from] -= _value;
allowed[_from][msg.sender] -= _value;
Transfer(_from, _to, _value);
return true;
} else { return false; }
}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. This means that in order for the test to be more correct, it is necessary to calculate the fee, add it to the amount and check against tokens[tokenGet][sender].
This issue will lead to the fact that the transaction of a user who wants to exchange the entire balance of one asset for another will be thrown if the amount is equal to their total balance, since they will not have enough tokens to cover the exchange cost.
Please note that this issue is applicable for users with accountLevels set to false.
The testTrade function is used in the UI to prevent users from making incorrect trades. However, as explained above, this can allow users to accept the offer and fail to execute it, causing them to lose all transaction gas since revert is not used.
function testTrade(address tokenGet, uint amountGet, address tokenGive, uint amountGive, uint expires, uint nonce, address user, uint8 v, bytes32 r, bytes32 s, uint amount, address sender) constant returns(bool) {
if (!(
tokens[tokenGet][sender] >= amount &&
availableVolume(tokenGet, amountGet, tokenGive, amountGive, expires, nonce, user, v, r, s) >= amount
)) return false;
return true;
}Incoming addresses should be checked for an empty value(0x0 address) to avoid loss of funds or to block some functionality.
Admin address reset is handled by changeAdmin function. Multiple issues can occur if the input address is wrong:
-
If
admin_address is set to zero accidentally then it will lock out the contract administrator forever. It is recommended to add a zero address check before setting the address. -
Smart-contract developer can use two-step address verification to avoid setting the wrong address. This means that in the second step, the address that will be set as the administrator must call a function confirming that it was not an incorrect 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.
A call method has no gas limit, and it is possible to make reentrancy from another contract. There is no danger in this implementation but this can pose a potential threat.
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.
I would recommend deploying 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.
It is advised to design contracts so that it is possible to migrate/upgrade them in the 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 implementing 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, one year.
A throw was used multiple times inside the audited contract. Please note that it is deprecated in favor of revert, require, or assert since solidity 0.4.13 version.
Please note that, using require in some specific cases will allow returning the users' remaining gas after transaction execution. This will also leave a return message that provides readable reasons about the throw.
Refer to Solidity docs for more details.
Developers can use the latest solidity version for contract development.
feeRebateXfer is set to zero and should be removed from tradeBalances function since it adds extra complexity and gas usage.
function tradeBalances(address tokenGet, uint amountGet, address tokenGive, uint amountGive, address user, uint amount) private {
uint feeMakeXfer = safeMul(amount, feeMake) / (1 ether);
uint feeTakeXfer = safeMul(amount, feeTake) / (1 ether);
uint feeRebateXfer = 0;
if (accountLevels[msg.sender] == true) {
feeTakeXfer = 0;
}
tokens[tokenGet][msg.sender] = safeSub(tokens[tokenGet][msg.sender], safeAdd(amount, feeTakeXfer));
tokens[tokenGet][user] = safeAdd(tokens[tokenGet][user], safeSub(safeAdd(amount, feeRebateXfer), feeMakeXfer));
tokens[tokenGet][feeAccount] = safeAdd(tokens[tokenGet][feeAccount], safeSub(safeAdd(feeMakeXfer, feeTakeXfer), feeRebateXfer));
tokens[tokenGive][user] = safeSub(tokens[tokenGive][user], safeMul(amountGive, amount) / amountGet);
tokens[tokenGive][msg.sender] = safeAdd(tokens[tokenGive][msg.sender], safeMul(amountGive, amount) / amountGet);
}AccountLevels and AccountLevelsTest contracts can be removed since only two-level user logic is implemented. It is possible to use accountLevels mapping instead.
contract AccountLevels {
//given a user, returns an account level
//0 = regular user (pays take fee and make fee)
//1 = market maker silver (pays take fee, no make fee, gets rebate)
//2 = market maker gold (pays take fee, no make fee, gets entire counterparty's take fee as rebate)
function accountLevel(address user) constant returns(uint) {}
}
contract AccountLevelsTest is AccountLevels {
mapping (address => uint) public accountLevels;
function setAccountLevel(address user, uint level) {
accountLevels[user] = level;
}
function accountLevel(address user) constant returns(uint) {
return accountLevels[user];
}
}The AccountLevelsTest contract member function setAccountLevel allow to set any level to any account without owner restriction.
-
accountLevelsAddris not used anywhere atSwitchDexcontract. The functionality ofAccountLevelscontract is not used as well. -
feeRebatevariable is not used properly.
Contract owner allows himself to:
The audited smart contract must not be deployed. Reported issues must be fixed prior to the usage of this contract.
Correct stile