Merge pull request from GHSA-7q6p-qcmj-q28v

Bez/advisory/patch unbonding fork

CHANGELOG.md

@@ -1,5 +1,14 @@
 # Changelog
 
+## 0.34.6
+
+### Bug Fixes
+
+#### SDK
+
+* Unbonding from a validator is now only considered "complete" after the full
+unbonding period has elapsed regardless of the validator's status.
+
 ## 0.34.5
 
 ### Bug Fixes

x/staking/handler_test.go

@@ -796,16 +796,73 @@ func TestUnbondingFromUnbondingValidator(t *testing.T) {
 	got = handleMsgUndelegate(ctx, msgUndelegateDelegator, keeper)
 	require.True(t, got.IsOK(), "expected no error")
 
-	// move the Block time forward by one second
-	ctx = ctx.WithBlockTime(ctx.BlockHeader().Time.Add(time.Second * 1))
+	ctx = ctx.WithBlockTime(ctx.BlockHeader().Time.Add(keeper.UnbondingTime(ctx)))
+	EndBlocker(ctx, keeper)
+
+	// validate the unbonding object does not exist as it was never created
+	_, found := keeper.GetUnbondingDelegation(ctx, delegatorAddr, validatorAddr)
+	require.False(t, found, "unbonding object should not exist")
+}
+
+func TestUnbondingFork(t *testing.T) {
+	ctx, _, keeper := keep.CreateTestInput(t, false, 1000)
+	validatorAddr, delegatorAddr := sdk.ValAddress(keep.Addrs[0]), keep.Addrs[1]
+
+	// create the validator
+	msgCreateValidator := NewTestMsgCreateValidator(validatorAddr, keep.PKs[0], sdk.NewInt(10))
+	got := handleMsgCreateValidator(ctx, msgCreateValidator, keeper)
+	require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator")
+
+	// bond a delegator
+	msgDelegate := NewTestMsgDelegate(delegatorAddr, validatorAddr, sdk.NewInt(10))
+	got = handleMsgDelegate(ctx, msgDelegate, keeper)
+	require.True(t, got.IsOK(), "expected ok, got %v", got)
+
+	// unbond the delegator from the validator
+	unbondAmt := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(10))
+	msgUndelegateDelegator := NewMsgUndelegate(delegatorAddr, validatorAddr, unbondAmt)
+	got = handleMsgUndelegate(ctx, msgUndelegateDelegator, keeper)
+	require.True(t, got.IsOK(), "expected no error")
 
-	// Run the EndBlocker
 	EndBlocker(ctx, keeper)
 
-	// Check to make sure that the unbonding delegation is no longer in state
-	// (meaning it was deleted in the above EndBlocker)
+	// validate the unbonding object does not exist as it was never created
 	_, found := keeper.GetUnbondingDelegation(ctx, delegatorAddr, validatorAddr)
