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.

Sign up for GitHub

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

Harden ICS20 Eureka Module #7908

Merged
merged 7 commits into from
Feb 5, 2025
Merged

Harden ICS20 Eureka Module #7908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
632b67c
enforce strict portID check in eureka module
AdityaSripal Feb 4, 2025
46180b6
make base denom check
AdityaSripal Feb 4, 2025
ccc971e
fix comments
AdityaSripal Feb 4, 2025
f5ae10a
lint
AdityaSripal Feb 4, 2025
c022302
lint moar
AdityaSripal Feb 4, 2025
d19ae88
one last time
AdityaSripal Feb 4, 2025
9dbd153
Merge branch 'feat/ibc-eureka' into aditya/transfer-eureka-harden
gjermundgaraba Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 54 additions & 21 deletions modules/apps/transfer/v2/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,16 @@ package v2
import (
"context"
"fmt"
"strings"

errorsmod "cosmossdk.io/errors"

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/v9/modules/apps/transfer/keeper"
transfertypes "github.com/cosmos/ibc-go/v9/modules/apps/transfer/types"
"github.com/cosmos/ibc-go/v9/modules/apps/transfer/types"
channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types"
"github.com/cosmos/ibc-go/v9/modules/core/04-channel/v2/types"
channeltypesv2 "github.com/cosmos/ibc-go/v9/modules/core/04-channel/v2/types"
"github.com/cosmos/ibc-go/v9/modules/core/api"
ibcerrors "github.com/cosmos/ibc-go/v9/modules/core/errors"
)
Expand All @@ -29,8 +30,16 @@ type IBCModule struct {
keeper keeper.Keeper
}

