From 5f575ce963190a7b0b2df8507f0b4076618d8ad2 Mon Sep 17 00:00:00 2001 From: Stefano Angieri Date: Mon, 28 Oct 2024 14:10:10 +0100 Subject: [PATCH 1/8] enforce maxTimeout --- .../core/04-channel/v2/keeper/msg_server.go | 6 ++++++ .../04-channel/v2/keeper/msg_server_test.go | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/modules/core/04-channel/v2/keeper/msg_server.go b/modules/core/04-channel/v2/keeper/msg_server.go index 1acaf49e30c..f108ea7e3a2 100644 --- a/modules/core/04-channel/v2/keeper/msg_server.go +++ b/modules/core/04-channel/v2/keeper/msg_server.go @@ -63,12 +63,18 @@ func (k *Keeper) RegisterCounterparty(goCtx context.Context, msg *channeltypesv2 // SendPacket defines a rpc handler method for MsgSendPacket. func (k *Keeper) SendPacket(ctx context.Context, msg *channeltypesv2.MsgSendPacket) (*channeltypesv2.MsgSendPacketResponse, error) { sdkCtx := sdk.UnwrapSDKContext(ctx) + sequence, destChannel, err := k.sendPacket(ctx, msg.SourceChannel, msg.TimeoutTimestamp, msg.Payloads) if err != nil { sdkCtx.Logger().Error("send packet failed", "source-channel", msg.SourceChannel, "error", errorsmod.Wrap(err, "send packet failed")) return nil, errorsmod.Wrapf(err, "send packet failed for source id: %s", msg.SourceChannel) } + // Note, the validate basic function in sendPacket does the TimeoutTimestamp != 0 check and other stateless checks on the packet. + if uint64(sdkCtx.BlockTime().Unix()) > msg.TimeoutTimestamp || msg.TimeoutTimestamp > uint64(sdkCtx.BlockTime().Unix())+channeltypesv2.MaxTimeout { + return nil, errorsmod.Wrap(channeltypesv2.ErrInvalidTimeout, "The timeout exceeds the maximum expected value") + } + signer, err := sdk.AccAddressFromBech32(msg.Signer) if err != nil { sdkCtx.Logger().Error("send packet failed", "error", errorsmod.Wrap(err, "invalid address for msg Signer")) diff --git a/modules/core/04-channel/v2/keeper/msg_server_test.go b/modules/core/04-channel/v2/keeper/msg_server_test.go index fa690b8087e..c8895915fcc 100644 --- a/modules/core/04-channel/v2/keeper/msg_server_test.go +++ b/modules/core/04-channel/v2/keeper/msg_server_test.go @@ -116,6 +116,16 @@ func (suite *KeeperTestSuite) TestMsgSendPacket() { malleate: func() {}, expError: nil, }, + { + name: "success: valid timeout timestamp", + malleate: func() { + // ensure a message timeout. + timeoutTimestamp = uint64(suite.chainA.GetContext().BlockTime().Unix()) + channeltypesv2.MaxTimeout - 10 + expectedPacket = channeltypesv2.NewPacket(1, path.EndpointA.ChannelID, path.EndpointB.ChannelID, timeoutTimestamp, payload) + + }, + expError: nil, + }, { name: "failure: timeout elapsed", malleate: func() { @@ -124,6 +134,14 @@ func (suite *KeeperTestSuite) TestMsgSendPacket() { }, expError: channeltypesv1.ErrTimeoutElapsed, }, + { + name: "failure: timeout timestamp exceeds max allowed input", + malleate: func() { + // ensure message timeout exceeds max allowed input. + timeoutTimestamp = uint64(suite.chainA.GetContext().BlockTime().Unix()) + channeltypesv2.MaxTimeout + 10 + }, + expError: channeltypesv2.ErrInvalidTimeout, + }, { name: "failure: inactive client", malleate: func() { From 72d4d8cd42b914f4c83866c2f130e7737969f493 Mon Sep 17 00:00:00 2001 From: Stefano Angieri Date: Mon, 28 Oct 2024 14:12:36 +0100 Subject: [PATCH 2/8] fixes --- modules/core/04-channel/v2/types/errors.go | 1 + modules/core/04-channel/v2/types/msgs.go | 2 ++ 2 files changed, 3 insertions(+) diff --git a/modules/core/04-channel/v2/types/errors.go b/modules/core/04-channel/v2/types/errors.go index 7a1cabe9fc0..6e1cc76377c 100644 --- a/modules/core/04-channel/v2/types/errors.go +++ b/modules/core/04-channel/v2/types/errors.go @@ -13,4 +13,5 @@ var ( ErrInvalidAcknowledgement = errorsmod.Register(SubModuleName, 7, "invalid acknowledgement") ErrPacketCommitmentNotFound = errorsmod.Register(SubModuleName, 8, "packet commitment not found") ErrAcknowledgementNotFound = errorsmod.Register(SubModuleName, 9, "packet acknowledgement not found") + ErrInvalidTimeout = errorsmod.Register(SubModuleName, 10, "invalid channel") ) diff --git a/modules/core/04-channel/v2/types/msgs.go b/modules/core/04-channel/v2/types/msgs.go index 9c6f724e40d..44d2082220f 100644 --- a/modules/core/04-channel/v2/types/msgs.go +++ b/modules/core/04-channel/v2/types/msgs.go @@ -13,6 +13,8 @@ import ( ibcerrors "github.com/cosmos/ibc-go/v9/modules/core/errors" ) +const MaxTimeout uint64 = 86400 // 24h in seconds + var ( _ sdk.Msg = (*MsgCreateChannel)(nil) _ sdk.HasValidateBasic = (*MsgCreateChannel)(nil) From 3891f7e52a3c798f0cbca738ec35f580b8bf3c44 Mon Sep 17 00:00:00 2001 From: Stefano Angieri Date: Mon, 28 Oct 2024 14:22:17 +0100 Subject: [PATCH 3/8] fix error --- modules/core/04-channel/v2/types/errors.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/04-channel/v2/types/errors.go b/modules/core/04-channel/v2/types/errors.go index 6e1cc76377c..1cc831b1385 100644 --- a/modules/core/04-channel/v2/types/errors.go +++ b/modules/core/04-channel/v2/types/errors.go @@ -13,5 +13,5 @@ var ( ErrInvalidAcknowledgement = errorsmod.Register(SubModuleName, 7, "invalid acknowledgement") ErrPacketCommitmentNotFound = errorsmod.Register(SubModuleName, 8, "packet commitment not found") ErrAcknowledgementNotFound = errorsmod.Register(SubModuleName, 9, "packet acknowledgement not found") - ErrInvalidTimeout = errorsmod.Register(SubModuleName, 10, "invalid channel") + ErrInvalidTimeout = errorsmod.Register(SubModuleName, 10, "timeout exceeds max allowed value") ) From ecfb9904cf6295ddfbbc971c8659ceff5e0f6e58 Mon Sep 17 00:00:00 2001 From: Stefano Angieri Date: Mon, 28 Oct 2024 14:35:10 +0100 Subject: [PATCH 4/8] run gofumpt --- modules/core/04-channel/v2/keeper/msg_server_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/core/04-channel/v2/keeper/msg_server_test.go b/modules/core/04-channel/v2/keeper/msg_server_test.go index c8895915fcc..eba1ee7f351 100644 --- a/modules/core/04-channel/v2/keeper/msg_server_test.go +++ b/modules/core/04-channel/v2/keeper/msg_server_test.go @@ -122,7 +122,6 @@ func (suite *KeeperTestSuite) TestMsgSendPacket() { // ensure a message timeout. timeoutTimestamp = uint64(suite.chainA.GetContext().BlockTime().Unix()) + channeltypesv2.MaxTimeout - 10 expectedPacket = channeltypesv2.NewPacket(1, path.EndpointA.ChannelID, path.EndpointB.ChannelID, timeoutTimestamp, payload) - }, expError: nil, }, From ac8fa26136f09f0fd6e280a890758df70cf5b462 Mon Sep 17 00:00:00 2001 From: sangier <45793271+sangier@users.noreply.github.com> Date: Mon, 28 Oct 2024 14:42:36 +0100 Subject: [PATCH 5/8] Apply review suggestions Co-authored-by: Aditya <14364734+AdityaSripal@users.noreply.github.com> --- modules/core/04-channel/v2/types/msgs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/04-channel/v2/types/msgs.go b/modules/core/04-channel/v2/types/msgs.go index 44d2082220f..4c45b3e9d05 100644 --- a/modules/core/04-channel/v2/types/msgs.go +++ b/modules/core/04-channel/v2/types/msgs.go @@ -13,7 +13,7 @@ import ( ibcerrors "github.com/cosmos/ibc-go/v9/modules/core/errors" ) -const MaxTimeout uint64 = 86400 // 24h in seconds +const MaxTimeoutDelta uint64 = 24 * 60 * 60 // 24h in seconds var ( _ sdk.Msg = (*MsgCreateChannel)(nil) From 536de70a3382ee705ffcf630eba8ea4e79b5f1f9 Mon Sep 17 00:00:00 2001 From: Stefano Angieri Date: Mon, 28 Oct 2024 15:06:15 +0100 Subject: [PATCH 6/8] address review comments --- modules/core/04-channel/v2/keeper/msg_server.go | 12 +++++++++--- .../core/04-channel/v2/keeper/msg_server_test.go | 14 +++++++++++--- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/modules/core/04-channel/v2/keeper/msg_server.go b/modules/core/04-channel/v2/keeper/msg_server.go index f108ea7e3a2..83a1b4a923a 100644 --- a/modules/core/04-channel/v2/keeper/msg_server.go +++ b/modules/core/04-channel/v2/keeper/msg_server.go @@ -70,9 +70,15 @@ func (k *Keeper) SendPacket(ctx context.Context, msg *channeltypesv2.MsgSendPack return nil, errorsmod.Wrapf(err, "send packet failed for source id: %s", msg.SourceChannel) } - // Note, the validate basic function in sendPacket does the TimeoutTimestamp != 0 check and other stateless checks on the packet. - if uint64(sdkCtx.BlockTime().Unix()) > msg.TimeoutTimestamp || msg.TimeoutTimestamp > uint64(sdkCtx.BlockTime().Unix())+channeltypesv2.MaxTimeout { - return nil, errorsmod.Wrap(channeltypesv2.ErrInvalidTimeout, "The timeout exceeds the maximum expected value") + // Note, the validate basic function in sendPacket does the timeoutTimestamp != 0 check and other stateless checks on the packet. + // timeoutTimestamp must be less than current block time + MaxTimeoutDelta + if msg.TimeoutTimestamp > uint64(sdkCtx.BlockTime().Unix())+channeltypesv2.MaxTimeoutDelta { + return nil, errorsmod.Wrap(channeltypesv2.ErrMaxTimeoutDeltaExceeded, "timeout exceeds the maximum expected value") + } + + // timeoutTimestamp must be greater than current block time + if uint64(sdkCtx.BlockTime().Unix()) > msg.TimeoutTimestamp { + return nil, errorsmod.Wrap(channeltypesv2.ErrTimeoutTooLow, "timeout is less than the current block timestamp") } signer, err := sdk.AccAddressFromBech32(msg.Signer) diff --git a/modules/core/04-channel/v2/keeper/msg_server_test.go b/modules/core/04-channel/v2/keeper/msg_server_test.go index eba1ee7f351..9a46480688d 100644 --- a/modules/core/04-channel/v2/keeper/msg_server_test.go +++ b/modules/core/04-channel/v2/keeper/msg_server_test.go @@ -120,7 +120,7 @@ func (suite *KeeperTestSuite) TestMsgSendPacket() { name: "success: valid timeout timestamp", malleate: func() { // ensure a message timeout. - timeoutTimestamp = uint64(suite.chainA.GetContext().BlockTime().Unix()) + channeltypesv2.MaxTimeout - 10 + timeoutTimestamp = uint64(suite.chainA.GetContext().BlockTime().Unix()) + channeltypesv2.MaxTimeoutDelta - 10 expectedPacket = channeltypesv2.NewPacket(1, path.EndpointA.ChannelID, path.EndpointB.ChannelID, timeoutTimestamp, payload) }, expError: nil, @@ -137,9 +137,17 @@ func (suite *KeeperTestSuite) TestMsgSendPacket() { name: "failure: timeout timestamp exceeds max allowed input", malleate: func() { // ensure message timeout exceeds max allowed input. - timeoutTimestamp = uint64(suite.chainA.GetContext().BlockTime().Unix()) + channeltypesv2.MaxTimeout + 10 + timeoutTimestamp = uint64(suite.chainA.GetContext().BlockTime().Unix()) + channeltypesv2.MaxTimeoutDelta + 10 }, - expError: channeltypesv2.ErrInvalidTimeout, + expError: channeltypesv2.ErrMaxTimeoutDeltaExceeded, + }, + { + name: "failure: timeout timestamp less than current block timestamp", + malleate: func() { + // ensure message timeout exceeds max allowed input. + timeoutTimestamp = uint64(suite.chainA.GetContext().BlockTime().Unix()) - 1 + }, + expError: channeltypesv2.ErrTimeoutTooLow, }, { name: "failure: inactive client", From a76ffa838733df56b99daee2d3d17aeff01db2db Mon Sep 17 00:00:00 2001 From: Stefano Angieri Date: Mon, 28 Oct 2024 15:11:23 +0100 Subject: [PATCH 7/8] errors --- modules/core/04-channel/v2/types/errors.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/core/04-channel/v2/types/errors.go b/modules/core/04-channel/v2/types/errors.go index 1cc831b1385..478440e3912 100644 --- a/modules/core/04-channel/v2/types/errors.go +++ b/modules/core/04-channel/v2/types/errors.go @@ -13,5 +13,6 @@ var ( ErrInvalidAcknowledgement = errorsmod.Register(SubModuleName, 7, "invalid acknowledgement") ErrPacketCommitmentNotFound = errorsmod.Register(SubModuleName, 8, "packet commitment not found") ErrAcknowledgementNotFound = errorsmod.Register(SubModuleName, 9, "packet acknowledgement not found") - ErrInvalidTimeout = errorsmod.Register(SubModuleName, 10, "timeout exceeds max allowed value") + ErrMaxTimeoutDeltaExceeded = errorsmod.Register(SubModuleName, 10, "timeoutTimestamp exceeds max allowed value") + ErrTimeoutTooLow = errorsmod.Register(SubModuleName, 11, "timeoutTimestamp is less than current block timestamp") ) From 45e7a0d25af045f87c940c254fddb8551fb97a6c Mon Sep 17 00:00:00 2001 From: Stefano Angieri Date: Thu, 31 Oct 2024 18:33:29 +0100 Subject: [PATCH 8/8] use time package --- modules/core/04-channel/v2/keeper/msg_server.go | 14 ++++++++------ .../core/04-channel/v2/keeper/msg_server_test.go | 5 +++-- modules/core/04-channel/v2/types/msgs.go | 4 +++- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/modules/core/04-channel/v2/keeper/msg_server.go b/modules/core/04-channel/v2/keeper/msg_server.go index 83a1b4a923a..883431f2fc8 100644 --- a/modules/core/04-channel/v2/keeper/msg_server.go +++ b/modules/core/04-channel/v2/keeper/msg_server.go @@ -3,6 +3,7 @@ package keeper import ( "context" "slices" + "time" errorsmod "cosmossdk.io/errors" @@ -71,16 +72,17 @@ func (k *Keeper) SendPacket(ctx context.Context, msg *channeltypesv2.MsgSendPack } // Note, the validate basic function in sendPacket does the timeoutTimestamp != 0 check and other stateless checks on the packet. - // timeoutTimestamp must be less than current block time + MaxTimeoutDelta - if msg.TimeoutTimestamp > uint64(sdkCtx.BlockTime().Unix())+channeltypesv2.MaxTimeoutDelta { - return nil, errorsmod.Wrap(channeltypesv2.ErrMaxTimeoutDeltaExceeded, "timeout exceeds the maximum expected value") - } - // timeoutTimestamp must be greater than current block time - if uint64(sdkCtx.BlockTime().Unix()) > msg.TimeoutTimestamp { + timeout := time.Unix(int64(msg.TimeoutTimestamp), 0) + if timeout.Before(sdkCtx.BlockTime()) { return nil, errorsmod.Wrap(channeltypesv2.ErrTimeoutTooLow, "timeout is less than the current block timestamp") } + // timeoutTimestamp must be less than current block time + MaxTimeoutDelta + if timeout.After(sdkCtx.BlockTime().Add(channeltypesv2.MaxTimeoutDelta)) { + return nil, errorsmod.Wrap(channeltypesv2.ErrMaxTimeoutDeltaExceeded, "timeout exceeds the maximum expected value") + } + signer, err := sdk.AccAddressFromBech32(msg.Signer) if err != nil { sdkCtx.Logger().Error("send packet failed", "error", errorsmod.Wrap(err, "invalid address for msg Signer")) diff --git a/modules/core/04-channel/v2/keeper/msg_server_test.go b/modules/core/04-channel/v2/keeper/msg_server_test.go index 9a46480688d..0d1b261512d 100644 --- a/modules/core/04-channel/v2/keeper/msg_server_test.go +++ b/modules/core/04-channel/v2/keeper/msg_server_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "time" sdk "github.com/cosmos/cosmos-sdk/types" @@ -120,7 +121,7 @@ func (suite *KeeperTestSuite) TestMsgSendPacket() { name: "success: valid timeout timestamp", malleate: func() { // ensure a message timeout. - timeoutTimestamp = uint64(suite.chainA.GetContext().BlockTime().Unix()) + channeltypesv2.MaxTimeoutDelta - 10 + timeoutTimestamp = uint64(suite.chainA.GetContext().BlockTime().Add(channeltypesv2.MaxTimeoutDelta - 10*time.Second).Unix()) expectedPacket = channeltypesv2.NewPacket(1, path.EndpointA.ChannelID, path.EndpointB.ChannelID, timeoutTimestamp, payload) }, expError: nil, @@ -137,7 +138,7 @@ func (suite *KeeperTestSuite) TestMsgSendPacket() { name: "failure: timeout timestamp exceeds max allowed input", malleate: func() { // ensure message timeout exceeds max allowed input. - timeoutTimestamp = uint64(suite.chainA.GetContext().BlockTime().Unix()) + channeltypesv2.MaxTimeoutDelta + 10 + timeoutTimestamp = uint64(suite.chainA.GetContext().BlockTime().Add(channeltypesv2.MaxTimeoutDelta + 10*time.Second).Unix()) }, expError: channeltypesv2.ErrMaxTimeoutDeltaExceeded, }, diff --git a/modules/core/04-channel/v2/types/msgs.go b/modules/core/04-channel/v2/types/msgs.go index 4c45b3e9d05..dfbcfee103b 100644 --- a/modules/core/04-channel/v2/types/msgs.go +++ b/modules/core/04-channel/v2/types/msgs.go @@ -1,6 +1,8 @@ package types import ( + "time" + errorsmod "cosmossdk.io/errors" sdk "github.com/cosmos/cosmos-sdk/types" @@ -13,7 +15,7 @@ import ( ibcerrors "github.com/cosmos/ibc-go/v9/modules/core/errors" ) -const MaxTimeoutDelta uint64 = 24 * 60 * 60 // 24h in seconds +const MaxTimeoutDelta time.Duration = 24 * time.Hour var ( _ sdk.Msg = (*MsgCreateChannel)(nil)