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.

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

Upgrade sucks. Wallet address should remain unchanged after an upgrade. #189

Open
polymorpher opened this issue Nov 10, 2021 · 8 comments
Open
Labels
big project Projects that will takes weeks to complete enhancement New feature or request

Comments

@polymorpher
Copy link
Owner

polymorpher commented Nov 10, 2021

It is indisputable that the current upgrade experience sucks (see issues). This proposal intends to fix that.

  • 1wallet upgrade will no longer produces a new address and migrate assets.
  • 1wallets without user-set recovery address will require 6-consecutive authenticator codes to initiate and approve the upgrade (to be implemented in Restore wallet without scanning exported QR code from Google Authenticator #191)
  • 1wallets with user-set recovery address can initiate the upgrade with only a single authenticator code, but they must send 0.1 ONE from their recovery address to approve the upgrade.
  • The wallet implementation will employ a proxy-based pattern, such that the logical code is pointing to another contract its address may change for each upgrade. However, until an upgrade is approved, no change would take effect.

To do that, we will combine the best parts of both the existing upgrade implementation and the standard proxy-based pattern used in other wallets.

We initially designed this upgrade mechanism to strengthen wallet security, as we noted that the common proxy-based upgrade implementations used by other wallet may not be secure enough: attackers who temporarily gain control on the client side may "upgrade" the wallet to an implementation under their control, thereby circumventing all security measures in the wallet's smart contract. The core weakness of the proxy-based implementation is that wallet's code is no longer immutable, and it poses too much risk if the user is allowed to instantly change the code (by pointing it to another contract address, as per standard in proxy-based implementations).

Under the principle that the user should be given the minimal permission possible over the contract to complete the intended operation, our initial upgrade mechanism disallows any mutation to the code. Instead, upgrades are achieved by securely deploying a new contract, securely migrating all assets over, and make the old address forward assets to the new address automatically. Inevitably, this process creates a new address (for the new contract, which hosts new version of the code). The user's old wallet address remains usable for all purposes as it shares requires exactly the same authentication used by the new wallet address, and assets are automatically transferred to new address whenever the old wallet address receives something.

Although the above mechanism is sound in theory, in practice the migration process is imperfect. Many users complained about "loss of funds", where in fact the assets are left over in the old wallet instead of being migrated. Mechanisms are implemented to manually reclaim these assets, but the user experience is apparently suboptimal.

Moreover, the old wallet cannot automatically transfer ERC-20 tokens if it receives some of those after the upgrade. Moreover, some non-standard NFTs fail to implement functions that notify the receiver, causing the NFT to be stuck in the old wallet address instead of being forwarded to the new address.

Another issue is we designed this mechanism with an understanding that the use of domain names should rapidly take over the use of standard addresses. The 20-byte long address is comparable to IP addresses in the traditional Internet. IP addresses are dynamically and infrequently reassigned, but domain names and personal identities (e.g. Google Login) are not. However, the reality is that the crypto infrastructure is far from ready to do away with the concept of address. Users are still expected to provide their hexadecimal or one1... addresses in almost every app, and it is often impossible to change that address in those apps, once the address is linked and established with the app.

Accordingly, it is apparent that the change-of-address issues substantially affect the users' trust and experience. To make 1wallet a wallet for everyone, we must fix this.

@polymorpher
Copy link
Owner Author

I have been looking into more into the possibility of retaining address while upgrading the wallet. On a second thought, I still feel very uncomfortable with the underlying mechanisms to make that happen, namely making the wallet address a “proxy” which has a mutable variable that points to the address of the actual implementation contract code. The “proxy” stores the state of the wallet, which includes a dozen of variables in a few complex structures. The issue is, it is very easy to mess up between upgrades and different compiler versions, causing catastrophic data loss or theft. The implementation code assumes the storage in the proxy conform with a particular layout. The layout is very delicate and is not checked, under the proxy mechanisms. The layout may change when we change to a different compiler version. Even for the same compiler version, the layout may change when we add some new variables in the implementation contract

I don’t think it is a responsible thing to do to use this pattern, until Ethereum VM and Solidity guys fully codify this pattern in reference manual, and fully specify the behavior of storage layout under these circumstances. They should add special function in Solidity and operation code in Yul (the assembly language) to allow developers to specify the desired storage layout, the proxy-fallback mechanisms, and so on. Right now this proxy pattern is more like a work-around, unintended by Ethereum VM developers and Solidity language developer.

So instead of doing what I wrote down above, I would choose another path to make the upgrade experience good:

  1. Double down on domain services - I may need to deploy a custom ENS contracts (instead of using crazy.one, which requires payment) and provide free domain to users, and auto-generate a domain upfront when they create the wallet
  2. In the wallet, display aggregated assets from all former addresses (pre-upgrade), so it won’t appear to the user that their assets are gone missing.
  3. Have a button that transfer all assets to the latest address (post-upgrade), if anything is left-over

@polymorpher polymorpher added big project Projects that will takes weeks to complete enhancement New feature or request labels Dec 9, 2021
@jump912
Copy link

jump912 commented Mar 7, 2022

How do we manually reclaim assets being left behind in the old address?

@polymorpher
Copy link
Owner Author

polymorpher commented Mar 8, 2022

Go to "About", find the address (move mouse over), and click Inspect. There shouldn't be left over in next major version upgrade (later this month)

@polymorpher
Copy link
Owner Author

I am considering to implement same-address upgradability in v16 such that most if not all upgrades will not require a change of address and migration of assets.

Some related EIPs:

https://eips.ethereum.org/EIPS/eip-1822
https://eips.ethereum.org/EIPS/eip-1967
https://eips.ethereum.org/EIPS/eip-2535

The implementation will incorporate some ideas in these EIPs but will be fairly custom

@dessertsparrow
Copy link

Go to "About", find the address (move mouse over), and click Inspect. There shouldn't be left over in next major version upgrade (later this month)

After inspecting the old wallet, how to send the remaining funds that weren't sent with the update? Inspecting will open the wallet but sending to the new wallet address doesn't seem to work.

@polymorpher
Copy link
Owner Author

Do you have something specific I can look at? Not sure what doesn't seem to work

@polymorpher
Copy link
Owner Author

polymorpher commented Apr 25, 2022

I checked. I think it is waiting for you to confirm the upgrade from your recovery wallet (by sending 0.1 ONE to old address, like what the page says when you upgrade), so it can move the remaining funds over to the upgraded wallet. This is to make sure the spending limit is always observed, unless you explicitly confirm, using 2+ factors.

Both of your wallets had a default spending limit of 1000 ONE. When you upgraded (within the last few hours), it used up all the limit.

@dessertsparrow
Copy link

I checked. I think it is waiting for you to confirm the upgrade from your recovery wallet (by sending 0.1 ONE to old address, like what the page says when you upgrade), so it can move the remaining funds over to the upgraded wallet. This is to make sure the spending limit is always observed, unless you explicitly confirm, using 2+ factors.

Both of your wallets had a default spending limit of 1000 ONE. When you upgraded (within the last few hours), it used up all the limit.

Thank you very much. I was making the error of sending the 0.1 ONE to the new wallet address. It worked almost immediately after sending to the old. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
big project Projects that will takes weeks to complete enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants