Incorrect code in staking module CompleteUnbonding can cause permanent loss or minting of funds.

[danwt]

This code is wrong

https://github.com/cosmos/cosmos-sdk/blob/c783aea68fbd856c2b188b2d467a7fa5cb4df1e6/x/staking/keeper/delegation.go#L788-L805

		entry := &ubd.Entries[i]
		if entry.IsMature(ctxTime) && !entry.UnbondingOnHold {
			// Proceed with unbonding
			ubd.RemoveEntry(int64(i))
			i--


			// track undelegation only when remaining or truncated shares are non-zero
			if !entry.Balance.IsZero() {
				amt := sdk.NewCoin(bondDenom, entry.Balance)
				if err := k.bankKeeper.UndelegateCoinsFromModuleToAccount(
					ctx, types.NotBondedPoolName, delegatorAddress, sdk.NewCoins(amt),
				); err != nil {
					return nil, err
				}


				balances = balances.Add(amt)
			}
		}

because the data the pointer (entry := &ubd.Entries[i]) points to is deleted (ubd.RemoveEntry(int64(i))) before it is referenced.

Note: this bug can lead to users not getting back all their funds.

Found with diff testing.

[mpoke]

That's the bug! 🎉 It actually fixes the test :) Great job @danwt

[andrey-kuprianov]

Congrats, Dan; great work! Really happy for you, as well for users who won't loose their funds because of this bug:)

[danwt]

I will add, this bug could also cause arbitrary minting of funds into a delegator address.

To see this, notice that the bug can cause the following:

Suppose ubd.Entries[i] of the form

[a,b]

with a.balance < b.balance. Then the incorrect code can cause b.balance to be refunded twice and a.balance to be refunded 0 times. Thus a user can receive funds 2 * b.balance while only investing a.balance + b.balance.

[jtremback]

Awesome! The code in our staking hooks is basically copied from the existing operations. Looking forward to figuring out how this crept in when I copied it when I get back

[danwt]

Will be fixed by

• fix: make ModuleAccountInvariants pass for IS SDK changes cosmos-sdk#12554

[mpoke]

@danwt Can we close this issue?