From d2c3fff71431410c8b8bd87ecd2be38afd9f2171 Mon Sep 17 00:00:00 2001 From: mattverse Date: Mon, 31 Jul 2023 18:49:05 +0900 Subject: [PATCH 1/6] Finish :) --- x/tokenfactory/keeper/before_send.go | 9 ++- x/tokenfactory/keeper/before_send_test.go | 69 +++++++++++++++++++++++ x/tokenfactory/types/constants.go | 5 ++ 3 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 x/tokenfactory/types/constants.go diff --git a/x/tokenfactory/keeper/before_send.go b/x/tokenfactory/keeper/before_send.go index 3ee8de72bab..769f6f5c05e 100644 --- a/x/tokenfactory/keeper/before_send.go +++ b/x/tokenfactory/keeper/before_send.go @@ -98,6 +98,9 @@ func (k Keeper) callBeforeSendListener(ctx sdk.Context, from, to sdk.AccAddress, var msgBz []byte // get msgBz, either BlockBeforeSend or TrackBeforeSend + // Note that for trackBeforeSend, we need to gas meter computations to prevent infinite loop + // specifically because module to module sends are not gas metered. + // We don't need to do this for blockBeforeSend since blockBeforeSend is not called during module to module sends. if blockBeforeSend { msg := types.BlockBeforeSendSudoMsg{ BlockBeforeSend: types.BlockBeforeSendMsg{ @@ -121,8 +124,12 @@ func (k Keeper) callBeforeSendListener(ctx sdk.Context, from, to sdk.AccAddress, return err } - em := sdk.NewEventManager() + // if its track before send, apply gas meter to prevent infinite loop + if !blockBeforeSend { + ctx = ctx.WithGasMeter(sdk.NewGasMeter(types.TrackBeforeSendGasLimit)) + } + em := sdk.NewEventManager() _, err = k.contractKeeper.Sudo(ctx.WithEventManager(em), cwAddr, msgBz) if err != nil { return errorsmod.Wrapf(err, "failed to call before send hook for denom %s", coin.Denom) diff --git a/x/tokenfactory/keeper/before_send_test.go b/x/tokenfactory/keeper/before_send_test.go index a22aed888e5..f2011f22fe7 100644 --- a/x/tokenfactory/keeper/before_send_test.go +++ b/x/tokenfactory/keeper/before_send_test.go @@ -124,3 +124,72 @@ func (s *KeeperTestSuite) TestBeforeSendHook() { }) } } + +// TestInfiniteTrackBeforeSend tests gas metering with infinite loop contract +// to properly test if we are gas metering trackBeforeSend properly. +func (s *KeeperTestSuite) TestInfiniteTrackBeforeSend() { + s.SkipIfWSL() + + // load wasm file + wasmFile := "./testdata/infinite_track_beforesend.wasm" + wasmCode, err := os.ReadFile(wasmFile) + s.Require().NoError(err) + + for _, tc := range []struct { + name string + useFactoryDenom bool + expectedPanic bool + }{ + { + name: "sending tokenfactory denom from module to module with infinite contract should panic", + useFactoryDenom: true, + expectedPanic: true, + }, + { + name: "sending non-tokenfactory denom from module to module with infinite contract should not panic", + useFactoryDenom: false, + expectedPanic: false, + }, + } { + s.Run(fmt.Sprintf("Case %s", tc.name), func() { + // setup test + s.SetupTest() + + // instantiate wasm code + codeID, _, err := s.contractKeeper.Create(s.Ctx, s.TestAccs[0], wasmCode, nil) + s.Require().NoError(err, "test: %v", tc.name) + cosmwasmAddress, _, err := s.contractKeeper.Instantiate(s.Ctx, codeID, s.TestAccs[0], s.TestAccs[0], []byte("{}"), "", sdk.NewCoins()) + s.Require().NoError(err, "test: %v", tc.name) + + // create new denom + res, err := s.msgServer.CreateDenom(sdk.WrapSDKContext(s.Ctx), types.NewMsgCreateDenom(s.TestAccs[0].String(), "bitcoin")) + s.Require().NoError(err, "test: %v", tc.name) + factoryDenom := res.GetNewTokenDenom() + + var tokenToSend sdk.Coins + if tc.useFactoryDenom { + tokenToSend = sdk.NewCoins(sdk.NewInt64Coin(factoryDenom, 1000000)) + } else { + tokenToSend = sdk.NewCoins(sdk.NewInt64Coin("foo", 1000000)) + } + + // send the mint module tokenToSend + s.FundModuleAcc("mint", tokenToSend) + + // set beforesend hook to the new denom + // we register infinite loop contract here to test if we are gas metering properly + _, err = s.msgServer.SetBeforeSendHook(sdk.WrapSDKContext(s.Ctx), types.NewMsgSetBeforeSendHook(s.TestAccs[0].String(), factoryDenom, cosmwasmAddress.String())) + s.Require().NoError(err, "test: %v", tc.name) + + if tc.expectedPanic { + s.Require().Panics(func() { + s.App.BankKeeper.SendCoinsFromModuleToModule(s.Ctx, "mint", "distribution", tokenToSend) + }) + } else { + s.Require().NotPanics(func() { + s.App.BankKeeper.SendCoinsFromModuleToModule(s.Ctx, "mint", "distribution", tokenToSend) + }) + } + }) + } +} diff --git a/x/tokenfactory/types/constants.go b/x/tokenfactory/types/constants.go new file mode 100644 index 00000000000..7cfe7cd30e5 --- /dev/null +++ b/x/tokenfactory/types/constants.go @@ -0,0 +1,5 @@ +package types + +var ( + TrackBeforeSendGasLimit = uint64(100_000) +) From fd3887403dcc5485ea079684c5ade9dd42d8a9b7 Mon Sep 17 00:00:00 2001 From: mattverse Date: Mon, 31 Jul 2023 18:51:40 +0900 Subject: [PATCH 2/6] Add comment --- x/tokenfactory/keeper/before_send.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x/tokenfactory/keeper/before_send.go b/x/tokenfactory/keeper/before_send.go index 769f6f5c05e..6a6a4a9aba4 100644 --- a/x/tokenfactory/keeper/before_send.go +++ b/x/tokenfactory/keeper/before_send.go @@ -86,6 +86,8 @@ func (h Hooks) BlockBeforeSend(ctx sdk.Context, from, to sdk.AccAddress, amount // callBeforeSendListener iterates over each coin and sends corresponding sudo msg to the contract address stored in state. // If blockBeforeSend is true, sudoMsg wraps BlockBeforeSendMsg, otherwise sudoMsg wraps TrackBeforeSendMsg. +// Note that we gas meter trackBeforeSend to prevent infinite contract calls. +// CONTRACT: this should not be called in beginBlock or endBlock since out of gas will cause this method to panic. func (k Keeper) callBeforeSendListener(ctx sdk.Context, from, to sdk.AccAddress, amount sdk.Coins, blockBeforeSend bool) error { for _, coin := range amount { cosmwasmAddress := k.GetBeforeSendHook(ctx, coin.Denom) From a709c832791c40b7503875a0c3ae3e63a42b2ff9 Mon Sep 17 00:00:00 2001 From: mattverse Date: Mon, 31 Jul 2023 19:08:18 +0900 Subject: [PATCH 3/6] Add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cf7bbb2223..2ec31cfa6d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * [#5855](https://github.com/osmosis-labs/osmosis/pull/5855) feat(x/cosmwasmpool): Sending token_in_max_amount to the contract before running contract msg * [#5893] (https://github.com/osmosis-labs/osmosis/pull/5893) Export createPosition method in CL so other modules can use it in testing * [#5870] (https://github.com/osmosis-labs/osmosis/pull/5870) Remove v14/ separator in protorev rest endpoints +* [#5927] (https://github.com/osmosis-labs/osmosis/pull/5927) Add gas metering to x/tokenfactory trackBeforeSend hook ### Minor improvements & Bug Fixes From 002868e9de4e64f80059c1ee0fa3841a0b936245 Mon Sep 17 00:00:00 2001 From: mattverse Date: Tue, 1 Aug 2023 00:19:37 +0900 Subject: [PATCH 4/6] Catch panic --- x/tokenfactory/keeper/before_send.go | 8 +++++++- x/tokenfactory/keeper/before_send_test.go | 20 ++++++++------------ x/tokenfactory/types/errors.go | 1 + 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/x/tokenfactory/keeper/before_send.go b/x/tokenfactory/keeper/before_send.go index 6a6a4a9aba4..21b0639a4f5 100644 --- a/x/tokenfactory/keeper/before_send.go +++ b/x/tokenfactory/keeper/before_send.go @@ -88,7 +88,13 @@ func (h Hooks) BlockBeforeSend(ctx sdk.Context, from, to sdk.AccAddress, amount // If blockBeforeSend is true, sudoMsg wraps BlockBeforeSendMsg, otherwise sudoMsg wraps TrackBeforeSendMsg. // Note that we gas meter trackBeforeSend to prevent infinite contract calls. // CONTRACT: this should not be called in beginBlock or endBlock since out of gas will cause this method to panic. -func (k Keeper) callBeforeSendListener(ctx sdk.Context, from, to sdk.AccAddress, amount sdk.Coins, blockBeforeSend bool) error { +func (k Keeper) callBeforeSendListener(ctx sdk.Context, from, to sdk.AccAddress, amount sdk.Coins, blockBeforeSend bool) (err error) { + defer func() { + if r := recover(); r != nil { + err = types.ErrTrackBeforeSendOutOfGas + } + }() + for _, coin := range amount { cosmwasmAddress := k.GetBeforeSendHook(ctx, coin.Denom) if cosmwasmAddress != "" { diff --git a/x/tokenfactory/keeper/before_send_test.go b/x/tokenfactory/keeper/before_send_test.go index f2011f22fe7..7186e903005 100644 --- a/x/tokenfactory/keeper/before_send_test.go +++ b/x/tokenfactory/keeper/before_send_test.go @@ -138,17 +138,15 @@ func (s *KeeperTestSuite) TestInfiniteTrackBeforeSend() { for _, tc := range []struct { name string useFactoryDenom bool - expectedPanic bool + expectedError bool }{ { name: "sending tokenfactory denom from module to module with infinite contract should panic", useFactoryDenom: true, - expectedPanic: true, }, { name: "sending non-tokenfactory denom from module to module with infinite contract should not panic", useFactoryDenom: false, - expectedPanic: false, }, } { s.Run(fmt.Sprintf("Case %s", tc.name), func() { @@ -181,15 +179,13 @@ func (s *KeeperTestSuite) TestInfiniteTrackBeforeSend() { _, err = s.msgServer.SetBeforeSendHook(sdk.WrapSDKContext(s.Ctx), types.NewMsgSetBeforeSendHook(s.TestAccs[0].String(), factoryDenom, cosmwasmAddress.String())) s.Require().NoError(err, "test: %v", tc.name) - if tc.expectedPanic { - s.Require().Panics(func() { - s.App.BankKeeper.SendCoinsFromModuleToModule(s.Ctx, "mint", "distribution", tokenToSend) - }) - } else { - s.Require().NotPanics(func() { - s.App.BankKeeper.SendCoinsFromModuleToModule(s.Ctx, "mint", "distribution", tokenToSend) - }) - } + // track before send suppresses in any case, thus we expect no error + err = s.App.BankKeeper.SendCoinsFromModuleToModule(s.Ctx, "mint", "distribution", tokenToSend) + s.Require().NoError(err) + + distributionModuleAddress := s.App.AccountKeeper.GetModuleAddress("distribution") + distributionModuleBalances := s.App.BankKeeper.GetAllBalances(s.Ctx, distributionModuleAddress) + s.Require().True(distributionModuleBalances.IsEqual(tokenToSend)) }) } } diff --git a/x/tokenfactory/types/errors.go b/x/tokenfactory/types/errors.go index a66d4b6b417..aa78ca24ae0 100644 --- a/x/tokenfactory/types/errors.go +++ b/x/tokenfactory/types/errors.go @@ -20,4 +20,5 @@ var ( ErrCreatorTooLong = errorsmod.Register(ModuleName, 9, fmt.Sprintf("creator too long, max length is %d bytes", MaxCreatorLength)) ErrDenomDoesNotExist = errorsmod.Register(ModuleName, 10, "denom does not exist") ErrBurnFromModuleAccount = errorsmod.Register(ModuleName, 11, "burning from Module Account is not allowed") + ErrTrackBeforeSendOutOfGas = errorsmod.Register(ModuleName, 12, "gas meter hit maximum limit") ) From cb7ff38bde2d35c1ee6b96b57fea225129631674 Mon Sep 17 00:00:00 2001 From: mattverse Date: Tue, 1 Aug 2023 00:20:05 +0900 Subject: [PATCH 5/6] Update comment --- x/tokenfactory/keeper/before_send_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/x/tokenfactory/keeper/before_send_test.go b/x/tokenfactory/keeper/before_send_test.go index 7186e903005..cd43204280a 100644 --- a/x/tokenfactory/keeper/before_send_test.go +++ b/x/tokenfactory/keeper/before_send_test.go @@ -183,6 +183,7 @@ func (s *KeeperTestSuite) TestInfiniteTrackBeforeSend() { err = s.App.BankKeeper.SendCoinsFromModuleToModule(s.Ctx, "mint", "distribution", tokenToSend) s.Require().NoError(err) + // send should happen regardless of trackBeforeSend results distributionModuleAddress := s.App.AccountKeeper.GetModuleAddress("distribution") distributionModuleBalances := s.App.BankKeeper.GetAllBalances(s.Ctx, distributionModuleAddress) s.Require().True(distributionModuleBalances.IsEqual(tokenToSend)) From f8619ff632c03746bb1b8b92938356fd80a65770 Mon Sep 17 00:00:00 2001 From: mattverse Date: Tue, 1 Aug 2023 16:07:11 +0900 Subject: [PATCH 6/6] Try changing to use child ctx --- x/tokenfactory/keeper/before_send.go | 21 ++++++++++++++------- x/tokenfactory/keeper/before_send_test.go | 23 ++++++++++++++++------- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/x/tokenfactory/keeper/before_send.go b/x/tokenfactory/keeper/before_send.go index 21b0639a4f5..bfe4d36e8b2 100644 --- a/x/tokenfactory/keeper/before_send.go +++ b/x/tokenfactory/keeper/before_send.go @@ -131,16 +131,23 @@ func (k Keeper) callBeforeSendListener(ctx sdk.Context, from, to sdk.AccAddress, if err != nil { return err } + em := sdk.NewEventManager() // if its track before send, apply gas meter to prevent infinite loop - if !blockBeforeSend { - ctx = ctx.WithGasMeter(sdk.NewGasMeter(types.TrackBeforeSendGasLimit)) - } + if blockBeforeSend { + _, err = k.contractKeeper.Sudo(ctx.WithEventManager(em), cwAddr, msgBz) + if err != nil { + return errorsmod.Wrapf(err, "failed to call before send hook for denom %s", coin.Denom) + } + } else { + childCtx := ctx.WithGasMeter(sdk.NewGasMeter(types.TrackBeforeSendGasLimit)) + _, err = k.contractKeeper.Sudo(childCtx.WithEventManager(em), cwAddr, msgBz) + if err != nil { + return errorsmod.Wrapf(err, "failed to call before send hook for denom %s", coin.Denom) + } - em := sdk.NewEventManager() - _, err = k.contractKeeper.Sudo(ctx.WithEventManager(em), cwAddr, msgBz) - if err != nil { - return errorsmod.Wrapf(err, "failed to call before send hook for denom %s", coin.Denom) + // consume gas used for calling contract to the parent ctx + ctx.GasMeter().ConsumeGas(childCtx.GasMeter().GasConsumed(), "track before send gas") } } } diff --git a/x/tokenfactory/keeper/before_send_test.go b/x/tokenfactory/keeper/before_send_test.go index cd43204280a..c4ff10756e9 100644 --- a/x/tokenfactory/keeper/before_send_test.go +++ b/x/tokenfactory/keeper/before_send_test.go @@ -130,29 +130,38 @@ func (s *KeeperTestSuite) TestBeforeSendHook() { func (s *KeeperTestSuite) TestInfiniteTrackBeforeSend() { s.SkipIfWSL() - // load wasm file - wasmFile := "./testdata/infinite_track_beforesend.wasm" - wasmCode, err := os.ReadFile(wasmFile) - s.Require().NoError(err) - for _, tc := range []struct { name string + wasmFile string + tokenToSend sdk.Coins useFactoryDenom bool expectedError bool }{ { name: "sending tokenfactory denom from module to module with infinite contract should panic", + wasmFile: "./testdata/infinite_track_beforesend.wasm", useFactoryDenom: true, }, { name: "sending non-tokenfactory denom from module to module with infinite contract should not panic", + wasmFile: "./testdata/infinite_track_beforesend.wasm", + tokenToSend: sdk.NewCoins(sdk.NewInt64Coin("foo", 1000000)), useFactoryDenom: false, }, + { + name: "Try using no 100 ", + wasmFile: "./testdata/no100.wasm", + useFactoryDenom: true, + }, } { s.Run(fmt.Sprintf("Case %s", tc.name), func() { // setup test s.SetupTest() + // load wasm file + wasmCode, err := os.ReadFile(tc.wasmFile) + s.Require().NoError(err) + // instantiate wasm code codeID, _, err := s.contractKeeper.Create(s.Ctx, s.TestAccs[0], wasmCode, nil) s.Require().NoError(err, "test: %v", tc.name) @@ -166,9 +175,9 @@ func (s *KeeperTestSuite) TestInfiniteTrackBeforeSend() { var tokenToSend sdk.Coins if tc.useFactoryDenom { - tokenToSend = sdk.NewCoins(sdk.NewInt64Coin(factoryDenom, 1000000)) + tokenToSend = sdk.NewCoins(sdk.NewInt64Coin(factoryDenom, 100)) } else { - tokenToSend = sdk.NewCoins(sdk.NewInt64Coin("foo", 1000000)) + tokenToSend = tc.tokenToSend } // send the mint module tokenToSend