func (im *IBCModule) OnSendPacket(goCtx context.Context, sourceChannel string, destinationChannel string, sequence uint64, payload types.Payload, signer sdk.AccAddress) error {
data, err := transfertypes.UnmarshalPacketData(payload.Value, payload.Version, payload.Encoding)
func (im *IBCModule) OnSendPacket(goCtx context.Context, sourceChannel string, destinationChannel string, sequence uint64, payload channeltypesv2.Payload, signer sdk.AccAddress) error {
// Enforce that the source and destination portIDs are the same and equal to the transfer portID
// This is necessary for IBC Eureka since the portIDs (and thus the application-application connection) is not prenegotiated
// by the channel handshake
// This restriction can be removed in a future where the trace hop on receive commits to **both** the source and destination portIDs
// rather than just the destination port
if payload.SourcePort != types.PortID || payload.DestinationPort != types.PortID {
return errorsmod.Wrapf(channeltypesv2.ErrInvalidPacket, "payload port ID is invalid: expected %s, got sourcePort: %s destPort: %s", types.PortID, payload.SourcePort, payload.DestinationPort)
}
data, err := types.UnmarshalPacketData(payload.Value, payload.Version, payload.Encoding)
if err != nil {
return err
}
Expand All @@ -44,6 +53,19 @@ func (im *IBCModule) OnSendPacket(goCtx context.Context, sourceChannel string, d
return errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "sender %s is different from signer %s", sender, signer)
}

// Enforce that the base denom does not contain any slashes
// Since IBC v2 packets will no longer have channel identifiers, we cannot rely
// on the channel format to easily divide the trace from the base denomination in ICS20 v1 packets
// The simplest way to prevent any potential issues from arising is to simply disallow any slashes in the base denomination
// This prevents such denominations from being sent with IBCV v2 packets, however we can still support them in IBC v1 packets
// If we enforce that IBC v2 packets are sent with ICS20 v2 and above versions that separate the trace from the base denomination
// in the packet data, then we can remove this restriction.
for _, token := range data.Tokens {
if strings.Contains(token.Denom.Base, "/") {
return errorsmod.Wrapf(types.ErrInvalidDenomForTransfer, "base denomination %s cannot contain slashes for IBC v2 packet", token.Denom.Base)
}
}

if err := im.keeper.SendTransfer(goCtx, payload.SourcePort, sourceChannel, data.Tokens, signer); err != nil {
return err
}
Expand All @@ -57,31 +79,42 @@ func (im *IBCModule) OnSendPacket(goCtx context.Context, sourceChannel string, d
return nil
}

func (im *IBCModule) OnRecvPacket(ctx context.Context, sourceChannel string, destinationChannel string, sequence uint64, payload types.Payload, relayer sdk.AccAddress) types.RecvPacketResult {
func (im *IBCModule) OnRecvPacket(ctx context.Context, sourceChannel string, destinationChannel string, sequence uint64, payload channeltypesv2.Payload, relayer sdk.AccAddress) channeltypesv2.RecvPacketResult {
// Enforce that the source and destination portIDs are the same and equal to the transfer portID
// This is necessary for IBC Eureka since the portIDs (and thus the application-application connection) is not prenegotiated
// by the channel handshake
// This restriction can be removed in a future where the trace hop on receive commits to **both** the source and destination portIDs
// rather than just the destination port
if payload.SourcePort != types.PortID || payload.DestinationPort != types.PortID {
return channeltypesv2.RecvPacketResult{
Status: channeltypesv2.PacketStatus_Failure,
Acknowledgement: channeltypes.NewErrorAcknowledgement(errorsmod.Wrapf(channeltypesv2.ErrInvalidPacket, "payload port ID is invalid: expected %s, got sourcePort: %s destPort: %s", types.PortID, payload.SourcePort, payload.DestinationPort)).Acknowledgement(),
}
}
var (
ackErr error
data transfertypes.FungibleTokenPacketDataV2
data types.FungibleTokenPacketDataV2
)

ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
recvResult := types.RecvPacketResult{
Status: types.PacketStatus_Success,
recvResult := channeltypesv2.RecvPacketResult{
Status: channeltypesv2.PacketStatus_Success,
Acknowledgement: ack.Acknowledgement(),
}
// we are explicitly wrapping this emit event call in an anonymous function so that
// the packet data is evaluated after it has been assigned a value.
defer func() {
if err := im.keeper.EmitOnRecvPacketEvent(ctx, data, ack, ackErr); err != nil {
im.keeper.Logger.Error(fmt.Sprintf("failed to emit %T event", types.EventTypeRecvPacket), "error", err)
im.keeper.Logger.Error(fmt.Sprintf("failed to emit %T event", channeltypesv2.EventTypeRecvPacket), "error", err)
}
}()

data, ackErr = transfertypes.UnmarshalPacketData(payload.Value, payload.Version, payload.Encoding)
data, ackErr = types.UnmarshalPacketData(payload.Value, payload.Version, payload.Encoding)
if ackErr != nil {
ack = channeltypes.NewErrorAcknowledgement(ackErr)
im.keeper.Logger.Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), sequence))
return types.RecvPacketResult{
Status: types.PacketStatus_Failure,
return channeltypesv2.RecvPacketResult{
Status: channeltypesv2.PacketStatus_Failure,
Acknowledgement: ack.Acknowledgement(),
}
}
Expand All @@ -96,8 +129,8 @@ func (im *IBCModule) OnRecvPacket(ctx context.Context, sourceChannel string, des
); ackErr != nil {
ack = channeltypes.NewErrorAcknowledgement(ackErr)
im.keeper.Logger.Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), sequence))
return types.RecvPacketResult{
Status: types.PacketStatus_Failure,
return channeltypesv2.RecvPacketResult{
Status: channeltypesv2.PacketStatus_Failure,
Acknowledgement: ack.Acknowledgement(),
}
}
Expand All @@ -110,8 +143,8 @@ func (im *IBCModule) OnRecvPacket(ctx context.Context, sourceChannel string, des
if data.HasForwarding() {
// we are now sending from the forward escrow address to the final receiver address.
ack = channeltypes.NewErrorAcknowledgement(fmt.Errorf("forwarding not yet supported"))
return types.RecvPacketResult{
Status: types.PacketStatus_Failure,
return channeltypesv2.RecvPacketResult{
Status: channeltypesv2.PacketStatus_Failure,
Acknowledgement: ack.Acknowledgement(),
}
// TODO: handle forwarding
Expand All @@ -130,8 +163,8 @@ func (im *IBCModule) OnRecvPacket(ctx context.Context, sourceChannel string, des
return recvResult
}

func (im *IBCModule) OnTimeoutPacket(ctx context.Context, sourceChannel string, destinationChannel string, sequence uint64, payload types.Payload, relayer sdk.AccAddress) error {
data, err := transfertypes.UnmarshalPacketData(payload.Value, payload.Version, payload.Encoding)
func (im *IBCModule) OnTimeoutPacket(ctx context.Context, sourceChannel string, destinationChannel string, sequence uint64, payload channeltypesv2.Payload, relayer sdk.AccAddress) error {
data, err := types.UnmarshalPacketData(payload.Value, payload.Version, payload.Encoding)
if err != nil {
return err
}
Expand All @@ -146,13 +179,13 @@ func (im *IBCModule) OnTimeoutPacket(ctx context.Context, sourceChannel string,
return im.keeper.EmitOnTimeoutEvent(ctx, data)
}

func (im *IBCModule) OnAcknowledgementPacket(ctx context.Context, sourceChannel string, destinationChannel string, sequence uint64, acknowledgement []byte, payload types.Payload, relayer sdk.AccAddress) error {
func (im *IBCModule) OnAcknowledgementPacket(ctx context.Context, sourceChannel string, destinationChannel string, sequence uint64, acknowledgement []byte, payload channeltypesv2.Payload, relayer sdk.AccAddress) error {
var ack channeltypes.Acknowledgement
if err := transfertypes.ModuleCdc.UnmarshalJSON(acknowledgement, &ack); err != nil {
if err := types.ModuleCdc.UnmarshalJSON(acknowledgement, &ack); err != nil {
return errorsmod.Wrapf(ibcerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet acknowledgement: %v", err)
}

data, err := transfertypes.UnmarshalPacketData(payload.Value, payload.Version, payload.Encoding)
data, err := types.UnmarshalPacketData(payload.Value, payload.Version, payload.Encoding)
if err != nil {
return err
}
Expand Down
139 changes: 102 additions & 37 deletions modules/apps/transfer/v2/ibc_module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

testifysuite "github.com/stretchr/testify/suite"

errorsmod "cosmossdk.io/errors"
sdkmath "cosmossdk.io/math"

sdk "github.com/cosmos/cosmos-sdk/types"
Expand All @@ -30,6 +31,8 @@ type TransferTestSuite struct {
pathBToC *ibctesting.Path
}

const invalidPortID = "invalidportid"

func (suite *TransferTestSuite) SetupTest() {
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 3)
suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1))
Expand Down Expand Up @@ -57,17 +60,46 @@ func TestTransferTestSuite(t *testing.T) {
}

func (suite *TransferTestSuite) TestOnSendPacket() {
var payload channeltypesv2.Payload
testCases := []struct {
name string
sourceDenomsToTransfer []string
malleate func()
expError error
}{
{
"transfer single denom",
[]string{sdk.DefaultBondDenom},
func() {},
nil,
},
{
"transfer multiple denoms",
[]string{sdk.DefaultBondDenom, ibctesting.SecondaryDenom},
func() {},
nil,
},
{
"transfer with invalid source port",
[]string{sdk.DefaultBondDenom},
func() {
payload.SourcePort = invalidPortID
},
channeltypesv2.ErrInvalidPacket,
},
{
"transfer with invalid destination port",
[]string{sdk.DefaultBondDenom},
func() {
payload.DestinationPort = invalidPortID
},
channeltypesv2.ErrInvalidPacket,
},
{
"transfer with slashes in base denom",
[]string{"base/coin"},
func() {},
types.ErrInvalidDenomForTransfer,
},
}

Expand All @@ -81,8 +113,6 @@ func (suite *TransferTestSuite) TestOnSendPacket() {
originalBalances = originalBalances.Add(originalBalance)
}

timeoutTimestamp := uint64(suite.chainB.GetContext().BlockTime().Add(time.Hour).Unix())

amount, ok := sdkmath.NewIntFromString("9223372036854775808") // 2^63 (one above int64)
suite.Require().True(ok)
originalCoins := sdk.NewCoins()
Expand All @@ -107,49 +137,75 @@ func (suite *TransferTestSuite) TestOnSendPacket() {
types.ForwardingPacketData{},
)
bz := suite.chainA.Codec.MustMarshal(&transferData)
payload := channeltypesv2.NewPayload(
payload = channeltypesv2.NewPayload(
types.PortID, types.PortID, types.V2,
types.EncodingProtobuf, bz,
)
msg := channeltypesv2.NewMsgSendPacket(
suite.pathAToB.EndpointA.ClientID,
timeoutTimestamp,
suite.chainA.SenderAccount.GetAddress().String(),
payload,
)

_, err := suite.chainA.SendMsgs(msg)
suite.Require().NoError(err) // message committed
// malleate payload
tc.malleate()

// TODO parse packet from result events and check against expected events
ctx := suite.chainA.GetContext()
cbs := suite.chainA.App.GetIBCKeeper().ChannelKeeperV2.Router.Route(ibctesting.TransferPort)

escrowAddress := types.GetEscrowAddress(types.PortID, suite.pathAToB.EndpointA.ClientID)
for _, coin := range originalCoins {
// check that the balance for chainA is updated
chainABalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), coin.Denom)
suite.Require().Equal(originalBalances.AmountOf(coin.Denom).Sub(amount).Int64(), chainABalance.Amount.Int64())
err := cbs.OnSendPacket(ctx, suite.pathAToB.EndpointA.ClientID, suite.pathAToB.EndpointB.ClientID, 1, payload, suite.chainA.SenderAccount.GetAddress())

// check that module account escrow address has locked the tokens
chainAEscrowBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), escrowAddress, coin.Denom)
suite.Require().Equal(coin, chainAEscrowBalance)
if tc.expError != nil {
suite.Require().Contains(err.Error(), tc.expError.Error())
} else {
suite.Require().NoError(err)

escrowAddress := types.GetEscrowAddress(types.PortID, suite.pathAToB.EndpointA.ClientID)
for _, coin := range originalCoins {
// check that the balance for chainA is updated
chainABalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), coin.Denom)
suite.Require().Equal(originalBalances.AmountOf(coin.Denom).Sub(amount).Int64(), chainABalance.Amount.Int64())

// check that module account escrow address has locked the tokens
chainAEscrowBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), escrowAddress, coin.Denom)
suite.Require().Equal(coin, chainAEscrowBalance)

}
}
})
}
}