-	require.False(t, found, "should be removed from state")
+	require.False(t, found, "unbonding object should not exist")
+
+	// reproduce same sequence except past fork height
+	ctx, _, keeper = keep.CreateTestInput(t, false, 1000)
+	validatorAddr, delegatorAddr = sdk.ValAddress(keep.Addrs[0]), keep.Addrs[1]
+	ctx = ctx.WithBlockHeight(keep.UndelegatePatchHeight)
+
+	// create the validator
+	msgCreateValidator = NewTestMsgCreateValidator(validatorAddr, keep.PKs[0], sdk.NewInt(10))
+	got = handleMsgCreateValidator(ctx, msgCreateValidator, keeper)
+	require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator")
+
+	// bond a delegator
+	msgDelegate = NewTestMsgDelegate(delegatorAddr, validatorAddr, sdk.NewInt(10))
+	got = handleMsgDelegate(ctx, msgDelegate, keeper)
+	require.True(t, got.IsOK(), "expected ok, got %v", got)
+
+	// unbond the delegator from the validator
+	msgUndelegateDelegator = NewMsgUndelegate(delegatorAddr, validatorAddr, unbondAmt)
+	got = handleMsgUndelegate(ctx, msgUndelegateDelegator, keeper)
+	require.True(t, got.IsOK(), "expected no error")
+
+	EndBlocker(ctx, keeper)
+
+	// validate the unbonding object exists prior to the unbonding period expiring
+	_, found = keeper.GetUnbondingDelegation(ctx, delegatorAddr, validatorAddr)
+	require.True(t, found, "unbonding object should exist")
+
+	// move to past unbonding period
+	ctx = ctx.WithBlockTime(ctx.BlockHeader().Time.Add(keeper.UnbondingTime(ctx)))
+	EndBlocker(ctx, keeper)
+
+	// validate the unbonding object no longer exists as the unbonding period has expired
+	_, found = keeper.GetUnbondingDelegation(ctx, delegatorAddr, validatorAddr)
+	require.False(t, found, "unbonding object should not exist")
 }
 
 func TestRedelegationPeriod(t *testing.T) {

x/staking/keeper/delegation.go

@@ -2,12 +2,17 @@ package keeper
 
 import (
 	"bytes"
+	"fmt"
 	"time"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/staking/types"
 )
 
+// UndelegatePatchHeight reflects the height at which to switch to the
+// undelegating patch.
+const UndelegatePatchHeight = 482100
+
 // return a specific delegation
 func (k Keeper) GetDelegation(ctx sdk.Context,
 	delAddr sdk.AccAddress, valAddr sdk.ValAddress) (
@@ -544,16 +549,18 @@ func (k Keeper) unbond(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValA
 	return amount, nil
 }
 
-// get info for begin functions: completionTime and CreationHeight
-func (k Keeper) getBeginInfo(ctx sdk.Context, valSrcAddr sdk.ValAddress) (
-	completionTime time.Time, height int64, completeNow bool) {
+// getBeginInfo returns the completion time and height of a redelegation, along
+// with a boolean signaling if the redelegation is complete based on the source
+// validator.
+func (k Keeper) getBeginInfo(
+	ctx sdk.Context, valSrcAddr sdk.ValAddress,
+) (completionTime time.Time, height int64, completeNow bool) {
 
 	validator, found := k.GetValidator(ctx, valSrcAddr)
 
+	// TODO: When would the validator not be found?
 	switch {
-	// TODO: when would the validator not be found?
 	case !found || validator.Status == sdk.Bonded:
-
 		// the longest wait - just unbonding period from now
 		completionTime = ctx.BlockHeader().Time.Add(k.UnbondingTime(ctx))
 		height = ctx.BlockHeight()
@@ -563,48 +570,70 @@ func (k Keeper) getBeginInfo(ctx sdk.Context, valSrcAddr sdk.ValAddress) (
 		return completionTime, height, true
 
 	case validator.Status == sdk.Unbonding:
-		completionTime = validator.UnbondingCompletionTime
-		height = validator.UnbondingHeight
-		return completionTime, height, false
+		return validator.UnbondingCompletionTime, validator.UnbondingHeight, false
 
 	default:
-		panic("unknown validator status")
+		panic(fmt.Sprintf("unknown validator status: %s", validator.Status))
 	}
 }
 
-// begin unbonding part or all of a delegation
-func (k Keeper) Undelegate(ctx sdk.Context, delAddr sdk.AccAddress,
-	valAddr sdk.ValAddress, sharesAmount sdk.Dec) (completionTime time.Time, sdkErr sdk.Error) {
+// Undelegate unbonds an amount of delegator shares from a given validator. It
+// will verify that the unbonding entries between the delegator and validator
+// are not exceeded and unbond the staked tokens (based on shares) by creating
+// an unbonding object and inserting it into the unbonding queue which will be
+// processed during the staking EndBlocker.
+func (k Keeper) Undelegate(
+	ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress, sharesAmount sdk.Dec,
+) (time.Time, sdk.Error) {
 
-	// create the unbonding delegation
-	completionTime, height, completeNow := k.getBeginInfo(ctx, valAddr)
+	// Undelegating must obey the unbonding period regardless of the validator's
+	// status for safety. Block heights prior to undelegatePatchHeight allowed
+	// delegates to unbond from an unbonded validator immediately.
+	if ctx.BlockHeight() < UndelegatePatchHeight {
+		completionTime, height, completeNow := k.getBeginInfo(ctx, valAddr)
 
-	returnAmount, err := k.unbond(ctx, delAddr, valAddr, sharesAmount)
-	if err != nil {
-		return completionTime, err
-	}
-	balance := sdk.NewCoin(k.BondDenom(ctx), returnAmount)
+		returnAmount, err := k.unbond(ctx, delAddr, valAddr, sharesAmount)
+		if err != nil {
+			return completionTime, err
+		}
 
-	// no need to create the ubd object just complete now
-	if completeNow {
-		// track undelegation only when remaining or truncated shares are non-zero
-		if !balance.IsZero() {
-			if _, err := k.bankKeeper.UndelegateCoins(ctx, delAddr, sdk.Coins{balance}); err != nil {
-				return completionTime, err
+		// no need to create the ubd object just complete now
+		if completeNow {
+			balance := sdk.NewCoin(k.BondDenom(ctx), returnAmount)
+
+			// track undelegation only when remaining or truncated shares are non-zero
+			if !balance.IsZero() {
+				if _, err := k.bankKeeper.UndelegateCoins(ctx, delAddr, sdk.Coins{balance}); err != nil {
+					return completionTime, err
+				}
 			}
+
+			return completionTime, nil
+		}
+
+		if k.HasMaxUnbondingDelegationEntries(ctx, delAddr, valAddr) {
+			return time.Time{}, types.ErrMaxUnbondingDelegationEntries(k.Codespace())
 		}
 
+		ubd := k.SetUnbondingDelegationEntry(ctx, delAddr, valAddr, height, completionTime, returnAmount)
+		k.InsertUBDQueue(ctx, ubd, completionTime)
+
 		return completionTime, nil
 	}
 
+	returnAmount, err := k.unbond(ctx, delAddr, valAddr, sharesAmount)
+	if err != nil {
+		return time.Time{}, err
+	}
+
 	if k.HasMaxUnbondingDelegationEntries(ctx, delAddr, valAddr) {
 		return time.Time{}, types.ErrMaxUnbondingDelegationEntries(k.Codespace())
 	}
 
-	ubd := k.SetUnbondingDelegationEntry(ctx, delAddr,
-		valAddr, height, completionTime, returnAmount)
-
+	completionTime := ctx.BlockHeader().Time.Add(k.UnbondingTime(ctx))
+	ubd := k.SetUnbondingDelegationEntry(ctx, delAddr, valAddr, ctx.BlockHeight(), completionTime, returnAmount)
 	k.InsertUBDQueue(ctx, ubd, completionTime)
+
 	return completionTime, nil
 }
 

x/staking/keeper/delegation_test.go

@@ -351,12 +351,10 @@ func TestUndelegateFromUnbondingValidator(t *testing.T) {
 	require.True(t, blockTime.Add(params.UnbondingTime).Equal(validator.UnbondingCompletionTime))
 
 	//change the context
-	header = ctx.BlockHeader()
-	blockHeight2 := int64(20)
-	header.Height = blockHeight2
-	blockTime2 := time.Unix(444, 0)
-	header.Time = blockTime2
-	ctx = ctx.WithBlockHeader(header)
+	blockHeight2 := int64(2000000)
+	blockTime2 := time.Unix(444, 0).UTC()
+	ctx = ctx.WithBlockHeight(blockHeight2)
+	ctx = ctx.WithBlockTime(blockTime2)
 
 	// unbond some of the other delegation's shares
 	_, err = keeper.Undelegate(ctx, addrDels[0], addrVals[0], sdk.NewDec(6))
@@ -367,8 +365,8 @@ func TestUndelegateFromUnbondingValidator(t *testing.T) {
 	require.True(t, found)
 	require.Len(t, ubd.Entries, 1)
 	require.True(t, ubd.Entries[0].Balance.Equal(sdk.NewInt(6)))
-	assert.Equal(t, blockHeight, ubd.Entries[0].CreationHeight)
-	assert.True(t, blockTime.Add(params.UnbondingTime).Equal(ubd.Entries[0].CompletionTime))
+	assert.Equal(t, blockHeight2, ubd.Entries[0].CreationHeight)
+	assert.True(t, blockTime2.Add(params.UnbondingTime).Equal(ubd.Entries[0].CompletionTime))
 }
 
 func TestUndelegateFromUnbondedValidator(t *testing.T) {
@@ -377,7 +375,7 @@ func TestUndelegateFromUnbondedValidator(t *testing.T) {
 	startTokens := sdk.TokensFromTendermintPower(20)
 	pool.NotBondedTokens = startTokens
 
-	//create a validator with a self-delegation
+	// create a validator with a self-delegation
 	validator := types.NewValidator(addrVals[0], PKs[0], types.Description{})
 
 	valTokens := sdk.TokensFromTendermintPower(10)
@@ -432,10 +430,6 @@ func TestUndelegateFromUnbondedValidator(t *testing.T) {
 	_, err = keeper.Undelegate(ctx, addrDels[0], addrVals[0], unbondTokens.ToDec())
 	require.NoError(t, err)
 
-	// no ubd should have been found, coins should have been returned direcly to account
-	ubd, found := keeper.GetUnbondingDelegation(ctx, addrDels[0], addrVals[0])
-	require.False(t, found, "%v", ubd)
-
 	// unbond rest of the other delegation's shares
 	remainingTokens := delTokens.Sub(unbondTokens)
 	_, err = keeper.Undelegate(ctx, addrDels[0], addrVals[0], remainingTokens.ToDec())