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.

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)