Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(callbacks)!: SendPacket validation bypass and erroneous event emission are fixed #4568

Merged
merged 11 commits into from
Sep 7, 2023
Merged
14 changes: 12 additions & 2 deletions modules/apps/callbacks/ibc_middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package ibccallbacks
import (
"fmt"

errorsmod "cosmossdk.io/errors"
storetypes "cosmossdk.io/store/types"

sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -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
Expand All @@ -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)
chatton marked this conversation as resolved.
Show resolved Hide resolved
}

// 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)
Copy link
Contributor

@colin-axner colin-axner Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we are okay overwriting the returned error if the cached context runs of gas?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we have an order of precedence now:

regularErr < panicErr < pastLimitErr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with that since the panics are likely due to out of gas error anyways

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we update godoc with this order of precedence and behaviour?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added godocs.

}

// allow the transaction to be committed, continuing the packet lifecycle
Expand Down
15 changes: 12 additions & 3 deletions modules/apps/callbacks/ibc_middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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",
Expand All @@ -834,7 +843,7 @@ func (s *CallbacksTestSuite) TestProcessCallback() {
}
},
false,
nil,
errorsmod.Wrapf(types.ErrCallbackOutOfGas, "ibc %s callback out of gas", callbackType),
},
{
"failure: callbackExecutor error",
Expand Down
2 changes: 2 additions & 0 deletions modules/apps/callbacks/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
)
Loading