diff --git a/x/marker/keeper/keeper_test.go b/x/marker/keeper/keeper_test.go index 9f04034a5e..8c4f2c438d 100644 --- a/x/marker/keeper/keeper_test.go +++ b/x/marker/keeper/keeper_test.go @@ -429,7 +429,7 @@ func TestAccountKeeperMintBurnCoins(t *testing.T) { require.Error(t, app.MarkerKeeper.DeleteMarker(ctx, user, "testcoin")) // Remove escrow balance from account - require.NoError(t, app.BankKeeper.SendCoinsFromAccountToModule(ctx, addr, "mint", sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.OneInt()))), "sending coins to module") + require.NoError(t, app.BankKeeper.SendCoinsFromAccountToModule(types.WithBypass(ctx), addr, "mint", sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.OneInt()))), "sending coins to module") // Succeeds because the bond denom coin was removed. require.NoError(t, app.MarkerKeeper.DeleteMarker(ctx, user, "testcoin")) diff --git a/x/marker/keeper/marker.go b/x/marker/keeper/marker.go index 7b14970ab9..853c23807d 100644 --- a/x/marker/keeper/marker.go +++ b/x/marker/keeper/marker.go @@ -640,12 +640,22 @@ func (k Keeper) TransferCoin(ctx sdk.Context, from, to, admin sdk.AccAddress, am if err != nil { return fmt.Errorf("marker not found for %s: %w", amount.Denom, err) } + if m.GetMarkerType() != types.MarkerType_RestrictedCoin { return fmt.Errorf("marker type is not restricted_coin, brokered transfer not supported") } + if !m.AddressHasAccess(admin, types.Access_Transfer) { return fmt.Errorf("%s is not allowed to broker transfers", admin.String()) } + + // If going to a restricted marker, the admin must have deposit access on that marker too. + if toMarker, _ := k.GetMarker(ctx, to); toMarker != nil && toMarker.GetMarkerType() == types.MarkerType_RestrictedCoin { + if !toMarker.AddressHasAccess(admin, types.Access_Deposit) { + return fmt.Errorf("%s does not have deposit access on %s (%s)", admin, to, toMarker.GetDenom()) + } + } + if !admin.Equals(from) { switch { case !m.AllowsForcedTransfer(): @@ -657,9 +667,11 @@ func (k Keeper) TransferCoin(ctx sdk.Context, from, to, admin sdk.AccAddress, am return fmt.Errorf("funds are not allowed to be removed from %s", from) } } + if k.bankKeeper.BlockedAddr(to) { return fmt.Errorf("%s is not allowed to receive funds", to) } + // set context to having access to bypass attribute restriction test // send the coins between accounts (does not check send_enabled on coin denom) if err = k.bankKeeper.SendCoins(types.WithBypass(ctx), from, to, sdk.NewCoins(amount)); err != nil { diff --git a/x/marker/keeper/send_restrictions.go b/x/marker/keeper/send_restrictions.go index 8ef4bd3c45..6c8b7a4e9f 100644 --- a/x/marker/keeper/send_restrictions.go +++ b/x/marker/keeper/send_restrictions.go @@ -22,8 +22,48 @@ func (k Keeper) SendRestrictionFn(ctx sdk.Context, fromAddr, toAddr sdk.AccAddre return toAddr, nil } + // If it's coming from a marker, make sure the withdraw is allowed. + admin := types.GetTransferAgent(ctx) + if fromMarker, _ := k.GetMarker(ctx, fromAddr); fromMarker != nil { + // It shouldn't be possible for the fromAddr to be a marker without a transfer + // agent because no keys exist for any marker accounts (so it can't sign a Tx). + // So, to be on the safe side, we return an error if that's the case. + if len(admin) == 0 { + return nil, fmt.Errorf("cannot withdraw from marker account %s (%s)", + fromAddr.String(), fromMarker.GetDenom()) + } + + // That transfer agent must have withdraw access. + if !fromMarker.AddressHasAccess(admin, types.Access_Withdraw) { + return nil, fmt.Errorf("%s does not have withdraw access for %s (%s)", + admin.String(), fromAddr.String(), fromMarker.GetDenom()) + } + + // Check to see if marker is active; the coins created by a marker can only be withdrawn when it is active. + // Any other coins that may be present (collateralized assets?) can be transferred. + if fromMarker.GetStatus() != types.StatusActive && !amt.AmountOf(fromMarker.GetDenom()).IsZero() { + return nil, fmt.Errorf("cannot withdraw marker created coins (%s) from marker %s (%s) that is not in active status (%s)", + amt, fromAddr, fromMarker.GetDenom(), fromMarker.GetStatus().String()) + } + } + + // If it's going to a restricted marker, either the admin (if there is one) or + // fromAddr (if there isn't an admin) must have deposit access on that marker. + toMarker, _ := k.GetMarker(ctx, toAddr) + if toMarker != nil && toMarker.GetMarkerType() == types.MarkerType_RestrictedCoin { + addr := admin + if len(addr) == 0 { + addr = fromAddr + } + if !toMarker.AddressHasAccess(addr, types.Access_Deposit) { + return nil, fmt.Errorf("%s does not have deposit access for %s (%s)", + addr.String(), toAddr.String(), toMarker.GetDenom()) + } + } + + // Check the ability to send each denom involved. for _, coin := range amt { - if err := k.validateSendDenom(ctx, fromAddr, toAddr, coin.Denom); err != nil { + if err := k.validateSendDenom(ctx, fromAddr, toAddr, admin, coin.Denom, toMarker); err != nil { return nil, err } } @@ -33,19 +73,7 @@ func (k Keeper) SendRestrictionFn(ctx sdk.Context, fromAddr, toAddr sdk.AccAddre // validateSendDenom makes sure a send of the given denom is allowed for the given addresses. // This is NOT the validation that is needed for the marker Transfer endpoint. -func (k Keeper) validateSendDenom(ctx sdk.Context, fromAddr, toAddr sdk.AccAddress, denom string) error { - // If it's going to a restricted marker, fromAddr must have deposit access on that marker. - var toMarker types.MarkerAccountI - if toAcct := k.authKeeper.GetAccount(ctx, toAddr); toAcct != nil { - toAcctAsMarker, isMarker := toAcct.(types.MarkerAccountI) - if isMarker { - toMarker = toAcctAsMarker - } - } - if toMarker != nil && toMarker.GetMarkerType() == types.MarkerType_RestrictedCoin && !toMarker.AddressHasAccess(fromAddr, types.Access_Deposit) { - return fmt.Errorf("%s does not have deposit access for %s (%s)", fromAddr.String(), toAddr.String(), toMarker.GetDenom()) - } - +func (k Keeper) validateSendDenom(ctx sdk.Context, fromAddr, toAddr, admin sdk.AccAddress, denom string, toMarker types.MarkerAccountI) error { markerAddr := types.MustGetMarkerAddress(denom) marker, err := k.GetMarker(ctx, markerAddr) if err != nil { @@ -57,7 +85,15 @@ func (k Keeper) validateSendDenom(ctx sdk.Context, fromAddr, toAddr sdk.AccAddre return nil } - // If from address is in deny list, prevent sending of restricted marker + // 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 + } + + // If from address is in the deny list, prevent sending of restricted marker. + // If the fromAddr is both on the send-deny list and has transfer access, we want to deny this send. + // They can either take themselves off the list and do the send again, or just use the transfer endpoint. + // But for normal sends (without a transfer agent), we want the send-deny list enforced first. if k.IsSendDeny(ctx, markerAddr, fromAddr) { return fmt.Errorf("%s is on deny list for sending restricted marker", fromAddr.String()) } @@ -67,24 +103,23 @@ func (k Keeper) validateSendDenom(ctx sdk.Context, fromAddr, toAddr sdk.AccAddre return nil } - reqAttr := marker.GetRequiredAttributes() - // If going to a marker, transfer permission is required regardless of whether it's coming from a bypass. // If someone wants to deposit funds from a bypass account, they can either send the funds to a valid // intermediary account and deposit them from there, or give the bypass account deposit and transfer permissions. // It's assumed that a marker address cannot be in the bypass list. if toMarker != nil { - return fmt.Errorf("%s does not have transfer permissions", fromAddr.String()) + return fmt.Errorf("%s does not have transfer access for %s", fromAddr.String(), denom) } // If there aren't any required attributes, transfer permission is required unless coming from a bypass account. - // It's assumed that the only way the restricted coins without required attributes can got into a bypass + // It's assumed that the only way the restricted coins without required attributes can get into a bypass // account is by someone with transfer permission, which is then conveyed for this transfer too. + reqAttr := marker.GetRequiredAttributes() if len(reqAttr) == 0 { if k.IsReqAttrBypassAddr(fromAddr) { return nil } - return fmt.Errorf("%s does not have transfer permissions", fromAddr.String()) + return fmt.Errorf("%s does not have transfer permissions for %s", fromAddr.String(), denom) } // At this point, we know there are required attributes and that fromAddr does not have transfer permission. diff --git a/x/marker/keeper/send_restrictions_test.go b/x/marker/keeper/send_restrictions_test.go index af01e4b175..6ac905f565 100644 --- a/x/marker/keeper/send_restrictions_test.go +++ b/x/marker/keeper/send_restrictions_test.go @@ -162,7 +162,7 @@ func TestSendRestrictionFn(t *testing.T) { from: owner, to: addrWithAttrs, amt: cz(c(1, rDenomNoAttr)), - expErr: fmt.Sprintf("%s does not have transfer permissions", owner.String()), + expErr: fmt.Sprintf("%s does not have transfer permissions for %s", owner.String(), rDenomNoAttr), }, { name: "restricted marker with required attributes but none match", @@ -284,7 +284,7 @@ func TestSendRestrictionFn(t *testing.T) { from: addrWithDeposit, to: rMarkerNoAttr.GetAddress(), amt: cz(c(1, rDenomNoAttr)), - expErr: addrWithDeposit.String() + " does not have transfer permissions", + expErr: addrWithDeposit.String() + " does not have transfer access for " + rDenomNoAttr, }, { name: "send to another marker with transfer on denom but no deposit on to", @@ -299,7 +299,7 @@ func TestSendRestrictionFn(t *testing.T) { from: addrWithDeposit, to: rMarker1Attr.GetAddress(), amt: cz(c(1, rDenomNoAttr)), - expErr: addrWithDeposit.String() + " does not have transfer permissions", + expErr: addrWithDeposit.String() + " does not have transfer access for " + rDenomNoAttr, }, { name: "send to another marker with transfer on denom and deposit on to", @@ -328,14 +328,14 @@ func TestSendRestrictionFn(t *testing.T) { from: addrWithBypass, to: rMarker1Attr.GetAddress(), amt: cz(c(1, rDenom1Attr)), - expErr: addrWithBypass.String() + " does not have transfer permissions", + expErr: addrWithBypass.String() + " does not have transfer access for " + rDenom1Attr, }, { name: "to marker without req attrs from addr with bypass", from: addrWithBypass, to: rMarkerNoAttr.GetAddress(), amt: cz(c(1, rDenomNoAttr)), - expErr: addrWithBypass.String() + " does not have transfer permissions", + expErr: addrWithBypass.String() + " does not have transfer access for " + rDenomNoAttr, }, { name: "no req attrs from addr with bypass", @@ -357,7 +357,7 @@ func TestSendRestrictionFn(t *testing.T) { from: addrOther, to: addrWithBypass, amt: cz(c(1, rDenomNoAttr)), - expErr: addrOther.String() + " does not have transfer permissions", + expErr: addrOther.String() + " does not have transfer permissions for " + rDenomNoAttr, }, { name: "no req attrs to addr with bypass from with transfer", @@ -892,7 +892,7 @@ func TestQuarantineOfRestrictedCoins(t *testing.T) { } setAttr(t, addrQWithAttr) - noTransErr := addrWithoutTransfer.String() + " does not have transfer permissions" + noTransErr := addrWithoutTransfer.String() + " does not have transfer permissions for " + denomNoReqAttr noAttrErr := func(addr sdk.AccAddress) string { return fmt.Sprintf("address %s does not contain the %q required attribute: %q", addr, denom1ReqAttr, reqAttr) }