Closed AugustoL closed 2 years ago
I really like the fact that it remains fully ERC20 compatible, and does not force receivers of the token to implement a certain function to signal that they accept them.
One question though: what is the rationale behind making the external call before the transfer or approval? I would have expected the call to occur afterwards, so the receiver of the tokens can check the transferred/approved balance and act upon it on that same call. And by throw
ing if the call goes wrong, the transfer gets rolled back all the same.
@spalladino In the approveData
method the call is executed after the approve
and in the transfer methods the call is executed before the transfer. At first all calls were executed before the ERC20 methods, but I changed the order in the approveData
because I think the next call to be executed after an approve will want to have the amount to be transfered already approved, doing this you can execute a transferFrom
inside the call and if all the things you wanted to do and check went fine you claim the tokens.
I think the order of the function call in approveData
is not explained correctly in the issue, Im going to update it.
You are right that if the call fails the transfer
and transferFrom
will be reverted, the order can be changed and it will have the same effect I guess.
I have no problem on changing the order :)
Oops sorry, I misunderstood the order in approveData
. As for the order in transferData
and transferDataFrom
, I'd follow the same approach of transfer-then-call as in approve, for the same reasons you mention. WDYT?
I agree, im going to change the description in the issue.
I like this. Where do you envision this standard relative to ERCs 20, 223, and 777? It seems like 827 is a fantastic stopgap that's 20-compatible that should be adopted relatively quickly before the community gets around to ratifying 223 or 777.
Thoughts?
I really like this, thanks a lot! What do you think about overloading transfer
, approve
and transferFrom
functions considering the new bytes _data
argument?
@Shrugs yes I agree, less code is safer and easier to test, and I think this new methods fulfill the needs that the rest of the standards are trying to cover.
@facuspagnuolo but if we do that we will change the ERC20 function signatures and it wont be ERC20 compatible anymore, right? I think we cant have functions with the same name and different arguments, or we can? :thinking:
@AugustoL as far as I know, if we provide an overloaded function changing the amount of parameters, I think it won't change the signature of the ERC20 functions, for example:
transfer
function will keep being: keccak256("transfer(address,uint256)")
transfer
function will be: keccak256("transfer(address,uint256,bytes)")
am I right?
@facuspagnuolo I think you are, let me do a quick test! :eyes:
Another quick thing, we might want to distinguish between throw
and revert
in the spec
@facuspagnuolo I changed the method names to be the same as ERC20 but with the data argument and the signature is different so there is not issue on having the same name but with the _data
argument.
I think that allowing the token contract to call any function at any address is much less useful than calling some sort of tokensReceived
method on the receiving contract (as in ERC223/777). Receiving contracts have no way of knowing that the calling address is a token contract, so they can't really do anything useful with the passed in data.
A big part of this is that msg.sender
will always be the token address, where a tokensReceived
method knows the intended sender from the passed in arguments. There is also no explicit way to link the token transfer to the function call, which really limits the possibilities.
It also seems like it could have unintended consequences to allow a token to call any arbitrary data. For example, if tokenA is accidentally transferred to an ERC827 token contract, anyone would be able to claim these tokens by simply calling tokenA.transfer(self, value)
from the ERC827 token.
@abandeali1 thanks for the feedback! But I think this is more useful because you dont need to have any specific method in the receiver contract to call it, but if the contract has it you can still use it, so you can call any other contract and make use of the tokenFallback
function. In fact I think we can say that this token is also ERC223-Receiver compatible ?
There is way to know who executed the transaction, you can send a signature of the msg.sata
as a parameter and verify the signer in the contract that is receiving the call form the token. you can also use tx.origin
but I understand that is not recommended.
And if there is tokens transfered to this contract they can be claimed by anyone, I dont see any problem there, at least they are not stuck there, which I think is the main problem, having tokens that are not able to being used.
@AugustoL this is partially true. The major difference is that in ERC223/777, from
and value
are always passed to the tokensReceived
callback, so receiving contacts can rely on those arguments with the assumption that the token conforms to the standard. With ERC827, what is preventing me from telling the receiving contract that I transferred 10000000 tokens when I actually only transferred 1?
To the second point, this particular case might not be that bad. But this is just a single example of unintended consequences, and I can't confidently say that there aren't other cases out there that would lead to a worse outcome.
As of yesterday's release of zeppelin, those links in the above post no longer work because the contract code has been migrated into separate directories:
https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/contracts/token/ERC827/ERC827Token.sol
Thanks @strotter ! fixed :)
I find one important problem with this standard. The call to the receiver's contract is not authenticated. The receiver's contract cannot tell if the data received comes in fact from the sender or anyone else. I see people will start using it in this way:
token.transfer(receiver,amount, abiEncodingOf(received(sender ,amount)) The receiver contract would have a method received(address sender, uint amount). The security of this pattern is obviously completely broken.
It would be much much useful if the receiver would receive the arguments token, sender and amount right from the token contract, and not from the supposedly sender.
The pattern that uses approve/transferFrom can also lead to common mistakes if a honest sender uses token.transferFrom(receiver,amount), and then a malicious user uses token.transfer(receiver,0,simData) just to simulate the previous transferFrom() has a data argument.
Also this standard will break any other future standard that tries to fix this problem and try to notify a receiver with authenticated source/amount information. Once you allow the token to emit any message, you cannot authenticate anything later.
Still another problem is that you can use the token to send messages to itself. If the token contract is owner of tokens, then you can use this to steal the tokens from the token contract.
Very bad standard IMHO.
Now that I read the previous comments, I note that ALL that I said has been already said in a previous comment by @abandeali1
@SergioDemianLerner Thanks for your feedback!
We require that the the _spender
and _to
cant be the token address itself but I didnt had any note about that in the spec.
If the contract have any other tokens or assets they are open for grabs, I dont see a problem there since the contract in my opinion in only responsible of handling the internal balances.
Authentication is something that I think it can be done from the other contract too, without requiring the contract to be called to have a function to receive tokens.
How do you see message signing/verification being used in the receiver contract for authentication? If you need to authenticate a user it can send a signature of a hash generated inside the function and verify the real sender there.
I use a lot more transferFrom
(with _data param) and approval
to check the availability of the value that the sender is willing to transfer, and claiming it afterwards. The use of transfer
(with _data param) can be useful for some cases, but I see much more utility in transferFrom & approve strategy for transferring value.
@AugustoL To be clear,
I use a lot more transferFrom (with _data param) and approval to check the availability of the value that the sender is willing to transfer, and claiming it afterwards. The use of transfer (with _data param) can be useful for some cases, but I see much more utility in transferFrom & approve strategy for transferring value.
You are talking about a pattern like this?
token.approve(receiver, value, keccak256("canRecieve(sender)");
Contract Receiver {
function canReceive(address sender) public {
uint256 allowedValue = tokenContract.allowance(sender, self);
tokenContract.transferFrom(sender, self, allowedValue);
doBusinessLogic();
}
function doBusinessLogic() { ... }
}
This would require the receiving contract be aware of the token contract address. It would be nice if the sender and value were passed as arguments so we would not need to do the approval-allowance-transferFrom dance.
@dadeg Something like that, yes! Keep in mind that you can send the value in the call data and you also can instantiate a ERC20 token using the msg.sender address.
token.approve(receiver, value, keccak256("canRecieve(sender, value)");
Contract Receiver {
function canReceive(address sender, uint256 value) public {
ERC20 tokenContract = ERC20(msg.sender);
uint256 allowedValue = tokenContract.allowance(sender, value);
tokenContract.transferFrom(sender, value, allowedValue);
doBusinessLogic();
}
function doBusinessLogic() { ... }
}
Although I would recommend to have some way to specify the token address in the contract itself, if not it can be called from any ERC827 token.
Thanks! I am unsure if sending value is a wise idea, considering there is no guarantee that the value mentioned in the _data
argument matches the value
argument in the original approve
call: token.approve(receiver, 10, keccak256("canReceive(sender, 1000)"))
As long as the receiving contract knows the address of the token contract, this approve-allowance-transferFrom
pattern seems like a strong guarantee that nothing bad will happen. Even if the user initially screws up the approve() call, like typo-ing the _data
string, token.approve(receiver, 10, keccak256("someTypoFunction(sender, 1000)"))
, they can recover by simply calling canReceive
directly themselves, or calling approve
again with the correct data signature. And the rest of the pattern is hardcoded in a single transaction to limit the UI/UX complications.
Edit: To add on to this, There is no risk of a malicious actor calling receiver.canReceive
because the method is hardcoded to just check the token for the allowance from any arbitrary user. The assumption being that a user wouldn't call approve previously unless they actually intended on having the receiver take ownership of all the tokens immediately.
Please correct me if I'm wrong, but doesn't this standard mean that the resulting 827 tokens will not be compatible with most (all?) ERC223 #223 receiver contracts (to the extent they exist)? ERC223 requires that the receiving contract accept tokenFallback from pre-approved sending contract addresses to validate that the second argument 'value' represents a guaranteed transfer to the receiver. In this case, however, the invocation of tokenFallback will be composed by the caller of the transfer(with bytes) function. You can't know that they are not nefarious and crediting themselves with more tokens than they are indeed transferring.
This is essentially the same argument that @SergioDemianLerner and @abandeali1 made - and I concur. For the "transfer" function, this is at least useless and quite possibly dangerous. For the "approve", it works because the receiver then has to call transferFrom and can inspect the implementation of transferFrom to ensure they are satisfied with its behavior.
What happened to the old "approveAndCall" pattern? OpenZeppelin/zeppelin-solidity#346
@yarrumretep as far as I can tell, the current implementation of microraiden, which is a #223 receiver contract, is susceptible to this attack:
But I wonder if this an issue with ERC827, or with #223 receivers (and #777 receivers for that matter). They probably shouldn't trust that tokens have been transferred. Otherwise they need to implement a white/black list to be safe.
Since when is it a good idea to execute arbitrary calls to contracts? Let alone make that part of a token standard?
Just popping in since I heard "microraiden".
Basically re-iterating what @abandeali1, @SergioDemianLerner and @yarrumretep mentioned.
In the spec you have this function:
function transfer(address _to, uint256 _value, bytes _data) returns (bool success)
You describe it as:
Execute a function on _to with the _data parameter, if the function ends successfully execute the transfer of _value amount of tokens to address _to, and MUST fire the Transfer event.
The function SHOULD revert if the call to _to address fails or if _from account balance does not have enough tokens to spend. The ERC20 transfer method is called before the _to.call(_data).
So essentialy I can do an ERC20 transfer of 1 token to contract _to
but in the data I provide to it say I sent it 1 million tokens.
@LefterisJP i think your example highlights a weakness in the target contract, not in this token standard.
@dadeg As a 'receiving' contract under this standard as described above all I effectively get is a ping that my balance may change in the future by some unspecified amount. Because the standard does not mandate anything about the contents of the _data (nor could it really control that without a significant on-chain burden), the receiver isn't guaranteed anything in particular about the arguments passed therein and their relation to the 827 token contract's actual anticipated transfer. Receiver can't even keep his old balance and compare to verify because the actual transfer is not completed until after his call.
Any contract expecting to receive ERC223 tokens (with some kind of tokenFallback function) would not be able to receive tokens that implemented this function without specific precautions to prevent callers invoking tokenFallback in the _data parameter. tokenFallback value parameter can be trusted with sending contract source inspection. _data cannot be trusted to report the quantity of the transfer.
Also, couldn't this pattern of calling out prior to making data changes open this function to reentrancy issues?
The approveAndCall variant above, on the other hand, seems more reasonable and useful to me. There is no incentive to 'lie' about the amount being approved in the _data invocation - because the amount is either approved or not in the sending contract. Although it takes more gas than the 223 method, the only trust that must be verified before accepting calls from a sending token is that the tranferFrom function functions as advertised (and the rest of the token behaves in an ERC20 way). This is, in essence, a way to chain 2 calls together (approve and then doSomethingWithThisApproval). Existing ERC20 handlers expecting the approve/doSomething pattern might even be usable directly without modification (if they have a doSomething variant that doesn't depend on msg.sender).
@yarrumretep "Receiver can't even keep his old balance and compare to verify because the actual transfer is not completed until after his call."
This is interesting. If a user called approve, would the receiving contract be able to inspect the allowed amount (by calling allowance on the token) within the same transaction, or would it fail (or return 0) because the approve call does not finish until the receiving contract returns?
@dadeg not only inspect, but modify. Receiver can consume the allowance in the call directly.
@yarrumretep ok, that seems like a good thing. Would a receiving contract be able to do the same sort of inspection on a transfer call? Seems so. But there would be no way to guarantee the _data had the correct sender address. Transfer seems useless like you said.
Although a 223 tokenFallback receiver would have the same issue. There is no guarantee that tokenFallback is being called by a trustworthy 223 token contract. Even inspecting the source code of the caller would be problematic.
@yarrumretep @dadeg @LefterisJP @ptrwtts Thanks for the discussion and your feedback!
A quick summary of each function and their capabilities:
To work with this standard a developer must understand all capabilities of each function and the big problems of using a wrong function can bring.
I think it would be enough to add a section in the proposal with this information.
What do you think? am I missing something? you would add something else?
I also though in removing the functions, but there are use cases for them and if they are used correctly it wont bring any issues. It should also be a decision of the developer which functions of this standard he needs and if he wants to add only approve
he can do it.
@AugustoL what would be the difference between ERC677 ? it seems pretty much the same
@rstormsf The ERC677 adds only a call after transfer and the contract receiver need to have implemented the tokenFallback
function, ERC827 dont ask the receiver contractto have any fallback function implemented since it can use approveAndCall strategy to work with authenticated/allowed balance.
ok, I see...hmmm. so technically, I can send a transfer of 10 tokens and have some data that would send even more tokens like 1mm if the user accidentally got malicious data parameter. I think in that case, I'd prefer ERC777
Yes, thats why you should use the transfer
method with a fallback function to that will receive the value transfered as parameter. You should use approve
in that case.
I added some notes about howto/when use each function, if you do it that way you shouldnt have any issue.
I have found the transfer data very useful in discovering Proof of Valuation.. Thanks to the ERC827, I am able to attach a CCVO (Certificate of Valuation and Acknowldgment) to my ERC20 Token which provides critical reference to supporting documentation that verifies the VALUE assigned to the token.
Is there an example of ERC827 receiver?
@facundomedica for example over tx.origin
:
ERC827 public token;
function payForTask(uint256 taskId, uint256 tokenAmount) public {
token.transferFrom(tx.origin, this, tokenAmount);
tasks[taskId].reward += tokenAmount;
}
You should call:
const data = contract.methods.payForTask(taskId, amount).encodeABI();
token.methods.approve(contract.options.address, amount, data).send();
@k06a should I add an address check? So I know that the tokens being received are coming from my deployed token and not somebody else's token.
@facundomedica token
should be specified inside contract. May be a contact address.
@k06a Okay, I think I understood. That call will be called by the approve
function. So if that function is called from other contract that is not the "real one" the transferFrom
will just fail because of not having the required allowance. Thanks!
Hey, guys, if you wanna upgrade already launched token for example to support ERC827
, this can be made without holders transition if your token is at least Pausable
: https://medium.com/bitclave/the-easy-way-to-upgrade-smart-contracts-ba30ba012784
Let's make ERC827 approve method payable
?
function approve(address _spender, uint256 _value, bytes _data) public payable returns (bool) {
require(_spender != address(this));
super.approve(_spender, _value);
require(_spender.call.value(msg.value)(_data));
return true;
}
I wanna chain this operations into single tx:
PR: https://github.com/OpenZeppelin/zeppelin-solidity/pull/838
Hi Everyone,
I am a contributor on vyper (https://github.com/ethereum/vyper/issues/738) and was pointed here to the fact that function overloading gets used. Could we consider another alternative like renaming of the functions for clarity?
Vyper purposefully does not support function overloading, and probably never will. Consider that this is to be a standard, I don't think forcing all underlying contract languages to have function overloading seems reasonable like a reasonable standard. Function overloading is quite risky from an auditing perspective, specially in this case because of the additional call happening on each of the methods (function overloading introduces a lot of ambiguity).
@jacqueswww thx for bringing up that! If function overloading does not allow the implementation of this standard in vyper I think the function names should be changed. Im going to try to change it in the implementation of zeppelin-solidity first before change the standard doc here.
@AugustoL what about making ERC827 approve
method payable
?
Does the _data in ERC827 functions means that the for the *.call(_data) to work the invoker has to be a contract .
Thank you , it will be great if we add a line in the spec telling that , if the *.call(_data) , has to work , the programmer has to be sure that , the "whoever".call(_data) , the whoever parameter passed should be a contract and 827 is intended to enable contracts to work more efficiently .
Other wise a naive person like me will get confused .
@vu3mmg k06a it can be any address, account or contract. The only address that cant be use is the ERC827 token itself.
@AugustoL basic query , due to my ignorance I am asking this . if the address is that of an account , the invocation of whoever.call(_data ) will fail ? since the account address does not have the method /ABI encoded in _data ?
This standard is still a draft and is proven to be unsafe to be used
Simple Summary
A extension of the standard interface ERC20 for tokens with methods that allows the execution of calls inside transfer and approvals.
Abstract
This standard provides basic functionality to transfer tokens, as well as allow tokens to be approved so they can be spent by another on-chain third party. Also it allows to execute calls on transfers and approvals.
Motivation
This extension of the ERC20 interface allows the token to execute a function in the receiver contract contract after the approval or transfer happens. The function is executed by the token proxy, a simple proxy which goal is to mask the msg.sender to prevent the token contract to execute the function calls itself. The ERC20 token standard is widely accepted but it only allows the transfer of value, ethereum users are available to transfer value and data on transactions, with these extension of the ERC20 token standard they will be able to do the same with ERC20 tokens.
I saw a lot of new standards being proposed in the community and I think the way to improve the current ERC20 standard is with an extension that is fully compatible with the original standard and also add new methods, but keeping it simple at the same time, the code to be added to the ERC20 standard is near 150 lines of code.
When to use each function
approveAndCall: Probably the one that you will need, maybe the only one since it only allows the receiver contract to use approved balance. The best practice is to check the allowance of the sender and then do your stuff using the transferFromAndCall method.
transferAndCall: There is no way to check that the balance that will be transferred is the correct one, this function is useful when a function dont need to check any transfer of value.
transferFromAndCall: Same as transferAndCall, only useful when there is no need to check the transfered amount of tokens and want to spend approved balance.
Specification
Token
Methods
NOTE: Callers MUST handle
false
fromreturns (bool success)
. Callers MUST NOT assume thatfalse
is never returned!name - ERC20
Returns the name of the token - e.g.
"MyToken"
.OPTIONAL - This method can be used to improve usability, but interfaces and other contracts MUST NOT expect these values to be present.
symbol - ERC20
Returns the symbol of the token. E.g. "HIX".
OPTIONAL - This method can be used to improve usability, but interfaces and other contracts MUST NOT expect these values to be present.
decimals - ERC20
Returns the number of decimals the token uses - e.g.
8
, means to divide the token amount by100000000
to get its user representation.OPTIONAL - This method can be used to improve usability, but interfaces and other contracts MUST NOT expect these values to be present.
totalSupply - ERC20
Returns the total token supply.
balanceOf - ERC20
Returns the account balance of another account with address
_owner
.transfer - ERC20
Transfers
_value
amount of tokens to address_to
, and MUST fire theTransfer
event. The function SHOULDrevert
if the_from
account balance does not have enough tokens to spend.A token contract which creates new tokens SHOULD trigger a Transfer event with the
_from
address set to0x0
when tokens are created.Note Transfers of 0 values MUST be treated as normal transfers and fire the
Transfer
event.transferFrom - ERC20
Transfers
_value
amount of tokens from address_from
to address_to
, and MUST fire theTransfer
event.The
transferFrom
method is used for a withdraw workflow, allowing contracts to transfer tokens on your behalf. This can be used for example to allow a contract to transfer tokens on your behalf and/or to charge fees in sub-currencies. The function SHOULDrevert
unless the_from
account has deliberately authorized the sender of the message via some mechanism.Note Transfers of 0 values MUST be treated as normal transfers and fire the
Transfer
event.approve - ERC20
Allows
_spender
to withdraw from your account multiple times, up to the_value
amount. If this function is called again it overwrites the current allowance with_value
.Users SHOULD make sure to create user interfaces in such a way that they set the allowance first to
0
before setting it to another value for the same spender. THOUGH The contract itself shouldn't enforce it, to allow backwards compatibility with contracts deployed beforeallowance - ERC20
Returns the amount which
_spender
is still allowed to withdraw from_owner
.ERC827 Proxy
A very simple proxy contract used to forward the calls form the token contract.
There is a public variable called proxy in the ERC827 token, this can be used to check if the call is coming from the ERC827 token since the proxy can only forward calls from the token contract.
ERC827 methods
transferAndCall - ERC827
Execute a function on
_to
with the_data
parameter, if the function ends successfully execute the transfer of_value
amount of tokens to address_to
, and MUST fire theTransfer
event.This method is
payable
, which means that ethers can be sent when calling it, but the transfer of ether needs to be handled in the call is executed after transfer since the one who receives the ether is the token contract and not the token receiver.The function SHOULD
revert
if the call to_to
address fails or if_from
account balance does not have enough tokens to spend. The ERC20transfer
method is called before the_call(_to, _data)
.Note Transfers of 0 values MUST be treated as normal transfers and fire the
Transfer
event.Important Note Do not use this method with fallback functions that receive the value transferred as parameter, there is not way to verify how much value was transferred on the fallback function.
transferFromAndCall - ERC827
Execute a function on
_to
with the_data
parameter, if the function ends successfully execute the transfer of_value
amount of tokens from address_from
to address_to
, and MUST fire theTransfer
event.This method is
payable
, which means that ethers can be sent when calling it, but the transfer of ether needs to be handled in the call is executed after transfer since the one who receives the ether is the token contract and not the token receiver.The
transferFromAndCall
method is used for a withdraw workflow, allowing contracts to transfer tokens on your behalf before executing a function. The ERC20transferFrom
method is called before the_call(_to, _data)
. This can be used for example to allow a contract to transfer tokens on your behalf and/or to charge fees in sub-currencies. The function SHOULDrevert
if the call to_to
address fails or if the_from
approved balance by_from
tomsg.sender
is not enough to execute the transfer.Note Transfers of 0 values MUST be treated as normal transfers and fire the
Transfer
event.Important Note Do not use this method with fallback functions that receive the value transferred as parameter, there is not way to verify how much value was transferred on the fallback function.
approveAndCall - ERC827
Execute a function on
_spender
with the_data
parameter, if the function ends successfully allows_spender
to withdraw from your account multiple times, up to the_value
amount. If this function is called again it overwrites the current allowance with_value
.This method is
payable
, which means that ethers can be sent when calling it, but the transfer of ether needs to be handled in the call is executed after transfer since the one who receives the ether is the token contract and not the token receiver.Clients SHOULD make sure to create user interfaces in such a way that they set the allowance first to
0
before setting it to another value for the same spender. The ERC20approve
method is called before the_call(_spender, _data)
. The function SHOULDrevert
if the call to_spender
address fails. THOUGH The contract itself shouldn't enforce it, to allow backwards compatibility with contracts deployed beforeEvents
Transfer - ERC20
MUST trigger when tokens are transferred, including zero value transfers.
Approval - ERC20
MUST trigger on any successful call to
approve(address _spender, uint256 _value)
.Past Issues
The main issue that has been recognized by the community is that the standard does not follow the assumption about executing calls in behalf of a token contract, every smart contract that handle token balances assume the token contract will execute only the common methods and maybe a callback that is implemented by the token itself. This standard break that rule and allow the execution of arbitrary calls making it hard to integrate in current solutions. UPDATE This was solved by adding a simple proxy to the token and forwarding the calls coming from the token contract, the proxy ensure that the calls come only from the token contract and allows this to be verified on chain, this prevents the token address to be used as
msg.sender
allowing the integration with current solutions.Discussion channel
https://gitter.im/ERC827
Revisions
Implementation
ERC827 Interface in Winding Tree
[ERC827 Standard Token implementation in Winding Tree](https://github.com/windingtree/erc827/blob/master/contracts/ERC827/ERC827.sol
Copyright
Copyright and related rights waived via CC0