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

Implement migration path for stakvaults #127

Closed
0x-r4bbit opened this issue Feb 19, 2025 · 1 comment · Fixed by #128
Closed

Implement migration path for stakvaults #127

0x-r4bbit opened this issue Feb 19, 2025 · 1 comment · Fixed by #128

Comments

@0x-r4bbit
Copy link
Collaborator

When the stake manager is upgraded, there's a chance that new APIs are added to the stake managers interface.
Since the stake manager only allows for interaction with stake vaults, it's the stake vaults job to provide the necessary functions to interact with these new interface capabilities.

Currently StakeVaults are not upgradeable.
It's also not really possible for us to make them upgradeable in the traditional sense because that would mean the protocol owner would ultimately control the funds of all users.

So for cases like described above (in fact, we now have such a case), we need to offer users a way to migrate their stake to a newer vault.
We need to take into account that users might have vaults that are locked, so it's important all state is transferred accordingly.

Here's how this could work:

  1. Let's take the situation we have right now. The RewardsStreamerMP introduces a new function to claim "live" rewards, let's call it compound()
contract RewardsStreamerMP is
    Initializable,
    UUPSUpgradeable,
    IStakeManager,
    TrustedCodehashAccess,
    ReentrancyGuardUpgradeable,
    IRewardProvider,
    StakeMath
{
  ...

  function compound() external onlyTrustedCodehash onlyNotEmergencyMode onlyRegisteredVault {
    ...
  }
}

The function requires msg.sender to be a registered vault and a trusted vault implementation.
So this means, some StakeVault implementation codehash (**in reality, this is a proxy clone codehash, but since it has the implementation address baked into its source, it will inherently represent a stakevault) must have been whitelisted for this to work.

  1. Let's say the new StakeVault looks like this:
contract StakeVault is IStakeVault, Initializable, OwnableUpgradeable {
  ...

  function compound() external onlyOwner onlyTrustedStakeManager {
    ...
  }
}

This stakevault implementation comes with the necessary function, but existing stake vaults don't have this.
So users have to take action.

  1. At this point, they can either leave() the system (this is because the stake manager's implementation address has changed, so they are allowed to opt out), then create a new vault and stake() again. However, this means, the system forces them to step out and back in again, because they can't use the new function otherwise.

  2. Instead, they will first create a new proxy clone off the new StakeVault and register it with the stake manager using the VaultFactory. So they'll call VaultFactory.createVault().

  3. Now, with the new vault and hand, they can tell the system that they want to migrate their old vault to a new one. We could have a migrateToVault() function that looks something like this:

function migrateToVault(address migrateTo) onlyTrustedCodehash onlyNotEmergencyMode onlyRegisteredVault {

  // first ensure the vault to migrate to is actually owned by the same user
  if (IStakeVault(msg.sender).owner()!= IStakeVault(migrateTo).owner()) {
    // revert
  }

  // I think, generally it's fine to allow migration back and forth (if one wants to) but we
  // have to at least make sure that the vault we're migrating to is untouched
  if (vaultData[migrateTo].stakedBalance > 0) {
    // revert
  }

  _updateGlobalState();
  _updateVaultMP(msg.sender, true);

  VaultData oldVault = vaultData[msg.sender];
  VaultData newVault = vaultData[migrateTo];

  // migrate vault data to new vault
  newVault.stakedBalance = oldVault.stakedBalance;
  newVault.rewardIndex = oldVault.rewardIndex;
  newVault.mpAccrued = oldVault.mpAccrued;
  newVault.maxMP = oldVault.maxMP;
  newVault.lastUpdateTime = oldVault.lastUpdateTime;
  newVault.lockUntil = oldVault.lockUntil;

  delete vaultData[oldVault];

  emit VaultMigrated(msg.sender, migrateTo);
}
  1. Of course, the actual stake has to be transferred to the new vault as well, but since the funds live in the stake vault itself, it's also the StakeVault that has to perform this transfer:
contract StakeVault is IStakeVault, Initializable, OwnableUpgradeable {
  ...

  function migrateToVault(address migrateTo) external onlyOwner onlyTrustedStakeManager onlyValidDestination (migrateTo) {
    stakeManager.migrateToVault(migrateTo);
    bool success = STAKING_TOKEN.transfer(migrateTo, STAKING_TOKEN.balanceOf(address(this))));
    if (!success) {
      // revert
    }
  }
}

