Fix the MarkerTransferAuthorization Accept function and TransferCoin authz handling to prevent problems when other authorization types are used

[egaxhaj]

Summary of Bug

When there is a MsgTransferRequest the Accept method decrements funds and returns an updated MarkerTrasnferAuthorization with the new limit amount when there exists an msg.administrator. Then the funds are again decremented in the authzHandler function in the Marker module. This may lead to double decrementing when the transfer type is something other than MsgTransferRequest.

Line 710:

provenance/x/marker/keeper/marker.go

Lines 699 to 717 in 1283582

	func (k Keeper) authzHandler(ctx sdk.Context, admin sdk.AccAddress, from sdk.AccAddress, amount sdk.Coin) error {
	markerAuth := types.MarkerTransferAuthorization{}
	authorization, expireTime := k.authzKeeper.GetCleanAuthorization(ctx, admin, from, markerAuth.MsgTypeURL())
	if authorization == nil {
	return fmt.Errorf("%s account has not been granted authority to withdraw from %s account", admin, from)
	}
	accept, err := authorization.Accept(ctx, &types.MsgTransferRequest{Amount: amount})
	if err != nil {
	return err
	}
	if accept.Accept {
	limitLeft, _ := authorization.(*types.MarkerTransferAuthorization).DecreaseTransferLimit(amount)
	if limitLeft.IsZero() {
	return k.authzKeeper.DeleteGrant(ctx, admin, from, markerAuth.MsgTypeURL())
	}
	return k.authzKeeper.SaveGrant(ctx, admin, from, &types.MarkerTransferAuthorization{TransferLimit: limitLeft}, expireTime)
	}
	return fmt.Errorf("authorization was not accepted for %s", admin)
	}

We're deleting here when limitLeft.IsZero() and ignoring accept.Delete which is set by shouldDelete (see below). If we check for if accept.Delete { ... } it helps avoid type casting and allows for other authorization types to work as well.

The logic in authzHandler() and MarkerTransferAuthorization.Accept() are confusing.

provenance/x/marker/types/authz.go

Lines 26 to 44 in 1283582

	func (a MarkerTransferAuthorization) Accept(ctx sdk.Context, msg sdk.Msg) (authz.AcceptResponse, error) {
	switch msg := msg.(type) {
	case *MsgTransferRequest:
	limitLeft, isNegative := a.DecreaseTransferLimit(msg.Amount)
	if isNegative {
	return authz.AcceptResponse{}, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFunds, "requested amount is more than spend limit")
	} else if msg.Administrator != "" && msg.Administrator == msg.FromAddress {
	shouldDelete := false
	if limitLeft.IsZero() {
	shouldDelete = true
	}
	return authz.AcceptResponse{Accept: true, Delete: shouldDelete, Updated: &MarkerTransferAuthorization{TransferLimit: limitLeft}}, nil
	}
	// does not return and an updated transfer limit, this is handled in marker module
	return authz.AcceptResponse{Accept: true, Delete: false, Updated: &MarkerTransferAuthorization{TransferLimit: a.TransferLimit}}, nil
	default:
	return authz.AcceptResponse{}, sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "type mismatch")
	}
	}

Line 40: Why is the update occurring in the marker module?

Version

Steps to Reproduce

---

For Admin Use

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

[dwedul-figure]

After some research, it looks like this is working as intended, except accidentally, and in such a way that would be easy to have things go wrong.

In the Accept method, the limit is decremented if the from == admin. In the TransferCoin endpoint, we only call Accept when from != admin. But then, in the TransferCoin endpoint, if from != admin, we then manually decrement the limit.

That decrementing should ONLY be handled in the Accept method. In the TransferCoin endpoint, where we call Accept, we then need to properly handle its result, i.e. deleting if appropriate, or updating with the returned authorization if appropriate.

Basically, the MarkerTransferAuthorization.Accept method should always return the updated authorization and delete flag (without the from/admin comparison). Then the authzHandler should not care what type of authorization is being used as long as Accept says it's okay. It should also use the result of Accept to decide when to update or delete the authorization entry.

[dwedul-figure]

There also needs to be unit tests on the MarkerTransferAuthorization stuff and authzHandler.

[dwedul-figure]

I guess there is still actually a bug.

Because authzHandler assumes that the authorization is a MarkerTransferAuthorization authorization, any other type of authorization will get messed up. E.g. if it's a generic authorization, limitLeft, _ := authorization.(*types.MarkerTransferAuthorization).DecreaseTransferLimit(amount) will always result in a negative limit (if it doesn't panic), which will then cause the authorization to be deleted.