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

f1fdde5d1b18b995afb3b2802b43593ae6b8c2b3

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[bot]]

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.