func (suite *TransferTestSuite) TestOnRecvPacket() {
var payload channeltypesv2.Payload
testCases := []struct {
name string
sourceDenomsToTransfer []string
malleate func()
expErrAck []byte
}{
{
"transfer single denom",
[]string{sdk.DefaultBondDenom},
func() {},
nil,
},
{
"transfer multiple denoms",
[]string{sdk.DefaultBondDenom, ibctesting.SecondaryDenom},
func() {},
nil,
},
{
"transfer with invalid source port",
[]string{sdk.DefaultBondDenom},
func() {
payload.SourcePort = invalidPortID
},
channeltypes.NewErrorAcknowledgement(errorsmod.Wrapf(channeltypesv2.ErrInvalidPacket, "payload port ID is invalid: expected %s, got sourcePort: %s destPort: %s", types.PortID, payload.SourcePort, payload.DestinationPort)).Acknowledgement(),
},
{
"transfer with invalid dest port",
[]string{sdk.DefaultBondDenom},
func() {
payload.DestinationPort = invalidPortID
},
channeltypes.NewErrorAcknowledgement(errorsmod.Wrapf(channeltypesv2.ErrInvalidPacket, "payload port ID is invalid: expected %s, got sourcePort: %s destPort: %s", types.PortID, payload.SourcePort, payload.DestinationPort)).Acknowledgement(),
},
}

Expand Down Expand Up @@ -189,7 +245,7 @@ func (suite *TransferTestSuite) TestOnRecvPacket() {
types.ForwardingPacketData{},
)
bz := suite.chainA.Codec.MustMarshal(&transferData)
payload := channeltypesv2.NewPayload(
payload = channeltypesv2.NewPayload(
types.PortID, types.PortID, types.V2,
types.EncodingProtobuf, bz,
)
Expand All @@ -206,32 +262,41 @@ func (suite *TransferTestSuite) TestOnRecvPacket() {
ctx := suite.chainB.GetContext()
cbs := suite.chainB.App.GetIBCKeeper().ChannelKeeperV2.Router.Route(ibctesting.TransferPort)

// malleate payload after it has been sent but before OnRecvPacket callback is called
tc.malleate()

recvResult := cbs.OnRecvPacket(
ctx, suite.pathAToB.EndpointA.ClientID, suite.pathAToB.EndpointB.ClientID,
1, payload, suite.chainB.SenderAccount.GetAddress(),
)

suite.Require().Equal(channeltypesv2.PacketStatus_Success, recvResult.Status)
suite.Require().Equal(channeltypes.NewResultAcknowledgement([]byte{byte(1)}).Acknowledgement(), recvResult.Acknowledgement)
if tc.expErrAck == nil {

escrowAddress := types.GetEscrowAddress(types.PortID, suite.pathAToB.EndpointA.ClientID)
for _, coin := range originalCoins {
// check that the balance for chainA is updated
chainABalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), coin.Denom)
suite.Require().Equal(originalBalances.AmountOf(coin.Denom).Sub(amount).Int64(), chainABalance.Amount.Int64())
suite.Require().Equal(channeltypesv2.PacketStatus_Success, recvResult.Status)
suite.Require().Equal(channeltypes.NewResultAcknowledgement([]byte{byte(1)}).Acknowledgement(), recvResult.Acknowledgement)

// check that module account escrow address has locked the tokens
chainAEscrowBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), escrowAddress, coin.Denom)
suite.Require().Equal(coin, chainAEscrowBalance)
escrowAddress := types.GetEscrowAddress(types.PortID, suite.pathAToB.EndpointA.ClientID)
for _, coin := range originalCoins {
// check that the balance for chainA is updated
chainABalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), coin.Denom)
suite.Require().Equal(originalBalances.AmountOf(coin.Denom).Sub(amount).Int64(), chainABalance.Amount.Int64())

