auth: v0.43 migration breaks all slashed vesting accounts

[RiccardoM]

Summary of Bug

When a VestedAccount is slashed for a large amount of the delegated tokens, they won't be able to perform any operation later if they undergo the v0.43 x/auth migration

Version

v0.44.5

Steps to Reproduce

1. Create a VestedAccount with
   • Original vesting: 10000000000uatom (10.000 ATOM)
   • First vesting period with
     • length: 0
     • amount: 10000000uatom (10 ATOM)
       This will make sure that you have most of the tokens vesting and some of them free when the chain starts
2. Delegate from that account almost all the vesting tokens (eg. 9998 ATOM)
3. Slash that validator for whatever reason (double sign, downtime)
4. Run an on-chain upgrade that migrates the x/auth storage by using the MigrateAccount method
5. Try running any transaction with that account: you will end up with a panic caused by the Sub method of sdk.Coin

Explanation

• When a VestedAccount gets slashed, their OriginalVesting and VestingPeriod amounts are not slashed.
• While running the MigrateAccount method, the DelegatedVesting amounts are retrieved from the x/staking module which returns the slashed amounts

This causes a problem during the handling of every transaction that includes a fee because, while trying to pay for the fee, the following calls are made:

DeductFeeDecorator.AnteHandler -> DeductFees -> BaseKeeper.SendCoinsFromAccountToModule -> 
BaseSendKeeper.SendCoins -> BaseSendKeeper.subUnlockedCoins

Now, the subUnlockedCoins method is written as follows:

// subUnlockedCoins removes the unlocked amt coins of the given account. An error is
// returned if the resulting balance is negative or the initial amount is invalid.
// A coin_spent event is emitted after.
func (k BaseSendKeeper) subUnlockedCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) error {
  if !amt.IsValid() {
    return sdkerrors.Wrap(sdkerrors.ErrInvalidCoins, amt.String())
  }

  lockedCoins := k.LockedCoins(ctx, addr)

  for _, coin := range amt {
    balance := k.GetBalance(ctx, addr, coin.Denom)
    locked := sdk.NewCoin(coin.Denom, lockedCoins.AmountOf(coin.Denom))
    spendable := balance.Sub(locked)

    _, hasNeg := sdk.Coins{spendable}.SafeSub(sdk.Coins{coin})
    if hasNeg {
      return sdkerrors.Wrapf(sdkerrors.ErrInsufficientFunds, "%s is smaller than %s", spendable, coin)
    }

    newBalance := balance.Sub(coin)

    err := k.setBalance(ctx, addr, newBalance)
    if err != nil {
      return err
    }
  }

  // emit coin spent event
  ctx.EventManager().EmitEvent(
    types.NewCoinSpentEvent(addr, amt),
  )
  return nil
}

The main focus here should be the following:

1. The LockedCoins method is called, to get the amount of vesting coins that are not delegated
2. The amount of lockedCoins is subtracted from the current balance of the account

Particularly, the lockedCoins amount are computed as follows:

vestingCoins = account.OriginalVesting - account.Vested 
lockedCoins = vestingCoins - account.DelegatedVesting

The problem with all this flow is that, if the account was slashed, we will have the follow:

1. OriginalVesting is never changed, so
   1. VestingCoins will return the amount not considering the slash
      In our example 10.000 - 10 = 9.990 ATOM
   2. LockedCoins will return the amount considering the slashed DelegatedVesting
      In our example 9.990 - (9.998 - 5%) = 491,9 ATOM
2. Balance will continue to be equals to InitialBalance - Delegated
   In our example 10.000 - 9.998 = 2 ATOM

So, lockedCoins will be larger than the balance and thus the following line inside subUnlockedCoins will throw a panic with error "negative coin amount":

spendable := balance.Sub(locked)

Since this method is called every time a fee needs to be deducted from the account, it will always error making it impossible for the owner of such account to perform any operation whatsoever.

Side note

Due to the above computation between the balance and the lockedCoins amounts, I believe that this bug will only raise for those accounts who have delegated almost entirely their vesting coins. If the account leaves enough vesting coins free, the balance will result higher than the lockedCoins amount and thus the problem will not be raised initially.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[alexanderbez]

