Share checking of ValidateUnbondAmount has bug

[wangkui0508]

Summary of Bug

The checking logic in ValidateUnbondAmount is strange. It requires sharesTruncated <= delShares <= shares. At some corner cases, this requirement prevents me from unbonding all my shares.

Version

f1fdde5

Steps to Reproduce

Use the following Unit Test file:

package staking_test

import (
	"fmt"
	"testing"

	sdk "github.com/cosmos/cosmos-sdk/types"
	"github.com/cosmos/cosmos-sdk/x/staking/types"
)

func TestTokenAndShareCalc(t *testing.T) {
	validator := types.NewValidator(nil, PKs[0], types.Description{})
	// Due to slashing, this validator's DelegatorShares is 10000 but has only 5000 Tokens remained
	validator.DelegatorShares = sdk.NewDec(10000)
	validator.Tokens = sdk.NewInt(5000)

	//Suppose a delegator's share is 7 and she wants to unbond all of her share
	del := types.Delegation{Shares: sdk.NewDec(7)}

	//But no matter she sets the unbonding amount to 3 or 4, she has no way to successfully unbonds 7:
	shares, err := calcShares(validator, del, sdk.NewInt(3))
	fmt.Printf("%#v %#v\n", shares, err)
	//6.000000000000000000 <nil>
	shares, err = calcShares(validator, del, sdk.NewInt(4))
	fmt.Printf("%#v %#v\n", shares, err)
	//8.000000000000000000 &errors.Error{codespace:"staking", code:0x1e, desc:"invalid shares amount"}
}

// following lines are copied from ValidateUnbondAmount
func calcShares(validator types.Validator, del types.Delegation, amt sdk.Int) (shares sdk.Dec, err error) {
	shares, err = validator.SharesFromTokens(amt)
	if err != nil {
		return shares, err
	}

	sharesTruncated, err := validator.SharesFromTokensTruncated(amt)
	if err != nil {
		return shares, err
	}

	delShares := del.GetShares()
	if sharesTruncated.GT(delShares) {
		return shares, types.ErrBadSharesAmount
	}

	// Cap the shares at the delegation's shares. Shares being greater could occur
	// due to rounding, however we don't want to truncate the shares or take the
	// minimum because we want to allow for the full withdraw of shares from a
	// delegation.
	if shares.GT(delShares) {
		shares = delShares
	}

	return shares, nil
}

---

For Admin Use

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

[alexanderbez]

As always, we're thankful that you're looking into finding new bugs and improving the SDK @wangkui0508. I noticed that you're manually setting up the validator and setting the shares/delegation. Could you perhaps try to replicate this behavior using the actual execution flow/APIs instead of manually setting fields? Essentially, I'd like to see if this still happens under normal execution.

[wangkui0508]

Here you are

package staking_test

import (
	"fmt"
	"testing"

	"github.com/stretchr/testify/require"

	sdk "github.com/cosmos/cosmos-sdk/types"
	"github.com/cosmos/cosmos-sdk/x/staking"
	"github.com/cosmos/cosmos-sdk/x/staking/types"
)


