Backport: 1855 (forced xfers from marker markets), 1856 (fix marker x…

…fer authz validation). (#1876) * Allow force transfers from marker and market accounts. (#1855) * Allow force transfers out of marker and market accounts. * Add some test cases for marker and market accounts with canForceTransferFrom. * Add changelog entry. * Tweak TestCanForceTransferFrom a little based on coderabbit feedback. Namely, have the account creators take in a string to use for the addr instead of the full address that it's going to just turn around and return. * Lint fix (needed extra empty line in keeper/marker.go). * 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. * Fix one of the changelog lines.
provenance-io · Mar 18, 2024 · 8a47ce3 · 8a47ce3
1 parent 96259c6
commit 8a47ce3
Show file tree

Hide file tree

Showing 5 changed files with 170 additions and 44 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -37,6 +37,14 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
 
+### Improvements
+
+* 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).
+
 ### Dependencies
 
 - Bump `google.golang.org/grpc` from 1.61.1 to 1.62.1 ([#1850](https://github.com/provenance-io/provenance/pull/1850), [#1864](https://github.com/provenance-io/provenance/pull/1864))

diff --git a/x/marker/keeper/keeper_test.go b/x/marker/keeper/keeper_test.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"sort"
+	"strings"
 	"testing"
 	"time"
 
@@ -30,6 +31,7 @@ import (
 
 	simapp "github.com/provenance-io/provenance/app"
 	"github.com/provenance-io/provenance/testutil/assertions"
+	"github.com/provenance-io/provenance/x/exchange"
 	markerkeeper "github.com/provenance-io/provenance/x/marker/keeper"
 	"github.com/provenance-io/provenance/x/marker/types"
 	rewardtypes "github.com/provenance-io/provenance/x/reward/types"
@@ -1565,10 +1567,16 @@ func TestCanForceTransferFrom(t *testing.T) {
 	app := simapp.Setup(t)
 	ctx := app.BaseApp.NewContext(false, tmproto.Header{})
 
-	setAcc := func(addr sdk.AccAddress, sequence uint64) {
+	testAddr := func(prefix string) sdk.AccAddress {
+		return sdk.AccAddress(prefix + strings.Repeat("_", 20-len(prefix)))
+	}
+
+	setAcc := func(addrPrefix string, sequence uint64) sdk.AccAddress {
+		addr := testAddr(addrPrefix)
 		acc := app.AccountKeeper.NewAccountWithAddress(ctx, addr)
 		require.NoError(t, acc.SetSequence(sequence), "%s.SetSequence(%d)", string(addr), sequence)
 		app.AccountKeeper.SetAccount(ctx, acc)
+		return addr
 	}
 
 	createGroup := func() sdk.AccAddress {
@@ -1589,12 +1597,36 @@ func TestCanForceTransferFrom(t *testing.T) {
 		return sdk.MustAccAddressFromBech32(res.GroupPolicyAddress)
 	}
 
-	addrNoAcc := sdk.AccAddress("addrNoAcc___________")
-	addrSeq0 := sdk.AccAddress("addrSeq0____________")
-	addrSeq1 := sdk.AccAddress("addrSeq1____________")
+	createMarkerAcc := func(addrPrefix string) sdk.AccAddress {
+		addr := testAddr(addrPrefix)
+		acc := &types.MarkerAccount{
+			BaseAccount: authtypes.NewBaseAccountWithAddress(addr),
+			Status:      types.StatusActive,
+			Denom:       "whatever",
+			Supply:      math.NewInt(0),
+			MarkerType:  types.MarkerType_RestrictedCoin,
+		}
+		app.AccountKeeper.SetAccount(ctx, acc)
+		return addr
+	}
+
+	createMarketAcc := func(addrPrefix string) sdk.AccAddress {
+		addr := testAddr(addrPrefix)
+		acc := &exchange.MarketAccount{
+			BaseAccount:   authtypes.NewBaseAccountWithAddress(addr),
+			MarketId:      97531,
+			MarketDetails: exchange.MarketDetails{},
+		}
+		app.AccountKeeper.SetAccount(ctx, acc)
+		return addr
+	}
+
+	addrNoAcc := testAddr("addrNoAcc")
+	addrSeq0 := setAcc("addrSeq0", 0)
+	addrSeq1 := setAcc("addrSeq1", 1)
 	addrGroup := createGroup()
-	setAcc(addrSeq0, 0)
-	setAcc(addrSeq1, 1)
+	addrMarker := createMarkerAcc("addrMarker")
+	addrMarket := createMarketAcc("addrMarket")
 
 	tests := []struct {
 		name string
@@ -1605,6 +1637,8 @@ func TestCanForceTransferFrom(t *testing.T) {
 		{name: "address with sequence 0", from: addrSeq0, exp: false},
 		{name: "address with sequence 1", from: addrSeq1, exp: true},
 		{name: "group address", from: addrGroup, exp: true},
+		{name: "marker address", from: addrMarker, exp: true},
+		{name: "market address", from: addrMarket, exp: true},
 	}
 
 	for _, tc := range tests {

diff --git a/x/marker/keeper/marker.go b/x/marker/keeper/marker.go
@@ -12,6 +12,7 @@ import (
 	ibctypes "github.com/cosmos/ibc-go/v6/modules/apps/transfer/types"
 	clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types"
 
+	"github.com/provenance-io/provenance/x/exchange"
 	"github.com/provenance-io/provenance/x/marker/types"
 )
 
@@ -708,19 +709,40 @@ func (k Keeper) TransferCoin(ctx sdk.Context, from, to, admin sdk.AccAddress, am
 
 // canForceTransferFrom returns true if funds can be forcefully transferred out of the provided address.
 func (k Keeper) canForceTransferFrom(ctx sdk.Context, from sdk.AccAddress) bool {
-	acc := k.authKeeper.GetAccount(ctx, from)
-	// If the account is a group address, then it will allow the transfer.
+	// If the account is a group address, then allow the force transfer.
 	if k.groupChecker != nil && k.groupChecker.IsGroupAddress(ctx, from) {
 		return true
 	}
 
 	// If acc is nil, there's no funds in it, so the transfer will fail anyway.
-	// In that case, return true from here so it can fail later with a more accurate message.
-	// If there is an account, only allow force transfers if the sequence number isn't zero.
-	// This is to prevent forced transfer from module accounts and smart contracts.
-	// It will also block forced transfers from new or dead accounts, though.
+	// In that case, return true here so it can fail later with a more accurate message.
+	acc := k.authKeeper.GetAccount(ctx, from)
+	if acc == nil {
+		return true
+	}
+
+	// We can't allow forced transfers from module accounts and smart contract accounts.
+	// Doing so can result in a chain halt. Unfortunately, specifically checking if an
+	// account is a smart contract account is expensive. Instead, we only allow the forced
+	// transfers from accounts that have signed at least one Tx and therefore have a non-zero
+	// sequence. This also prevents other force transfers that wouldn't cause problems, though.
+	// For example, some funds got erroneously sent to a new account that no one has a key for.
 	// If the forced transfer is absolutely required, use a governance proposal with a MsgSend.
-	return acc == nil || acc.GetSequence() != 0
+	if acc.GetSequence() != 0 {
+		return true
+	}
+
+	// Allow force transfers out of marker accounts still.
+	if _, isMarker := acc.(types.MarkerAccountI); isMarker {
+		return true
+	}
+
+	// Allow force transfers out of market accounts too.
+	if _, isMarket := acc.(*exchange.MarketAccount); isMarket {
+		return true
+	}
+
+	return false
 }
 
 // IbcTransferCoin transfers restricted coins between two chains when the administrator account holds the transfer

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")
 		})
 	}
 }