New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ERC827 Token Standard (ERC20 Extension) #827

Open
AugustoL opened this Issue Jan 11, 2018 · 118 comments

Comments

Projects
None yet
@AugustoL
Copy link

AugustoL commented Jan 11, 2018

EIP: 827
Title: ERC827 Token Standard (ERC20 Extension)
Author: Augusto Lemble <me@augustolemble.com>
Type: Token Standard
Status: Draft
Category: ERC
Created: 2018-01-11
Updated: 2018-06-28

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 any tokens on Ethereum to be re-used by other applications: from wallets to decentralized exchanges. 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 new functions that I propose are less than 100 lines of code taking in count the documentation.

When to use each function

  • approveAndCall: Probably the one that you will need, maybe the only one since it allows the receiver contract to use authenticated/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 transfered 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 from returns (bool success). Callers MUST NOT assume that false 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.

function name() constant returns (string name)

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.

function symbol() constant returns (string symbol)

decimals - ERC20

Returns the number of decimals the token uses - e.g. 8, means to divide the token amount by 100000000 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.

function decimals() constant returns (uint8 decimals)

totalSupply - ERC20

Returns the total token supply.

function totalSupply() constant returns (uint256 totalSupply)

balanceOf - ERC20

Returns the account balance of another account with address _owner.

function balanceOf(address _owner) constant returns (uint256 balance)

transfer - ERC20

Transfers _value amount of tokens to address _to, and MUST fire the Transfer event.
The function SHOULD revert 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 to 0x0 when tokens are created.

Note Transfers of 0 values MUST be treated as normal transfers and fire the Transfer event.

function transfer(address _to, uint256 _value) returns (bool success)

transferFrom - ERC20

Transfers _value amount of tokens from address _from to address _to, and MUST fire the Transfer 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 SHOULD revert 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.

function transferFrom(address _from, address _to, uint256 _value) returns (bool success)

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 before

function approve(address _spender, uint256 _value) returns (bool success)

allowance - ERC20

Returns the amount which _spender is still allowed to withdraw from _owner.

function allowance(address _owner, address _spender) constant returns (uint256 remaining)

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 the Transfer 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 ERC20 transfer method is called before the _to.call(_data).