Now, this has introduced a new issue:

The whole point of this issue here is that we need a way to move to stake vaults that provide the functions we need.
The current StakeVaults do not have this migrateToVault function.

One way around this would be to allow the owner of a stakevault to perform the migration straight on the stake manager:


function migrationToVaultAsAccount(address from, address migrateTo) external onlyNotEmergencyMode {

  // first ensure the vaults exist and are actually owned by the account
  if (vaultOwners[from] != msg.sender && vaultOwners[migrateTo] != msg.sender) {
    // revert
  }

  if (vaultData[migrateTo].stakedBalance > 0) {
    // revert
  }

  _updateGlobalState();
  _updateVaultMP(from, true);

  VaultData oldVault = vaultData[from];
  VaultData newVault = vaultData[migrateTo];

  // migrate vault data to new vault
  newVault.stakedBalance = oldVault.stakedBalance;
  newVault.rewardIndex = oldVault.rewardIndex;
  newVault.mpAccrued = oldVault.mpAccrued;
  newVault.maxMP = oldVault.maxMP;
  newVault.lastUpdateTime = oldVault.lastUpdateTime;
  newVault.lockUntil = oldVault.lockUntil;

  delete vaultData[oldVault];

  // this however, requires approval
  bool success = STAKING_TOKEN.transferFrom(from, migrateTo, STAKING_TOKEN.balanceOf(from));
  if (!success) {
    // revert
  }
}

As I'm writing this, I realized that StakeVault now has to approve that stakemanager can move the funds on behalf of it..
Approval is another function that is not exposed by the StakeVault as of now...

@0x-r4bbit
Copy link
Collaborator Author

So yes, we can't have the appoval on the stakevault for the stakemanager because existing vaults don't have that functrion yet. Therefore, the second version of migrate to won't work.

It might not even be desired to have a possiblilty to approve in the future because that would essentially give the stakemanager ownership of vault funds, which is something we want to avoid.

Verdict:

Existing vaults on testnet, will have to unstake/leave and then create a new vault.

Otherwise the migrateToVault function can be implemented like this.

0x-r4bbit added a commit that referenced this issue Feb 19, 2025
This commit introduces a function `migrateToVault(address)` that allows
`StakeVault`s to migrate to other `StakeVault` instances.

The idea is that, when an upgrade was done on the stake manager, it
might introduces functions that can't be accessed through the stake
vaults that are out there.

Users will have to create new stake vault instances that provide the
necessary functionality.

`migrateToVault()` allows them to do so.

Closes #127
@3esmit 3esmit self-assigned this Feb 19, 2025
0x-r4bbit added a commit that referenced this issue Feb 19, 2025
This commit introduces a function `migrateToVault(address)` that allows
`StakeVault`s to migrate to other `StakeVault` instances.

The idea is that, when an upgrade was done on the stake manager, it
might introduces functions that can't be accessed through the stake
vaults that are out there.

Users will have to create new stake vault instances that provide the
necessary functionality.

`migrateToVault()` allows them to do so.

Closes #127
0x-r4bbit added a commit that referenced this issue Feb 19, 2025
This commit introduces a function `migrateToVault(address)` that allows
`StakeVault`s to migrate to other `StakeVault` instances.

The idea is that, when an upgrade was done on the stake manager, it
might introduces functions that can't be accessed through the stake
vaults that are out there.

Users will have to create new stake vault instances that provide the
necessary functionality.

`migrateToVault()` allows them to do so.

Closes #127
0x-r4bbit added a commit that referenced this issue Feb 19, 2025
This commit introduces a function `migrateToVault(address)` that allows
`StakeVault`s to migrate to other `StakeVault` instances.

The idea is that, when an upgrade was done on the stake manager, it
might introduces functions that can't be accessed through the stake
vaults that are out there.

Users will have to create new stake vault instances that provide the
necessary functionality.

`migrateToVault()` allows them to do so.

Closes #127
0x-r4bbit added a commit that referenced this issue Feb 19, 2025
This commit introduces a function `migrateToVault(address)` that allows
`StakeVault`s to migrate to other `StakeVault` instances.

The idea is that, when an upgrade was done on the stake manager, it
might introduces functions that can't be accessed through the stake
vaults that are out there.

Users will have to create new stake vault instances that provide the
necessary functionality.

`migrateToVault()` allows them to do so.

Closes #127
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment