Audit report of "ERC-7417 Reference Implementation"

erc7417_audit.md

"ERC-7417 Reference Implementation" audit report

Summary

This is a security audit report performed on ERC-7417 Reference Implementation by Aleksandr Gorbunov.

The subject of the audit is a smart contract service that converts token of ERC-20 version to ERC-223 version.

The audit takes into account the safety concerns described in EIP-223 and is not reviewed again here. You can read them here: https://github.com/Dexaran/ERCs/blob/master/ERCS/erc-7417.md

In scope

1. erc7417.sol

Findings

In total, 3 issues were reported including:

• 0 critical severity issues.

• 1 high severity issues.

• 0 medium severity issues.

• 2 low severity issues.

• 0 notes.

Issues requiring attention have been found.

Security issues

1. Transfer of ether without control

Severity: high

Description

Reference Implementation proposed to implement the transfer(address,uint,bytes) function with the ability to accept ethereum for subsequent transfer to the recipient. Thus ensuring cost-effectiveness and convenience for the user - to transfer both tokens and ethers in one transaction.

However, let's consider the case where the recipient is a smart contract. Then transferring ether using payable(_to).transfer(msg.value) below the recipient.tokenReceived() call does not account for the incoming ether on the receiving end.

First, all the basic logic of erc-223 token deposit accounting will be implemented in the tokenReceived function in the recipient smart contract. fallback() payable function call will only happen after that. But in this function it will be impossible to make any accounting of the incoming ether, because the payable(addr).transfer(msg.value) function is limited of gas, which is enough only for the transmission of ether, without making any changes to the state variables.

Code

erc7417.sol, line 205 - sending ether.

function transfer(address _to, uint _value, bytes calldata _data) public payable override returns (bool success)
{
    balances[msg.sender] = balances[msg.sender] - _value;
    balances[_to] = balances[_to] + _value;
    if(Address.isContract(_to)) {
        IERC223Recipient(_to).tokenReceived(msg.sender, _value, _data);
    }
    if (msg.value > 0) payable(_to).transfer(msg.value);
    emit Transfer(msg.sender, _to, _value, _data);
    emit Transfer(msg.sender, _to, _value); // Old ERC-20 compatible event. Added for backwards compatibility reasons.

    return true;
}

Recommendation

One possible solution is to send ether before calling the receiver.tokenReceived() function. However, this method has some disadvantages. For example, anyone can send ether to the fallback function without calling the tokenRecevied(), by simply sending ether to the contract. In this case, the calculations will also be distorted.

Another possible solution with full control over incoming funds is to process incoming ethers and tokens in one tokenReceived() call:

function transfer(address _to, uint _value, bytes calldata _data) public payable override returns (bool success)
{
    balances[msg.sender] = balances[msg.sender] - _value;
    balances[_to] = balances[_to] + _value;
    if(Address.isContract(_to)) {

        if (msg.value > 0) {
            // Possible solution
            IERC223Recipient(_to).tokenReceived{value: msg.value}(msg.sender, _value, _data);
        } else {
            IERC223Recipient(_to).tokenReceived(msg.sender, _value, _data);
        }

    } else {
        if (msg.value > 0) payable(_to).transfer(msg.value);
    }
    emit Transfer(msg.sender, _to, _value, _data);
    emit Transfer(msg.sender, _to, _value); // Old ERC-20 compatible event. Added for backwards compatibility reasons.

    return true;
}

2. Useless require

Severity: low

Description

In the tokenReceived function, there is a require in the first line, with the condition erc223Origins[msg.sender] == address(0). It checks whether the tokenReceived function is called by the contract(wrapper) of the original ERC-223 token. But the wrapper is created automatically, according to the template(erc-20) and it does not have the ability to call the tokenReceived function.

Code

erc7417.sol, line 440 - useless require.

function tokenReceived(address _from, uint _value, bytes memory /* _data */) public override returns (bytes4)
{
    require(erc223Origins[msg.sender] == address(0), "Error: creating wrapper for a wrapper token.");
    // ...
}

erc7417.sol, line 495 - assignment of the variable "erc223Origins".

function createERC20Wrapper(address _token) public returns (address)
{
    // ...
    ERC20WrapperToken _newERC20Wrapper       = new ERC20WrapperToken{salt: keccak256(abi.encode(_token))}();
    // ...
    erc223Origins[address(_newERC20Wrapper)] = _token;
    // ...
}

Recommendation

Remove this condition.

3. It is impossible to mint ERC-20 tokens of wrapper for original ERC-20

Severity: low

Description

From EIP-7417:

It is possible to call createERC20Wrapper function and provide an address of an existing ERC-20 token. The Token Converter will successfully create a ERC-20 wrapper for that ERC-20 original token.

It is worth considering that when created a ERC-20 wrapper for that ERC-20 original it will be impossible to mint tokens for wrapper. The reason is that the tokenReceived function (where minting is implemented) cannot be called from the original ERC-20 token.

This could be ignored, since this functionality is a side effect of the implementation. But minting for ERC-223 wrapper of original ERC-223 token works.

Code

erc7417.sol, line 469 - minting ERC-20 tokens of wrapper.

function tokenReceived(address _from, uint _value, bytes memory /* _data */) public override returns (bytes4)
{
    // ...
    erc20Wrappers[msg.sender].mint(_from, _value);
    return this.tokenReceived.selector;
}

Recommendation

Consider similar work for both standards.

Issue 1 was fixed: ethereum/ERCs@62bdf2d

I assume that it may be the most comfortable approach to declare tokenReceived function as payable and handle the deposits of Ether there. In this case a contract developer would be able to implement all the logic related to the deposits of any assets in one place without a need to declare receive() function and worry about concurrently executed functions and the order of events.

However, this creates a difficulty related to the overriding of functions in Solidity. For example, if we want to use this approach then we need to declare tokenReceived as payable in our IERC223Recipient. Otherwise the compiler would throw a "sending ether to nonpayable function" error in case of this code:

if (msg.value > 0) {
    // Possible solution
    IERC223Recipient(_to).tokenReceived{value: msg.value}(msg.sender, _value, _data);
}

If we will declare tokenReceived as payable in the IERC223Recipient - then this would automatically make all the implementations that inherit this interface also payable but this is not what we want to achieve. In some contracts which are designed to receive ERC-223 tokens but not Ether the tokenReceived must be non-payable. This would prevent accidental deposits of Ether. Only the contracts intended to receive Ether must declare tokenReceived payable.

I'm afraid that if we will make tokenReceived payable in the basic implementation in IERC223Recipient then if ERC-223 standard will get adopted and more developers will start writing their code - they will start mindlessly reimplementing it in the same way without investigating whether it needs to be payable or non-payable in their particular case. This would violate a principle of failsafe defaults and potentially result in lost money (in a form of accidentally deposited Ether).

As the result the decision was made to leave tokenReceived and receive() as separate functions each explicitly declaring that a contract is designed to receive one type of assets (ERC-223 tokens and Ether respectively).

The arguments are reasonable.

Issue 1 was fixed: ethereum/ERCs@62bdf2d

The low-level function call requires a success check, otherwise Ether may remain locked on ERC223WrapperToken contract. This may happen in case of a revert in the recipient contract and Ether will return to ERC223WrapperToken contract, but not to the user(caller).

Recommended solution:

if (msg.value > 0) 
{
    (bool sent, bytes memory data) = _to.call{value: msg.value}("");
    require(sent, "Ether transfer failed.");
}

Alternative solution (without interruption):

if (msg.value > 0)
{
    (bool sent, bytes memory data) = _to.call{value: msg.value}("");
    if (!sent) {
        payable(msg.sender).transfer(msg.value);
    }
}

Fixed in Dexaran/ERCs@b40317e

Security issue discovered by @u59149403:

Okay, now I found actual bug. Let's assume that 0x01000B5fE61411C466b70631d7fF070187179Bbf address owner (presumably this is you) will call TokenStandardConverter.extractStuckERC20 with original ERC-223 token (for which ERC-20 wrapper exists) as an argument. Then you will be able to withdraw all amount of this token.

Link: ethereum/ERCs#658 (comment)