From 86251e1e0599d992da27ecae29c21414fd35bce6 Mon Sep 17 00:00:00 2001 From: javiersuweijie Date: Mon, 24 Jul 2023 14:20:59 +0800 Subject: [PATCH] feat: enforce intermediate forwarder --- middleware/packet-forward-middleware/Makefile | 1 + .../router/ibc_middleware.go | 6 +++ .../router/keeper/keeper.go | 7 +++ .../router/module_test.go | 54 +++++++++++++++++++ .../router/types/expected_keepers.go | 4 ++ .../test/mock/auth_keeper.go | 49 +++++++++++++++++ .../packet-forward-middleware/test/setup.go | 7 ++- 7 files changed, 127 insertions(+), 1 deletion(-) create mode 100644 middleware/packet-forward-middleware/test/mock/auth_keeper.go diff --git a/middleware/packet-forward-middleware/Makefile b/middleware/packet-forward-middleware/Makefile index 06a6bd5a..4266c60e 100644 --- a/middleware/packet-forward-middleware/Makefile +++ b/middleware/packet-forward-middleware/Makefile @@ -147,6 +147,7 @@ mocks: $(MOCKS_DIR) mockgen -package=mock -destination=./test/mock/bank_keeper.go $(GOMOD)/router/types BankKeeper mockgen -package=mock -destination=./test/mock/ics4_wrapper.go github.com/cosmos/ibc-go/v7/modules/core/05-port/types ICS4Wrapper mockgen -package=mock -destination=./test/mock/ibc_module.go github.com/cosmos/ibc-go/v7/modules/core/05-port/types IBCModule + mockgen -package=mock -destination=./test/mock/auth_keeper.go $(GOMOD)/router/types AuthKeeper .PHONY: mocks diff --git a/middleware/packet-forward-middleware/router/ibc_middleware.go b/middleware/packet-forward-middleware/router/ibc_middleware.go index 77508e09..dfd2b406 100644 --- a/middleware/packet-forward-middleware/router/ibc_middleware.go +++ b/middleware/packet-forward-middleware/router/ibc_middleware.go @@ -162,6 +162,12 @@ func (im IBCMiddleware) OnRecvPacket( im.keeper.Logger(ctx).Debug("packetForwardMiddleware OnRecvPacket forward metadata does not exist") return im.app.OnRecvPacket(ctx, packet, relayer) } + + forwarder := im.keeper.GetForwarderAddress(ctx) + if data.Receiver != forwarder.String() { + return channeltypes.NewErrorAcknowledgement(fmt.Errorf("packetForwardMiddleware invalid receiver address, expected %s, got %s", forwarder, data.Receiver)) + } + m := &types.PacketMetadata{} err = json.Unmarshal([]byte(data.Memo), m) if err != nil { diff --git a/middleware/packet-forward-middleware/router/keeper/keeper.go b/middleware/packet-forward-middleware/router/keeper/keeper.go index 3715ed9c..50b71e51 100644 --- a/middleware/packet-forward-middleware/router/keeper/keeper.go +++ b/middleware/packet-forward-middleware/router/keeper/keeper.go @@ -54,6 +54,7 @@ type Keeper struct { distrKeeper types.DistributionKeeper bankKeeper types.BankKeeper ics4Wrapper porttypes.ICS4Wrapper + authKeeper types.AuthKeeper } // NewKeeper creates a new forward Keeper instance @@ -66,6 +67,7 @@ func NewKeeper( distrKeeper types.DistributionKeeper, bankKeeper types.BankKeeper, ics4Wrapper porttypes.ICS4Wrapper, + authKeeper types.AuthKeeper, ) *Keeper { // set KeyTable if it has not already been set if !paramSpace.HasKeyTable() { @@ -81,6 +83,7 @@ func NewKeeper( distrKeeper: distrKeeper, bankKeeper: bankKeeper, ics4Wrapper: ics4Wrapper, + authKeeper: authKeeper, } } @@ -474,3 +477,7 @@ func (k *Keeper) GetAppVersion( func (k *Keeper) LookupModuleByChannel(ctx sdk.Context, portID, channelID string) (string, *capabilitytypes.Capability, error) { return k.channelKeeper.LookupModuleByChannel(ctx, portID, channelID) } + +func (k *Keeper) GetForwarderAddress(ctx sdk.Context) sdk.AccAddress { + return k.authKeeper.GetModuleAddress(types.ModuleName) +} diff --git a/middleware/packet-forward-middleware/router/module_test.go b/middleware/packet-forward-middleware/router/module_test.go index 82c49d04..9298589e 100644 --- a/middleware/packet-forward-middleware/router/module_test.go +++ b/middleware/packet-forward-middleware/router/module_test.go @@ -259,6 +259,8 @@ func TestOnRecvPacket_ForwardNoFee(t *testing.T) { // Expected mocks gomock.InOrder( + setup.Mocks.AuthKeeperMock.EXPECT().GetModuleAddress(types.ModuleName).Return(sdk.MustAccAddressFromBech32(hostAddr)), + setup.Mocks.IBCModuleMock.EXPECT().OnRecvPacket(ctx, packetOrig, senderAccAddr). Return(acknowledgement), @@ -326,6 +328,8 @@ func TestOnRecvPacket_ForwardAmountInt256(t *testing.T) { // Expected mocks gomock.InOrder( + setup.Mocks.AuthKeeperMock.EXPECT().GetModuleAddress(types.ModuleName).Return(sdk.MustAccAddressFromBech32(hostAddr)), + setup.Mocks.IBCModuleMock.EXPECT().OnRecvPacket(ctx, packetOrig, senderAccAddr). Return(acknowledgement), @@ -393,6 +397,8 @@ func TestOnRecvPacket_ForwardWithFee(t *testing.T) { // Expected mocks gomock.InOrder( + setup.Mocks.AuthKeeperMock.EXPECT().GetModuleAddress(types.ModuleName).Return(sdk.MustAccAddressFromBech32(hostAddr)), + setup.Mocks.IBCModuleMock.EXPECT().OnRecvPacket(ctx, packetOrig, senderAccAddr). Return(acknowledgement), @@ -504,6 +510,8 @@ func TestOnRecvPacket_ForwardMultihopStringNext(t *testing.T) { // Expected mocks gomock.InOrder( + setup.Mocks.AuthKeeperMock.EXPECT().GetModuleAddress(types.ModuleName).Return(sdk.MustAccAddressFromBech32(hostAddr)), + setup.Mocks.IBCModuleMock.EXPECT().OnRecvPacket(ctx, packetOrig, senderAccAddr). Return(acknowledgement), @@ -512,6 +520,8 @@ func TestOnRecvPacket_ForwardMultihopStringNext(t *testing.T) { msgTransfer1, ).Return(&transfertypes.MsgTransferResponse{Sequence: 0}, nil), + setup.Mocks.AuthKeeperMock.EXPECT().GetModuleAddress(types.ModuleName).Return(sdk.MustAccAddressFromBech32(hostAddr2)), + setup.Mocks.IBCModuleMock.EXPECT().OnRecvPacket(ctx, packet2, senderAccAddr2). Return(acknowledgement), @@ -623,6 +633,8 @@ func TestOnRecvPacket_ForwardMultihopJSONNext(t *testing.T) { // Expected mocks gomock.InOrder( + setup.Mocks.AuthKeeperMock.EXPECT().GetModuleAddress(types.ModuleName).Return(sdk.MustAccAddressFromBech32(hostAddr)), + setup.Mocks.IBCModuleMock.EXPECT().OnRecvPacket(ctx, packetOrig, senderAccAddr). Return(acknowledgement), @@ -631,6 +643,8 @@ func TestOnRecvPacket_ForwardMultihopJSONNext(t *testing.T) { msgTransfer1, ).Return(&transfertypes.MsgTransferResponse{Sequence: 0}, nil), + setup.Mocks.AuthKeeperMock.EXPECT().GetModuleAddress(types.ModuleName).Return(sdk.MustAccAddressFromBech32(hostAddr2)), + setup.Mocks.IBCModuleMock.EXPECT().OnRecvPacket(ctx, packet2, senderAccAddr2). Return(acknowledgement), @@ -662,3 +676,43 @@ func TestOnRecvPacket_ForwardMultihopJSONNext(t *testing.T) { err = forwardMiddleware.OnAcknowledgementPacket(ctx, packet2, successAck, senderAccAddr) require.NoError(t, err) } + +func TestOnRecvPacket_WrongHostAddr(t *testing.T) { + ctl := gomock.NewController(t) + defer ctl.Finish() + setup := test.NewTestSetup(t, ctl) + ctx := setup.Initializer.Ctx + cdc := setup.Initializer.Marshaler + forwardMiddleware := setup.ForwardMiddleware + + // Test data + const ( + hostAddr = "cosmos1vzxkv3lxccnttr9rs0002s93sgw72h7ghukuhs" + hostAddr2 = "cosmos1q4p4gx889lfek5augdurrjclwtqvjhuntm6j4m" + destAddr = "cosmos16plylpsgxechajltx9yeseqexzdzut9g8vla4k" + port = "transfer" + channel = "channel-0" + ) + senderAccAddr := test.AccAddress() + + packetOrig := transferPacket(t, hostAddr2, &types.PacketMetadata{ + Forward: &types.ForwardMetadata{ + Receiver: destAddr, + Port: port, + Channel: channel, + }, + }) + + // Expected mocks + gomock.InOrder( + setup.Mocks.AuthKeeperMock.EXPECT().GetModuleAddress(types.ModuleName).Return(sdk.MustAccAddressFromBech32(hostAddr)), + ) + + ack := forwardMiddleware.OnRecvPacket(ctx, packetOrig, senderAccAddr) + require.False(t, ack.Success()) + + expectedAck := &channeltypes.Acknowledgement{} + err := cdc.UnmarshalJSON(ack.Acknowledgement(), expectedAck) + require.NoError(t, err) + require.Equal(t, "ABCI code: 1: error handling packet: see events for details", expectedAck.GetError()) +} diff --git a/middleware/packet-forward-middleware/router/types/expected_keepers.go b/middleware/packet-forward-middleware/router/types/expected_keepers.go index b9bf9fb2..caa2a603 100644 --- a/middleware/packet-forward-middleware/router/types/expected_keepers.go +++ b/middleware/packet-forward-middleware/router/types/expected_keepers.go @@ -35,3 +35,7 @@ type BankKeeper interface { SendCoinsFromAccountToModule(ctx sdk.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins) error BurnCoins(ctx sdk.Context, moduleName string, amt sdk.Coins) error } + +type AuthKeeper interface { + GetModuleAddress(name string) sdk.AccAddress +} diff --git a/middleware/packet-forward-middleware/test/mock/auth_keeper.go b/middleware/packet-forward-middleware/test/mock/auth_keeper.go new file mode 100644 index 00000000..af4b815e --- /dev/null +++ b/middleware/packet-forward-middleware/test/mock/auth_keeper.go @@ -0,0 +1,49 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v7/router/types (interfaces: AuthKeeper) + +// Package mock is a generated GoMock package. +package mock + +import ( + reflect "reflect" + + types "github.com/cosmos/cosmos-sdk/types" + gomock "github.com/golang/mock/gomock" +) + +// MockAuthKeeper is a mock of AuthKeeper interface. +type MockAuthKeeper struct { + ctrl *gomock.Controller + recorder *MockAuthKeeperMockRecorder +} + +// MockAuthKeeperMockRecorder is the mock recorder for MockAuthKeeper. +type MockAuthKeeperMockRecorder struct { + mock *MockAuthKeeper +} + +// NewMockAuthKeeper creates a new mock instance. +func NewMockAuthKeeper(ctrl *gomock.Controller) *MockAuthKeeper { + mock := &MockAuthKeeper{ctrl: ctrl} + mock.recorder = &MockAuthKeeperMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockAuthKeeper) EXPECT() *MockAuthKeeperMockRecorder { + return m.recorder +} + +// GetModuleAddress mocks base method. +func (m *MockAuthKeeper) GetModuleAddress(arg0 string) types.AccAddress { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetModuleAddress", arg0) + ret0, _ := ret[0].(types.AccAddress) + return ret0 +} + +// GetModuleAddress indicates an expected call of GetModuleAddress. +func (mr *MockAuthKeeperMockRecorder) GetModuleAddress(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetModuleAddress", reflect.TypeOf((*MockAuthKeeper)(nil).GetModuleAddress), arg0) +} diff --git a/middleware/packet-forward-middleware/test/setup.go b/middleware/packet-forward-middleware/test/setup.go index 96876049..054292b5 100644 --- a/middleware/packet-forward-middleware/test/setup.go +++ b/middleware/packet-forward-middleware/test/setup.go @@ -36,9 +36,10 @@ func NewTestSetup(t *testing.T, ctl *gomock.Controller) *Setup { bankKeeperMock := mock.NewMockBankKeeper(ctl) ibcModuleMock := mock.NewMockIBCModule(ctl) ics4WrapperMock := mock.NewMockICS4Wrapper(ctl) + authKeeperMock := mock.NewMockAuthKeeper(ctl) paramsKeeper := initializer.paramsKeeper() - routerKeeper := initializer.routerKeeper(paramsKeeper, transferKeeperMock, channelKeeperMock, distributionKeeperMock, bankKeeperMock, ics4WrapperMock) + routerKeeper := initializer.routerKeeper(paramsKeeper, transferKeeperMock, channelKeeperMock, distributionKeeperMock, bankKeeperMock, ics4WrapperMock, authKeeperMock) // routerModule := initializer.routerModule(routerKeeper) require.NoError(t, initializer.StateStore.LoadLatestVersion()) @@ -58,6 +59,7 @@ func NewTestSetup(t *testing.T, ctl *gomock.Controller) *Setup { DistributionKeeperMock: distributionKeeperMock, IBCModuleMock: ibcModuleMock, ICS4WrapperMock: ics4WrapperMock, + AuthKeeperMock: authKeeperMock, }, ForwardMiddleware: initializer.forwardMiddleware(ibcModuleMock, routerKeeper, 0, keeper.DefaultForwardTransferPacketTimeoutTimestamp, keeper.DefaultRefundTransferPacketTimeoutTimestamp), @@ -83,6 +85,7 @@ type testMocks struct { DistributionKeeperMock *mock.MockDistributionKeeper IBCModuleMock *mock.MockIBCModule ICS4WrapperMock *mock.MockICS4Wrapper + AuthKeeperMock *mock.MockAuthKeeper } type initializer struct { @@ -133,6 +136,7 @@ func (i initializer) routerKeeper( distributionKeeper types.DistributionKeeper, bankKeeper types.BankKeeper, ics4Wrapper porttypes.ICS4Wrapper, + authKeeper types.AuthKeeper, ) *keeper.Keeper { storeKey := sdk.NewKVStoreKey(types.StoreKey) i.StateStore.MountStoreWithDB(storeKey, storetypes.StoreTypeIAVL, i.DB) @@ -147,6 +151,7 @@ func (i initializer) routerKeeper( distributionKeeper, bankKeeper, ics4Wrapper, + authKeeper, ) return routerKeeper