Note The _to address cant be the token address itself.
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 transfered as parameter, there is not way to verify how much value was transfered on the fallback function.

  function transferAndCall(address _to, uint256 _value, bytes _data) public payable returns (bool) {
    require(_to != address(this));
    require(super.transfer(_to, _value));
    require(_to.call(_data));
    return true;
  }

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 the Transfer 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 ERC20 transferFrom method is called before the _to.call(_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 SHOULD revert if the call to _to address fails or if the _from approved balance by _from to msg.sender is not enough to execute the transfer.

Note The _to address cant be the token address itself.
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 transfered as parameter, there is not way to verify how much value was transfered on the fallback function.

  function transferFromAndCall(
    address _from, address _to, uint256 _value, bytes _data
  ) public payable returns (bool) {
    require(_to != address(this));
    require(super.transferFrom(_from, _to, _value));
    require(_to.call(_data));
    return true;
  }

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 ERC20 approve method is called before the _spender.call(_data).
The function SHOULD revert if the call to _spender address fails.
THOUGH The contract itself shouldn't enforce it, to allow backwards compatibility with contracts deployed before

Note The _spender address cant be the token address itself.

  function approveAndCall(address _spender, uint256 _value, bytes _data) public payable returns (bool) {
    require(_spender != address(this));
    require(super.approve(_spender, _value));
    require(_spender.call(_data));
    return true;
  }

Events

Transfer - ERC20

MUST trigger when tokens are transferred, including zero value transfers.

event Transfer(address indexed _from, address indexed _to, uint256 _value)

Approval - ERC20

MUST trigger on any successful call to approve(address _spender, uint256 _value).

event Approval(address indexed _owner, address indexed _spender, uint256 _value)

Current 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.

Discussion channel

https://gitter.im/ERC827

Revisions

  • 2018/06/28: Changed implementation link in zeppelin-solidity for windingtree/erc827 repository.
  • 2018/06/27: Added warning, current issued of the standard and public channel link.
  • 2018/04/17: Rename of functions to avoid function overloading and added payable modifier to ERC827 functions.
  • 2018/02/13: Added CC0 copyright
  • 2018/02/13: Added complete function code and notes abouts usage of each function
  • 2018/01/11: Initial Draft

Implementation

ERC827 Interface in Winding Tree

[ERC827 Standard Token implementation in Winding Tree](https://github.com/windingtree/erc827/blob/master/contracts/ERC827/ERC827Token.sol

Copyright

Copyright and related rights waived via CC0

@AugustoL AugustoL changed the title ERC826 token standard ERC827 token standard Jan 11, 2018

@AugustoL AugustoL changed the title ERC827 token standard ERC827 Token Standard (ERC20 Extension) Jan 11, 2018

@spalladino

This comment has been minimized.

Copy link

spalladino commented Jan 11, 2018

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 throwing if the call goes wrong, the transfer gets rolled back all the same.

@AugustoL

This comment has been minimized.

Copy link

AugustoL commented Jan 11, 2018

@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 :)

@spalladino

This comment has been minimized.

Copy link

spalladino commented Jan 11, 2018

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?

@AugustoL

This comment has been minimized.

Copy link

AugustoL commented Jan 11, 2018

I agree, im going to change the description in the issue.

@shrugs

This comment has been minimized.

Copy link

shrugs commented Jan 11, 2018

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?

@facuspagnuolo

This comment has been minimized.

Copy link

facuspagnuolo commented Jan 11, 2018

I really like this, thanks a lot! What do you think about overloading transfer, approve and transferFrom functions considering the new bytes _data argument?

@AugustoL

This comment has been minimized.

Copy link

AugustoL commented Jan 11, 2018

@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? 🤔

@facuspagnuolo

This comment has been minimized.

Copy link

facuspagnuolo commented Jan 11, 2018

@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:

  • ERC20 transfer function will keep being: keccak256("transfer(address,uint256)")
  • ERC827 transfer function will be: keccak256("transfer(address,uint256,bytes)")

am I right?

@AugustoL

This comment has been minimized.

Copy link

AugustoL commented Jan 11, 2018

@facuspagnuolo I think you are, let me do a quick test! 👀

@facuspagnuolo

This comment has been minimized.

Copy link

facuspagnuolo commented Jan 11, 2018

Another quick thing, we might want to distinguish between throw and revert in the spec

@AugustoL

This comment has been minimized.

Copy link

AugustoL commented Jan 11, 2018

@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.

@abandeali1

This comment has been minimized.

Copy link

abandeali1 commented Jan 19, 2018

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.

@AugustoL

This comment has been minimized.

Copy link

AugustoL commented Jan 20, 2018

@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.

@abandeali1

This comment has been minimized.

Copy link

abandeali1 commented Jan 20, 2018

@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.

@strotter

This comment has been minimized.

Copy link

strotter commented Jan 24, 2018

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

@AugustoL

This comment has been minimized.

Copy link

AugustoL commented Jan 24, 2018

Thanks @strotter ! fixed :)

@SergioDemianLerner

This comment has been minimized.

Copy link

SergioDemianLerner commented Jan 26, 2018

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

@AugustoL

This comment has been minimized.

Copy link

AugustoL commented Jan 26, 2018

@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.

@dadeg

This comment has been minimized.

Copy link

dadeg commented Jan 29, 2018

@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.

@AugustoL

This comment has been minimized.

Copy link

AugustoL commented Jan 29, 2018

@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.

@dadeg

This comment has been minimized.

Copy link

dadeg commented Jan 29, 2018

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.

@yarrumretep

This comment has been minimized.

Copy link
Contributor

yarrumretep commented Feb 2, 2018

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/openzeppelin-solidity#346

@ptrwtts

This comment has been minimized.

Copy link

ptrwtts commented Feb 10, 2018

@yarrumretep as far as I can tell, the current implementation of microraiden, which is a #223 receiver contract, is susceptible to this attack:

