dexaran / tokenstandardconverter

Ethereum Token Converter

This service is created to introduce "cross-standard" interoperability of tokens on Ethereum and EVM-compatible chains.

The most used Ethereum fungible token standard is ERC-20 and this standard is quite old. Therefore it is necessary to introduce a clear procedure of "standard upgrade" without requiring redeploying of existing token contracts. The Token Converter smart-contract can be exactly the tool that ensures smooth transition from the old standard to a newer one.

This particular implementation converts ERC-20 tokens to ERC-223 tokens. ERC-223 tokens can be converted back to the ERC-20 origin anytime.

Deployment

Converter v4 (CLO mainnet): https://explorer.callisto.network/address/0xf0ddb84596C9B52981C2bFf35c8B21d2b8FEd64c/transactions

Ethereum Mainnet v4: https://etherscan.io/address/0x1e9d6cba29e4aa4a9e2587b19d3f0e68de9b6552

Old Version

Converter v3: https://explorer.callisto.network/address/0xc68AD4DDCB3C9cAd52852E6dF7102b77c32865A5/transactions

tokenstandardconverter's Issues

Wrapped tokens did not take into account original decimals

If the user wraps tokens with decimals different than 18, the wrapped token will have an incorrect representation.

TokenStandardConverter/TokenConverter.sol

Lines 122 to 131 in c740cec

	function createERC223Wrapper(address _erc20Token) public returns (bool)
	{
	require(address(erc223Wrappers[_erc20Token]) == address(0), "ERROR: Wrapper already exists.");
	ERC223WrapperToken _newERC223Wrapper = new ERC223WrapperToken(_erc20Token, "ERC-223-Wrapper", "Wrapped Token", 18);
	erc223Wrappers[_erc20Token] = _newERC223Wrapper;
	erc20Origins[address(_newERC223Wrapper)] = _erc20Token;
	return true;
	}

Never use function `transfer` and `transferFrom` for arbitrary third-party tokens.

You are using the function transfer and transferFrom for any arbitrary tokens (unknown tokens). It's high severity issue.
You should use safeTransfer and safeTransferFrom function instead of them

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/f347b410cf6aeeaaf5197e1fece139c793c03b2b/contracts/token/ERC20/utils/SafeERC20.sol#L33-L46

For example, if you deploy this converter on ETH chain and users wrap USDT tokens it will stuck on the contract forever.