TBH I really don't know how to go about fixing this in either the migration logic or for existing chains that already have migrated. Will need to marinate on this for a bit...

[RiccardoM]

Are there any updates on this?

[aaronc]

What is the reason it breaks?

[RiccardoM]

What is the reason it breaks?

What do you mean? I've detailed out everything inside the issue description. Do you need something else?

[tac0turtle]

@RiccardoM is this issue still present?

[RiccardoM]

@RiccardoM is this issue still present?

Yes, it is.

[tac0turtle]

Thank you. @alexanderbez I see you assigned yourself, do you still want to lead this?

[alexanderbez]

Yeah I'm happy to take lead on this but I just can't think of a solution TBH, at least not without really complicating the migration logic.

[arhamchordia]

I am upgrading chain from v42 to v44 and came across this issue.
My tokens were slashed but was not reflecting in auth account.

Steps to replicate :

1. Staked tokens from vesting account to a validator.
2. Made validator miss blocks and got it jailed.
3. Upgrade the chain to v0.44.5
4. Unbonded tokens of the vesting account
5. Proper balance not reflected in auth.

All vesting account created for PersistenceCore-1 had v42 vesting account bug.
Any suggestions on migration? Doing this for PersistenceCore-1 upgrade.
@alexanderbez

[alexanderbez]

Hey @arhamchordia I already commented above ^ (#10712 (comment)).

There isn't a migration solution, at least not one that I think is practical.

Honestly, I think the best bet is for chains to identify all affected vesting account(s) and manually fix their balances in a chain upgrade.

We can probably write a guide on this or something...not sure, but we should consider closing this issue perhaps. Just my two cents.

[robert-zaremba]

Is anyone here willing to put a deep look if it's possible to fix the vesting account migration? Or can someone else also confirm that it's not possible?

[robert-zaremba]

Now discussing about it: it seams that the solution is to require that the x/auth migration is the last one.
For v0.44 we documented how to change the migration order: #10608

[RiccardoM]

Now discussing about it: it seams that the solution is to require that the x/auth migration is the last one. For v0.44 we documented how to change the migration order: #10608

No, it is not the solution. Please read again the issue description, I've included a detailed explanation of what is happening and why.

[alexanderbez]

I don't think it's that simple @robert-zaremba.

The crux of the problem is that the migration logic needs to know delegation amounts at specific heights/points in time, making the migration logic essentially require being executed on archive nodes making any sort of solution unrealistic. Correct me if I'm wrong @RiccardoM.

Hence, my proposal is that chains just deal with these accounts manually.

[RiccardoM]

The crux of the problem is that the migration logic needs to know delegation amounts at specific heights/points in time, making the migration logic essentially require being executed on archive nodes making any sort of solution unrealistic. Correct me if I'm wrong @RiccardoM.

You're not wrong. To fix this there's the need to know the entire history of delegations and unbonding delegations for all vesting accounts since when they were created, so that the tracking functions can be called and the on-chain state can relfect the truth.

[arhamchordia]

@alexanderbez how about migration logic, that directly writes delegated token amount to delegated vesting amount in auth vesting account?
Is it not possible to write in upgrade handler?
I am unsure if auth module will have access to delegations in general.
@puneet2019

[alexanderbez]

I think you need to first (1) identify all affected vesting accounts, and (2) iterate over those accounts manually setting the fields to the correct values. How to do this is probably not trivial.

@RiccardoM how did you end up solving this (assuming you didi)?

[puneet2019]

@alexanderbez , do you suggest a genesis.json manual migration?

We stop the chain at upgrade height, export genesis.json, change the vesting accounts delegated vesting/ delegated free entries. match them with the current delegation/ undelegation state?

Current delegations will show the -tokens delegated, shares(not required)
current unbondings will also show unbonding tokens.
redelegations- no clue, don't think should be a part of solution
can't we do simple arithmetic to derive vesting accounts - delegated free and delegated vesting data?

do this for all vesting accounts ( affected or not ), I think the state should come out the same?

I do not understand why we need an archival node for this? is there something I am missing out on?

Please correct me if I am wrong,
also I have never carried out migration in previous versions as well. might require some of your expertise.

[tac0turtle]

closing this for now, if you are running into this issue please reach out to the team. 0.43 is no longer supported