fix: Add gas metering to BlockBeforeSend (#6757)

* Add test case and fix for before send * Change gas limit to 500k * Add changelog --------- Co-authored-by: Alpo <[email protected]>
osmosis-labs · Nov 3, 2023 · ed031e3 · ed031e3
1 parent f721f73
commit ed031e3
Show file tree

Hide file tree

Showing 5 changed files with 37 additions and 27 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -83,6 +83,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * [#6666](https://github.com/osmosis-labs/osmosis/pull/6666) fix: cosmwasmpool state export bug
 * [#6674](https://github.com/osmosis-labs/osmosis/pull/6674) fix: remove dragonberry replace directive
 * [#6692](https://github.com/osmosis-labs/osmosis/pull/6692) chore: add cur sqrt price to LiquidityNetInDirection return value
+* [#6757](https://github.com/osmosis-labs/osmosis/pull/6757) fix: add gas metering to block before send for token factory bank hook
 * [#6710](https://github.com/osmosis-labs/osmosis/pull/6710) fix: `{overflow}` bug when querying cosmwasmpool spot price
 
 ## v19.2.0

diff --git a/x/tokenfactory/keeper/before_send.go b/x/tokenfactory/keeper/before_send.go
@@ -91,7 +91,7 @@ func (h Hooks) BlockBeforeSend(ctx sdk.Context, from, to sdk.AccAddress, amount
 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 = errorsmod.Wrapf(types.ErrTrackBeforeSendOutOfGas, "%v", r)
+			err = errorsmod.Wrapf(types.ErrBeforeSendHookOutOfGas, "%v", r)
 		}
 	}()
 
@@ -133,22 +133,14 @@ func (k Keeper) callBeforeSendListener(ctx sdk.Context, from, to sdk.AccAddress,
 			}
 			em := sdk.NewEventManager()
 
-			// if its track before send, apply gas meter to prevent infinite loop
-			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)
-				}
-
-				// consume gas used for calling contract to the parent ctx
-				ctx.GasMeter().ConsumeGas(childCtx.GasMeter().GasConsumed(), "track before send gas")
+			childCtx := ctx.WithGasMeter(sdk.NewGasMeter(types.BeforeSendHookGasLimit))
+			_, 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)
 			}
+
+			// consume gas used for calling contract to the parent ctx
+			ctx.GasMeter().ConsumeGas(childCtx.GasMeter().GasConsumed(), "track before send gas")
 		}
 	}
 	return nil

diff --git a/x/tokenfactory/keeper/before_send_test.go b/x/tokenfactory/keeper/before_send_test.go
@@ -51,7 +51,7 @@ func (s *KeeperTestSuite) TestBeforeSendHook() {
 					expectPass: true,
 				},
 				{
-					desc: "sending 100 of factorydenom should not work",
+					desc: "sending 100 of factorydenom should error",
 					msg: func(factorydenom string) *banktypes.MsgSend {
 						return banktypes.NewMsgSend(
 							s.TestAccs[0],
@@ -135,13 +135,20 @@ func (s *KeeperTestSuite) TestInfiniteTrackBeforeSend() {
 		wasmFile        string
 		tokenToSend     sdk.Coins
 		useFactoryDenom bool
+		blockBeforeSend 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 tokenfactory denom from module to module with infinite contract should panic",
+			wasmFile:        "./testdata/infinite_track_beforesend.wasm",
+			useFactoryDenom: true,
+			blockBeforeSend: true,
+		},
 		{
 			name:            "sending non-tokenfactory denom from module to module with infinite contract should not panic",
 			wasmFile:        "./testdata/infinite_track_beforesend.wasm",
@@ -181,21 +188,31 @@ func (s *KeeperTestSuite) TestInfiniteTrackBeforeSend() {
 			}
 
 			// send the mint module tokenToSend
-			s.FundModuleAcc("mint", tokenToSend)
+			if tc.blockBeforeSend {
+				s.FundAcc(s.TestAccs[0], tokenToSend)
+			} else {
+				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)
 
-			// 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)
+			if tc.blockBeforeSend {
+				err = s.App.BankKeeper.SendCoins(s.Ctx, s.TestAccs[0], s.TestAccs[1], tokenToSend)
+				s.Require().Error(err)
+			} else {
+				// 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)
+
+				// 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))
+			}
 
-			// 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))
 		})
 	}
 }
diff --git a/x/tokenfactory/types/constants.go b/x/tokenfactory/types/constants.go
@@ -1,5 +1,5 @@
 package types
 
 var (
-	TrackBeforeSendGasLimit = uint64(100_000)
+	BeforeSendHookGasLimit = uint64(500_000)
 )
diff --git a/x/tokenfactory/types/errors.go b/x/tokenfactory/types/errors.go
@@ -20,5 +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")
+	ErrBeforeSendHookOutOfGas   = errorsmod.Register(ModuleName, 12, "gas meter hit maximum limit")
 )