From e192899909ed1f610a1fc30399dc664297199a2f Mon Sep 17 00:00:00 2001 From: srdtrk <59252793+srdtrk@users.noreply.github.com> Date: Thu, 7 Sep 2023 15:02:20 +0300 Subject: [PATCH] fix(callbacks)!: SendPacket validation bypass and erroneous event emission 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 --- modules/apps/callbacks/ibc_middleware.go | 14 ++++++++++++-- modules/apps/callbacks/ibc_middleware_test.go | 15 ++++++++++++--- modules/apps/callbacks/types/errors.go | 2 ++ 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/modules/apps/callbacks/ibc_middleware.go b/modules/apps/callbacks/ibc_middleware.go index 8a562743473..ea4835b590a 100644 --- 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 index 705beb2a706..902626320bb 100644 --- a/modules/apps/callbacks/ibc_middleware_test.go +++ b/modules/apps/callbacks/ibc_middleware_test.go @@ -137,7 +137,7 @@ 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) }, @@ -145,6 +145,15 @@ func (s *CallbacksTestSuite) TestSendPacket() { 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 index b1b37209625..df2d2ef2938 100644 --- 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") )