fix(callbacks)!: SendPacket validation bypass and erroneous event emi…

…ssion are fixed (#4568) * fix: fixed callbacks out of gas handling * fix: fixed panics and oog errors not showing up on events * docs(callbacks): added godocs for error precedence --------- Co-authored-by: Carlos Rodriguez <[email protected]>
cosmos · Sep 7, 2023 · e192899 · e192899
1 parent 7be1785
commit e192899
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 5 deletions.
diff --git a/modules/apps/callbacks/ibc_middleware.go b/modules/apps/callbacks/ibc_middleware.go
@@ -3,6 +3,7 @@ package ibccallbacks
 import (
 	"fmt"
 
+	errorsmod "cosmossdk.io/errors"
 	storetypes "cosmossdk.io/store/types"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -261,6 +262,11 @@ func (im IBCMiddleware) WriteAcknowledgement(
 
 // processCallback executes the callbackExecutor and reverts contract changes if the callbackExecutor fails.
 //
+// Error Precedence and Returns:
+//   - oogErr: Takes the highest precedence. If the callback runs out of gas, an error wrapped with types.ErrCallbackOutOfGas is returned.
+//   - panicErr: Takes the second-highest precedence. If a panic occurs and it is not propagated, an error wrapped with types.ErrCallbackPanic is returned.
+//   - callbackErr: If the callbackExecutor returns an error, it is returned as-is.
+//
 // panics if
 //   - the contractExecutor panics for any reason, and the callbackType is SendPacket, or
 //   - the contractExecutor runs out of gas and the relayer has not reserved gas grater than or equal to
@@ -281,11 +287,15 @@ func (IBCMiddleware) processCallback(
 			if callbackType == types.CallbackTypeSendPacket {
 				panic(r)
 			}
+			err = errorsmod.Wrapf(types.ErrCallbackPanic, "ibc %s callback panicked with: %v", callbackType, r)
 		}
 
 		// if the callback ran out of gas and the relayer has not reserved enough gas, then revert the state
-		if cachedCtx.GasMeter().IsPastLimit() && callbackData.AllowRetry() {
-			panic(storetypes.ErrorOutOfGas{Descriptor: fmt.Sprintf("ibc %s callback out of gas; commitGasLimit: %d", callbackType, callbackData.CommitGasLimit)})
+		if cachedCtx.GasMeter().IsPastLimit() {
+			if callbackData.AllowRetry() {
+				panic(storetypes.ErrorOutOfGas{Descriptor: fmt.Sprintf("ibc %s callback out of gas; commitGasLimit: %d", callbackType, callbackData.CommitGasLimit)})
+			}
+			err = errorsmod.Wrapf(types.ErrCallbackOutOfGas, "ibc %s callback out of gas", callbackType)
 		}
 
 		// allow the transaction to be committed, continuing the packet lifecycle

diff --git a/modules/apps/callbacks/ibc_middleware_test.go b/modules/apps/callbacks/ibc_middleware_test.go
@@ -137,14 +137,23 @@ func (s *CallbacksTestSuite) TestSendPacket() {
 			ibcmock.MockApplicationCallbackError, // execution failure on SendPacket should prevent packet sends
 		},
 		{
-			"failure: callback execution reach out of gas, but sufficient gas provided by relayer",
+			"failure: callback execution reach out of gas panic, but sufficient gas provided",
 			func() {
 				packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"400000"}}`, simapp.OogPanicContract)
 			},
 			types.CallbackTypeSendPacket,
 			true,
 			storetypes.ErrorOutOfGas{Descriptor: fmt.Sprintf("mock %s callback oog panic", types.CallbackTypeSendPacket)},
 		},
+		{
+			"failure: callback execution reach out of gas error, but sufficient gas provided",
+			func() {
+				packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"400000"}}`, simapp.OogErrorContract)
+			},
+			types.CallbackTypeSendPacket,
+			false,
+			errorsmod.Wrapf(types.ErrCallbackOutOfGas, "ibc %s callback out of gas", types.CallbackTypeSendPacket),
+		},
 	}
 
 	for _, tc := range testCases {
@@ -821,7 +830,7 @@ func (s *CallbacksTestSuite) TestProcessCallback() {
 				}
 			},
 			false,
-			nil,
+			errorsmod.Wrapf(types.ErrCallbackPanic, "ibc %s callback panicked with: %v", callbackType, "callbackExecutor panic"),
 		},
 		{
 			"success: callbackExecutor oog panic, but retry is not allowed",
@@ -834,7 +843,7 @@ func (s *CallbacksTestSuite) TestProcessCallback() {
 				}
 			},
 			false,
-			nil,
+			errorsmod.Wrapf(types.ErrCallbackOutOfGas, "ibc %s callback out of gas", callbackType),
 		},
 		{
 			"failure: callbackExecutor error",

diff --git a/modules/apps/callbacks/types/errors.go b/modules/apps/callbacks/types/errors.go
@@ -9,4 +9,6 @@ var (
 	ErrNotPacketDataProvider     = errorsmod.Register(ModuleName, 3, "packet is not a PacketDataProvider")
 	ErrCallbackKeyNotFound       = errorsmod.Register(ModuleName, 4, "callback key not found in packet data")
 	ErrCallbackAddressNotFound   = errorsmod.Register(ModuleName, 5, "callback address not found in packet data")
+	ErrCallbackOutOfGas          = errorsmod.Register(ModuleName, 6, "callback out of gas")
+	ErrCallbackPanic             = errorsmod.Register(ModuleName, 7, "callback panic")
 )