func TestBug(t *testing.T) {
	app, ctx, delAddrs, valAddrs := bootstrapHandlerGenesisTest(t, 1000, 3, 1000000000)

	handler := staking.NewHandler(app.StakingKeeper)

	valA, valB, del := valAddrs[0], valAddrs[1], delAddrs[2]
	consAddr0 := sdk.ConsAddress(PKs[0].Address())

	valTokens := sdk.TokensFromConsensusPower(10)//.AddRaw(1)
	msgCreateValidator := NewTestMsgCreateValidator(valA, PKs[0], valTokens)
	res, err := handler(ctx, msgCreateValidator)
	require.NoError(t, err)
	require.NotNil(t, res)

	msgCreateValidator = NewTestMsgCreateValidator(valB, PKs[1], valTokens)
	res, err = handler(ctx, msgCreateValidator)
	require.NoError(t, err)
	require.NotNil(t, res)

	// delegate 10 stake
	msgDelegate := NewTestMsgDelegate(del, valA, valTokens.AddRaw(1))
	res, err = handler(ctx, msgDelegate)
	require.NoError(t, err)
	require.NotNil(t, res)

	// apply Tendermint updates
	updates := app.StakingKeeper.ApplyAndReturnValidatorSetUpdates(ctx)
	require.Equal(t, 2, len(updates))

	// a block passes
	ctx = ctx.WithBlockHeight(1)

	// must apply validator updates
	updates = app.StakingKeeper.ApplyAndReturnValidatorSetUpdates(ctx)
	require.Equal(t, 0, len(updates))

	// slash the validator by half
	app.StakingKeeper.Slash(ctx, consAddr0, 0, 20, sdk.NewDecWithPrec(5, 1))

	validator, found := app.StakingKeeper.GetValidator(ctx, valA)
	fmt.Printf("validator.Tokens: %s validator.DelegatorShares: %s %#v\n", validator.Tokens, validator.DelegatorShares, found)
	// validator.Tokens: 10000001 validator.DelegatorShares: 20000001.000000000000000000 true

	delegation, found := app.StakingKeeper.GetDelegation(ctx, del, valA)
	fmt.Printf("%#v %v\n", delegation, found)
	// types.Delegation{DelegatorAddress:A58856F0FD53BF058B4909A21AEC019107BA6102, ValidatorAddress:A58856F0FD53BF058B4909A21AEC019107BA6100, Shares:10000001.000000000000000000} true

	unbondAmt := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(5000001)) // will fail to Undelegate
	msgUndelegate := types.NewMsgUndelegate(del, valA, unbondAmt)
	res, err = handler(ctx, msgUndelegate)
	require.Equal(t, "invalid shares amount", err.Error())

	delegation, found = app.StakingKeeper.GetDelegation(ctx, del, valA)
	fmt.Printf("%#v %v\n", delegation, found)
	// types.Delegation{DelegatorAddress:A58856F0FD53BF058B4909A21AEC019107BA6102, ValidatorAddress:A58856F0FD53BF058B4909A21AEC019107BA6100, Shares:10000001.000000000000000000} true

	unbondAmt = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(5000000)) // can not Undelegate all the shares (1.499999950000005000 remained)
	msgUndelegate = types.NewMsgUndelegate(del, valA, unbondAmt)
	res, err = handler(ctx, msgUndelegate)
	require.NoError(t, err)
	require.NotNil(t, res)

	delegation, found = app.StakingKeeper.GetDelegation(ctx, del, valA)
	fmt.Printf("%#v %v\n", delegation, found)
	// types.Delegation{DelegatorAddress:A58856F0FD53BF058B4909A21AEC019107BA6102, ValidatorAddress:A58856F0FD53BF058B4909A21AEC019107BA6100, Shares:1.499999950000005000} true

}

[alexanderbez]

@cwgoes do you know if this is a bug or a corner-case that is OK under conditions of slashing?

[cwgoes]

Oh boy, I don't remember, there were a bunch of checks due to rounding errors which we encountered at various points, we should be careful changing this. cc @rigelrozanski

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[youngqqcn]

How are this issue going?

[robert-zaremba]

Did anyone confirm this bug?

[xloem]

I encountered two more bugs in the codepaths around this one that together resulted in booking errors of 1 uatom in tx BFB8C435EC6EB33D257F7FBA284E51A2A7D6ED5A15F42741D22D52F824E124A3. I posted briefly in discord about this.

It would be good to review the unbonding codepaths for logic errors. There seem to be a few of them.

With the addition of liquid staking it was worrisome to see booking off by 1 uatom; could a user game these small errors to do strange things, if they were to call the unbonding functions frequently?

[alexanderbez]

1uatom sounds like a rounding error, which can happen as noted above since we use fixed point decimal arithmetic.