WASM Restricted Marker Transfer Question

[ghost]

Problem Definition

Currently, it is not possible to perform a restricted marker transfer from a smart contract, even if the contract has transfer permission. This is due to the fact that the marker module requires chain of trust (ie both admin and sender must sign). See here.

However, the wasm module requires that the contract be the only signer of Msgs encoded through the internal API. See here.

Leaving this as an open question at the moment. What is the appropriate (secure) way to allow smart contracts to utilize transfer of restricted marker tokens?

This is important for the ATS exchange contract work being done by @ktalley-figure - used by @jtalis and @leeduan

Some food for thought:

• Create a Msg only used by provwasm
• Some sort of "pre-authorized" transfer permission for smart contracts on accounts holding restricted markers.
• Both the above?
• Can we just allow 'admin' as the signer?

Any other ideas?

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[arnabmitra]

@dpederson-figure @iramiller i'll start with the dumb question, since the smart contract has already transfer permission on the marker, why can't the smart contract be made an // ACCESS_ADMIN is the ability to add access grants for accounts to the list of marker permissions. ACCESS_ADMIN = 6 [(gogoproto.enumvalue_customname) = "Admin"]; admin on the marker? i mean not ideal, but we are giving it transfer permission ...so 🤔

[iramiller]

Transfer isn't change permission. This as removing something someone owns via transfer should use a two stage signed commit.

[arnabmitra]

gotcha.. next question ..thoughts on ..how about using the approach of allowance in erc-20 spec..pretty similar to a multi party signed contract..but on the marker struct itself, maybe a new struct(1:1 with the marker)

📋 Copy
The ERC-20 standard allow an address to give an allowance to another address to be able to retrieve tokens from it. This getter returns the remaining number of tokens that the spender will be allowed to spend on behalf of owner. This function is a getter and does not modify the state of the contract and should return 0 by default.

@dpederson-figure @iramiller

[iramiller]

I would add a method to the marker module that can call a smart contract for approval. This allows the coin holder to sign the move, the smart contract with transfer auth to approve the move, and the marker module to actually move the coin. The smart contract in this case would simply return true or false to approve or deny the transfer similar to a signature approval.

That's all for me today. Good luck.

[ghost]

Right now, the marker keeper must be initialized before the wasm keeper due to the existing integrations. There will be runtime panics if the order is reversed. Given that, I'm not sure it's possible to query a contract from within the marker module itself (add bidirectional queries). Probably easier to have the marker module store the pre-authorized state, and verify upon receiving a MsgPreauthTransfer to handle from the contract.

[iramiller]

Good points... if the pre-auth approach is used then note that there is a module built entirely for this purpose already with auth amounts, operations, etc which is in the SDK master but not cut into release until 0.43

[leeduan]

Any update on an approach here?

[iramiller]

The ideal approach is likely to use the upcoming authz module to submit an approval for the withdraw that the smart contract can reference... the 0.43 sdk release will include this module (and fee grant support too)... we could work out a way to test this module now (it’s been code complete for a while but the SDK team has held up the overall release for other pieces). Alternatively we can attempt some sort of hack or disable the security check to accelerate the timeline. Neither is these last two feel like a great choice especially if we have a couple more weeks to wait (eta on 0.43 is about a week out right now).

[leeduan]

My only concern is a week out can turn into two or three; then it leaves us very little time to execute on deliverables

[ghost]

It is also unlikely there will be a CosmWasm release we can use for our smart contract lib immediately when 0.43 is released. There will be, at best, a couple weeks lag time there.

[arnabmitra]

i might be totally wrong here:
But i think Authz will not work out of the box

// SendAuthorization allows the grantee to spend up to spend_limit coins from
// the granter's account.
type SendAuthorization struct {
	SpendLimit github_com_cosmos_cosmos_sdk_types.Coins `protobuf:"bytes,1,rep,name=spend_limit,json=spendLimit,proto3,castrepeated=github.com/cosmos/cosmos-sdk/types.Coins" json:"spend_limit"`
}

