From 574c7d2d7f19bda04aa58226634870323521f17d Mon Sep 17 00:00:00 2001 From: Simon Warta <2603011+webmaster128@users.noreply.github.com> Date: Mon, 17 Aug 2020 16:07:27 +0200 Subject: [PATCH] Merge PR #7053: Add address length validation to MsgSend and MsgMultiSend --- x/bank/keeper/keeper_test.go | 18 +++--- x/bank/types/msgs.go | 16 +++--- x/bank/types/msgs_test.go | 103 +++++++++++++++++++---------------- 3 files changed, 73 insertions(+), 64 deletions(-) diff --git a/x/bank/keeper/keeper_test.go b/x/bank/keeper/keeper_test.go index 8cc1b126ec..7bdd47d59b 100644 --- a/x/bank/keeper/keeper_test.go +++ b/x/bank/keeper/keeper_test.go @@ -297,7 +297,7 @@ func (suite *IntegrationTestSuite) TestInputOutputNewAccount() { app, ctx := suite.app, suite.ctx balances := sdk.NewCoins(newFooCoin(100), newBarCoin(50)) - addr1 := sdk.AccAddress([]byte("addr1")) + addr1 := sdk.AccAddress([]byte("addr1_______________")) acc1 := app.AccountKeeper.NewAccountWithAddress(ctx, addr1) app.AccountKeeper.SetAccount(ctx, acc1) suite.Require().NoError(app.BankKeeper.SetBalances(ctx, addr1, balances)) @@ -305,7 +305,7 @@ func (suite *IntegrationTestSuite) TestInputOutputNewAccount() { acc1Balances := app.BankKeeper.GetAllBalances(ctx, addr1) suite.Require().Equal(balances, acc1Balances) - addr2 := sdk.AccAddress([]byte("addr2")) + addr2 := sdk.AccAddress([]byte("addr2_______________")) suite.Require().Nil(app.AccountKeeper.GetAccount(ctx, addr2)) suite.Require().Empty(app.BankKeeper.GetAllBalances(ctx, addr2)) @@ -329,15 +329,15 @@ func (suite *IntegrationTestSuite) TestInputOutputCoins() { app, ctx := suite.app, suite.ctx balances := sdk.NewCoins(newFooCoin(90), newBarCoin(30)) - addr1 := sdk.AccAddress([]byte("addr1")) + addr1 := sdk.AccAddress([]byte("addr1_______________")) acc1 := app.AccountKeeper.NewAccountWithAddress(ctx, addr1) app.AccountKeeper.SetAccount(ctx, acc1) - addr2 := sdk.AccAddress([]byte("addr2")) + addr2 := sdk.AccAddress([]byte("addr2_______________")) acc2 := app.AccountKeeper.NewAccountWithAddress(ctx, addr2) app.AccountKeeper.SetAccount(ctx, acc2) - addr3 := sdk.AccAddress([]byte("addr3")) + addr3 := sdk.AccAddress([]byte("addr3_______________")) acc3 := app.AccountKeeper.NewAccountWithAddress(ctx, addr3) app.AccountKeeper.SetAccount(ctx, acc3) @@ -576,10 +576,10 @@ func (suite *IntegrationTestSuite) TestMsgMultiSendEvents() { app.BankKeeper.SetParams(ctx, types.DefaultParams()) - addr := sdk.AccAddress([]byte("addr1")) - addr2 := sdk.AccAddress([]byte("addr2")) - addr3 := sdk.AccAddress([]byte("addr3")) - addr4 := sdk.AccAddress([]byte("addr4")) + addr := sdk.AccAddress([]byte("addr1_______________")) + addr2 := sdk.AccAddress([]byte("addr2_______________")) + addr3 := sdk.AccAddress([]byte("addr3_______________")) + addr4 := sdk.AccAddress([]byte("addr4_______________")) acc := app.AccountKeeper.NewAccountWithAddress(ctx, addr) acc2 := app.AccountKeeper.NewAccountWithAddress(ctx, addr2) diff --git a/x/bank/types/msgs.go b/x/bank/types/msgs.go index 9d79f37c90..a169c5df48 100644 --- a/x/bank/types/msgs.go +++ b/x/bank/types/msgs.go @@ -26,12 +26,12 @@ func (msg MsgSend) Type() string { return TypeMsgSend } // ValidateBasic Implements Msg. func (msg MsgSend) ValidateBasic() error { - if msg.FromAddress.Empty() { - return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "missing sender address") + if err := sdk.VerifyAddressFormat(msg.FromAddress); err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "Invalid sender address (%s)", err) } - if msg.ToAddress.Empty() { - return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "missing recipient address") + if err := sdk.VerifyAddressFormat(msg.ToAddress); err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "Invalid recipient address (%s)", err) } if !msg.Amount.IsValid() { @@ -100,8 +100,8 @@ func (msg MsgMultiSend) GetSigners() []sdk.AccAddress { // ValidateBasic - validate transaction input func (in Input) ValidateBasic() error { - if len(in.Address) == 0 { - return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "input address missing") + if err := sdk.VerifyAddressFormat(in.Address); err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "Invalid input address (%s)", err) } if !in.Coins.IsValid() { @@ -125,8 +125,8 @@ func NewInput(addr sdk.AccAddress, coins sdk.Coins) Input { // ValidateBasic - validate transaction output func (out Output) ValidateBasic() error { - if len(out.Address) == 0 { - return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "output address missing") + if err := sdk.VerifyAddressFormat(out.Address); err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "Invalid output address (%s)", err) } if !out.Coins.IsValid() { diff --git a/x/bank/types/msgs_test.go b/x/bank/types/msgs_test.go index a0fe50b2bf..ff11c851be 100644 --- a/x/bank/types/msgs_test.go +++ b/x/bank/types/msgs_test.go @@ -20,33 +20,36 @@ func TestMsgSendRoute(t *testing.T) { } func TestMsgSendValidation(t *testing.T) { - addr1 := sdk.AccAddress([]byte("from")) - addr2 := sdk.AccAddress([]byte("to")) + addr1 := sdk.AccAddress([]byte("from________________")) + addr2 := sdk.AccAddress([]byte("to__________________")) + addrEmpty := sdk.AccAddress([]byte("")) + addrTooLong := sdk.AccAddress([]byte("Accidentally used 33 bytes pubkey")) + atom123 := sdk.NewCoins(sdk.NewInt64Coin("atom", 123)) atom0 := sdk.NewCoins(sdk.NewInt64Coin("atom", 0)) atom123eth123 := sdk.NewCoins(sdk.NewInt64Coin("atom", 123), sdk.NewInt64Coin("eth", 123)) atom123eth0 := sdk.Coins{sdk.NewInt64Coin("atom", 123), sdk.NewInt64Coin("eth", 0)} - var emptyAddr sdk.AccAddress - cases := []struct { - valid bool - tx *MsgSend + expectedErr string // empty means no error expected + msg *MsgSend }{ - {true, NewMsgSend(addr1, addr2, atom123)}, // valid send - {true, NewMsgSend(addr1, addr2, atom123eth123)}, // valid send with multiple coins - {false, NewMsgSend(addr1, addr2, atom0)}, // non positive coin - {false, NewMsgSend(addr1, addr2, atom123eth0)}, // non positive coin in multicoins - {false, NewMsgSend(emptyAddr, addr2, atom123)}, // empty from addr - {false, NewMsgSend(addr1, emptyAddr, atom123)}, // empty to addr + {"", NewMsgSend(addr1, addr2, atom123)}, // valid send + {"", NewMsgSend(addr1, addr2, atom123eth123)}, // valid send with multiple coins + {": invalid coins", NewMsgSend(addr1, addr2, atom0)}, // non positive coin + {"123atom,0eth: invalid coins", NewMsgSend(addr1, addr2, atom123eth0)}, // non positive coin in multicoins + {"Invalid sender address (incorrect address length (expected: 20, actual: 0)): invalid address", NewMsgSend(addrEmpty, addr2, atom123)}, + {"Invalid sender address (incorrect address length (expected: 20, actual: 33)): invalid address", NewMsgSend(addrTooLong, addr2, atom123)}, + {"Invalid recipient address (incorrect address length (expected: 20, actual: 0)): invalid address", NewMsgSend(addr1, addrEmpty, atom123)}, + {"Invalid recipient address (incorrect address length (expected: 20, actual: 33)): invalid address", NewMsgSend(addr1, addrTooLong, atom123)}, } for _, tc := range cases { - err := tc.tx.ValidateBasic() - if tc.valid { + err := tc.msg.ValidateBasic() + if tc.expectedErr == "" { require.Nil(t, err) } else { - require.NotNil(t, err) + require.EqualError(t, err, tc.expectedErr) } } } @@ -85,84 +88,90 @@ func TestMsgMultiSendRoute(t *testing.T) { } func TestInputValidation(t *testing.T) { - addr1 := sdk.AccAddress([]byte{1, 2}) - addr2 := sdk.AccAddress([]byte{7, 8}) + addr1 := sdk.AccAddress([]byte("_______alice________")) + addr2 := sdk.AccAddress([]byte("________bob_________")) + addrEmpty := sdk.AccAddress([]byte("")) + addrTooLong := sdk.AccAddress([]byte("Accidentally used 33 bytes pubkey")) + someCoins := sdk.NewCoins(sdk.NewInt64Coin("atom", 123)) multiCoins := sdk.NewCoins(sdk.NewInt64Coin("atom", 123), sdk.NewInt64Coin("eth", 20)) - var emptyAddr sdk.AccAddress emptyCoins := sdk.NewCoins() emptyCoins2 := sdk.NewCoins(sdk.NewInt64Coin("eth", 0)) someEmptyCoins := sdk.Coins{sdk.NewInt64Coin("eth", 10), sdk.NewInt64Coin("atom", 0)} unsortedCoins := sdk.Coins{sdk.NewInt64Coin("eth", 1), sdk.NewInt64Coin("atom", 1)} cases := []struct { - valid bool - txIn Input + expectedErr string // empty means no error expected + txIn Input }{ // auth works with different apps - {true, NewInput(addr1, someCoins)}, - {true, NewInput(addr2, someCoins)}, - {true, NewInput(addr2, multiCoins)}, + {"", NewInput(addr1, someCoins)}, + {"", NewInput(addr2, someCoins)}, + {"", NewInput(addr2, multiCoins)}, - {false, NewInput(emptyAddr, someCoins)}, // empty address - {false, NewInput(addr1, emptyCoins)}, // invalid coins - {false, NewInput(addr1, emptyCoins2)}, // invalid coins - {false, NewInput(addr1, someEmptyCoins)}, // invalid coins - {false, NewInput(addr1, unsortedCoins)}, // unsorted coins + {"Invalid input address (incorrect address length (expected: 20, actual: 0)): invalid address", NewInput(addrEmpty, someCoins)}, + {"Invalid input address (incorrect address length (expected: 20, actual: 33)): invalid address", NewInput(addrTooLong, someCoins)}, + {": invalid coins", NewInput(addr1, emptyCoins)}, // invalid coins + {": invalid coins", NewInput(addr1, emptyCoins2)}, // invalid coins + {"10eth,0atom: invalid coins", NewInput(addr1, someEmptyCoins)}, // invalid coins + {"1eth,1atom: invalid coins", NewInput(addr1, unsortedCoins)}, // unsorted coins } for i, tc := range cases { err := tc.txIn.ValidateBasic() - if tc.valid { + if tc.expectedErr == "" { require.Nil(t, err, "%d: %+v", i, err) } else { - require.NotNil(t, err, "%d", i) + require.EqualError(t, err, tc.expectedErr, "%d", i) } } } func TestOutputValidation(t *testing.T) { - addr1 := sdk.AccAddress([]byte{1, 2}) - addr2 := sdk.AccAddress([]byte{7, 8}) + addr1 := sdk.AccAddress([]byte("_______alice________")) + addr2 := sdk.AccAddress([]byte("________bob_________")) + addrEmpty := sdk.AccAddress([]byte("")) + addrTooLong := sdk.AccAddress([]byte("Accidentally used 33 bytes pubkey")) + someCoins := sdk.NewCoins(sdk.NewInt64Coin("atom", 123)) multiCoins := sdk.NewCoins(sdk.NewInt64Coin("atom", 123), sdk.NewInt64Coin("eth", 20)) - var emptyAddr sdk.AccAddress emptyCoins := sdk.NewCoins() emptyCoins2 := sdk.NewCoins(sdk.NewInt64Coin("eth", 0)) someEmptyCoins := sdk.Coins{sdk.NewInt64Coin("eth", 10), sdk.NewInt64Coin("atom", 0)} unsortedCoins := sdk.Coins{sdk.NewInt64Coin("eth", 1), sdk.NewInt64Coin("atom", 1)} cases := []struct { - valid bool - txOut Output + expectedErr string // empty means no error expected + txOut Output }{ // auth works with different apps - {true, NewOutput(addr1, someCoins)}, - {true, NewOutput(addr2, someCoins)}, - {true, NewOutput(addr2, multiCoins)}, + {"", NewOutput(addr1, someCoins)}, + {"", NewOutput(addr2, someCoins)}, + {"", NewOutput(addr2, multiCoins)}, - {false, NewOutput(emptyAddr, someCoins)}, // empty address - {false, NewOutput(addr1, emptyCoins)}, // invalid coins - {false, NewOutput(addr1, emptyCoins2)}, // invalid coins - {false, NewOutput(addr1, someEmptyCoins)}, // invalid coins - {false, NewOutput(addr1, unsortedCoins)}, // unsorted coins + {"Invalid output address (incorrect address length (expected: 20, actual: 0)): invalid address", NewOutput(addrEmpty, someCoins)}, + {"Invalid output address (incorrect address length (expected: 20, actual: 33)): invalid address", NewOutput(addrTooLong, someCoins)}, + {": invalid coins", NewOutput(addr1, emptyCoins)}, // invalid coins + {": invalid coins", NewOutput(addr1, emptyCoins2)}, // invalid coins + {"10eth,0atom: invalid coins", NewOutput(addr1, someEmptyCoins)}, // invalid coins + {"1eth,1atom: invalid coins", NewOutput(addr1, unsortedCoins)}, // unsorted coins } for i, tc := range cases { err := tc.txOut.ValidateBasic() - if tc.valid { + if tc.expectedErr == "" { require.Nil(t, err, "%d: %+v", i, err) } else { - require.NotNil(t, err, "%d", i) + require.EqualError(t, err, tc.expectedErr, "%d", i) } } } func TestMsgMultiSendValidation(t *testing.T) { - addr1 := sdk.AccAddress([]byte{1, 2}) - addr2 := sdk.AccAddress([]byte{7, 8}) + addr1 := sdk.AccAddress([]byte("_______alice________")) + addr2 := sdk.AccAddress([]byte("________bob_________")) atom123 := sdk.NewCoins(sdk.NewInt64Coin("atom", 123)) atom124 := sdk.NewCoins(sdk.NewInt64Coin("atom", 124)) eth123 := sdk.NewCoins(sdk.NewInt64Coin("eth", 123))