https://github.com/raiden-network/microraiden/blob/master/contracts/contracts/RaidenMicroTransferChannels.sol#L149

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.

@LefterisJP

This comment has been minimized.

Copy link
Contributor

LefterisJP commented Feb 10, 2018

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.

@dadeg

This comment has been minimized.

Copy link

dadeg commented Feb 11, 2018

@LefterisJP i think your example highlights a weakness in the target contract, not in this token standard.

@yarrumretep

This comment has been minimized.

Copy link
Contributor

yarrumretep commented Feb 11, 2018

@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).

@mg6maciej

This comment has been minimized.

Copy link
Contributor

mg6maciej commented Jun 25, 2018

@p0n1 I agree this is not too much of a problem with another tokens accidentally sent to ERC827 contract.
I'm concerned mostly with something like this: https://gist.github.com/mg6maciej/8af66a0cfb6ac318fa8cedeb4fd246d3
This code is safe for ERC20, ERC223, ERC667 and ERC777, but anyone putting ERC827 will lose them to an attacker.
If you look at this code as a base for DEX like EtherDelta, but with support for ERC677 and ERC777, you may immediately notice the problem. Attacker can grab all the ERC827 tokens, fill orders that want to buy these tokens for different tokens, do it multiple times if they want to fill more orders, after that withdraw all these ERC827 tokens, transfer to another exchange and sell them one more time.

@AugustoL

This comment has been minimized.

Copy link

AugustoL commented Jun 27, 2018

I created a gitter channel to discuss about ERC827 standard, implementations and governance of the standard.

https://gitter.im/ERC827/Lobby

I also created a public calendar on google calendar for ERC827 community calls, maybe to happen once every two weeks, where anyone in the community can join. The first call will be 5 of june at 5pm GMT +2.

Link to the call: https://calendar.google.com/event?action=TEMPLATE&tmeid=NjU5a2VnZmxsaTBycXVxdXQzYmJkcGpzZDkgbjg3bDdvcXVmMTQybmY4MGxlMGtoM3J2cThAZw&tmsrc=n87l7oquf142nf80le0kh3rvq8%40group.calendar.google.com

Link to the ERC827 calendar: https://calendar.google.com/calendar?cid=bjg3bDdvcXVmMTQybmY4MGxlMGtoM3J2cThAZ3JvdXAuY2FsZW5kYXIuZ29vZ2xlLmNvbQ

mg6maciej added a commit to mg6maciej/openzeppelin-solidity that referenced this issue Jun 28, 2018

Remove ERC827 token.
Implementing this token is unsafe as per comments here:
ethereum/EIPs#827 (comment)

frangio added a commit to OpenZeppelin/openzeppelin-solidity that referenced this issue Jun 28, 2018

