Fix MarkerTransferAuthorization validation. (#1856)

* Fix MarkerTransferAuthorization validation to make sure the limit is valid Coins and the addresses in the allow list are valid. * Add changelog entry.
provenance-io · Mar 1, 2024 · abd2da1 · abd2da1
1 parent 8bd98ae
commit abd2da1
Show file tree

Hide file tree

Showing 3 changed files with 97 additions and 31 deletions.
diff --git 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
@@ -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
@@ -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")
 		})
 	}
 }