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 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.
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.
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;
}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;
}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.
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;
// ...
}Remove this condition.
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.
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;
}Consider similar work for both standards.
The arguments are reasonable.
The low-level function call requires a success check, otherwise Ether may remain locked on
ERC223WrapperTokencontract. This may happen in case of arevertin the recipient contract and Ether will return toERC223WrapperTokencontract, but not to the user(caller).Recommended solution:
Alternative solution (without interruption):