From 83cd0ec3115cf1dc956bc6ce65d21e42f629dda2 Mon Sep 17 00:00:00 2001 From: Joe Bowman Date: Sat, 18 Nov 2023 02:59:12 +0000 Subject: [PATCH] fix: defensive checks in UpdateWithdrawalRecordsForSlash; add tests; fixes #607 --- x/interchainstaking/keeper/keeper.go | 23 -- .../keeper/withdrawal_record.go | 38 +++ .../keeper/withdrawal_record_test.go | 262 ++++++++++++++++++ 3 files changed, 300 insertions(+), 23 deletions(-) create mode 100644 x/interchainstaking/keeper/withdrawal_record_test.go diff --git a/x/interchainstaking/keeper/keeper.go b/x/interchainstaking/keeper/keeper.go index 10040a5cc..9781964f3 100644 --- a/x/interchainstaking/keeper/keeper.go +++ b/x/interchainstaking/keeper/keeper.go @@ -416,29 +416,6 @@ func (k *Keeper) SetValidatorForZone(ctx sdk.Context, zone *types.Zone, data []b return nil } -func (k *Keeper) UpdateWithdrawalRecordsForSlash(ctx sdk.Context, zone *types.Zone, valoper string, delta sdk.Dec) error { - var err error - k.IterateZoneStatusWithdrawalRecords(ctx, zone.ChainId, types.WithdrawStatusUnbond, func(_ int64, record types.WithdrawalRecord) bool { - recordSubAmount := sdkmath.ZeroInt() - distr := record.Distribution - for _, d := range distr { - if d.Valoper != valoper { - continue - } - newAmount := sdk.NewDec(int64(d.Amount)).Quo(delta).TruncateInt() - thisSubAmount := sdkmath.NewInt(int64(d.Amount)).Sub(newAmount) - recordSubAmount = recordSubAmount.Add(thisSubAmount) - d.Amount = newAmount.Uint64() - k.Logger(ctx).Info("Updated withdrawal record due to slashing", "valoper", valoper, "old_amount", d.Amount, "new_amount", newAmount.Int64(), "sub_amount", thisSubAmount.Int64()) - } - record.Distribution = distr - record.Amount = record.Amount.Sub(sdk.NewCoin(zone.BaseDenom, recordSubAmount)) - k.SetWithdrawalRecord(ctx, record) - return false - }) - return err -} - func (k *Keeper) depositInterval(ctx sdk.Context) zoneItrFn { return func(index int64, zone *types.Zone) (stop bool) { if zone.DepositAddress != nil { diff --git a/x/interchainstaking/keeper/withdrawal_record.go b/x/interchainstaking/keeper/withdrawal_record.go index 56e4b1378..3affe9ebb 100644 --- a/x/interchainstaking/keeper/withdrawal_record.go +++ b/x/interchainstaking/keeper/withdrawal_record.go @@ -3,8 +3,10 @@ package keeper import ( "encoding/binary" "encoding/hex" + "fmt" "time" + sdkmath "cosmossdk.io/math" "github.com/cosmos/cosmos-sdk/store/prefix" sdk "github.com/cosmos/cosmos-sdk/types" @@ -222,3 +224,39 @@ func (k *Keeper) AllZoneUnbondingRecords(ctx sdk.Context, chainID string) []type }) return records } + +func (k *Keeper) UpdateWithdrawalRecordsForSlash(ctx sdk.Context, zone *types.Zone, valoper string, delta sdk.Dec) error { + var err error + k.IterateZoneStatusWithdrawalRecords(ctx, zone.ChainId, types.WithdrawStatusUnbond, func(_ int64, record types.WithdrawalRecord) bool { + recordSubAmount := sdkmath.ZeroInt() + distr := record.Distribution + for _, d := range distr { + if d.Valoper != valoper { + continue + } + distAmount := sdk.NewDec(int64(d.Amount)) + if distAmount.IsNegative() { + err = fmt.Errorf("distAmount cannot be negative; suspected overflow") + return true + } + + newAmount := distAmount.Quo(delta).TruncateInt() + thisSubAmount := distAmount.TruncateInt().Sub(newAmount) + recordSubAmount = recordSubAmount.Add(thisSubAmount) + d.Amount = newAmount.Uint64() + k.Logger(ctx).Info("Updated withdrawal record due to slashing", "valoper", valoper, "old_amount", d.Amount, "new_amount", newAmount.Int64(), "sub_amount", thisSubAmount.Int64()) + } + record.Distribution = distr + subAmount := sdk.NewCoins(sdk.NewCoin(zone.BaseDenom, recordSubAmount)) + fmt.Println(subAmount) + fmt.Println(record.Amount) + if !record.Amount.IsAllGT(subAmount) { + err = fmt.Errorf("deductedTotal cannot contain negative coins; suspected overflow") + return true + } + record.Amount = record.Amount.Sub(subAmount...) + k.SetWithdrawalRecord(ctx, record) + return false + }) + return err +} diff --git a/x/interchainstaking/keeper/withdrawal_record_test.go b/x/interchainstaking/keeper/withdrawal_record_test.go new file mode 100644 index 000000000..7c896baf4 --- /dev/null +++ b/x/interchainstaking/keeper/withdrawal_record_test.go @@ -0,0 +1,262 @@ +package keeper_test + +import ( + "cosmossdk.io/math" + sdk "github.com/cosmos/cosmos-sdk/types" + icskeeper "github.com/quicksilver-zone/quicksilver/x/interchainstaking/keeper" + "github.com/quicksilver-zone/quicksilver/x/interchainstaking/types" +) + +func (suite *KeeperTestSuite) TestUpdateWithdrawalRecordsForSlash() { + tcs := []struct { + Name string + InitialRecords func(ctx sdk.Context, keeper *icskeeper.Keeper) + ExpectedRecords func(ctx sdk.Context, keeper *icskeeper.Keeper) (out []types.WithdrawalRecord) + Validator func(validators []string) string + Delta sdk.Dec + ExpectError bool + }{ + { + Name: "single 5% slashing", + InitialRecords: func(ctx sdk.Context, keeper *icskeeper.Keeper) { + zone, _ := keeper.GetZone(ctx, suite.chainB.ChainID) + validators := keeper.GetValidatorAddresses(ctx, zone.ChainId) + withdrawalRecord := types.WithdrawalRecord{ + ChainId: zone.ChainId, + Delegator: zone.DelegationAddress.Address, + Recipient: "cosmos1v4gek4mld0k5yhpe0fsln4takg558cdpyexv2rxr3dh45f2fqrgsw52m97", + Distribution: []*types.Distribution{ + {Amount: 10000, Valoper: validators[0]}, + {Amount: 10000, Valoper: validators[1]}, + {Amount: 10000, Valoper: validators[2]}, + {Amount: 10000, Valoper: validators[3]}, + }, + Amount: sdk.NewCoins(sdk.NewCoin(zone.BaseDenom, math.NewInt(40000))), + BurnAmount: sdk.NewCoin(zone.LocalDenom, math.NewInt(32356)), + Txhash: "3BE21C1057ABBFBC44BE8993D2A4701C751507FF9901AA110B5993BA070C176B", + Status: types.WithdrawStatusUnbond, + } + keeper.SetWithdrawalRecord(ctx, withdrawalRecord) + }, + ExpectedRecords: func(ctx sdk.Context, keeper *icskeeper.Keeper) (out []types.WithdrawalRecord) { + zone, _ := keeper.GetZone(ctx, suite.chainB.ChainID) + validators := keeper.GetValidatorAddresses(ctx, zone.ChainId) + out = []types.WithdrawalRecord{ + { + ChainId: zone.ChainId, + Delegator: zone.DelegationAddress.Address, + Recipient: "cosmos1v4gek4mld0k5yhpe0fsln4takg558cdpyexv2rxr3dh45f2fqrgsw52m97", + Distribution: []*types.Distribution{ + {Amount: 10000, Valoper: validators[0]}, + {Amount: 9500, Valoper: validators[1]}, + {Amount: 10000, Valoper: validators[2]}, + {Amount: 10000, Valoper: validators[3]}, + }, + Amount: sdk.NewCoins(sdk.NewCoin(zone.BaseDenom, math.NewInt(39500))), + BurnAmount: sdk.NewCoin(zone.LocalDenom, math.NewInt(32356)), + Txhash: "3BE21C1057ABBFBC44BE8993D2A4701C751507FF9901AA110B5993BA070C176B", + Status: types.WithdrawStatusUnbond, + }, + } + return out + }, + Delta: sdk.NewDecWithPrec(100, 2).Quo(sdk.NewDecWithPrec(95, 2)), + Validator: func(validators []string) string { return validators[1] }, + + ExpectError: false, + }, + { + Name: "multi record 5% slashing", + InitialRecords: func(ctx sdk.Context, keeper *icskeeper.Keeper) { + zone, _ := keeper.GetZone(ctx, suite.chainB.ChainID) + validators := keeper.GetValidatorAddresses(ctx, zone.ChainId) + withdrawalRecord := types.WithdrawalRecord{ + ChainId: zone.ChainId, + Delegator: zone.DelegationAddress.Address, + Recipient: "cosmos1v4gek4mld0k5yhpe0fsln4takg558cdpyexv2rxr3dh45f2fqrgsw52m97", + Distribution: []*types.Distribution{ + {Amount: 10000, Valoper: validators[0]}, + {Amount: 10000, Valoper: validators[1]}, + {Amount: 10000, Valoper: validators[2]}, + {Amount: 10000, Valoper: validators[3]}, + }, + Amount: sdk.NewCoins(sdk.NewCoin(zone.BaseDenom, math.NewInt(40000))), + BurnAmount: sdk.NewCoin(zone.LocalDenom, math.NewInt(32356)), + Txhash: "3BE21C1057ABBFBC44BE8993D2A4701C751507FF9901AA110B5993BA070C176B", + Status: types.WithdrawStatusUnbond, + } + keeper.SetWithdrawalRecord(ctx, withdrawalRecord) + withdrawalRecord = types.WithdrawalRecord{ + ChainId: zone.ChainId, + Delegator: zone.DelegationAddress.Address, + Recipient: "cosmos1nvkpj5n5mhy2ntvgn2cklntwx9ujvfvcacz5et", + Distribution: []*types.Distribution{ + {Amount: 13000, Valoper: validators[0]}, + {Amount: 14000, Valoper: validators[1]}, + {Amount: 10000, Valoper: validators[2]}, + {Amount: 11000, Valoper: validators[3]}, + }, + Amount: sdk.NewCoins(sdk.NewCoin(zone.BaseDenom, math.NewInt(48000))), + BurnAmount: sdk.NewCoin(zone.LocalDenom, math.NewInt(36503)), + Txhash: "FB087A50A4836CBDFACA70D393AF110C28935276267B7BA2838BE3CEEA08F762", + Status: types.WithdrawStatusUnbond, + } + keeper.SetWithdrawalRecord(ctx, withdrawalRecord) + }, + ExpectedRecords: func(ctx sdk.Context, keeper *icskeeper.Keeper) (out []types.WithdrawalRecord) { + zone, _ := keeper.GetZone(ctx, suite.chainB.ChainID) + validators := keeper.GetValidatorAddresses(ctx, zone.ChainId) + out = []types.WithdrawalRecord{ + { + ChainId: zone.ChainId, + Delegator: zone.DelegationAddress.Address, + Recipient: "cosmos1v4gek4mld0k5yhpe0fsln4takg558cdpyexv2rxr3dh45f2fqrgsw52m97", + Distribution: []*types.Distribution{ + {Amount: 10000, Valoper: validators[0]}, + {Amount: 9500, Valoper: validators[1]}, + {Amount: 10000, Valoper: validators[2]}, + {Amount: 10000, Valoper: validators[3]}, + }, + Amount: sdk.NewCoins(sdk.NewCoin(zone.BaseDenom, math.NewInt(39500))), + BurnAmount: sdk.NewCoin(zone.LocalDenom, math.NewInt(32356)), + Txhash: "3BE21C1057ABBFBC44BE8993D2A4701C751507FF9901AA110B5993BA070C176B", + Status: types.WithdrawStatusUnbond, + }, + { + ChainId: zone.ChainId, + Delegator: zone.DelegationAddress.Address, + Recipient: "cosmos1nvkpj5n5mhy2ntvgn2cklntwx9ujvfvcacz5et", + Distribution: []*types.Distribution{ + {Amount: 13000, Valoper: validators[0]}, + {Amount: 13300, Valoper: validators[1]}, + {Amount: 10000, Valoper: validators[2]}, + {Amount: 11000, Valoper: validators[3]}, + }, + Amount: sdk.NewCoins(sdk.NewCoin(zone.BaseDenom, math.NewInt(47300))), + BurnAmount: sdk.NewCoin(zone.LocalDenom, math.NewInt(36503)), + Txhash: "FB087A50A4836CBDFACA70D393AF110C28935276267B7BA2838BE3CEEA08F762", + Status: types.WithdrawStatusUnbond, + }, + } + return out + }, + Delta: sdk.NewDecWithPrec(100, 2).Quo(sdk.NewDecWithPrec(95, 2)), + Validator: func(validators []string) string { return validators[1] }, + ExpectError: false, + }, + { + Name: "overflow test", + InitialRecords: func(ctx sdk.Context, keeper *icskeeper.Keeper) { + zone, _ := keeper.GetZone(ctx, suite.chainB.ChainID) + validators := keeper.GetValidatorAddresses(ctx, zone.ChainId) + withdrawalRecord := types.WithdrawalRecord{ + ChainId: zone.ChainId, + Delegator: zone.DelegationAddress.Address, + Recipient: "cosmos1v4gek4mld0k5yhpe0fsln4takg558cdpyexv2rxr3dh45f2fqrgsw52m97", + Distribution: []*types.Distribution{ + {Amount: 9223372036854775807 + 1, Valoper: validators[1]}, // max int64 +1 - check for overflow + }, + Amount: sdk.NewCoins(sdk.NewCoin(zone.BaseDenom, math.NewInt(40000))), + BurnAmount: sdk.NewCoin(zone.LocalDenom, math.NewInt(32356)), + Txhash: "3BE21C1057ABBFBC44BE8993D2A4701C751507FF9901AA110B5993BA070C176B", + Status: types.WithdrawStatusUnbond, + } + keeper.SetWithdrawalRecord(ctx, withdrawalRecord) + }, + ExpectedRecords: func(ctx sdk.Context, keeper *icskeeper.Keeper) (out []types.WithdrawalRecord) { + zone, _ := keeper.GetZone(ctx, suite.chainB.ChainID) + validators := keeper.GetValidatorAddresses(ctx, zone.ChainId) + out = []types.WithdrawalRecord{ + { + ChainId: zone.ChainId, + Delegator: zone.DelegationAddress.Address, + Recipient: "cosmos1v4gek4mld0k5yhpe0fsln4takg558cdpyexv2rxr3dh45f2fqrgsw52m97", + Distribution: []*types.Distribution{ + {Amount: 9223372036854775807 + 1, Valoper: validators[1]}, + }, + Amount: sdk.NewCoins(sdk.NewCoin(zone.BaseDenom, math.NewInt(40000))), + BurnAmount: sdk.NewCoin(zone.LocalDenom, math.NewInt(32356)), + Txhash: "3BE21C1057ABBFBC44BE8993D2A4701C751507FF9901AA110B5993BA070C176B", + Status: types.WithdrawStatusUnbond, + }, + } + return out + }, + Delta: sdk.NewDecWithPrec(100, 2).Quo(sdk.NewDecWithPrec(95, 2)), + Validator: func(validators []string) string { return validators[1] }, + ExpectError: true, + }, + + { + Name: "mismatch test", + InitialRecords: func(ctx sdk.Context, keeper *icskeeper.Keeper) { + zone, _ := keeper.GetZone(ctx, suite.chainB.ChainID) + validators := keeper.GetValidatorAddresses(ctx, zone.ChainId) + withdrawalRecord := types.WithdrawalRecord{ + ChainId: zone.ChainId, + Delegator: zone.DelegationAddress.Address, + Recipient: "cosmos1nna7k5lywn99cd63elcfqm6p8c5c4qcug4aef5", + Distribution: []*types.Distribution{ + {Amount: 100000000, Valoper: validators[1]}, // slashed amount exceeds total amount + }, + Amount: sdk.NewCoins(sdk.NewCoin(zone.BaseDenom, math.NewInt(10))), + BurnAmount: sdk.NewCoin(zone.LocalDenom, math.NewInt(10)), + Txhash: "8A698A142447087E5DC01F7BC3886EC1A6606D377D1FAC766FB279AD09F1407C", + Status: types.WithdrawStatusUnbond, + } + keeper.SetWithdrawalRecord(ctx, withdrawalRecord) + }, + ExpectedRecords: func(ctx sdk.Context, keeper *icskeeper.Keeper) (out []types.WithdrawalRecord) { + zone, _ := keeper.GetZone(ctx, suite.chainB.ChainID) + validators := keeper.GetValidatorAddresses(ctx, zone.ChainId) + out = []types.WithdrawalRecord{ + { + ChainId: zone.ChainId, + Delegator: zone.DelegationAddress.Address, + Recipient: "cosmos1nna7k5lywn99cd63elcfqm6p8c5c4qcug4aef5", + Distribution: []*types.Distribution{ + {Amount: 100000000, Valoper: validators[1]}, // slashed amount exceeds total amount + }, + Amount: sdk.NewCoins(sdk.NewCoin(zone.BaseDenom, math.NewInt(10))), + BurnAmount: sdk.NewCoin(zone.LocalDenom, math.NewInt(10)), + Txhash: "8A698A142447087E5DC01F7BC3886EC1A6606D377D1FAC766FB279AD09F1407C", + Status: types.WithdrawStatusUnbond, + }, + } + return out + }, + Delta: sdk.NewDecWithPrec(100, 2).Quo(sdk.NewDecWithPrec(95, 2)), + Validator: func(validators []string) string { return validators[1] }, + ExpectError: true, + }, + } + + for _, tc := range tcs { + suite.Run(tc.Name, func() { + suite.SetupTest() + suite.setupTestZones() + + app := suite.GetQuicksilverApp(suite.chainA) + ctx := suite.chainA.GetContext() + + zone, found := app.InterchainstakingKeeper.GetZone(ctx, suite.chainB.ChainID) + suite.True(found) + + tc.InitialRecords(ctx, app.InterchainstakingKeeper) + + err := app.InterchainstakingKeeper.UpdateWithdrawalRecordsForSlash(ctx, &zone, tc.Validator(app.InterchainstakingKeeper.GetValidatorAddresses(ctx, zone.ChainId)), tc.Delta) + if tc.ExpectError { + suite.Error(err) + } else { + suite.NoError(err) + } + ctx = suite.chainA.GetContext() + for _, expected := range tc.ExpectedRecords(ctx, app.InterchainstakingKeeper) { + wrd, found := app.InterchainstakingKeeper.GetWithdrawalRecord(ctx, zone.ChainId, expected.Txhash, types.WithdrawStatusUnbond) + suite.True(found) + suite.EqualValues(expected, wrd) + } + }) + } + +}