[Bug]: AfterUnbondingInitiated can be called with invalid unbondingID

[MSalopek]

Summary of Bug

A change to was introduced in cosmos-sdk when #12967 was merged, resolving this issue #12931.

The change introduced causes the AfterUnbondingInitiated staking hook to be called with an invalid unbondingId in SetUnbondingDelegationEntry. The issue affects unbonding operations, redelegation behaviour does not seem to be affected.

The behaviour could cause chains using interchain-security to panic if AfterUnbondingInitiated gets called with an invalid id.

Version

v0.47.x

Steps to reproduce

Attempt to add 2 undelegation entries for the same UnbondingDelegation at the same height.

Details

Previously, the AddEntry function would always add an unbonding entry. With the new behaviour, the function call can result in two unbonding entries referring to the same creationHeight being merged into a single entry. The main issue is that the AfterUnbondingInitiated hook is not informed about the merge and gets invoked with an id that does not have a corresponding UnbondingEntry.

Current implementation:

func (ubd *UnbondingDelegation) AddEntry(creationHeight int64, minTime time.Time, balance math.Int, unbondingID uint64) {
	entryIndex := -1
  // this bit of code will cause entries to be merged
	for index, ubdEntry := range ubd.Entries {
		if ubdEntry.CreationHeight == creationHeight && ubdEntry.CompletionTime.Equal(minTime) {
			entryIndex = index
			break
		}
	}
	// entryIndex exists
	if entryIndex != -1 {
		ubdEntry := ubd.Entries[entryIndex]
		ubdEntry.Balance = ubdEntry.Balance.Add(balance)
		ubdEntry.InitialBalance = ubdEntry.InitialBalance.Add(balance)

		// update the entry
		ubd.Entries[entryIndex] = ubdEntry
	} else {
		// append the new unbond delegation entry
		entry := NewUnbondingDelegationEntry(creationHeight, minTime, balance, unbondingID)
		ubd.Entries = append(ubd.Entries, entry)
	}
}

Proposed solution:

Inform the caller whether or not a new entry was created using a boolean flag and calling the hook only for new entries.

// x/staking/types/delegation.go
func (ubd *UnbondingDelegation) AddEntry(
	creationHeight int64, minTime time.Time, balance math.Int, unbondingID uint64,
) bool {
	// Check the entries exists with creation_height and complete_time
	entryIndex := -1
	for index, ubdEntry := range ubd.Entries {
		if ubdEntry.CreationHeight == creationHeight && ubdEntry.CompletionTime.Equal(minTime) {
			entryIndex = index
			break
		}
	}
	// entryIndex exists
	if entryIndex != -1 {
		ubdEntry := ubd.Entries[entryIndex]
		ubdEntry.Balance = ubdEntry.Balance.Add(balance)
		ubdEntry.InitialBalance = ubdEntry.InitialBalance.Add(balance)

		// update the entry
		ubd.Entries[entryIndex] = ubdEntry
		return false
	}
	// append the new unbond delegation entry
	entry := NewUnbondingDelegationEntry(creationHeight, minTime, balance, unbondingID)
	ubd.Entries = append(ubd.Entries, entry)
	return true
}


// SetUnbondingDelegationEntry adds an entry to the unbonding delegation at
// the given addresses. It creates the unbonding delegation if it does not exist.
func (k Keeper) SetUnbondingDelegationEntry(
	ctx sdk.Context, delegatorAddr sdk.AccAddress, validatorAddr sdk.ValAddress,
	creationHeight int64, minTime time.Time, balance math.Int,
) types.UnbondingDelegation {
	ubd, found := k.GetUnbondingDelegation(ctx, delegatorAddr, validatorAddr)
	id := k.IncrementUnbondingID(ctx)
	isNewUbdEntry := true
	if found {
		isNewUbdEntry = ubd.AddEntry(creationHeight, minTime, balance, id)
	} else {
		ubd = types.NewUnbondingDelegation(delegatorAddr, validatorAddr, creationHeight, minTime, balance, id)
	}

	k.SetUnbondingDelegation(ctx, ubd)

	// only call the hook for new entries since
	// calls to AfterUnbondingInitiated are not idempotent
	if isNewUbdEntry {
		// Add to the UBDByUnbondingOp index to look up the UBD by the UBDE ID
		k.SetUnbondingDelegationByUnbondingID(ctx, ubd, id)

		if err := k.Hooks().AfterUnbondingInitiated(ctx, id); err != nil {
			k.Logger(ctx).Error("failed to call after unbonding initiated hook", "error", err)
		}
	}
	return ubd
}