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).

CHANGELOG.md

@@ -37,7 +37,9 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
 
-* nothing
+### Improvements
+
+* Allow force transfers from marker and market accounts [#1855](https://github.com/provenance-io/provenance/pull/1855).
 
 ---
 

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 {

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