Closed TrejGun closed 10 months ago
related discussion https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3575
There is advanced work to do this in https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3017. It's been delayed due to issues around interpretation of the spec. We'll get back to it as soon as priorities allow.
@ernestognw
I did my research on EIP-1363 and I'd like to provide my feedback.
Pull transactions are very bad choice from security point of view. I wrote an article about Pull transactions vs Push transactions recently. Pull transactions are not applicable to decentralized trustless systems at all. They are not designed for it. We can't remove approvals right now, I understand, but at least the standard must not contain them as a musthave feature. They are supposed to be deprecated eventually.
Minimalistic design. Less code. The more code and features a program has - the harder it is for a developer to understand it correctly and implement without mistakes. We must not focus on multitool solutions that have 30 versions of functions for everything - we must focus on implementations that are viable with as few code as possible to reduce the number of hacks in the industry. Elon said "write less code". Elon is correct because he works with rockets and "security" makes sense there: https://twitter.com/elonmusk/status/1211557592125857793
Error handling / transaction handling.
There is no point in redefining existing function signatures or adding new functions that do exactly the same as existing ones. What needs to be redefined is transferring method logic, not function signatures. If a standards does not redefine the logic of transfer
ERC-20 function then it is a security vulnerability. If it does redefine the logic of transfer
function similar to how ERC-223 does this - then there is no point in adding new functions like transferAndCall
as the basic transfer() will be sufficient.
ERC-1363 logic is inherently misleading. This token standard declares 10 functions that can transfer tokens. EOS token has only one transferring function and there is only one method of transferring a token. ERC-223 has only one method of transferring a token (but it is overloaded for backwards compatibility with ERC-20 UIs). Ether has only one way of transferring.
Because it requires a champion to be adopted as I said earlier. And the champion of ERC-1363 is not here it seems.
It has approvals as a core part of the standard. It is a security flaw (that I described in the article above). Approvals must be an optional feature because they are totally unnecessary if you implement transaction handling. Eventually approvals must be deprecated as they pose a threat to safety of users funds.
It redefines transferring function names and ABI. This will be extremely hard to convince wallet devs and UI devs to restandardize their services. With ERC-223 you don't need to do anything because its ABI is the same as ERC-20 and every UI that works with ERC-20 will work with ERC-223 by default and it will be already auto-secure because the ERC-223 solves most problems of ERC-20 tokens on standard level.
Hey @Dexaran I'm very excited to see you here
First of all, let me point out some facts:
using these statements it's easy to come to the conclusion that people will use standard at the final stage integrated into their favorite framework.
while all of your arguments seem to be reasonable, the fact of the existence of erc1363 (and eip4524) means they are not enough
on the other hand:
This basically means you should not worry about my custom implementation of erc998. I can easily switch back to erc223 when:
So I wish you luck, strength, and lots of community support to finish what was started back in the days
while all of your arguments seem to be reasonable, the fact of the existence of erc1363 (and eip4524) means they are not enough
I have pointed out what was not enough - marketing component of advertising the new standard.
And I also pointed out why ERC-1363 and other existing standards are not a solution to the problem that ERC-223 solves.
This basically means you should not worry about my custom implementation of erc998.
My comment was mostly for OpenZeppelin staff as we discussed a critical vulnerability of ERC-20.sol implementation and they pointed me here stating that this proposal can be a solution. And I'm saying that in my opinion it is not a viable solution in its current state.
I personally have no objections against ERC-1363 but if it is going to be supported I would recommend to re-define the logic of transfer
function of ERC-20 so that in a basic ERC-1363 implementation it would not allow transferring to contract addresses.
This thread is about EIP-1363 support and it's best to keep it that way. The standard is already finalized so there's no way to make changes to it, even by the original author.
The EIP process is about ecosystem consensus and there seems to be demand for EIP-1363 while others are not yet finalized. We can see a significant amount of verified contracts including "ERC1363"
The current issue with EIP-1363 is that the return value of onTransferReceived
can't be returned by an EOA. We'd like to asses community consensus around this particular ambiguity.
The EIP process is about ecosystem consensus and there seems to be demand for EIP-1363 while others are not yet finalized.
That's great but it's as insecure as ERC-20. So there must be a clear restriction on transfer
function that would prevent it from sending tokens to contracts. Otherwise it will be compatible with the (insecure) standard and will inevitably result in a permanent freeze of tokens in exactly the same way as it can happen with ERC-20.
So this standard inherits all the security problems of ERC-20 and I recommend:
transfer
functionThe current issue with EIP-1363 is that the return value of
onTransferReceived
can't be returned by an EOA.
Can't speak on the behalf of the "majority" but it is logical to examine the recipient and not to send via transferAndCall
to EOA. If the expected return of onTransferReceived
is not returned - consider transfer invalid and revert()
the transaction.
The specification of the EIP-1363 does not provide reference implementation or any description that declares token behavior in such scenarios.
@Dexaran if you are serious about reviving erc223 please open a new thread, make an RP with implementation and I promise to test it against my codebase and give you feedback
meanwhile, you can check how erc1363 is used in my system.
bytes4 constant IERC1363_RECEIVER_ID = 0x88a7ca5c;
bytes4 constant IERC1363_ID = 0xb0202a11;
library ExchangeUtils {
using Address for address;
using SafeERC20 for IERC20;
function spendFrom(
address token,
uint256 amount,
address spender,
address receiver
) internal {
if (_isERC1363Supported(receiver, token)) {
IERC1363(token).transferFromAndCall(spender, receiver, amount);
} else {
SafeERC20.safeTransferFrom(IERC20(token), spender, receiver, amount);
}
}
function _isERC1363Supported(address receiver, address token) internal view returns (bool) {
return
(receiver == address(this) ||
(receiver.isContract() && _tryGetSupportedInterface(receiver, IERC1363_RECEIVER_ID))) &&
_tryGetSupportedInterface(token, IERC1363_ID);
}
function _tryGetSupportedInterface(address account, bytes4 interfaceId) internal view returns (bool) {
try IERC165(account).supportsInterface(interfaceId) returns (bool isSupported) {
return isSupported;
} catch (bytes memory) {
return false;
}
}
}
@ernestognw that comment about EOA, man you just can't prevent people from shooting their foot
@TrejGun can you show the code of isContract()
? Is it the function from Address lib? I recall there was a change of its logic and I'd like to review which implementation is used in your case
yes this function comes from OZ framework
this one doesn't have .isContract() implemented https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol it seems
well you are right it was recently removed https://github.com/OpenZeppelin/openzeppelin-contracts/commit/c5d040beb9a951b00e9cb57c4e7dd97cd04b45ac
Initially we were using a method for identifying which address is a EOA and which one is a contract that I proposed in 2017. We were assemblying the code and if it's length was non-zero then it was considered that examined address is a contract:
https://github.com/Dexaran/ERC223-token-standard/blob/development/utils/Address.sol#L24
function isContract(address account) internal view returns (bool) {
// This method relies in extcodesize, which returns 0 for contracts in
// construction, since the code is only stored at the end of the
// constructor execution.
uint256 size;
// solhint-disable-next-line no-inline-assembly
assembly { size := extcodesize(account) }
return size > 0;
}
There is a caveat that must be taken into account: if you will call extcodesize
on an account in the deployment transaction - it will say it is not a contract even though the contract will be deployed at this address.
is there any difference in
return address(this).code.length == 0
and
uint256 size;
assembly { size := extcodesize(this) }
return size > 0;
rather than a few saved gas units
I don't think there is any significant difference
@ernestognw that comment about EOA, man you just can't prevent people from shooting their foot
It's true we can't prevent people's mistakes, but we would prefer providing the simplest secure implementation. The fact that ERC-1363 does not specify the EOA behavior leaves two alternatives:
On one hand, if transferAndCall
doesn't revert implementers will need to check whether the specific transferAndCall
implementation they're calling reverts or not with EOAs. This is because some implementations out there (including #3017 and #4631) already do that and is highly likely that somebody has already deployed versions like this (see @vittominacori's erc-contract-payable dependants)
The pro of this approach is that reverting when calling EOAs is a behavior that can be implemented on top:
if (target.code.length == 0) revert SomeError();
token.transferAndCall(target, amount);
On the other side, if transferAndCall
reverts, implementers will need to check if the callee is an EOA anyway to use transfer
/transferAndCall
accordingly:
if (target.code.length == 0) {
token.transfer(target, amount);
} else {
token.transferAndCall(target, amount);
}
I agree with @Amxx comment's that not reverting is the most efficient alternative because it leaves the target.code.length == 0
check to the contract calling a token.transferCall
.
However, because this approach still requires user checking, we may want to provide a safeTransferAndCall
(or similar) utility to deal with transferAndCall
implementations that revert, but that may add an extra code.length == 0
check. This is how we've dealt with EIP ambiguities in the past but I'm not a fan of these utilities.
Since there are EOA-reverting implementations out there, I'd think it's safer to just revert for EOAs and let the users check if it's an EOA or not. They'll most likely do it anyway for safety when interacting with untrusted tokens and the savings of avoiding the target.code.length == 0
aren't in a high order value (perhaps a few gas units).
@ernestognw thanks for pointing out. I agree with your considerations and understand that you need to implement something to be included in a mass adopted library.
The EIP was developed to add functionalities to ERC20 in order to be able to perform actions after transfers or approvals.
The *andCall
methods were not intended to replace the transfer
or approve
behaviors or to be used in their place or having different behaviors dealing with contracts or EOA. While it might seem ambiguous, the original specifications say that A contract that wants to accept token payments via *andCall ...
and doesn't mention EOA since they are not included in (my) proposal.
Users who simply want to transfer tokens can continue to use the transfer
method or implement the contract using approve
and transferFrom
. But users who want to be sure to perform an action after a transfer or approval can use the *andCall
methods. This was my purpose.
I see your intention was to revert, which in my opinion is what the EIP attempted to state. However, I think we've agreed the EIP is finalized and that should be our source of truth. It doesn't mention the EOA case at all, thus is an undefined behavior.
Given a regular user can expect both behaviors, I think the best way to implement ERC1363 in OZ Contracts will be to pick one and provide a utility along with it to deal with the selected scenario:
transfer/approve
and *andCall
Personally, I'd not revert to avoid the extra check @Amxx pointed out but given that there are multiple reverting implementations out there, I'd go for reverting and include a routing function as a utility (or maybe just docs recommendations).
Hi @Dexaran any news on ERC223?
ERC-223 is now "Final". Can be used in production (after 6 years of discussions I don't believe we can discover something "new" about this standard).
Here you can track the resources https://dexaran.github.io/erc223/
We've built a Token Converter service https://dexaran.github.io/token-converter/ The token converter will transform ERC-20 tokens to ERC-223 or vice versa i.e. for every token on Ethereum chain there will be two versions: ERC-20 and ERC-223
Converter is undergoing a security audit now, after that we will proceed to deployment. It has its own EIP 7417 https://eips.ethereum.org/EIPS/eip-7417
My team is developing a ERC-223 compatible exchange at the moment https://dex223.io/
btw here is a script that calculates the amount of "Lost" ERC-20 tokens https://dexaran.github.io/erc20-losses
Now with improved UX
oh wow! really cool Will you make a PR here with a new extension?
Probably later. I predict that OZ staff will reply "we analyzed the chain and there are no ERC-223 contracts so we are not interested" as they did before.
So it's better to deploy the token converter first.
Probably later. I predict that OZ staff will reply "we analyzed the chain and there are no ERC-223 contracts so we are not interested" as they did before.
So it's better to deploy the token converter first.
Basically, yes. Can you create an issue specific to ERC-223? We're overpopulating this one. Just hid the comments as off-topic.
🧐 Motivation
I was using ERC223 for some time with ERC998 but it seems to be obsolete and superseded by EIP-1363. It would be good to have the out-of-the-box implementation of a notification mechanism for receiving ERC20 tokens, the same as you have for ERC721 and ERC1155. We would like to use this standard for ERC998 tokens and for OZ VestingWallet contract.
📝 Details