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

…ssion are fixed (cosmos#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]>
mmsqe · Nov 27, 2023 · 52080fb · 52080fb
1 parent 7b759dd
commit 52080fb
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 5 deletions.
diff --git a/modules/apps/callbacks/ibc_middleware.go b/modules/apps/callbacks/ibc_middleware.go
@@ -3,6 +3,8 @@ package ibccallbacks
 import (
 	"fmt"
 
+	errorsmod "cosmossdk.io/errors"
+
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
 
@@ -239,6 +241,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
@@ -259,11 +266,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(sdk.ErrorOutOfGas{Descriptor: fmt.Sprintf("ibc %s callback out of gas; commitGasLimit: %d", callbackType, callbackData.CommitGasLimit)})
+		if cachedCtx.GasMeter().IsPastLimit() {
+			if callbackData.AllowRetry() {
+				panic(sdk.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
@@ -136,14 +136,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,
 			sdk.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 {
@@ -771,7 +780,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",
@@ -783,7 +792,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")
 )