Remove ERC827 token. (#1045)
Implementing this token is unsafe as per comments here:
ethereum/EIPs#827 (comment)
@pemulis

This comment has been minimized.

Copy link

pemulis commented Jun 28, 2018

Is this bug actually a feature if a token is only meant to be used within a particular ecosystem, targeting specific contracts that don't have a tokenFallback function, and not freely traded or sent to arbitrary contracts? This issue pretty much guarantees that ERC827 tokens couldn't be listed on decentralized exchanges, and would therefore be less vulnerable to securitization if the token is meant to be a marker within a limited system, and not a tradeable store of value subject to market forces and speculation.

@Dohko Dohko referenced this issue Jul 4, 2018

Closed

ERC827 token issue #27

@AugustoL

This comment has been minimized.

Copy link

AugustoL commented Jul 4, 2018

I just added a PR with a proposal with changes over current standard that adds more control to the receiver contract allowing it to specify which functions can be executed over it from the token contract.

windingtree/erc827#2

@k06a

This comment has been minimized.

Copy link

k06a commented Jul 4, 2018

Let's just add ERC827Caller contract:

contract ERC827Caller {
    function makeCall(address _target, bytes _data) external payable returns (bool) {
        // solium-disable-next-line security/no-call-value
        return _target.call.value(msg.value)(_data);
    }
}

And use it this way:

contract ERC827Token {
    ERC827Caller internal caller_;

    constructor() public {
        caller_ = new ERC827Caller();
    }

    function approveAndCall(
        address _spender,
        uint256 _value,
        bytes _data
    )
        public
        payable
        returns (bool)
    {
        require(_spender != address(this));

        super.approve(_spender, _value);
        require(caller_.makeCall.value(msg.value)(_spender, _data));

        return true;
    }
}
@AugustoL

This comment has been minimized.

Copy link

AugustoL commented Jul 5, 2018

@k06a I dont see how forwarding the calls trough another contract will fix the current issue of allowing arbitrary calls executed by default to any contract from the token contract.

I think the solution in the PR windingtree/erc827#2 might be better because it removes the allowance of arbitrary calls from the token contract by default, but any contract can allow the execution of specific functions on it.

Here is the summary of the PR:

The receiver contract where the callback function will be executed now needs to allow the execution of that callback itself, if the sender attempt to execute a callback that is not allowed on a receiver contract it will fail.
The callbacks can be allowed to certain address or they can be public, which means that anyone can execute it. There is also the option for a receiver contract to allow the execution of any function from any address.

I invite you to create a proposal with the changes you suggest on the windingtree/erc827 repository where we can test it against another proposals.

@k06a

This comment has been minimized.

Copy link

k06a commented Jul 5, 2018

@AugustoL the main issue of old ERC827 implementation was that the token may own some other tokens/resources and performing arbitary calls allows someone to steal this resources. I propose to forward calls with another contract, which should own nothing... and nothing could be stolen...

@AugustoL

This comment has been minimized.

Copy link

AugustoL commented Jul 5, 2018

@k06a yes thats one of the problems, but another one is the compatibility with another token standards that assume that no arbitrary calls can be executed from a token contract, like https://github.com/windingtree/erc827/blob/master/contracts/examples/VaultAttack.sol.

But reviewing it again I saw that it may also solve that issue too, which is great.
We should consider those changes and compare it to the existent proposals :)

@k06a

This comment has been minimized.

Copy link

k06a commented Jul 5, 2018

@AugustoL I personally think I could use the proposed version with my current project: MultiToken - ERC20 token abstraction, which contains some nested ERC20 tokens. It was impossible with old version because someone was able to steal upgraded or AirDropped tokens from MultiToken contract.

@AugustoL

This comment has been minimized.

Copy link

AugustoL commented Jul 5, 2018

@k06a why not collaborate here first?

@k06a

This comment has been minimized.

Copy link

k06a commented Jul 5, 2018

@AugustoL sure! Not going to use it until we found a best possible solution. Working on PR to this repo: https://github.com/windingtree/erc827

@k06a

This comment has been minimized.

Copy link

k06a commented Jul 5, 2018

@AugustoL

This comment has been minimized.

Copy link

AugustoL commented Jul 10, 2018

We have a proposal for changes to fix current issues in the token standard and also add more control over which functions can be executed from the token contract allowing the receiver contracts to have control ownership over it.

Proposal Summary

The receiver contract where the callback function will be executed now needs to allow the execution of that callback itself, if the sender attempt to execute a function that is not allowed on a receiver contract it will fail.
The callback can be allowed to certain address or they can be public, which means that anyone can execute it. There is also the option for a receiver contract to allow the execution of any function from any address.

The allowedCallbacks mapping works like this:

Receiver => Sender => Function Signature => FunctionType => Allowed

The receiver is the contract where the callbak function will be executed.

The sender can be an specific address or it can be 0x0, if it is 0x0 means that is a public callback.

The function signature can be an specific signature or it can be 0x0, if it is 0x0 means that any function can be executed.

The function type can be 1(approval), 2(transfer) or 3(transferFrom).

The allowed variable is a boolean that is true or false depending if the callback is allowed by the sender or not.

To manage the allowance of the callback the proposal adds two new functions:

function allowCallback(address from, bytes4 functionSignature, uint8 fType) public
function removeCallback(address from, bytes4 functionSignature, uint8 fType) public

Implementation: https://github.com/windingtree/erc827/blob/master/contracts/ERC827/proposals/ERC827TokenMockAllowedCallbacks.sol