Moreover, functions transfer and transferFrom may return false if the sender has insufficient balance (like DAO token. You did not check the return value of transferFrom here:

TokenStandardConverter/TokenConverter.sol

Line 136 in c740cec

IERC20(_ERC20token).transferFrom(msg.sender, address(this), _amount);

So users can mint wrapped tokens even with 0 original tokens on balance.

returned value never checked.

The function tokenReceived returns magic number:

TokenStandardConverter/TokenConverter.sol

Lines 112 to 120 in c740cec

	function tokenReceived(address _from, uint _value, bytes memory _data) public override returns (bytes4)
	{
	require(erc20Origins[msg.sender] != address(0), "ERROR: The received token is not a ERC-223 Wrapper for any ERC-20 token.");
	IERC20(erc20Origins[msg.sender]).transfer(_from, _value);
	erc223Wrappers[msg.sender].burn(_value);
	return 0x8943ec02;
	}

But in the only place where this return value can be checked ignore it.

TokenStandardConverter/TokenConverter.sol

Lines 67 to 69 in c740cec

	if(Address.isContract(_to)) {
	bytes4 ret = IERC223Recipient(_to).tokenReceived(msg.sender, _value, _empty);
	}

Standard event should be emitted

According to ERC223 standard on each transfer this event should be emitted:

https://github.com/Dexaran/ERC223-token-standard/blob/7c35af76f0175f1260ef1cf01e62ece787c1d086/token/ERC223/IERC223.sol#L31-L34

So uncomment this row
//emit Transfer(msg.sender, _to, _value, _data);
and here.

Developers are free to add more events if needed, so I recommend to emit
emit Transfer(msg.sender, _to, _value); (as it is, to be compatible with explorers)

but this event is unnecessary and can be removed
https://github.com/Dexaran/TokenStandardConverter/blob/5690be8dfb80903d0ea5d553f20b266baf503549/TokenConverter.sol#L112C34-L112C34

Owner can withdraw all erc20 and erc223 tokens from wrapper contract

1. When rescuing ERC20 tokens, the _stuckTokens is calculated but is not used.
   

   TokenStandardConverter/TokenConverter.sol

   Line 445 in 5690be8

   safeTransfer(_token, msg.sender, IERC20(_token).balanceOf(address(this)));

must be:

        safeTransfer(_token, msg.sender, _stuckTokens);

2. This function allows the owner to withdraw ERC223 tokens which were deposited to wrap to ERC20. Therefore, to protect users you should add:

        require(address(erc20Wrappers[_token]) == address(0), "ERC223 origin is not allowed");

wasting of gas in function convertERC20toERC223

You don't need to check balanceOf(msg.sender) before and after the token transfer. Removing these calls will save about 5K gas.

TokenStandardConverter/TokenConverter.sol

Lines 136 to 144 in 2d297bc

	uint256 _callerBalance = IERC20(_ERC20token).balanceOf(msg.sender); // Safety variable.
	uint256 _converterBalance = IERC20(_ERC20token).balanceOf(address(this)); // Safety variable.
	IERC20(_ERC20token).transferFrom(msg.sender, address(this), _amount);
	erc20Supply[_ERC20token] += _amount;
	require(
	IERC20(_ERC20token).balanceOf(msg.sender) + _amount == _callerBalance &&

The contract should check only own balance balanceOf(address(this)). The contract should mint the amount which it receives. Does not matter what msg.sender balance is.

medium severity issue in unwrap process

When a user sends wrapped tokens to contract to unwrap it, the value of erc20Supply[_ERC20token] was not decreased by unwrap amount.

All wrapped tokens has the same name and symbol

TokenStandardConverter/TokenConverter.sol

Line 126 in c740cec

ERC223WrapperToken _newERC223Wrapper = new ERC223WrapperToken(_erc20Token, "ERC-223-Wrapper", "Wrapped Token", 18);

I don't think such kind of tokens will be usefull

Wrapper is not compatible with tokens with transfer fee

Some tokens (like reflecting tokens) take a fee on transfer. So receiver will receive fewer tokens than the sender sends.

This requirement does not allow to use such tokens

TokenStandardConverter/TokenConverter.sol

Lines 399 to 401 in 5690be8

	require(
	IERC20(_ERC20token).balanceOf(address(this)) - _amount == _converterBalance,
	"ERROR: The transfer have not subtracted tokens from callers balance.");

I would recommend replacing this part:

TokenStandardConverter/TokenConverter.sol

Lines 397 to 401 in 5690be8

	erc20Supply[_ERC20token] += _amount;
	require(
	IERC20(_ERC20token).balanceOf(address(this)) - _amount == _converterBalance,
	"ERROR: The transfer have not subtracted tokens from callers balance.");

with:

        _amount = IERC20(_ERC20token).balanceOf(address(this)) - _converterBalance;
        erc20Supply[_ERC20token] += _amount;

Incompatible Transfer event

TokenStandardConverter/TokenConverter.sol

Line 70 in c740cec

emit Transfer(msg.sender, _to, _value, _empty);

Transfer event has new format (different than in ERC20), so it will not be recognized by current explorers and wallets.

Follow good coding practice

If you want to use this contract to publish EIP, then better to follow good code practice.

1. Follow Style Guide.

https://docs.soliditylang.org/en/v0.8.23/style-guide.html

2. Missing docstrings.

The contracts in the code base lack documentation. This hinders reviewers’ understanding of the code’s intention, which is fundamental to correctly assess not only security but also correctness. Additionally, docstrings improve readability and ease maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, the values returned, and the events emitted.

Consider thoroughly documenting all functions (and their parameters) that are part of the contracts’ public API. Functions implementing sensitive functionality, even if not public, should be documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).

https://docs.soliditylang.org/en/v0.8.23/natspec-format.html

3. Functions not used internally could be marked as external.

It's a good coding practice to mark a function external when it's not called within the contract but only from outside the contract.

Use safeTransfer for third-party tokens

The function unwrapERC20toERC223 transfers third-party ERC223 to the user. We can't be sure if the developer strictly follows the ERC223 standard. So better to replace this:

TokenStandardConverter/TokenConverter.sol

Line 415 in 5690be8

IERC223(erc223Origins[_ERC20token]).transfer(msg.sender, _amount);

with:

            safeTransfer(erc223Origins[_ERC20token], msg.sender, _amount);

supportsInterface

In the function supportsInterface() you should use standard IERC20 interface to be recognizable by others, instead of using yours IERC20

TokenStandardConverter/TokenConverter.sol

Lines 10 to 23 in 5690be8

	/**
	* @dev Interface of the ERC20 standard as defined in the EIP.
	*/
	interface IERC20 {
	function name() external view returns (string memory);
	function symbol() external view returns (string memory);
	function decimals() external view returns (uint8);
	function totalSupply() external view returns (uint256);
	function balanceOf(address account) external view returns (uint256);
	function transfer(address to, uint256 value) external returns (bool);
	function allowance(address owner, address spender) external view returns (uint256);
	function approve(address spender, uint256 value) external returns (bool);
	function transferFrom(address from, address to, uint256 value) external returns (bool);
	}

The interface mustn't contain OPTIONAL functions (name, symbol, decimals).
https://eips.ethereum.org/EIPS/eip-20

Unnecessary code

1. The wrapper token contract can't call transferFrom in itself, so this can be removed:

• TokenStandardConverter/TokenConverter.sol

  Line 163 in 5690be8

  require(_spender != address(this), "ERC-223: Approving to token contract error.");

• TokenStandardConverter/TokenConverter.sol

  Line 250 in 5690be8

  require(_spender != address(this), "ERC-20: Approving to token contract error.");

2. The variable wrapper_for should be private (to avoid generating a useless getter function), since the getter function origin() public view returns (address) { return wrapper_for; } already exists.

• TokenStandardConverter/TokenConverter.sol

  Line 188 in 5690be8

  address public wrapper_for;

• TokenStandardConverter/TokenConverter.sol

  Line 76 in 5690be8

  address public wrapper_for;

3. The variable balances should be private (to avoid generating a useless getter function), since the getter function balanceOf(address _owner) public view override returns (uint256) { return balances[_owner]; } already exists.

• TokenStandardConverter/TokenConverter.sol

  Line 90 in 5690be8

  mapping(address => uint256) public balances; // List of user balances.

• TokenStandardConverter/TokenConverter.sol

  Line 200 in 5690be8

  mapping(address => uint256) public balances; // List of user balances.

4. Variable empty is not used

• TokenStandardConverter/TokenConverter.sol

  Line 220 in 5690be8

  bytes memory _empty = hex"00000000";

5. The variables
   

   TokenStandardConverter/TokenConverter.sol

   Lines 276 to 280 in 5690be8

   	mapping (address => ERC223WrapperToken) public erc223Wrappers; // A list of token wrappers. First one is ERC-20 origin, second one is ERC-223 version.
   	mapping (address => ERC20WrapperToken) public erc20Wrappers;
   	mapping (address => address) public erc223Origins;
   	mapping (address => address) public erc20Origins;

should be private (to avoid generating useless getter functions), since the getters already exist:

TokenStandardConverter/TokenConverter.sol

Lines 283 to 311 in 5690be8

	function getERC20WrapperFor(address _token) public view returns (address, string memory)
	{
	if ( address(erc20Wrappers[_token]) != address(0) )
	{
	return (address(erc20Wrappers[_token]), "ERC-20");
	}
	return (address(0), "Error");
	}
	function getERC223WrapperFor(address _token) public view returns (address, string memory)
	{
	if ( address(erc223Wrappers[_token]) != address(0) )
	{
	return (address(erc223Wrappers[_token]), "ERC-223");
	}
	return (address(0), "Error");
	}
	function getERC20OriginFor(address _token) public view returns (address)
	{
	return (address(erc20Origins[_token]));
	}
	function getERC223OriginFor(address _token) public view returns (address)
	{
	return (address(erc223Origins[_token]));
	}