diff --git a/CHANGELOG.md b/CHANGELOG.md index 16a514386b..3120a7d55f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,10 @@ Ref: https://keepachangelog.com/en/1.0.0/ * Allow force transfers from marker and market accounts [#1855](https://github.com/provenance-io/provenance/pull/1855). +### Bug Fixes + +* Fix `MarkerTransferAuthorization` validation to ensure the coins and addresses are all valid [1856](https://github.com/provenance-io/provenance/pull/1856). + --- ## [v1.18.0-rc2](https://github.com/provenance-io/provenance/releases/tag/v1.18.0-rc2) - 2024-02-22 diff --git a/x/marker/types/authz.go b/x/marker/types/authz.go index 970136c8e8..ddb9b67f7d 100644 --- a/x/marker/types/authz.go +++ b/x/marker/types/authz.go @@ -60,18 +60,22 @@ func (a MarkerTransferAuthorization) Accept(_ sdk.Context, msg sdk.Msg) (authz.A // ValidateBasic implements Authorization.ValidateBasic. func (a MarkerTransferAuthorization) ValidateBasic() error { - if a.TransferLimit == nil { - return sdkerrors.ErrInvalidCoins.Wrap("spend limit cannot be nil") + if err := a.TransferLimit.Validate(); err != nil { + return sdkerrors.ErrInvalidCoins.Wrapf("invalid transfer limit: %v", err) } - if !a.TransferLimit.IsAllPositive() { - return sdkerrors.ErrInvalidCoins.Wrap("spend limit cannot be negitive") + if a.TransferLimit.IsZero() { + return sdkerrors.ErrInvalidCoins.Wrap("invalid transfer limit: cannot be zero") } + found := make(map[string]bool, 0) - for i := 0; i < len(a.AllowList); i++ { - if found[a.AllowList[i]] { - return ErrDuplicateEntry.Wrap("all allow list addresses must be unique") + for i, addr := range a.AllowList { + if _, err := sdk.AccAddressFromBech32(addr); err != nil { + return sdkerrors.ErrInvalidAddress.Wrapf("invalid allow list entry [%d] %q: %v", i, addr, err) + } + if found[addr] { + return ErrDuplicateEntry.Wrapf("invalid allow list entry [%d] %s", i, addr) } - found[a.AllowList[i]] = true + found[addr] = true } return nil diff --git a/x/marker/types/authz_test.go b/x/marker/types/authz_test.go index 813906ee48..398c2b88c0 100644 --- a/x/marker/types/authz_test.go +++ b/x/marker/types/authz_test.go @@ -5,11 +5,14 @@ import ( "github.com/stretchr/testify/require" - sdk "github.com/cosmos/cosmos-sdk/types" - tmproto "github.com/tendermint/tendermint/proto/tendermint/types" + sdkmath "cosmossdk.io/math" + + sdk "github.com/cosmos/cosmos-sdk/types" + simapp "github.com/provenance-io/provenance/app" + "github.com/provenance-io/provenance/testutil/assertions" . "github.com/provenance-io/provenance/x/marker/types" ) @@ -56,40 +59,95 @@ func TestMarkerTransferAuthorization(t *testing.T) { } func TestMarkerTransferAuthorizationValidateBasic(t *testing.T) { - addr1, _ := sdk.AccAddressFromBech32("cosmos1sh49f6ze3vn7cdl2amh2gnc70z5mten3y08xck") + coin := func(amount int64, denom string) sdk.Coin { + return sdk.Coin{Denom: denom, Amount: sdkmath.NewInt(amount)} + } + addr1 := sdk.AccAddress("addr1_______________") + addr2 := sdk.AccAddress("addr2_______________") + addr3 := sdk.AccAddress("addr3_______________") cases := []struct { - name string - msg *MarkerTransferAuthorization - errorMsg string + name string + msg MarkerTransferAuthorization + expErr string }{ { - "valid msg with empty allow list", - NewMarkerTransferAuthorization(sdk.NewCoins(coin500), []sdk.AccAddress{}), - "", + name: "nil allow list", + msg: MarkerTransferAuthorization{TransferLimit: sdk.NewCoins(coin500)}, + expErr: "", + }, + { + name: "empty allow list", + msg: MarkerTransferAuthorization{ + TransferLimit: sdk.NewCoins(coin500), + AllowList: []string{}, + }, + expErr: "", + }, + { + name: "non-empty allow list", + msg: MarkerTransferAuthorization{ + TransferLimit: sdk.NewCoins(coin500), + AllowList: []string{addr1.String(), addr2.String(), addr3.String()}, + }, + expErr: "", + }, + { + name: "nil transfer limit", + msg: MarkerTransferAuthorization{TransferLimit: nil}, + expErr: "invalid transfer limit: cannot be zero: invalid coins", }, { - "valid msg without non-empty allow list", - NewMarkerTransferAuthorization(sdk.NewCoins(coin500), []sdk.AccAddress{addr1}), - "", + name: "empty transfer limit", + msg: MarkerTransferAuthorization{TransferLimit: sdk.Coins{}}, + expErr: "invalid transfer limit: cannot be zero: invalid coins", }, { - "invalid msg with duplicate allow list", - NewMarkerTransferAuthorization(sdk.NewCoins(coin500), []sdk.AccAddress{addr1, addr1}), - "all allow list addresses must be unique: duplicate entry", + name: "transfer limit with invalid denom", + msg: MarkerTransferAuthorization{TransferLimit: sdk.Coins{coin(3, "x")}}, + expErr: "invalid transfer limit: invalid denom: x: invalid coins", + }, + { + name: "transfer limit with zero coin", + msg: MarkerTransferAuthorization{TransferLimit: sdk.Coins{coin(0, "catcoin")}}, + expErr: "invalid transfer limit: coin 0catcoin amount is not positive: invalid coins", + }, + { + name: "transfer limit with negative coin", + msg: MarkerTransferAuthorization{TransferLimit: sdk.Coins{coin(-3, "catcoin")}}, + expErr: "invalid transfer limit: coin -3catcoin amount is not positive: invalid coins", + }, + { + name: "unsorted transfer limit", + msg: MarkerTransferAuthorization{TransferLimit: sdk.Coins{coin(10, "banana"), coin(3, "apple")}}, + expErr: "invalid transfer limit: denomination apple is not sorted: invalid coins", + }, + { + name: "invalid allow list entry", + msg: MarkerTransferAuthorization{ + TransferLimit: sdk.NewCoins(coin500), + AllowList: []string{addr1.String(), "notgonnawork", addr3.String()}, + }, + expErr: "invalid allow list entry [1] \"notgonnawork\": decoding bech32 failed: invalid separator index -1: invalid address", + }, + { + name: "duplicate allow list entry", + msg: MarkerTransferAuthorization{ + TransferLimit: sdk.NewCoins(coin500), + AllowList: []string{addr1.String(), addr2.String(), addr1.String()}, + }, + expErr: "invalid allow list entry [2] " + addr1.String() + ": duplicate entry", }, } - for _, tc := range cases { - tc := tc + for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - err := tc.msg.ValidateBasic() - if len(tc.errorMsg) > 0 { - require.Error(t, err) - require.Equal(t, tc.errorMsg, err.Error()) - } else { - require.NoError(t, err) + var err error + testFunc := func() { + err = tc.msg.ValidateBasic() } + require.NotPanics(t, testFunc, "ValidateBasic") + assertions.AssertErrorValue(t, err, tc.expErr, "ValidateBasic error") }) } }