Skip to content

Commit 91519e4

Browse files
yihuangvladjdkaljo242
authored
fix: ibc middleware verify sender address incorrectly (#769)
* Problem: ibc middleware verify sender address incorrectly * Update CHANGELOG.md Signed-off-by: yihuang <[email protected]> * remove sendToSelf check * fix test build * fix test * Fix test * fix test --------- Signed-off-by: yihuang <[email protected]> Co-authored-by: Vlad J <[email protected]> Co-authored-by: Alex | Cosmos Labs <[email protected]>
1 parent 5262e65 commit 91519e4

File tree

7 files changed

+18
-19
lines changed

7 files changed

+18
-19
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
### BUG FIXES
1414

1515
- [\#748](https://github.com/cosmos/evm/pull/748) Fix DynamicFeeChecker in Cosmos ante handler to respect NoBaseFee feemarkets' parameter.
16+
- [\#769](https://github.com/cosmos/evm/pull/769) Fix erc20 ibc middleware to not to validate sender address format.
1617
- [\#756](https://github.com/cosmos/evm/pull/756) Fix error message typo in NewMsgCancelProposal.
1718
- [\#772](https://github.com/cosmos/evm/pull/772) Avoid panic on close if evm mempool not used.
1819
- [\#774](https://github.com/cosmos/evm/pull/774) Emit proper allowance amount in erc20 event.

‎tests/integration/x/erc20/test_ibc_callback.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -102,28 +102,28 @@ func (s *KeeperTestSuite) TestOnRecvPacketRegistered() {
102102
expCoins: coins,
103103
},
104104
{
105-
name: "error - invalid sender (no '1')",
105+
name: "success - invalid sender (no '1')",
106106
malleate: func() {
107107
transfer := transfertypes.NewFungibleTokenPacketData(registeredDenom, "100", "evmos", ethsecpAddrCosmos, "")
108108
bz := transfertypes.ModuleCdc.MustMarshalJSON(&transfer)
109109
packet = channeltypes.NewPacket(bz, 100, transfertypes.PortID, sourceChannel, transfertypes.PortID, cosmosEVMChannel, timeoutHeight, 0)
110110
},
111111
receiver: secpAddr,
112-
ackSuccess: false,
113-
checkBalances: false,
112+
ackSuccess: true,
113+
checkBalances: true,
114114
expErc20s: big.NewInt(0),
115115
expCoins: coins,
116116
},
117117
{
118-
name: "error - invalid sender (bad address)",
118+
name: "success - invalid sender (bad address)",
119119
malleate: func() {
120120
transfer := transfertypes.NewFungibleTokenPacketData(registeredDenom, "100", "badba1sv9m0g7ycejwr3s369km58h5qe7xj77hvcxrms", ethsecpAddrCosmos, "")
121121
bz := transfertypes.ModuleCdc.MustMarshalJSON(&transfer)
122122
packet = channeltypes.NewPacket(bz, 100, transfertypes.PortID, sourceChannel, transfertypes.PortID, cosmosEVMChannel, timeoutHeight, 0)
123123
},
124124
receiver: secpAddr,
125-
ackSuccess: false,
126-
checkBalances: false,
125+
ackSuccess: true,
126+
checkBalances: true,
127127
expErc20s: big.NewInt(0),
128128
expCoins: coins,
129129
},

‎tests/integration/x/erc20/test_mint.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111

1212
func (s *KeeperTestSuite) TestMintingEnabled() {
1313
var ctx sdk.Context
14-
sender := sdk.AccAddress(utiltx.GenerateAddress().Bytes())
1514
receiver := sdk.AccAddress(utiltx.GenerateAddress().Bytes())
1615
expPair := types.NewTokenPair(utiltx.GenerateAddress(), "coin", types.OWNER_MODULE)
1716
id := expPair.GetID()
@@ -97,7 +96,7 @@ func (s *KeeperTestSuite) TestMintingEnabled() {
9796

9897
tc.malleate()
9998

100-
pair, err := s.network.App.GetErc20Keeper().MintingEnabled(ctx, sender, receiver, expPair.Erc20Address)
99+
pair, err := s.network.App.GetErc20Keeper().MintingEnabled(ctx, receiver, expPair.Erc20Address)
101100
if tc.expPass {
102101
s.Require().NoError(err)
103102
s.Require().Equal(expPair, pair)

‎tests/integration/x/erc20/test_msg_server.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,7 @@ func (s *KeeperTestSuite) TestConvertERC20NativeERC20() {
261261
mockBankKeeper.EXPECT().SendCoinsFromModuleToAccount(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("failed to unescrow")).AnyTimes()
262262
mockBankKeeper.EXPECT().BlockedAddr(gomock.Any()).Return(false).AnyTimes()
263263
mockBankKeeper.EXPECT().GetBalance(gomock.Any(), gomock.Any(), gomock.Any()).Return(sdk.Coin{Denom: "coin", Amount: math.OneInt()}).AnyTimes()
264+
mockBankKeeper.EXPECT().IsSendEnabledCoin(gomock.Any(), gomock.Any()).Return(true).AnyTimes()
264265
},
265266
contractMinterBurner,
266267
false,
@@ -287,6 +288,7 @@ func (s *KeeperTestSuite) TestConvertERC20NativeERC20() {
287288
mockBankKeeper.EXPECT().SendCoinsFromModuleToAccount(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("failed to unescrow"))
288289
mockBankKeeper.EXPECT().BlockedAddr(gomock.Any()).Return(false)
289290
mockBankKeeper.EXPECT().GetBalance(gomock.Any(), gomock.Any(), gomock.Any()).Return(sdk.Coin{Denom: "coin", Amount: math.OneInt()})
291+
mockBankKeeper.EXPECT().IsSendEnabledCoin(gomock.Any(), gomock.Any()).Return(true).AnyTimes()
290292
},
291293
contractMinterBurner,
292294
false,
@@ -313,6 +315,7 @@ func (s *KeeperTestSuite) TestConvertERC20NativeERC20() {
313315
mockBankKeeper.EXPECT().SendCoinsFromModuleToAccount(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
314316
mockBankKeeper.EXPECT().BlockedAddr(gomock.Any()).Return(false)
315317
mockBankKeeper.EXPECT().GetBalance(gomock.Any(), gomock.Any(), gomock.Any()).Return(sdk.Coin{Denom: coinName, Amount: math.OneInt()}).AnyTimes()
318+
mockBankKeeper.EXPECT().IsSendEnabledCoin(gomock.Any(), gomock.Any()).Return(true).AnyTimes()
316319
},
317320
contractMinterBurner,
318321
false,
@@ -568,6 +571,7 @@ func (s *KeeperTestSuite) TestConvertNativeERC20ToEVMERC20() {
568571
mockBankKeeper.EXPECT().SendCoinsFromAccountToModule(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("failed to unescrow")).AnyTimes()
569572
mockBankKeeper.EXPECT().BlockedAddr(gomock.Any()).Return(false).AnyTimes()
570573
mockBankKeeper.EXPECT().GetBalance(gomock.Any(), gomock.Any(), gomock.Any()).Return(sdk.Coin{Denom: "coin", Amount: math.OneInt()}).AnyTimes()
574+
mockBankKeeper.EXPECT().IsSendEnabledCoin(gomock.Any(), gomock.Any()).Return(true).AnyTimes()
571575
},
572576
contractMinterBurner,
573577
false,
@@ -593,6 +597,7 @@ func (s *KeeperTestSuite) TestConvertNativeERC20ToEVMERC20() {
593597
mockBankKeeper.EXPECT().SendCoinsFromAccountToModule(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
594598
mockBankKeeper.EXPECT().BurnCoins(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("failed to burn")).AnyTimes()
595599
mockBankKeeper.EXPECT().BlockedAddr(gomock.Any()).Return(false)
600+
mockBankKeeper.EXPECT().IsSendEnabledCoin(gomock.Any(), gomock.Any()).Return(true).AnyTimes()
596601
},
597602
contractMinterBurner,
598603
false,

‎x/erc20/keeper/ibc_callbacks.go‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -62,12 +62,6 @@ func (k Keeper) OnRecvPacket(
6262
}
6363
recipient := sdk.AccAddress(recipientBz)
6464

65-
senderBz, err := k.addrCodec.StringToBytes(data.Sender)
66-
if err != nil {
67-
return channeltypes.NewErrorAcknowledgement(errorsmod.Wrap(err, "invalid sender"))
68-
}
69-
sender := sdk.AccAddress(senderBz)
70-
7165
receiverAcc := k.accountKeeper.GetAccount(ctx, recipient)
7266

7367
// return acknowledgement without conversion if receiver is a module account
@@ -128,7 +122,7 @@ func (k Keeper) OnRecvPacket(
128122
return ack
129123
}
130124

131-
pair, err := k.MintingEnabled(ctx, sender, recipient, coin.Denom)
125+
pair, err := k.MintingEnabled(ctx, recipient, coin.Denom)
132126
if err != nil {
133127
ctx.EventManager().EmitEvent(
134128
sdk.NewEvent("erc20_callback_failure",

‎x/erc20/keeper/mint.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import (
1717
// - bank module transfers are enabled for the Cosmos coin
1818
func (k Keeper) MintingEnabled(
1919
ctx sdk.Context,
20-
sender, receiver sdk.AccAddress,
20+
receiver sdk.AccAddress,
2121
token string,
2222
) (types.TokenPair, error) {
2323
if !k.IsERC20Enabled(ctx) {
@@ -57,7 +57,7 @@ func (k Keeper) MintingEnabled(
5757

5858
// check if minting to a recipient address other than the sender is enabled
5959
// for for the given coin denom
60-
if !sender.Equals(receiver) && !k.bankKeeper.IsSendEnabledCoin(ctx, coin) {
60+
if !k.bankKeeper.IsSendEnabledCoin(ctx, coin) {
6161
return types.TokenPair{}, errorsmod.Wrapf(
6262
banktypes.ErrSendDisabled, "minting '%s' coins to an external address is currently disabled", token,
6363
)

‎x/erc20/keeper/msg_server.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ func (k Keeper) ConvertERC20(
3333
receiver := sdk.MustAccAddressFromBech32(msg.Receiver)
3434
sender := common.HexToAddress(msg.Sender)
3535

36-
pair, err := k.MintingEnabled(ctx, sender.Bytes(), receiver, msg.ContractAddress)
36+
pair, err := k.MintingEnabled(ctx, receiver, msg.ContractAddress)
3737
if err != nil {
3838
return nil, err
3939
}
@@ -199,7 +199,7 @@ func (k Keeper) ConvertCoin(
199199
sender := sdk.MustAccAddressFromBech32(msg.Sender)
200200
receiver := common.HexToAddress(msg.Receiver)
201201

202-
pair, err := k.MintingEnabled(ctx, sender, receiver.Bytes(), msg.Coin.Denom)
202+
pair, err := k.MintingEnabled(ctx, receiver.Bytes(), msg.Coin.Denom)
203203
if err != nil {
204204
return nil, err
205205
}

0 commit comments

Comments
 (0)