// check that module account escrow address has locked the tokens
chainAEscrowBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), escrowAddress, coin.Denom)
suite.Require().Equal(coin, chainAEscrowBalance)

traceAToB := types.NewHop(types.PortID, suite.pathAToB.EndpointB.ClientID)
traceAToB := types.NewHop(types.PortID, suite.pathAToB.EndpointB.ClientID)

// check that voucher exists on chain B
chainBDenom := types.NewDenom(coin.Denom, traceAToB)
chainBBalance := suite.chainB.GetSimApp().BankKeeper.GetBalance(suite.chainB.GetContext(), suite.chainB.SenderAccount.GetAddress(), chainBDenom.IBCDenom())
coinSentFromAToB := sdk.NewCoin(chainBDenom.IBCDenom(), amount)
suite.Require().Equal(coinSentFromAToB, chainBBalance)
// check that voucher exists on chain B
chainBDenom := types.NewDenom(coin.Denom, traceAToB)
chainBBalance := suite.chainB.GetSimApp().BankKeeper.GetBalance(suite.chainB.GetContext(), suite.chainB.SenderAccount.GetAddress(), chainBDenom.IBCDenom())
coinSentFromAToB := sdk.NewCoin(chainBDenom.IBCDenom(), amount)
suite.Require().Equal(coinSentFromAToB, chainBBalance)

}
} else {
suite.Require().Equal(channeltypesv2.PacketStatus_Failure, recvResult.Status)
suite.Require().Equal(tc.expErrAck, recvResult.Acknowledgement)
}
})
}
Expand Down
Loading