Merged pull request with description and discussion: windingtree/erc827#2

Thanks @Perseverance for the help and review 👏

mtlacbaba added a commit to SoarEarth/skymap-token that referenced this issue Jul 11, 2018

@AugustoL

This comment has been minimized.

Copy link

AugustoL commented Jul 18, 2018

Second proposal submitted to the windingtree/erc827 repository: windingtree/erc827#4

Summary

The proposal adds an ERC827Proxy to the token to be used for every method that will execute arbitrary calls, doing this the msg.sender in the receiver contracts will be the ERC827Proxy address and not the ERC827 token,

Implementation

The implementation is done in the contracts/proposals folder on the ERC827TokenWithProxy.sol file.
The same tests that are being used for ERC827Token are used for this implementation with the same results.

My thoughts: I like a lot this one, it does not give as much control over the arbitrary calls as the AllowedCallbacks but it solves the main issue, making calls in behalf of the token contract.

Based on the proxy idea by @k06a

@AugustoL

This comment has been minimized.

Copy link

AugustoL commented Aug 3, 2018

Implementation of proxy proposal simplified and ready to be merged windingtree/erc827#4

@ptrwtts

This comment has been minimized.

Copy link

ptrwtts commented Oct 23, 2018

@AugustoL would you mind summarizing what the current state of this proposal. from what I can understand:

  • there are security issues with transferAndCall, and two solutions have been proposed
  • these vulnerabilities do not apply to approveAndCall

On a related note, do you know if the vulnerabilities also apply to #677, or does the fact that it defines a specific callback method (tokenFallback) mean it is not affected?

@k06a

This comment has been minimized.

Copy link

k06a commented Oct 23, 2018

@ptrwtts approveAndCall have issues related to arbitrary call attack from token smart contract. Attacker can spend anything this smart contract own. I personally don’t like idea of permissioning some method signatures, I prefer to extract arbitrary call to another smart contract called Caller, which owns nothing. But it seems we can’t reach the consensus in this question, I really wonder why :)

@ptrwtts

This comment has been minimized.

Copy link

ptrwtts commented Oct 23, 2018

@k06a thanks. why not just use the old school approveAndCall, as seen many past token implementations (e.g. MiniMe) , or the transferAndCall from #677? what are the main benefits of being able to call any arbitrary method?

@k06a

This comment has been minimized.

Copy link

k06a commented Oct 24, 2018

@ptrwtts calling arbitrary method allows you to send your token to any smart contract in a way it supports. There are some DEXes with different smart contract interfaces. No need for all contract to support special token protocol, just being compatible with ERC20. #677 has a problem with checks, anyone implementing receiveApproval should perform checks, that it was called from token and not from any hacker with random arguments. Please look at #1003, I am suggesting this implementation because it is fully compatible with ERC20, even transferFrom is called for msg.sender as in simple ERC20.

@RNHaoHao

This comment has been minimized.

Copy link

RNHaoHao commented Nov 5, 2018

TransferAndCall is this function ‘_to’ a contract address or a receiver address?

@AugustoL

This comment has been minimized.

Copy link

AugustoL commented Nov 5, 2018

@RNHaoHao to the contract address, then you should redistribute the approval from the contract.

@RNHaoHao

This comment has been minimized.

Copy link

RNHaoHao commented Nov 6, 2018

@AugustoL _to represents the address of other contracts. Functions that can be invoked by other contracts?

@RNHaoHao

This comment has been minimized.

Copy link

RNHaoHao commented Nov 6, 2018

"TransferAndCall" I always failed to call ("0xacb6208f4fbc6ac8fb60ad92f7096b4100538f45",100,"0xa9059cbb0000000000000000000000004ffe370c88549717a552f9ed7aac84b12e6bc1680000000000000000000000000000000000000000000000000de0b6b3a7640000")

@RNHaoHao

This comment has been minimized.

Copy link

RNHaoHao commented Nov 6, 2018

image

@jmank88 jmank88 referenced this issue Jan 21, 2019

Open

Support ERC-20 extensions, like ERC-223 #212

1 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment