Discussion about ERC20Migrator and related issues

Continuation from https://github.com/OpenZeppelin/openzeppelin-solidity/issues/795.

ERC20Migrator is one of our unstable contracts. It would be nice to stabilize soon but we need more people to review the API and the mechanism itself.

The contract implements a mechanism for migrating/upgrading an ERC20 token in an opt-in way. This allows upgrading even when an upgrade mechanism like ZeppelinOS isn't available. There are no requirements on the original token contract other than a working standard ERC20 interface. This means it can be used even when upgrading wasn't a consideration when deploying the first version of the token.

The way it works is: a new token contract is deployed, an ERC20Migrator instance is deployed, and the instance is given the ability to mint the new token. Once that is set up, each token holder can decide to migrate by using ERC20 approve to give the migrator instance the ability to transfer their tokens on their behalf. Then the migrator can be triggered to take those tokens and mint the equivalent amount of the new token. The last part can be done by anyone, not only the token owner.

The balance from the legacy token will be transferred to the migrator, as it is migrated, and remain there forever.

Although this contract can be used in many different scenarios, the main motivation was to provide a way to migrate ERC20 tokens into an upgradeable version of it using ZeppelinOS. To read more about how this can be done using this implementation, please follow the official documentation site of ZeppelinOS: https://docs.zeppelinos.org/docs/erc20_onboarding.html

Example of usage:

const migrator = await ERC20Migrator.new(legacyToken.address);
await newToken.addMinter(migrator.address);
await migrator.beginMigration(newToken.address);
3 Likes

I like the idea behind ERC20Migrator, it mostly conforms to my token upgradability vision: https://medium.com/@k06a/the-safest-and-probably-the-best-way-to-upgrade-smart-contracts-ea6e619d5dfd

2 Likes

Here is the summary of my conclusions on migrations including the problems in zeppelin os and ERCMigrator https://github.com/ethereum/EIPs/issues/644#issuecomment-494106553
The bottom line by the way is that I believe the idea proposed by k06a is the best one: https://medium.com/bitclave/the-easy-way-to-upgrade-smart-contracts-ba30ba012784
I don’t know if I came to this conclusion independently of k06a however I explained in https://github.com/ethereum/EIPs/issues/644#issuecomment-494106553 logical steps explaining why the idea is the best one right now in my opinion.

1 Like

I added some more detail in the original post to describe how ERC20Migrator works.

The following point is worthy of note:

This also implies, as @guylando has pointed out, that the original token cannot be paused. In some scenarios it can be useful to pause the contract when a vulnerability was found. I'm not sure if ERC20Migrator could be modified in some way to suppor that. Any ideas?

I wrote in (11),(12) here: https://github.com/ethereum/EIPs/issues/644#issuecomment-494106553 what I think is the better way which is to allow to migrate without modifying the old token. If you want to support both modes then maybe add a flag or separate functions in ERC20Migrator.

1 Like

I also believe the whole upgradability strategy in ZeppelinOS which is based on proxies, is not applicable to sensitive contracts which deal with funds such as tokens where the user shouldn't invest in such upgradable proxied contracts which he can't audit their changes.

@guylando also asked on GitHub:

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.

This is an interesting point in the API to consider. It's true that it can technically result in someone "front-running" the beginMigration call. Our reasoning was that adding permissions to the contract was too much overhead to protect against this. We didn't see it as a big problem because the window between deploying the migrator contract and beginning the migration can be very small, and it is easy to detect that someone else called beginMigration, because the next call to it would fail. If that is detected, one would have to deploy another migrator and set it up correctly, which is an annoyance but arguably a minor one.

It could be a problem if the person or script running the migration doesn't detect that they were frontrunned, and if they begin the migration and communicate the migrator address to their token holders, a bunch of tokens could be trapped.

The way we could fix it is by fixing the newToken address in the constructor. So the example code above would become:

const migrator = await ERC20Migrator.new(legacyToken.address, newToken.address);
await newToken.addMinter(migrator.address);
await migrator.beginMigration();