SendAuthorization will go through Msg/Send(bank module) which will error out for restricted coins

func (k msgServer) Send(goCtx context.Context, msg *types.MsgSend) (*types.MsgSendResponse, error) {
	ctx := sdk.UnwrapSDKContext(goCtx)

	if err := k.SendEnabledCoins(ctx, msg.Amount...); err != nil {
		return nil, err
	}

if we use Msg/Transfer from marker module, we could use the

// GenericAuthorization gives the grantee unrestricted permissions to execute
// the provided method on behalf of the granter's account.
type GenericAuthorization struct {
	// method name to grant unrestricted permissions to execute
	// Note: MethodName() is already a method on `GenericAuthorization` type,
	// we need some custom naming here so using `MessageName`
	MessageName string `protobuf:"bytes,1,opt,name=method_name,json=methodName,proto3" json:"method_name,omitempty"`
}

this however does not have a limit which to me as a big deal imo..

also another gotcha is the signers

if len(signers) != 1 {
			return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "authorization can be given to msg with only one signer")
		}

which if exposed can be called without the authz route and be abused

so what i am trying to say is that, we would need to customize Restricted marker transfer, maybe using authz module..

Maybe a discussion needed?
Could be i am overthinking this and maybe it all works..but just bringing it up

[iramiller]

Totally expected that we will have some tweaking to do here around authz... the premise was that using the standardized approaches to indicating authorized access vs a one off would bee preferred... we should start integrating on a branch though so we can ship as soon as the upstream release is ready... There will be quite a few changes with 0.43 sdk so getting ahead of that where possible is important.

[iramiller]

I feel like we should get a solution in the system for this in the release cut tomorrow. @arnabmitra / @leeduan / @dpederson-figure -- I propose that we disable the two signature requirement on the transfer function right now.

This does present a risk to users who believe that they hold a RESTRICTED_COIN withadmin [or more directly] transfer permissions assigned. These restricted markers can be stolen out of their account without their permission. Unfortunately the integration of 0.43 in a safe manner and then solving this constraint correctly will take significant time. Given our two week release cycle I would not expect a proper fix for this until perhaps v1.5.0.

In light of what amounts to opening what a user would perceive as security hole we will want to track a high priority issue to fix this correctly which means that we would release this update as soon as possible. This will result in a Client SDK breaking change for anyone relying on the temporary relaxed permissions. So long as @leeduan / @dpederson-figure / @ktalley-figure are aware of this and can commit to quickly updating their flow when we have a solution in place I think the temporary fix is the best fix. I imagine the upgraded solution will have 2-3 weeks of notice before it is active on mainnet which will give clients an opportunity to update.

[ghost]

I'm OK with this as a temporary solution. And if/when required, I'll make cutting a new release of provwasm my top priority.

[arnabmitra]

I feel like we should get a solution in the system for this in the release cut tomorrow. @arnabmitra / @leeduan / @dpederson-figure -- I propose that we disable the two signature requirement on the transfer function right now.

This does present a risk to users who believe that they hold a RESTRICTED_COIN withadmin [or more directly] transfer permissions assigned. These restricted markers can be stolen out of their account without their permission. Unfortunately the integration of 0.43 in a safe manner and then solving this constraint correctly will take significant time. Given our two week release cycle I would not expect a proper fix for this until perhaps v1.5.0.

In light of what amounts to opening what a user would perceive as security hole we will want to track a high priority issue to fix this correctly which means that we would release this update as soon as possible. This will result in a Client SDK breaking change for anyone relying on the temporary relaxed permissions. So long as @leeduan / @dpederson-figure / @ktalley-figure are aware of this and can commit to quickly updating their flow when we have a solution in place I think the temporary fix is the best fix. I imagine the upgraded solution will have 2-3 weeks of notice before it is active on mainnet which will give clients an opportunity to update.

^^ agree, we will let the group know when existing restrictions come back, from what i understand of authz 1. sender of the coin grants access to admin(grantee/sc) 2. grantee moves coin