Dex223 Token and Upgrade contract audit report

Dex223_token_and_upgrade_audit.md

Dex223 Token and Upgrade contract audit report

Summary

This is the report from a security audit performed on Dex223 Token and Dex223_Upgrader by Aleksandr Gorbunov.

The token is implemented in the ERC-223 standard. It also has functions for backward compatibility with the ERC-20 standard.

The migration contract changes the tokens of the old version to the tokens of the new version.

The report was based on the template proposed here: EthereumCommonwealth/Proposals#2 .

In scope

1. D223Token.sol
2. Dex223Upgrader.sol

Findings

In total, 7 issues were reported including:

• 0 critical severity issues.

• 0 high severity issues.

• 2 medium severity issues.

• 4 low severity issues.

• 1 notes.

Issues requiring attention have been found.

Security issues

1. Presence of the transferFrom function along with tokenReceived

Severity: medium

Description

The simultaneous presence of the transferFrom and tokenReceived functions in the contract can cause logical conflicts. This can lead to the loss of tokens.

For example, in the case of migration(Dex223Upgrader.sol), token receipt processing is implemented in the tokenReceived function, and if tokens are sent via transferFrom, the conversion will not occur. And the sent tokens will be lost.

Code

D223Token.sol, line 252, an implementation of transferFrom that does not have a tokenReceived call.

function transferFrom(address _from, address _to, uint _value) public returns (bool) {

    require(allowances[_from][msg.sender] >= _value, "ERC-223: Insufficient allowance.");

    balances[_from] -= _value;
    allowances[_from][msg.sender] -= _value;
    balances[_to] += _value;

    emit Transfer(_from, _to, _value, hex"00000000");

    return true;
}

Dex223Upgrader.sol, line 48, implementing the processing of receiving old tokens and sending new tokens in return. The whole process is initiated by calling the tokenReceived function, which does not occur when transferring tokens using transferFrom.

function tokenReceived(address _from, uint256 _amount, bytes memory _data) public returns (bytes4)
{
    // some code removed for simplicity

    require(msg.sender == d223_old, "Wrong token");
    // Send equal amount of new D223 tokens to the sender of the original D223 tokens.
    IERC223(d223_new).transfer(_from, _amount);

    // some code removed for simplicity

    return 0x8943ec02;
}

Recommendation

It is proposed to either implement the tokenReceived call inside the transferFrom function in the same way as it is done in the transfer function, or abandon backward compatibility with ERC-20.

2. Token extraction mechanism

Severity: medium

Description

This is related to the previous problem, but it specifically concerns the Dex223Upgrader contract. In the implementation of the new token, there is an opportunity to solve the issue, but the old one cannot be changed and there remains the possibility of losing tokens. Therefore, on the part of the migration contract, it is recommended to solve the problem.

Code

Dex223Upgrader.sol, token migration contract

Recommendation

Include a token extraction mechanism in the migration contract. And in order to avoid the question of using the already migrated funds received through tokenReceived by the contract owner, it is possible to burn the received tokens (old version) by sending them to the address 0x0.

function rescueERC223(address _token, uint256 _value) external
{
    require(msg.sender == owner);
    (bool success, bytes memory data) = _token.call(abi.encodeWithSelector(0xa9059cbb, msg.sender, _value));
    require(success && (data.length == 0 || abi.decode(data, (bool))), "Transfer failed");
}

3. User gas exploitation

Severity: low

Description

Third-party contracts can use users' gas for their own purposes. When transferring tokens, the contract calls tokenReceived in the recipient contract. In this way, third-party contracts can implement various regular actions that benefit them at the user's expense.

For example, an attacker can offer a profitable Dex223Token-USDT exchange rate, while the user will incur more damage by paying for gas in a token transfer transaction.

Recommendation

Limit the amount of gas to execute the callback function.

4. extcodesize is not an exhaustive method of determining the contract

Severity: low

Description

This is a known problem and it does not pose a threat in this contract. Nevertheless, it should be taken into account by users of third-party contracts using the erc-223 token.

For example, it seems logical to account for incoming tokens in the tokenReceived function. But the creator of a third-party contract in the constructor can add tokens that will be unaccounted for through the embedded mechanism. And this may not be obvious to users. Thus, the contract creator can create favorable conditions for himself in crowdsale, gaming, etc. contracts.

Code

Dex223Token.sol, line 206, sending tokens with a callback for contracts.

Recommendation

Take this issue into account when analyzing third-party contracts using ERC-223 tokens.

5. Taking ownership of the contract

Severity: low

Description

The token contract has a function for transferring ownership of the contract - newOwner. To prevent the transfer of rights to the wrong address, it would be a good practice to add a rights acceptance function.

Code

Dex223Token.sol, line 272, newOwner function.

Recommendation

Use the additional function of accepting ownership rights.