Is beginMigration even necessary? We added it to check that the migrator is a minter for the new token, but we could just remove it. If the migrator is not a minter the first attempt to migrate will fail. The person running the migration should definitely do a first test migration to check that everything works, anyway.

1 Like

Sounds like a good idea to move that logic into the constructor.
Now what if ERC20Migrator is deployed with CREATE2 and it has minter abilities and then it is redeployed with a different code which mints many new tokens by the malicious token creator? regarding CREATE2 you can see point (50) here: https://github.com/guylando/KnowledgeLists/blob/master/EthereumSmartContracts.md

In general I mean adding external minting to a token is problematic.

That’s a great point. For that to happen though, the migrator needs to selfdestruct. ERC20Migrator as it is in OpenZeppelin can never selfdestruct.

2 Likes

The reason why it is not passed there is that ERC20Migrator expects to have minting permissions over newToken. So newToken needs to know the migrator's address, but the migrator also needs to know the token's. The only way I can think of to solve this in a single transaction (i.e. remove beginMigration) is to have the token deploy the migration (or the other way around, but this seems more flexible).

1 Like

To add to the discussion about ERC20 token upgrading solutions, I implemented an up-to-date upgrading strategy here:


which does not make the upgrading ability lose trust for the token investors and does not open an external attack surface as far as I can tell (similar code appears in some popular tokens such as Storj however this one is compatible with latest solidity version and has some fixes).

With full coverage unit tests available here: https://github.com/guylando/EthereumSmartContracts/blob/master/SafeUpgradeableTokenERC20/test/SafeUpgradeableTokenERC20.test.js#L862

My point above was that we can remove the isMinter check entirely; it doesn't add a lot of value anyway. If so, it doesn't matter in what order things are deployed and minting permissions are granted. The natural order is to first deploy the token, then the migrator, and then give it minting permission. If the last step is omitted a call to migrate will not work, and this is reasonable I think.

1 Like

Ah, I see what you mean. Yes, I think that makes more sense than what we currently have.

1 Like

What was your main intent behind that solution? At a first glance, it is much more complex (even introducing concepts such as that of the UpgradeAgent): if we can make a simple one work, I'd rather keep it that way.

1 Like

The implementation of upgrading in the code I showed is better to be considered as an alternative to the proxy upgrade mechanism of zeppelin os instead of an alternative of ERC20Migrator. Regarding UpgradeAgent it can be considered as the new token (as happens in the implementation in the link) although it could also be an external contract with mint permissions in the new token.

The reasoning of why I decided to go with internal modifications to the token instead of taking all the upgrading outside is:

  1. First of all in case of a discovered vulnerability/bug the token will be paused and then it will not be possible to trigger transfer from the token.
  2. So there are two options: either the original token having additional functionality which is not paused and allows migration, or second option is for the external migrator to just look at the token balances and mint them in the new token without changing them in the original contract. I did not like the second option because it harms the investors trust because if suddenly the old token will get unpaused then token dellusion occurred because they were duplicated. Thus I prefer for the migrated tokens to get removed from original contract.
  3. In addition it will be good and transparent to have full tracking ability of the upgrade process of someone who looks at the token. The interaction with tokens is usually by searching the token symbol and if the migration is fully handled by an external contract it will make it harder for simple users to track it. If the original token has information on upgrade status and number of tokens upgraded then anybody can very easily and quickly understand the token status by searching the token on etherscan instead of searching for what is the migrator address.

(emphasis mine)

I'm copying below my original reply to @k06a about this very same topic:

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.

1 Like
  1. Providing the upgrade code at the token in place will allow to audit it so that token investors know more about what is the code they invest in including the upgrading part (although you can argue that they are welcome in the future to audit the upgrading external code however that code being available to them before investing would be preferable probably to them).
  2. If bugs will be found in the upgrading code, it is always possible to fallback to external upgrading process, so this is just an enhancement. You can argue the same about bugs being found in ERC20Migrator or other external migrator during migration.