Skip to content

Commit

Permalink
Prevent restricted coins from ending up in the fee collector account. (…
Browse files Browse the repository at this point in the history 
…#1846)

* [1845]: Update the marker SendRestrictionFn to not allow restricted coins to be sent to the fee collector account.

* [1845]: Add changelog entry.

* [1845]: Fix some exchange unit tests that broke because they were using restricted coins for the fees.

* [1845]: Update marker spec doc flowcharts.
  • Loading branch information
@SpicyLemon
SpicyLemon committed Feb 21, 2024
1 parent bf77a2e commit 16fb638
Show file tree
Hide file tree
Showing 7 changed files with 279 additions and 13 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* Bid order prices are no longer restricted to amounts that can be evenly applied to a buyer settlement fee ratio [1834](https://github.com/provenance-io/provenance/pull/1843).
* In the marker and exchange modules, help ensure funds don't get sent to blocked addresses [#1834](https://github.com/provenance-io/provenance/issues/1834).
* Update marker and exchange spec docs to include info about transfer agents [#1834](https://github.com/provenance-io/provenance/issues/1834).
* Prevent restricted markers from being sent to the fee collector account [#1845](https://github.com/provenance-io/provenance/issues/1845).

### Bug Fixes

Expand Down
198 changes: 194 additions & 4 deletions x/exchange/keeper/msg_server_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ func (s *TestSuite) requireSanctionAddress(addr sdk.AccAddress) {
s.Require().NoError(err, "SanctionAddresses(%s)", s.getAddrName(addr))
}

// requireAddFinalizeAndActivateMarker creates a marker, requiring it to not error.
// requireAddFinalizeAndActivateMarker creates a restricted marker, requiring it to not error.
func (s *TestSuite) requireAddFinalizeAndActivateMarker(coin sdk.Coin, manager sdk.AccAddress, reqAttrs ...string) {
markerAddr := s.markerAddr(coin.Denom)
marker := &markertypes.MarkerAccount{
Expand Down Expand Up @@ -273,6 +273,36 @@ func (s *TestSuite) requireAddFinalizeAndActivateMarker(coin sdk.Coin, manager s
s.Require().NoError(err, "AddFinalizeAndActivateMarker(%s)", coin.Denom)
}

// requireAddFinalizeAndActivateCoinMarker creates a coin marker, requiring it to not error.
func (s *TestSuite) requireAddFinalizeAndActivateCoinMarker(coin sdk.Coin, manager sdk.AccAddress) {
markerAddr := s.markerAddr(coin.Denom)
marker := &markertypes.MarkerAccount{
BaseAccount: &authtypes.BaseAccount{Address: markerAddr.String()},
Manager: manager.String(),
AccessControl: []markertypes.AccessGrant{
{
Address: manager.String(),
Permissions: markertypes.AccessList{
markertypes.Access_Mint, markertypes.Access_Burn,
markertypes.Access_Deposit, markertypes.Access_Withdraw, markertypes.Access_Delete,
markertypes.Access_Admin,
},
},
},
Status: markertypes.StatusProposed,
Denom: coin.Denom,
Supply: coin.Amount,
MarkerType: markertypes.MarkerType_Coin,
SupplyFixed: true,
AllowGovernanceControl: true,
}
nav := markertypes.NewNetAssetValue(s.coin("5navcoin"), 1)
err := s.app.MarkerKeeper.SetNetAssetValue(s.ctx, marker, nav, "testing")
s.Require().NoError(err, "SetNetAssetValue(%d)", coin.Denom)
err = s.app.MarkerKeeper.AddFinalizeAndActivateMarker(s.ctx, marker)
s.Require().NoError(err, "AddFinalizeAndActivateMarker(%s)", coin.Denom)
}

// requireGetMarker gets a marker requiring it to not come with an error.
func (s *TestSuite) requireGetMarker(denom string) markertypes.MarkerAccountI {
rv, err := s.app.MarkerKeeper.GetMarkerByDenom(s.ctx, denom)
Expand Down Expand Up @@ -1425,7 +1455,7 @@ func (s *TestSuite) TestMsgServer_FillBids() {
},
},
{
name: "okay: two bids, all req attrs and fees",
name: "fee in restricted denom",
setup: func() {
s.requireAddFinalizeAndActivateMarker(s.coin("13apple"), s.addr5, "*.gonna.have.it")
s.requireAddFinalizeAndActivateMarker(s.coin("70pear"), s.addr5, "*.gonna.have.it")
Expand Down Expand Up @@ -1471,6 +1501,58 @@ func (s *TestSuite) TestMsgServer_FillBids() {
SellerSettlementFlatFee: s.coinP("5pear"),
AskOrderCreationFee: s.coinP("10fig"),
},
expInErr: []string{invReqErr,
"error collecting exchange fee 4fig,1pear (based off 67fig,9pear) from market 1",
"restricted denom fig cannot be sent to the fee collector",
},
},
{
name: "okay: two bids, all req attrs and fees",
setup: func() {
s.requireAddFinalizeAndActivateMarker(s.coin("13apple"), s.addr5, "*.gonna.have.it")
s.requireAddFinalizeAndActivateCoinMarker(s.coin("70pear"), s.addr5)
s.requireAddFinalizeAndActivateCoinMarker(s.coin("300fig"), s.addr5)
s.requireSetNameRecord("buyer.gonna.have.it", s.addr5)
s.requireSetNameRecord("seller.gonna.have.it", s.addr5)
s.requireSetNameRecord("market.gonna.have.it", s.addr5)
s.requireSetAttr(s.addr1, "seller.gonna.have.it", s.addr5)
s.requireSetAttr(s.addr2, "buyer.gonna.have.it", s.addr5)
s.requireSetAttr(s.addr3, "buyer.gonna.have.it", s.addr5)
s.requireSetAttr(s.marketAddr1, "market.gonna.have.it", s.addr5)
s.requireFundAccount(s.addr1, "13apple,100fig")
s.requireFundAccount(s.addr2, "50pear,100fig")
s.requireFundAccount(s.addr3, "20pear,100fig")

s.requireCreateMarketUnmocked(exchange.Market{
MarketId: 1, AcceptingOrders: true, AllowUserSettlement: true,
FeeCreateAskFlat: s.coins("10fig"),
FeeCreateBidFlat: s.coins("200fig"),
FeeSellerSettlementFlat: s.coins("5pear"),
FeeSellerSettlementRatios: s.ratios("35pear:2pear"),
FeeBuyerSettlementFlat: s.coins("30fig"),
FeeBuyerSettlementRatios: s.ratios("10pear:1fig"),
ReqAttrCreateAsk: []string{"*.gonna.have.it"},
ReqAttrCreateBid: []string{"not.gonna.have.it"},
})
s.requireSetOrderInStore(s.getStore(), exchange.NewOrder(12345).WithBid(&exchange.BidOrder{
MarketId: 1, Buyer: s.addr2.String(), Assets: s.coin("10apple"), Price: s.coin("50pear"),
BuyerSettlementFees: s.coins("35fig"), ExternalId: "first order",
}))
s.requireAddHold(s.addr2, "50pear,35fig", 12345)
s.requireSetOrderInStore(s.getStore(), exchange.NewOrder(98765).WithBid(&exchange.BidOrder{
MarketId: 1, Buyer: s.addr3.String(), Assets: s.coin("3apple"), Price: s.coin("20pear"),
BuyerSettlementFees: s.coins("32fig"), ExternalId: "second order",
}))
s.requireAddHold(s.addr3, "20pear,32fig", 98765)
},
msg: exchange.MsgFillBidsRequest{
Seller: s.addr1.String(),
MarketId: 1,
TotalAssets: s.coins("13apple"),
BidOrderIds: []uint64{12345, 98765},
SellerSettlementFlatFee: s.coinP("5pear"),
AskOrderCreationFee: s.coinP("10fig"),
},
fArgs: []expBalances{
{
addr: s.addr1,
Expand Down Expand Up @@ -1859,7 +1941,7 @@ func (s *TestSuite) TestMsgServer_FillAsks() {
},
},
{
name: "okay: two asks, all req attrs and fees",
name: "fee in restricted denom",
setup: func() {
s.requireSetNameRecord("buyer.gonna.have.it", s.addr5)
s.requireSetNameRecord("seller.gonna.have.it", s.addr5)
Expand Down Expand Up @@ -1905,6 +1987,58 @@ func (s *TestSuite) TestMsgServer_FillAsks() {
BuyerSettlementFees: s.coins("37fig"),
BidOrderCreationFee: s.coinP("10fig"),
},
expInErr: []string{invReqErr,
"error collecting exchange fee 3fig,1pear (based off 49fig,10pear) from market 1",
"restricted denom fig cannot be sent to the fee collector",
},
},
{
name: "okay: two asks, all req attrs and fees",
setup: func() {
s.requireSetNameRecord("buyer.gonna.have.it", s.addr5)
s.requireSetNameRecord("seller.gonna.have.it", s.addr5)
s.requireSetNameRecord("market.gonna.have.it", s.addr5)
s.requireSetAttr(s.addr1, "buyer.gonna.have.it", s.addr5)
s.requireSetAttr(s.addr2, "seller.gonna.have.it", s.addr5)
s.requireSetAttr(s.addr3, "seller.gonna.have.it", s.addr5)
s.requireSetAttr(s.marketAddr1, "market.gonna.have.it", s.addr5)
s.requireFundAccount(s.addr1, "70pear,100fig")
s.requireFundAccount(s.addr2, "10apple,100fig")
s.requireFundAccount(s.addr3, "3apple,100fig")
s.requireAddFinalizeAndActivateMarker(s.coin("13apple"), s.addr5, "*.gonna.have.it")
s.requireAddFinalizeAndActivateCoinMarker(s.coin("70pear"), s.addr5)
s.requireAddFinalizeAndActivateCoinMarker(s.coin("300fig"), s.addr5)

s.requireCreateMarketUnmocked(exchange.Market{
MarketId: 1, AcceptingOrders: true, AllowUserSettlement: true,
FeeCreateAskFlat: s.coins("200fig"),
FeeCreateBidFlat: s.coins("10fig"),
FeeSellerSettlementFlat: s.coins("5pear,12fig"),
FeeSellerSettlementRatios: s.ratios("35pear:2pear"),
FeeBuyerSettlementFlat: s.coins("30fig"),
FeeBuyerSettlementRatios: s.ratios("10pear:1fig"),
ReqAttrCreateAsk: []string{"not.gonna.have.it"},
ReqAttrCreateBid: []string{"*.gonna.have.it"},
})
s.requireSetOrderInStore(s.getStore(), exchange.NewOrder(12345).WithAsk(&exchange.AskOrder{
MarketId: 1, Seller: s.addr2.String(), Assets: s.coin("10apple"), Price: s.coin("50pear"),
SellerSettlementFlatFee: s.coinP("5pear"), ExternalId: "first order",
}))
s.requireAddHold(s.addr2, "10apple", 12345)
s.requireSetOrderInStore(s.getStore(), exchange.NewOrder(98765).WithAsk(&exchange.AskOrder{
MarketId: 1, Seller: s.addr3.String(), Assets: s.coin("3apple"), Price: s.coin("20pear"),
SellerSettlementFlatFee: s.coinP("12fig"), ExternalId: "second order",
}))
s.requireAddHold(s.addr3, "3apple,12fig", 98765)
},
msg: exchange.MsgFillAsksRequest{
Buyer: s.addr1.String(),
MarketId: 1,
TotalPrice: s.coin("70pear"),
AskOrderIds: []uint64{12345, 98765},
BuyerSettlementFees: s.coins("37fig"),
BidOrderCreationFee: s.coinP("10fig"),
},
fArgs: []expBalances{
{
addr: s.addr1,
Expand Down Expand Up @@ -2714,7 +2848,7 @@ func (s *TestSuite) TestMsgServer_MarketSettle() {
},
},
{
name: "two of each with fees and req attrs",
name: "fee in restricted denom",
setup: func() {
s.requireAddFinalizeAndActivateMarker(s.coin("185pear"), s.addr5, "*.have.it")
s.requireAddFinalizeAndActivateMarker(s.coin("18apple"), s.addr5, "*.have.it")
Expand Down Expand Up @@ -2764,6 +2898,62 @@ func (s *TestSuite) TestMsgServer_MarketSettle() {
AskOrderIds: []uint64{1, 333},
BidOrderIds: []uint64{22, 4444},
},
expInErr: []string{invReqErr,
"error collecting exchange fee 2fig,2pear (based off 30fig,34pear) from market 2",
"restricted denom pear cannot be sent to the fee collector",
},
},
{
name: "two of each with fees and req attrs",
setup: func() {
s.requireAddFinalizeAndActivateCoinMarker(s.coin("185pear"), s.addr5)
s.requireAddFinalizeAndActivateMarker(s.coin("18apple"), s.addr5, "*.have.it")
s.requireSetNameRecord("buyer.have.it", s.addr5)
s.requireSetNameRecord("seller.have.it", s.addr5)
s.requireSetNameRecord("market.have.it", s.addr5)
s.requireSetAttr(s.addr1, "seller.have.it", s.addr5)
s.requireSetAttr(s.addr2, "buyer.have.it", s.addr5)
s.requireSetAttr(s.addr3, "seller.have.it", s.addr5)
s.requireSetAttr(s.addr4, "buyer.have.it", s.addr5)
s.requireSetAttr(s.marketAddr2, "market.have.it", s.addr5)
s.requireFundAccount(s.addr1, "20apple,100pear,100fig")
s.requireFundAccount(s.addr2, "20apple,100pear,100fig")
s.requireFundAccount(s.addr3, "20apple,100pear,100fig")
s.requireFundAccount(s.addr4, "20apple,100pear,100fig")

s.requireCreateMarketUnmocked(exchange.Market{
MarketId: 2, AccessGrants: []exchange.AccessGrant{s.agCanOnly(s.addr5, exchange.Permission_settle)},
FeeSellerSettlementRatios: s.ratios("10pear:1pear"),
})

store := s.getStore()
s.requireSetOrderInStore(store, exchange.NewOrder(1).WithAsk(&exchange.AskOrder{
MarketId: 2, Seller: s.addr1.String(), Assets: s.coin("7apple"), Price: s.coin("75pear"),
SellerSettlementFlatFee: s.coinP("10fig"),
}))
s.requireAddHold(s.addr1, "7apple,10fig", 1)
s.requireSetOrderInStore(store, exchange.NewOrder(22).WithBid(&exchange.BidOrder{
MarketId: 2, Buyer: s.addr2.String(), Assets: s.coin("10apple"), Price: s.coin("100pear"),
BuyerSettlementFees: s.coins("20fig"),
}))
s.requireAddHold(s.addr2, "100pear,20fig", 22)
s.requireSetOrderInStore(store, exchange.NewOrder(333).WithAsk(&exchange.AskOrder{
MarketId: 2, Seller: s.addr3.String(), Assets: s.coin("11apple"), Price: s.coin("105pear"),
SellerSettlementFlatFee: s.coinP("5pear"),
}))
s.requireAddHold(s.addr3, "11apple", 333)
s.requireSetOrderInStore(store, exchange.NewOrder(4444).WithBid(&exchange.BidOrder{
MarketId: 2, Buyer: s.addr4.String(), Assets: s.coin("8apple"), Price: s.coin("85pear"),
BuyerSettlementFees: s.coins("10pear"),
}))
s.requireAddHold(s.addr4, "95pear", 4444)
},
msg: exchange.MsgMarketSettleRequest{
Admin: s.addr5.String(),
MarketId: 2,
AskOrderIds: []uint64{1, 333},
BidOrderIds: []uint64{22, 4444},
},
fArgs: followupArgs{
expBals: []expBalances{
{
Expand Down
5 changes: 5 additions & 0 deletions x/marker/keeper/export_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ func (k Keeper) GetIbcTransferModuleAddr() sdk.AccAddress {
return k.ibcTransferModuleAddr
}

// GetFeeCollectorAddr is a TEST ONLY exposure of the feeCollectorAddr value.
func (k Keeper) GetFeeCollectorAddr() sdk.AccAddress {
return k.feeCollectorAddr
}

// CanForceTransferFrom is a TEST ONLY exposure of the canForceTransferFrom value.
func (k Keeper) CanForceTransferFrom(ctx sdk.Context, from sdk.AccAddress) bool {
return k.canForceTransferFrom(ctx, from)
Expand Down
3 changes: 3 additions & 0 deletions x/marker/keeper/keeper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ type Keeper struct {

ibcTransferModuleAddr sdk.AccAddress

feeCollectorAddr sdk.AccAddress

// Used to transfer the ibc marker
ibcTransferServer types.IbcTransferMsgServer

Expand Down Expand Up @@ -128,6 +130,7 @@ func NewKeeper(
authority: authtypes.NewModuleAddress(govtypes.ModuleName).String(),
markerModuleAddr: authtypes.NewModuleAddress(types.CoinPoolName),
ibcTransferModuleAddr: authtypes.NewModuleAddress(ibctypes.ModuleName),
feeCollectorAddr: authtypes.NewModuleAddress(authtypes.FeeCollectorName),
ibcTransferServer: ibcTransferServer,
reqAttrBypassAddrs: types.NewImmutableAccAddresses(reqAttrBypassAddrs),
groupChecker: checker,
Expand Down
18 changes: 18 additions & 0 deletions x/marker/keeper/send_restrictions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,19 @@ func (k Keeper) SendRestrictionFn(ctx sdk.Context, fromAddr, toAddr sdk.AccAddre
// In some cases, it might not be possible to add a bypass to the context.
// If it's from either the Marker or IBC Transfer module accounts, assume proper validation has been done elsewhere.
if types.HasBypass(ctx) || fromAddr.Equals(k.markerModuleAddr) || fromAddr.Equals(k.ibcTransferModuleAddr) {
// But still don't let restricted denoms get sent to the fee collector.
if toAddr.Equals(k.feeCollectorAddr) {
for _, coin := range amt {
markerAddr := types.MustGetMarkerAddress(coin.Denom)
marker, err := k.GetMarker(ctx, markerAddr)
if err != nil {
return nil, err
}
if marker != nil && marker.GetMarkerType() == types.MarkerType_RestrictedCoin {
return nil, fmt.Errorf("cannot send restricted denom %s to the fee collector", coin.Denom)
}
}
}
return toAddr, nil
}

Expand Down Expand Up @@ -91,6 +104,11 @@ func (k Keeper) validateSendDenom(ctx sdk.Context, fromAddr, toAddr, admin sdk.A
return nil
}

// We can't allow restricted coins to end up with the fee collector.
if toAddr.Equals(k.feeCollectorAddr) {
return fmt.Errorf("restricted denom %s cannot be sent to the fee collector", denom)
}

// If there's an admin that has transfer access, it's not a normal bank send and there's nothing more to do here.
if len(admin) > 0 && marker.AddressHasAccess(admin, types.Access_Transfer) {
return nil
Expand Down
44 changes: 42 additions & 2 deletions x/marker/keeper/send_restrictions_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,22 @@ func TestSendRestrictionFn(t *testing.T) {
addrWithDenySend := sdk.AccAddress("addrWithDenySend_____")
addrOther := sdk.AccAddress("addrOther___________")

addrFeeCollector := app.MarkerKeeper.GetFeeCollectorAddr()
bypassAddrs := app.MarkerKeeper.GetReqAttrBypassAddrs()
addrWithBypass := bypassAddrs[0]
addrWithBypassNoDep := bypassAddrs[1]
var addrWithBypass, addrWithBypassNoDep sdk.AccAddress
for _, addr := range bypassAddrs {
if addr.Equals(addrFeeCollector) {
continue
}
if len(addrWithBypass) == 0 {
addrWithBypass = addr
continue
}
addrWithBypassNoDep = addr
break
}
require.NotEmpty(t, addrWithBypass, "addrWithBypass (first bypass address that is not the fee collector)")
require.NotEmpty(t, addrWithBypassNoDep, "addrWithBypassNoDep (second bypass address that is not the fee collector)")

coin := types.MarkerType_Coin
restricted := types.MarkerType_RestrictedCoin
Expand Down Expand Up @@ -191,6 +204,33 @@ func TestSendRestrictionFn(t *testing.T) {
amt: cz(c(1, nrDenom)),
expErr: "",
},
{
name: "restricted to fee collector from normal account",
// include a transfer agent just to make sure that doesn't bypass anything.
ctx: ctxP(types.WithTransferAgent(ctx, addrWithTransfer)),
from: addrOther,
to: addrFeeCollector,
amt: cz(c(1, rDenomNoAttr)),
expErr: "restricted denom " + rDenomNoAttr + " cannot be sent to the fee collector",
},
{
name: "restricted to fee collector from marker module account",
// include a transfer agent just to make sure that doesn't bypass anything.
ctx: ctxP(types.WithTransferAgent(ctx, addrWithTranWithdraw)),
from: app.MarkerKeeper.GetMarkerModuleAddr(),
to: addrFeeCollector,
amt: cz(c(1, rDenomNoAttr)),
expErr: "cannot send restricted denom " + rDenomNoAttr + " to the fee collector",
},
{
name: "restricted to fee collector from ibc transfer module account",
// include a transfer agent just to make sure that doesn't bypass anything.
ctx: ctxP(types.WithTransferAgent(ctx, addrWithTransfer)),
from: app.MarkerKeeper.GetIbcTransferModuleAddr(),
to: addrFeeCollector,
amt: cz(c(1, rDenomNoAttr)),
expErr: "cannot send restricted denom " + rDenomNoAttr + " to the fee collector",
},
{
name: "addr has transfer, denom without attrs",
from: addrWithTransfer,
Expand Down
Loading

0 comments on commit 16fb638

Please sign in to comment.