ethereum
Update ERC-7417: Move to Review
When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md
We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met:
- The PR edits only existing draft PRs.
- The build passes.
- Your GitHub username or email address is listed in the 'author' header of all affected PRs, inside
. - If matching on email address, the email address is the one publicly listed on your GitHub profile.
@SamWilsn the rationale and motivation sections are updated. We are good to go.
Also I have a couple of questions:
- The code is currently under security audit. Once it's done - can we attach the audit report somehow? Or can we attach it as a comment in the code of the reference implementation?
- I'd like to deploy the code and write down the contract address to the proposal text. This needs to be done after the proposal will pass the
reviewstate (as it makes no sense to deploy something and then re-deploy if changes are requested). Can this final change be applied in thelast call?
The commit 62bdf2deb479199a2b219b268417fe0e9fabaa46 (as a parent of 680c733cbac86f75f500f7895b63ab11fe0ffa11) contains errors. Please inspect the Run Summary for details.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@Dexaran , some notes. My review applies to version published at https://eips.ethereum.org/EIPS/eip-7417 . Everywhere I say "what this means?" or ask any other question, this is not a question. This is statement "I was not able to understand immediately, and thus other people will probably have difficulties, too, so, please, rephrase". (Same applies to my bug https://github.com/ethereum/ERCs/issues/719 )
On ERC20Rescue.extractor in reference implementation: ha-ha. ERC with hard-coded address of particular person will never be accepted. Hard-coded address simply means that this contract is a little centralized, which is unacceptable for ERC. If you think this is needed, then simply deploy your converter as a public service, but don't submit it as ERC.
Also, I have an idea: instead of hard-coded address just let user submit cryptographically verified proof of log, which shows that the user mistakenly sent money.
As well as I understand contract cannot read logs. But contract can verify logs, see details here: https://blog.ethereum.org/2015/11/15/merkling-in-ethereum . If some user mistakenly sends money, then they can send cryptographically verified log entry of that transfer to your contract. And your contract will verify this transfer on-chain. And will send money back. I'm nearly sure this is totally possible. I'm very enthusiastic about this. If you think this is not possible, tell me your concerns, I will try to address them.
Obviously, this will depend on particular way Ethereum currently stores logs. As well as I understand they are currently stored as Merkle tree. If this ever changes, code will need to be rewritten.
Of course, such cryptographic verifying will possibly be gas-expensive. But if someone mistakenly sends a million USD, they will accept the cost.
On ERC223WrapperToken.transferFrom in reference implementation: there is inconsistency here: transferFrom logs one Transfer event with 3 values, but transfer(address _to, uint _value) logs 2 Transfer events (one of them with zero data). (Pause here. Day passed.) Okay, after 1 day after writing this paragraph I suddenly understood, what is going on. Both overloaded transfer functions attempt to call tokenReceived, but transferFrom do not. So, we emit 3-argument Transfer always when we transfer tokens, and we emit 4-argument Transfer when we attempt to call tokenReceived. Thus both transfer functions emit 2 events and transferFrom emit 1 event. So, okay, there seem to be no error here (right?!). But this was very hard to understand. There definitely need to exist comment in code explaining all these. And possibly even in main text.
On ERC223WrapperToken.TransferData: this event is never used. Did you run compiler or static analyzer on your code?
On ERC223WrapperToken.set: I suggest moving this code to constructor to make the code easier to follow
On ERC223WrapperToken.transfer: why 3-argument transfer is payable and 2-argument transfer is not? This is not explained anywhere. ERC-223 allows both versions to be payable. Moreover, in 3-argument transfer you first update token balances and then send ETH. This directly contradicts ERC-223, which states: "If ether was sent alongside tokens in this way then ether MUST be delivered first, then token balances must be updated". (Well, strictly speaking there is no contradiction, because this sentence from ERC-223 applies to 2-argument transfer. But I still think it is good idea to support this requirement in 3-argument transfer, too, assuming it is payable.)
On ERC223WrapperToken.standard: this function returns uint32, but function standard from https://eips.ethereum.org/EIPS/eip-223 reference implementation returns string. As well as I understand, EIP-223 is immutable now, but ERC-7417 is not, so I suggest replacing uint32 with string.
On ERC20WrapperToken.burn and all other burn and mint methods. There is a convention: emit Transfer to zero address when tokens are burnt and emit Transfer from zero when they are minted. For example: https://etherscan.io/tx/0x9a40a46b37740a7115f39e9bfdfbd044257143a57040df04c8674b5b0fae9cfe . As you can see on this Etherscan page in "ERC-20 Tokens Transferred" section, there was 5 ERC-20 token transfers in this transaction. One of them was burning of USDS and another was minting of DAI. So, this is already established convention, which is already integrated in Etherscan GUI (and possibly others). So, I suggest using this convention in all your ERC-20 and ERC-223 tokens. Moreover, in case of minting ERC-20 tokens, this is requirement by the standard. ERC-20 reads: "A token contract which creates new tokens SHOULD trigger a Transfer event with the _from address set to 0x0 when tokens are created"
On TokenStandardConverter.createERC223Wrapper and TokenStandardConverter.createERC20Wrapper: you should try harder to detect errors. If token's standard exists and returns "223", then this is ERC-223 token and thus createERC223Wrapper should return error. Similarly for ERC-20. Also, if supportsInterface(type(IERC223).interfaceId returns true, then createERC223Wrapper should fail and similarly for ERC-20. (Here I assume that type(IERC223).interfaceId and type(IERC20).interfaceId are not equal. And of course, you should check that these 2 values are equal to popular ones.)
I just spent some more time thinking about this. Now I think that possibility of creating both ERC-223 wrapper and ERC-20 wrapper for the same contract is absolute disaster. It will lead to confusion. So you should try really hard to detect tokens. So I suggest doing so: attempt to do zero-value transfer of a token to a contract. If it succeeds and tokenReceived is called, then this is ERC-223. If it succeeds and tokenReceived is not called, then this is ERC-20. If it doesn't succeed, then we abort. So, now we are absolutely sure that we will never create 2 wrappers for the same contract
ERC223WrapperToken has standard, but ERC20WrapperToken has no. Why? Also, in IERC223WrapperToken standard() returns string, but in ERC223WrapperToken it returns number. So, implementation and interface doesn't match. Also, you don't inherit ERC223WrapperToken from IERC223WrapperToken. If you did, then, as well as I understand, compiler did catch that type incompatibility. Similarly, IERC20WrapperToken has standard and ERC20WrapperToken has no, and compiler doesn't catch this, because ERC20WrapperToken doesn't inherit from IERC20WrapperToken.
ERC223WrapperToken.transfer first updates balances and then calls tokenReceived. So, tokenReceived sees transfer. Thus tokenReceived (from logical point of view) happens after transfer. But emit Transfer happens after tokenReceived! Thus any logs created by tokenReceived will appear before Transfer! Including any ERC-20 or ERC-223 token transfers! Thus blockchain explorers will show transfers in wrong order. So, please, do emit Transfer immediately after updating balances
"If there is no ERC-223 wrapper for the _ERC20token then creates it by calling a createERC223Wrapper(_erc20toke) function" - _ERC20token should be.
ERC223WrapperToken.transferFrom and ERC20WrapperToken.transferFrom: you violate widely known practices. First, if msg.sender == _from, then allowances is not checked and not changed. Second, if allowances is maximal value, then it is not changed, i. e. maximal allowance is infinite allowance. Go read WETH9 ( https://etherscan.io/token/0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2#code ) and OpenZeppelin's ERC-20 ( https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol ).
"When developing the Converter it was decided to prioritize compatibility with existing ecosystem" - this phrase implies that you don't emit ERC-223 4-argument Transfer events. But you do emit them in transfer functions in your ERC-223 wrapper!
"standard() function usage for the introspection" - this section is not convincing. We can distinguish ERC-20 and ERC-223 tokens using ERC-165. We just need to agree on concrete values for type(IERC20).interfaceId and type(IERC223).interfaceId. Yes, there are a lot of ERC-20 tokens, which do not support ERC-165 standard, but the same applies to standard()
ERC165: you invented _registerInterface logic in abstract contract ERC165, but you don't use it in inherited classes. Same for mapping _supportedInterfaces. So I suggest removing this logic
standardERC20 and IERC20: there is no any difference between these two classes. And I'm nearly sure that type(...).interfaceId for them is same (but I didn't checked). So, please, remove standardERC20
IERC223WrapperToken has comment "@ dev Interface of the ERC20 standard." 🤦 I highly suggest re-reading whole document.
ERC223WrapperToken.supportsInterface: please, don't return true on type(IERC20).interfaceId. ERC223WrapperToken doesn't support ERC-20 in full
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.
Anybody can call ERC223WrapperToken.rescueERC20. So, it is possible that in some point you will lose control over your wallet, then someone will call rescueERC20, and then the money will be stuck in your wallet
If you liked this review, feel free to ping me on next versions of this proposal and on anything related to tokens
@u59149403 Feel free to check out my fix for the vulnerability you discovered.
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.