function claimOwnership() external
{
    require(msg.sender == newOwner);
    owner = newOwner;
}

6. Transfer 0 tokens

Severity: low

Description

In the ERC-20 standard, it is possible to send 0 tokens. In the ERC-223 standard, this assumption has a slightly different meaning, since the callback function of a third-party contract is called. The name of the function also indicates that tokens have been received - tokenReceived. Thus, if the check for 0 is missed in the recipient's contract, the sender may be assigned a certain status that gives an advantage (for example, if it is a game). For example, the status of lastDepositor (or something similar) may be assigned, although there was no deposit in fact.

Code

Dex223Token.sol, line 223, transfer function.

Recommendation

It may be worth writing a security guide, an article, or a series of examples on working with the ERC-223 standard.

7. Function state mutability can be restricted to pure

Severity: note

Description

The standard function does not involve access to storage. Therefore, it would be more correct to use the pure modifier instead of view.

Code

Dex223Token.sol, line 172, standard function.

Recommendation

Replace view with pure.

function standard() public pure returns (uint32)
{
    return 223;
}

1. Presence of the transferFrom function along with tokenReceived

I wouldn't call it a high severity issue as it does not pose any direct threat to users funds safety. approve & transferFrom functions were added for the backwards compatibility reasons. There is no reason to implement tokenReceived within the transferFrom function logic as it will instantly break backwards compatibility with the ERC-20 ecosystem. This function is implemented as in ERC-20 token.

In ERC-20 standard no error handling is implemented in both transfer and transferFrom patterns. This is a security flaw and it resulted in financial losses because the transfer function is the default one used by all the services and wallets. approve & transferFrom pattern however is never used by any wallets and instead it is utilized through the interfaces that abstract a user from making function choices and instead feed the necessary data onto the users wallet automatically.

No contract is designed to receive funds via the transferFrom function directly. Instead all the interfaces that ever use trasnferFrom rely on the implementation of a function call in the targeted smart-contract which will call transferFrom to pull the tokens from the user.

I don't consider the implementation of the transferFrom pattern to be a threat here since it is not the default transferring method of our token, the default one is the transfer function and unlike ERC-20 transfer function our one is safe.

A user can not lost tokens by a mistake with approve & transferFrom. In order to lose tokens to the transferFrom's lack of error handling a user must:

• Learn how to call a function in a contract directly via the contract ABI since wallets do not provide an average user with this functionality
• Call approve on himself and allow to spend X tokens
• Call transferFrom function to move X tokens from his own wallet to a smart-contract address and therefore lose them

It is not possible for a user to do all this actions by a mistake. You have to be a technically educated person to be able to harm yourself with this pattern.

For comparison in order to lose tokens to ERC-20 transfer function flaw a user must:

• Copy & paste wrong address to the wallet when sending a transaction.

An average user can copy & paste a wrong address, therefore in case of a ERC-20 transfer function this is an issue.

Security warning was added to the code comment which will be verified on Etherscan to notify the developers: Dexaran/D223-token-v2@a088d86

2. Token extraction mechanism

We can not burn the old ERC-223 tokens transferred to the migration contract automatically as it will increase the gas costs for the end user. A function that would allow to the creator to extract the new ERC-223 tokens was added.

Since Dex223 team is handling the migration process we will be able to examine the improperly sent transactions and withdraw the required amount of new-D223 tokens to send them to the requestor manually.

Fixed: https://github.com/Dexaran/D223-token-v2/blob/a088d86ccb0fc83f87dd8b170cb855b72351ba08/Migration.sol#L70-L74

3. User gas exploitation

A user can configure the amount of gas he/she is trying to supply with the transaction in the wallet by setting the gasLimit parameter of the transaction to the desired value.

This is not an unique technique for ERC-223 tokens, the process is the same for Ether transfers. Anyone who transferred ETH to smart-contracts must be familiar with it.

Since you can't own D223 tokens and not have the ETH to cover gas fees we assume that users are aware of the process details.

4. extcodesize is not an exhaustive method of determining the contract

This is a known specific of EXTCODESIZE opcode, it is well documented in the ERC-223 standard description so we assume everyone is aware of how it works.

5. Taking ownership of the contract

Fixed at: Dexaran/D223-token-v2@a7018f4

6. Transfer 0 tokens

Transfer of 0 tokens must be valid for both ERC-20 and ERC-223 standards.
In case of ERC-223 it can be used as a standard introspection method by third party contracts. These don't need to own any ERC-223 tokens to send this call and therefore they can transfer 0 tokens and watch if the tokenReceived function was invoked as the result. If it was - then the examined token is ERC-223.

7. Function state mutability can be restricted to pure

Fixed at: https://github.com/Dexaran/D223-token-v2/blob/ea53ca774e4753faf370195bf336667f2da26f74/D223_token_v2.sol#L163