Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ERC 644: Token Standard for Modular and Upgradeable Tokens #644

Closed
chrisfranko opened this issue Jun 16, 2017 · 33 comments
Closed

ERC 644: Token Standard for Modular and Upgradeable Tokens #644

chrisfranko opened this issue Jun 16, 2017 · 33 comments
Labels
stale

Comments

@chrisfranko
Copy link

Preamble

EIP: <to be assigned>
Title: Token standard for upgradeable and modular tokens
Author: christopher@expanse.tech
Type: Standard Track
Category ERC
Status: Draft
Created: 2017/06/16

Simple Summary

The idea is to abstract user balances away from the token's business logic making the tokens upgradeable, and opens the door for modular functionality to be added over time.

Abstract

As it stands now, if any ERC20 tokens were to encounter any sort of fatal flaw, ever balance on that token would be at risk and recovering from the flaw would be cumbersome at best. This ERC proposes a way to accomplish the tasks below by abstracting the user balances away from the business logic.

  • Makes tokens upgradeable
  • Allows for modular functionality
  • Creates a way to insulate user balances from exploitable code

Motivation

Protecting consumers from potential exploits.

Specification

Balances.sol

Methods

getBalances

Returns an account's token balance.

function getBalance(address _acct) returns(uint balance)

incBalance

Increases an account's token balance.

function incBalance(address _acct, uint _val) onlyModule returns(bool success)

decBalance

Decreases an account's token balance.

function decBalance(address _acct, uint _val) onlyModule returns(bool success)

getAllowance

Returns an accounts allowed balance to be spent on behalf of owner.

function getAllowance(address _owner, address _spender) returns(uint remaining)

setApprove

Allows _spender to spend from _sender's account. When the function is called it over writes the current allowance with _value`.

function setApprove(address _sender, address _spender, uint256 _value) onlyModule returns(bool success)

decApprove

Decreases an accounts allowance amount by _value.

function decApprove(address _from, address _spender, uint _value) onlyModule returns(bool success)

getModule

Returns if module _acct is active or not.

function getModule(address _acct) returns (bool success)

setModule

Sets module _acct to true or false.

function setModule(address _acct, bool _set) onlyRoot returns(bool success)

getTotalSupply

Returns the total supply.

function getTotalSupply() returns(uint)

incTotalSupply

Increases the total supply.

function incTotalSupply(uint _val) onlyModule returns(bool success)

decTotalSupply

Decreases the total supply.

function decTotalSupply(uint _val) onlyModule returns(bool success)

transferRoot

Transfers to a new account.

function transferRoot(address _new) onlyRoot returns(bool success)

Events

BalanceAdj

Triggered when balances are adjusted.

event BalanceAdj(address indexed Module, address indexed Account, uint Amount, string Polarity);

ModuleSet

Triggered when modules are updated.

event ModuleSet(address indexed Module, bool indexed Set);

Implementation

https://github.com/expanse-org/Tokens/tree/master/Token_Contracts/contracts

Copyright

Copyright and related rights waived via CC0.
@jefffreey
Copy link

I came for a similar thing, which is upgradeable contracts. Do we have a standard Data/Interface contracts? Should I make an EIP for this?

@gislik
Copy link

gislik commented Jul 14, 2017

Having developed a token system I'm interested in this ERC. In addition I'm using Solidity libraries to reduce gas for deployment of the tokens and simplifying upgrades in case of bugs found or if the need for added functionality arises.

https://github.com/monerium/smart-contracts/tree/master/contracts

I'm guessing that your motivation for standardising the modules of the system is to mix-and-match different implementations of storage and business logic, am I right?

In my design I found it important to split the token not only into storage and business logic but:

  • Frontend
    This is a contract which implements the ERC20 standard by forwarding all method calls to the Controller. The main reason for this is to have a fixed address for users to interact with. This will become less important when ENS and Token registries become more widely used but for now most (if not all the wallets and other token software) uses hard coded address for token addresses.
  • Controller (ERC20Lib)
    Business logic implementation.
  • Storage (TokenStorageLib)
    This is what you call Balances. The main purpose is to keep the balances outside of the business logic and frontend so that it may be reused in case changes to the business logic is needed.

It seems that we've come up with a very comparable solution just using different terminology and I wanted to participate in defining a common set of interfaces. Unfortunately it may be too late for me to adapt my code to this standard since it has already been deployed to Rinkeby testnet and will later this year be deployed on the mainnet but I think I have something to contribute to the discussion :)

@chrisfranko
Copy link
Author

chrisfranko commented Jul 14, 2017 via email •
edited

@chrisfranko chrisfranko changed the title ERC: Token Standard for Modular and Upgradeable Tokens ERC 644: Token Standard for Modular and Upgradeable Tokens Jul 15, 2017
@davux
Copy link

davux commented Jul 16, 2017

The idea of separating balances from business logic seems not only fair, but necessary. However, why not keep ERC20 function naming like @gislik does in his approach?

If there is value in chaning the functions' logic such as e.g. incBalance() – and I would say there is –, then that should be discussed as a proper ERC20 evolution proposal. At any rate, name consistency between the core token interface (be it ERC20 or any evolution of it) and the storage contract should be considered important.

@Dexaran
Copy link
Contributor

Dexaran commented Jul 17, 2017 •
edited

You should keep in mind that ERC #20 token standard functions are leading to monetary losses. I wrote a number of articles about it (for example this one).

The main problem of ERC #20 token is that it has two ways of transferring funds:

  1. transfer
  2. approve + transferFrom

which assumes that first pattern is processed without handling by the receiver and the second pattern simulates transaction handling.

The purpose of approve + transferFrom was described in solidity documentation at page 39:

Warning: There are some dangers in using send: The transfer fails if the call stack depth is at 1024 (this can always be forced by the caller) and it also fails if the recipient runs out of gas. So in order to make safe Ether transfers, always check the return value of send, use transfer or even better: use a pattern where the recipient withdraws the money

This was relevant at the moment of creating ERC20 token standard, but the stack depth attack was fixed through EIP #150 . As the result of this approve + transferFrom must be considere deprecated now.

I think that there is no need to develop and implement the approve / allowance mechanism because:

  1. approve + transferFrom is leading to monetary losses since it assumes another way to transfer tokens (And an alternative way to execute a transaction often does not implement handling)
  2. vulnerability to re-approval attack is described here
  3. approve is superfluous since common pattern of transactions handling is fallback function execution. It is already described in ERC ERC223 token standard #223

I'd like to recommend to remove decApprove / setApprove / getAllowance from the token standard because of described problems of this approach.

@gislik
Copy link

gislik commented Jul 17, 2017 •
edited

@davux
I agree 💯 with your naming comments — coming up with good names is probably one of the hardest problems in software development :) I am however a bit confused since my implementation has an almost identical method calls to @chrisfranko version:

  • function addBalance(TokenStorage storage self, address to, uint amount)
  • function subBalance(TokenStorage storage self, address from, uint amount)

The purpose of those methods is to interact with the token storage (@chrisfranko calls this Balances.sol). As such these method names are actually pretty descriptive and while not a part of the ERC #20 they are used to implement the ERC20 methods, transfer and transferFrom. I'm willing to put my weight behind the names incBalance and decBalance.

Please let me know If I'm somehow misunderstanding your comment.

@Dexaran
When I was coming up with the the design for the token system I read the the ERC #223 thread and I was very impressed by your proposal. In fact I think your ideas solve many (if not most) of the problems discussed in the thread.

However it seems that ERC20 has already become the de facto standard even though it has not formally been accepted as an EIP. Both 0x and swap rely on the approve/transferFrom and in fact unless I'm misunderstanding something I think it's actually better to not send the 0x settlement contract my token with transfer (as suggested by ERC223) since I may want to cancel my trade and I don't understand how I would recover my token if I had already sent it away. How would you do that using ERC223 tokens?

I'm coming up with design ideas how to make my approve/transferFrom implementation safer but I didn't want to mix those ideas with the original presentation of the token system. However, one simple idea (inspired by ERC223) is to not allow transfering to contracts that don't implement the TokenRecipient. If the contract does indeed implement the interface, transfer would be equivalent of calling approveAndCall. Does this sound like a compromise that's worth exploring?

@chrisfranko
Copy link
Author

@Dexaran
RE approve: I'm completely A-OK with your suggestion of removing the approve and transferFrom methods from this ERC. Let me read ERC223 more and ill make an ERC223 branch.

@gislik
Thats right.

@chrisfranko
Copy link
Author

@davux It does. Anyone interfacing with the TokenStandard.sol contract will feel as if they are interfacing with regular ERC20 contracts.
incBalance and decBalance are methods in the Balances.sol contract.

@Dexaran
Copy link
Contributor

Dexaran commented Jul 18, 2017

Both 0x and swap rely on the approve/transferFrom

I think that relying on mechanism that is leading to monetary losses for your users is very bad idea.

it seems that ERC20 has already become the de facto standard

Due to the fact that many developers use this standard without any research or understanding of the consequences. They don't care about the fact than their users will lose money.

@Dexaran
Copy link
Contributor

Dexaran commented Jul 18, 2017 •
edited

I'm coming up with design ideas how to make my approve/transferFrom implementation safer

I think that it is possible to make approve / allowance mechanism safer but I think that it is not needed. I prefer to consider it deprecated because stack depth attack is already fixed (it is the only purpose of having approves. Approves are superfluous now).

Reasons:

  1. Irrational blockchain usage. Ethereum already has problems with network bandwidth. There is no need to introduce and use mechanisms which will cause extra blockchain bloating. Approve + transferFrom is a couple of two different transactions to perform a single action.

  2. Not user friendly. Can cause user mistakes.
    Why do we need two different functions (transfer and approveAndCall) to perform token transactions? Is here any advantages over having only transfer function to perform all transfers?

  3. More understandable for developers.
    Event handling and fallback executing is well-known method in programming. Approves are not.

@MicahZoltu
Copy link
Contributor

As mentioned in another EIP, this doesn't seem like something that should be an EIP. EIPs are not meant to be a source of best practices. They are meant to be a source of standardization of things that need or greatly benefit from standardization. This is not something that greatly benefits from standardization (even though it may be a best practice). I have other concerns with this EIP, but I'm going to leave them out of this discussion because this shouldn't be an EIP in the first place.

@chrisfranko
Copy link
Author

@MicahZoltu I'm sorry you feel that way. Maybe you should make an EIP to define EIPs better because you seem to be the only one who knows exactly what one is! That sounds like it would be a better use of your time.

@chrisfranko
Copy link
Author

#16

Reaches the conclusion that token standards do indeed belong as an EIP. -shrug-

@MicahZoltu
Copy link
Contributor

There is great value in a standard token interface. However, there is significantly less value in a standard mechanism for the mechanism by which token contracts are upgraded. The reason for this difference is because there are a lot of tokens and there are a number of people building user-facing tools/utilities that interface with tokens (block explorers, node UIs, signer UIs, wallets, etc.). However, for contract upgrade mechanisms there are very few people (no one?) writing generalized tools for token management and the users of these tools would not be end-users but instead far more tech savvy developers. On top of that, the token author can simply choose the token implementation that best aligns with his tool suite of choice, unlike the token user who doesn't get to choose the token contract they use but would still like to choose their tooling.

@chrisfranko
Copy link
Author

"However, for contract upgrade mechanisms there are very few people (no one?)"

That's the problem, there are very few people who actually give a damn about the future of the billion dollar assets they are launching on ethereum. Furthermore, I am working on tooling for it. Hence why I shared this ERC.

And that's quite the assumption that only "tech savvy people" would use token management tools. You are right the token author CAN simply choose the token implementation of his choice if everything is modularized that is.

Thank you for your comment on this request for comment.

@igordata
Copy link

I was wandering how one could upgrade the contract. I see some contracts have flaws in security.

I want to say, it would be great to have solution for upgrade contracts, and I'm interested in such solution.

I'm happy to see you Chris are putting an effort in developing upgradeable contracts!

@Saahiljeet-Singh
Copy link

I agree with @igordata .
Hey @chrisfranko : could you elaborate how could one upgrade a single module ( say a solidity file ) without impacting the others. Even if we change only the definitions keeping the interfaces intact, redeployment would still be an issue.

@Arachnid
Copy link
Contributor

Arachnid commented Sep 5, 2017

One thing to bear in mind: Implementing a generic upgrade mechanism like this opens users to the need to trust the token creator indefinitely (since they could upgrade to an 'unfair) token implementation.

@Dexaran I think that it is possible to make approve / allowance mechanism safer but I think that it is not needed. I prefer to consider it deprecated because stack depth attack is already fixed (it is the only purpose of having approves. Approves are superfluous now).

This is untrue. If you remove approve from ERC20, there is no way to transfer tokens to a contract such that the token knows who sent them the tokens.

@chrisfranko
Copy link
Author

chrisfranko commented Sep 5, 2017 via email •
edited

@Dexaran
Copy link
Contributor

Dexaran commented Sep 5, 2017

@Arachnid

This is untrue. If you remove approve from ERC20, there is no way to transfer tokens to a contract such that the token knows who sent them the tokens.

approve doesn't introduce any functionality that can't be implemented by token fallbacks execution without approves.
By the way, event handling function execution is a standard practice in programming (event handling) unlike approve + transferFrom mechanism that requires twice more transactions to make a token transaction and results in loss of money as well.

@Arachnid
Copy link
Contributor

Arachnid commented Sep 5, 2017

approve doesn't introduce any functionality that can't be implemented by token fallbacks execution without approves.

There are use-cases that are possible with approve but not fallbacks, though I don't find them particularly compelling. The issue I was raising is that you were suggesting that removing approve without any other changes would not eliminate any use-cases; it definitely would.

@Dexaran
Copy link
Contributor

Dexaran commented Sep 5, 2017

The issue I was raising is that you were suggesting that removing approve without any other changes would not eliminate any use-cases; it definitely would.

I agree, if you just exclude approves from the default ERC20 token then it will eliminate some use-cases.
I suggested an alternative standard with alternative ways of doing the same thing that approves do, but without loss of money.

@debkundu
Copy link

@Arachnid One thing to bear in mind: Implementing a generic upgrade mechanism like this opens users to the need to trust the token creator indefinitely (since they could upgrade to an 'unfair) token implementation.

@chrisfranko Yes you are correct Nick. A more ideal approach would be to have a
democratic DAO control the root address of Balances.sol

Agreed. Security, trust and transparency are of great importance in the business of token management in a democratic manner. Users cannot take the token creator indefinitely when it comes to activities like - incTotalSupply, decTotalSupply. So, so should we add another two items to the cart - (i) Token Governance body and (ii) Voting on token management issues? Something like an Association contract, that uses mintable token owned by a congress finally owned by a single account (https://www.ethereum.org/dao#the-shareholder-association). Thoughts welcome.

@MicahZoltu
Copy link
Contributor

Democratic mutability opens the system up to a Sybil attack against the governance system, which can be an even higher risk than centralized management. In general, I advise projects against upgradability outside of their beta. During beta I generally recommend a simple multi-sig upgrade mechanism as that introduces a clear target of trust and doesn't put the system at risk of a non-state attack from any party outside of the multisig.

@chrisfranko
Copy link
Author

chrisfranko commented Sep 13, 2017 via email

@micah
Copy link

micah commented Sep 13, 2017 via email

@kylerchin
Copy link

@Dexaran @chrisfranko I am looking for a token standard that allows for upgradability to the token code in the future and is able to be stored in a contract. How would I approach this problem? Do I just use the ERC223 recommended code and just slot in the Balances.sol code?

@facundomedica
Copy link

@chrisfranko Is this line ok? https://github.com/expanse-org/Tokens/blob/master/Token_Contracts/contracts/StandardToken.sol#L34 it should get the balance and decrease it for the _from no the message sender.

@chrisfranko
Copy link
Author

chrisfranko commented Mar 17, 2018 via email

@AnthonyAkentiev AnthonyAkentiev mentioned this issue Jun 15, 2018
Open
@guylando
Copy link

guylando commented May 20, 2019 •
edited

From investigating current solutions for token contracts bug mitigations and upgradability my conclusions are:

  1. Users SHOULD verify the token owner (that he is a legal entity which can be prosecuted in case of a foul-play) before buying the token + users SHOULD check that they trust the token smart contract before buying the token
  2. The contract SHOULD be pausable from security reasons and this is less risk than NOT making it pausable (under condition (1)), see: Token upgrade proposal OpenZeppelin/openzeppelin-contracts#795 (comment)
  3. Since upgradability through create2 and proxy removes the trust from the contract, probably better to plan a migration/pause possibly instead of redeployable/proxy upgradability. Not a DAO because the decentralized world is not so decentralized for now. Zeppelin os upgradability is based on proxy pattern where owner can modify the contract implementation at will which removes trust and is basically a backdoor for token investors who have no way to know what their token will turn to. https://blog.zeppelinos.org/getting-started-with-zeppelinos/ Upgradable ERC20 OpenZeppelin/openzeppelin-contracts#1499 (comment) https://github.com/zeppelinos/zos/blob/master/packages/lib/contracts/upgradeability/BaseAdminUpgradeabilityProxy.sol#L71
    What is important for (2) and (3) is that for a proxy based / redeployable contract the user can't really audit the whole functionality of the contract for the future because it can be changed, and on the other hand pausable token does not hide anything and the pausing ability as explained in Token upgrade proposal OpenZeppelin/openzeppelin-contracts#795 (comment) is even MORE important than redeploying ability for bugs/attacks/vulnerabilities protection.
  4. From (2)+(3) -> we are left with the implementation of migration mechanism
  5. UpgradeableToken used by several popular tokens allows the token the ability to declare a new contract and let users migrate their tokens to the new contract
  1. MKR migrated using a third "Redeemer" contract https://github.com/makerdao/redeem
  2. Here there is a recommendation to insert some upgrading ability to the contract: https://github.com/ethereum/wiki/wiki/Safety#upgrading-broken-contracts
  3. recommends to allow users to migrate their tokens like happens in UpgradeableToken https://medium.com/@k06a/the-safest-and-probably-the-best-way-to-upgrade-smart-contracts-ea6e619d5dfd
  4. compared minime cloning upgrade strategy and golem UpgradeableToken and seems UpgradeableToken migration strategy wins (in my opinion) https://medium.com/alice-si/upgradable-ethereum-contracts-b25f4c86c42b
  5. ERC20Migrator is openZeppelin's implementation of the migration which is totally external in third contract (unlike UpgradeableToken) and only requires user to give allowance to migrating contract for his balance in old token and then anybody can perform the migration https://docs.openzeppelin.org/docs/drafts_erc20migrator
  6. I believe UpgradeableToken is a good and audited approach however it requires some parts of the migration to be inside the original token which seems redundant. On the other hand ERC20Migrator does not allow to migrate paused token which was paused because of a bug because it calls transferFrom on the old contract and requires the users to call approve on the old contract. This CAN be solved by adding a function in the old contract which allows to set a "migrator" and allowing paused tokens to be approved to a "migrator" and allowing a "migrator" to call transferFrom for paused token. However this adds unnecessary complexity in my opinion to the original contract. I believe it is best to not insert any additional code to the original contract and instead in ERC20Migrator to just check the user balance in the old contract and use it to add the user tokens in the new contract. This is probably what EOS have done (https://etherscan.io/address/0x86fa049857e0209aa7d9e616f7eb3b3b78ecfdb0#code) and probably many other tokens have done this too. The "risk" here is if the owner will "unpause" the old contract after migration to the new contract however the migration will be performed by users approval for each user so the user can verify that new contract has the same symbol and name and then only one of those contracts will be listed on exchanges and the owner has no interest to ruin the token economy by creating a mass and exchanges will delist the token if there will be some foul-play so I believe that in the end only one of the two contracts will "survive" economically / in being used so this risk is "ok" (and like I said EOS and others did that so for example EOS could call start function and resume token trading today, so this risk was already taken before).
  7. Seems similar/identical idea and a possible implementation was presented here: https://github.com/bitclave/TokenWrapper https://medium.com/bitclave/the-easy-way-to-upgrade-smart-contracts-ba30ba012784 it was proposed to open zeppelin and declined here: Token upgrade proposal OpenZeppelin/openzeppelin-contracts#795

This was referenced May 20, 2019
Closed
Closed
@chrisfranko
Copy link
Author

chrisfranko commented May 23, 2019 via email

@yuriy77k yuriy77k mentioned this issue Jul 9, 2019
Closed
@github-actions
Copy link

github-actions bot commented Jan 2, 2022

There has been no activity on this issue for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the stale label Jan 2, 2022
@github-actions
Copy link

This issue was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
stale
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests