Openzeppelin-contracts: Token upgrade proposal

[k06a]

🎉 Description

Wanna introduce you the way of token smart contract upgrading without all holders migration by token owner and keeping all allowances.

• [ ] 🐛 This is a bug report.
• [x] 📈 This is a feature request.

📝 Details

This technique supports any Pausable tokens. It requires to pause() old contract and to create a new one with passing the address of old contract into the constructor. Holders will migrate their balances and allowances lazily in the first transaction.

🔢 Code To Reproduce Issue [ Good To Have ]

The source code is available here https://github.com/bitclave/TokenWrapper:

• BasicTokenWrapper
• StandardTokenWrapper
• BurnableTokenWrapper

Example of old token contract:

contract MyToken is StandardToken, PausableToken {

    string public constant symbol = "MYT";
    string public constant name = "MyToken";
    uint8 public constant decimals = 18;
    string public constant version = "1.0";

    function MyToken() public {
        totalSupply_ = 1000000 * 10**decimals;
        balances[msg.sender] = totalSupply_;
    }

}

Example of adding ERC827 approve(address,uint256,bytes) method (usually this will be done with the inheritance from ERC827Token):

contract MyToken2 is StandardTokenWrapper, PausableToken {

    string public constant symbol = "MYT";
    string public constant name = "MyToken";
    uint8 public constant decimals = 18;
    string public constant version = "2.0";

    function MyToken2(address _token) public StandardTokenWrapper(_token) {
    }

    // Modern approve method from ERC827
    function approve(address _spender, uint256 _value, bytes _data) public returns(bool) {
        require(_spender != address(this));
        super.approve(_spender, _value);
        require(_spender.call(_data));
        return true;
    }

}

[djdeniro]

Awesome!

[elopio]

Hey @k06a, thanks for the contribution.
Have you seen the upgradeable contracts from ZeppelinOS? https://docs.zeppelinos.org/docs/building.html

With that approach, there will be no need for pause nor lazy migration. Please let us know what do you think.

I think we can close this issue, but I'll wait for your comment.

[k06a]

@elopio, sure. This proposition is related to tokens which code currently do not support any kind of update.

[guylando]

was this merged to openzeppelin under a different name?

[frangio]

@guylando There is a similar contract for ERC20 tokens called ERC20Migrator.

[k06a]

@guylando @frangio please look at my latest article dedicated to upgradability, it also includes token upgrade example: https://medium.com/@k06a/the-safest-and-probably-the-best-way-to-upgrade-smart-contracts-ea6e619d5dfd

[frangio]

Interesting @k06a. If you're interested we would appreciate a review of ERC20Migrator. It would be nice to stabilize the API (i.e. remove it from drafts/) but we need some more comments before doing that.

[k06a]

@frangio sure. Will take a look.

[guylando]

@frangio thanks
@k06a yes already saw both of your articles on upgradability and so far I believe the correct way for trust+security is to add pausable ability to a token (many arguments that users will apreaciate this saving their tokens more than they are afraid of owner freezing them which is illegal and can be easily reverted) and then to provide a way to migrate to a new contract. meaning pause+migrate is better than upgradability (avoiding delegatecall)

[nventuro]

afraid of owner freezing them which is illegal and can be easily reverted

Could you clarify what you mean by this?

[guylando]

@frangio adding the ERC20Migrator ability to a token looses trust because owner can set any address for target token. better to migrate from new token to old instead of adding such dangerous power to the old token. I even think in current way the code is written, the owner can steale all tokens this way by temporary migrating to some contract which transfers him all tokens

[k06a]

@guylando pausing may break some smart contracts which own this tokens, including DEXes. I think allowing users to migrate when they want to is the most decentralized and safe strategy. Current ERC20Migrator implementation allows only migrate those accounts, who approved old tokens to migrator smart contract.

[guylando]

ah, this code is a separate "third" contract which is not part of the original contract?

@nventuro well several security audits consider "pausing" ability as too much power in the hands of the token owner but I argue against that

[guylando]

@k06a break how? the transfers will revert while paused and then get back to work as normal when pause is removed. that is the expected behavior and not a break

[frangio]

ah, this code is a separate "third" contract which is not part of the original contract?

Yes.

[k06a]

@guylando why do you need pausing if you is going to unpause em? You can create a token snapshot and airdrop new token easily without any pausing. I like the ERC20Migrator, will think about making it even more generalized.

[frangio]

Let's please continue the discussion about potential problems and other ideas in the forum.

https://forum.zeppelin.solutions/t/discussion-about-erc20migrator/661

[guylando]

@frangio I am sorry but I don't have a user there so I will just leave this last comment to stop discussion here.
@k06a Here are arguments why pause is ok security wise and even IMPROVES the security and why you need it:

1. BEC token where the pause ability saved a lot of money: https://medium.com/secbit-media/a-disastrous-vulnerability-found-in-smart-contracts-of-beautychain-bec-dbf24ddbc30e https://nvd.nist.gov/vuln/detail/CVE-2018-10299
2. MFT is another contract with vulnerability found which was saved by being pausable: https://blog.mainframe.com/important-mft-contract-redeployed-4f0b0bd8dc3b
3. Claiming its bad claims all the following tokens are bad: EOS, Tron, Icon, OmiseGo, Augur, Status, Aelf, Qash, and Maker tokens are pausable https://blog.cryptofin.io/what-we-learned-from-auditing-the-top-20-erc20-token-contracts-7526ef3b6fb1
4. although discussing security tokens, the arguments in the following link about why to make the token pausable are partially valid for utility tokens too: https://github.com/ethereum/EIPs/issues/1400#issuecomment-420490082
5. allows tokens to be freezed in order to move them in the future to a different blockchain if wanted/necessary which is an important ability
6. regarding trust, one can say that users should NOT trust tokens which are NOT pausable because they can lose their money with such tokens more easily where pausable tokens can pause in emergency and the users balances are publicly known and it would be a criminal act for the token issuer to just "walk away" with the money for no reason after pausing the token
7. the following reddit thread was opened at the time of the infamous DAO hack WHILE IT WAS HAPPENING and you can read the comments to see how the DAO users helplessly watched their money drained without any way to stop it. they would surely prefer a pause ability https://www.reddit.com/r/ethereum/comments/4oi2ta/i_think_thedao_is_getting_drained_right_now/
8. unlike redeployable tokens which break the trust more than pausable token (because pausable tokens are part of something the user agreed to while redeployable tokens can introduce a whole new undesired contract code. and also pausable tokens dont modify balances while upgradable tokens can cause modifications), the pausing ability is even more important than upgrading ability for security reasons. that is because the pause serves as a response to attack while the upgrade can be looked at as a second step of restoration and it can also be done manually by deploying a fixed contract and updating the balances in it.

[k06a]

@guylando ok, I was talking about pausing as part of upgradability process only. I agree pausing may help for centralized arbiter in many different situations.

[nventuro]

@k06a regarding your article

I believe there is a better approach to upgrade contracts gently: every user should migrate themselves with a mandatory transaction and smart contract could just provide functionality to perform this upgrade more slightly.
This approach allows community:
Audit new version of smart contract
Perform migration when everyone is ready (not to break DEXes on token upgrades)
Decide which new version to follow in case of project/team hard fork new version of smart contract

While nicer on paper, this ignores the possibility of a bug affecting the upgrade mechanism itself. As a real-life example, MakerDAO's bug could have been fixed by the community voting to migrate to a new contract, but _it was the voting itself that was broken_. Not only that, an exploit allowed for tokens to be forever locked inside the contract at no cost of the attacker, again preventing some users from migrating.

The way MakerDAO chose to tackle this was by using their own voting power in secret, before making the vulnerability public, so that malicious agents wouldn't get a chance to attack their users. I think this was the right call, and the migration plan was executed seamlessly, but it does mean that the underlying contract was swapped for a new one without anyone getting a say in it. And we're talking about an extremely high-profile project and technically-savvy team, and a contract that has been audited and public for almost two years.

I think that in the current state of the industry, (complex) smart contract systems can be used to _reduce_ trust, but they cannot remove the need for it completely.

[nventuro]

@guylando

I am sorry but I don't have a user there so I will just leave this last comment to stop discussion here.

This discussion is great! Please do keep commenting in our forum to add your voice, signing in is as easy as clicking on the 'sign in with GitHub' button.

[k06a]

@nventuro I agree, that's why token smart contracts should not have any other functionality.

[guylando]

@nventuro I believe if we take the assumption that there was a KYC on the token creator or in other words that he was verified in real life and that users made their checks before buying the token, then pausing ability is BETTER than the DAO voting ability you described. Pausing protects from attacks and does not perform any change at all to what the users agreed. On the other hand giving some people under some algorithm, even if decentralized the power to perform changes for EVERYONE is more problematic. You can see the ethereum all devs meetings consist of less than 30 active people so the decentralized is actually not so decentralized.
And since the creator was verified, it would be a criminal act for him not to unpause the tokens and he has no way to make it not be connected to him.

[guylando]

Why is beginMigration public without limiting the allowed callers? This can allow an attacker to "destroy" a deployed ERC20Migrator contract by setting newToken himself by calling beginMigration after the contract was deployed and before the legit owner calls beginMigration.
https://github.com/OpenZeppelin/openzeppelin-solidity/blob/v2.2.0/contracts/drafts/ERC20Migrator.sol#L70
+
ERC20Migrator requirement of transferFrom on old token prevents using it on a paused token which was paused because of a bug

[guylando]

I explained here: https://github.com/ethereum/EIPs/issues/644#issuecomment-494106553 why I think the upgrade strategy proposed by @k06a here (in the original issue message) is the "correct one"