Allow ics20 authorisation for a specific memo string

[womensrights]

Summary

Currently transfer authorisations are permitted for a specific json memo key or a wildcard allow anything in the memo field. This issue proposes granting a transfer authorisation for a specific memo string in entirety.

Problem Definition

If a transfer authorisation is granted for a specific memo key, it is likely because the transfer is to be used in combination with a middleware, for example pfm. In this case you would grant authorisation for the forward key, but this means that the receiver key does not have any restriction on the input.

  "forward": {
    "channel": "channel-11",
    "port": "transfer",
    "receiver": "stars1twfv52yxcyykx2lcvgl42svw46hsm5dd4ww6xy",
    "retries": 2,
    "timeout": 1712146014542131200
  }

To avoid this, this feature requests being able to grant a transfer authorisation specifying the entire memo string, rather than just a key.

The same problem applies to ibc-hooks, where is the wasm key or wildcard is used, funds could be diverted:

{
  "wasm": {
    "contract": "osmo1qxydza7ctzh9fn7sq5gcf0run8c78wh0jj47f4t5dwc5302r2dnqsp4hu6",
    "msg": {
      "swap_and_action": {
        "user_swap": {
          "swap_exact_asset_in": {
            "swap_venue_name": "osmosis-poolmanager",
            "operations": [
              {
                "pool": "1413",
                "denom_in": "ibc/6928AFA9EA721938FED13B051F9DBF1272B16393D20C49EA5E4901BB76D94A90",
                "denom_out": "ibc/D189335C6E4A68B513C10AB227BF1C1D38C746766278BA3EEB4FB14124F1D858"
              },
              {
                "pool": "1212",
                "denom_in": "ibc/D189335C6E4A68B513C10AB227BF1C1D38C746766278BA3EEB4FB14124F1D858",
                "denom_out": "ibc/498A0751C798A0D9A389AA3691123DADA57DAA4FE165D5C75894505B876BA6E4"
              }
            ]
          }
        },
        "min_asset": {
          "native": {
            "denom": "ibc/498A0751C798A0D9A389AA3691123DADA57DAA4FE165D5C75894505B876BA6E4",
            "amount": "52465"
          }
        },
        "timeout_timestamp": 1712145834030427600,
        "post_swap_action": {
          "transfer": {
            "to_address": "osmo1twfv52yxcyykx2lcvgl42svw46hsm5ddff2hm8"
          }
        },
        "affiliates": []
      }
    }
  }
}

Use cases

The requested use case is to safely be able to use a transfer authorisation in combination with existing middleware, by ensuring only a specific memo string is allowed, not only a key or the wildcard.

Proposal

The proposal would be to extend the transfer authorisation to only be permitted for a specific memo string. I would also consider removing a blanket authorisation for a given key as it seems a bit loose and open to misuse

---

For Admin Use

• [ ] Not duplicate issue
• [ ] Appropriate labels applied
• [ ] Appropriate contributors tagged/assigned

[george-aj]

Fully support this addition and we could make use of it as soon as it's available.

[crodriguezvega]

We discussed internally and we can support this if we implement the following change:

diff --git a/modules/apps/transfer/types/transfer_authorization.go b/modules/apps/transfer/types/transfer_authorization.go
index 242bc86e8..dc5bc2753 100644
--- a/modules/apps/transfer/types/transfer_authorization.go
+++ b/modules/apps/transfer/types/transfer_authorization.go
@@ -171,6 +171,12 @@ func validateMemo(ctx sdk.Context, memo string, allowedPacketDataList []string)
                return nil
        }
 
+       // if allowedPacketDataList has only 1 element and it equals the memo string
+       // then accept the memo
+       if len(allowedPacketDataList) == 1 && allowedPacketDataList[0] == memo {
+               return nil
+       }
+

The AllowedPacketData array of the allocation needs to have only one element and it needs to exactly match the memo used in MsgTransfer. @george-aj Would this work for your use case?

[kakucodes]

We discussed internally and we can support this if we implement the following change:

diff --git a/modules/apps/transfer/types/transfer_authorization.go b/modules/apps/transfer/types/transfer_authorization.go
index 242bc86e8..dc5bc2753 100644
--- a/modules/apps/transfer/types/transfer_authorization.go
+++ b/modules/apps/transfer/types/transfer_authorization.go
@@ -171,6 +171,12 @@ func validateMemo(ctx sdk.Context, memo string, allowedPacketDataList []string)
                return nil
        }
 
+       // if allowedPacketDataList has only 1 element and it equals the memo string
+       // then accept the memo
+       if len(allowedPacketDataList) == 1 && allowedPacketDataList[0] == memo {
+               return nil
+       }
+

The AllowedPacketData array of the allocation needs to have only one element and it needs to exactly match the memo used in MsgTransfer. @george-aj Would this work for your use case?

Am I right to think that this would mean there's only one match possible? If I wanted to give a transfer authorization to have a grantee be able to transfer ATOM both to Chain A via some PFM route and to transfer ATOM to Chain B via some other PFM route would that be expressible? @crodriguezvega

[crodriguezvega]

Thanks for the comment, @george-aj.

Am I right to think that this would mean there's only one match possible?

Yes, correct. The whole memo string in MsgTransfer should match the JSON-encoded string with which the authorisation was set up.

If I wanted to give a transfer authorization to have a grantee be able to transfer ATOM both to Chain A via some PFM route and to transfer ATOM to Chain B via some other PFM route would that be expressible?

So if I understand correctly, you would like to give a list of possible allowed memo object strings, and then the memo string in MsgTransfer should contain only one or many of those allowed memo object strings, right?

If we needed to support something like this, I guess we could implement something like the following in validateMemo, where we iterate over the list of allowed memo JSON-encoded strings and remove them from the input memo string in MsgTransfer. If the resulting memo string at the end is {}, then that would mean that it doesn't contain any non-allowed memo object:

dst := &bytes.Buffer{}
json.Compact(dst, []byte(memo))
memo = dst.String()

for _, allowJson := range allowedPacketDataList {
  ctx.GasMeter().ConsumeGas(gasCostPerIteration, "transfer authorization")

  dst := &bytes.Buffer{}
  json.Compact(dst, []byte(fmt.Sprintf("{%s}", allowJson)))
  allowJson = dst.String()

  allowJson, _ = strings.CutPrefix(allowJson, "{")
  allowJson, _ = strings.CutSuffix(allowJson, "}")
  memo = strings.Replace(memo, allowJson, "", 1)
}

if memo != "{}" {
  return errorsmod.Wrapf(ErrInvalidAuthorization, "not allowed packet data memo: %s", memo)
}

If this works, then maybe it's a simple enough way to satisfy the requirements (in general we would prefer not to implement complex logic on top of the memo field). There's definitely a bit of string magic going on that might be not very robust, but I hope at least the code gives an idea of the general intention; I am sure we could figure out ways of improving this (or doing something completely different that it's not too complicated!). The call to json.Compact is there to remove any insignificant space characters in the JSON-encoded strings.

We could add also a step, here or maybe better in ValidateBasic of TransferAuthorization) to make sure that all elements of AllowedPacketData are proper JSON-encoded strings (although we would need to take into account that this list could also be just a list of packet data keys and not complete memo objects, if we want to keep the current behaviour).

I would like to get also the opinion from the rest of the team, but how would this look to you, @george-aj?

[george-aj]

Hi @crodriguezvega,

I can't think of a scenario where we would be putting multiple abstract, but separate strings into a memo field. This is very much a black and white issue, either the string is allowed or it isn't. That being said to simplify your implementation of this I would be all for a simple iteration that checks the memo against the allowed list of memo strings and if there is a match it allows the MsgExec to move forward.

So if I understand correctly, you would like to give a list of possible allowed memo object strings, and then the memo string in MsgTransfer should contain only one or many of those allowed memo object strings, right?

[crodriguezvega]

Thanks for the clarification, @george-aj. Yes, that would simplify the code!

Assuming we still want to keep the current functionality of including packet data keys in AllowedPacketData then we could check in ValidateBasic whether all the elements of AllowedPacketData are JSON-encoded strings or not (i.e. we wouldn't support mixing JSON-encoded memo strings and packet data keys in AllowedPacketData). And in validateMemo we loop over the elements of AllowedPacketData and if the element is a JSON-encoded string we compare with the input memo, otherwise we do the current logic of removing the key from the input memo JSON object.

The changes would look like this:

diff --git a/modules/apps/transfer/types/transfer_authorization.go b/modules/apps/transfer/types/transfer_authorization.go
index 242bc86e8..6db7647fc 100644
--- a/modules/apps/transfer/types/transfer_authorization.go
+++ b/modules/apps/transfer/types/transfer_authorization.go
@@ -1,6 +1,7 @@
 package types

 import (
+       "bytes"
        "context"
        "encoding/json"
        "math/big"
@@ -131,6 +132,28 @@ func (a TransferAuthorization) ValidateBasic() error {
                        }
                        found[allocation.AllowList[i]] = true
                }
+
+               if len(allocation.AllowedPacketData) > 0 {
+                       // if the first item of AllowedPacketData is a JSON-encoded memo string,
+                       // then all items must be JSON-encoded memo strings
+                       mustBeMemoStrings := false
+                       jsonObject := make(map[string]interface{})
+                       err := json.Unmarshal([]byte(allocation.AllowedPacketData[0]), &jsonObject)
+                       if err == nil {
+                               mustBeMemoStrings = true
+                       }
+
+                       for _, elem := range allocation.AllowedPacketData {
+                               err := json.Unmarshal([]byte(elem), &jsonObject)
+                               if mustBeMemoStrings && err != nil {
+                                       return errorsmod.Wrapf(ErrInvalidAuthorization, "expected all items of AllowedPacketData to be JSON-encoded memo strings")
+                               }
+
+                               if !mustBeMemoStrings && err == nil {
+                                       return errorsmod.Wrapf(ErrInvalidAuthorization, "expected all items of AllowedPacketData to be packet data keys")
+                               }
+                       }
+               }
        }

        return nil
@@ -171,6 +194,10 @@ func validateMemo(ctx sdk.Context, memo string, allowedPacketDataList []string)
                return nil
        }

+       dst := &bytes.Buffer{}
+       json.Compact(dst, []byte(memo))
+       memo = dst.String()
+
        jsonObject := make(map[string]interface{})
        err := json.Unmarshal([]byte(memo), &jsonObject)
        if err != nil {
@@ -179,12 +206,27 @@ func validateMemo(ctx sdk.Context, memo string, allowedPacketDataList []string)

        gasCostPerIteration := ctx.KVGasConfig().IterNextCostFlat

-       for _, key := range allowedPacketDataList {
+       for _, elem := range allowedPacketDataList {
                ctx.GasMeter().ConsumeGas(gasCostPerIteration, "transfer authorization")

-               _, ok := jsonObject[key]
-               if ok {
-                       delete(jsonObject, key)
+               // elem is either a packet data key or a JSON-encoded memo string
+               // if it is a packet data key, then the key is removed from the memo JSON object
+               // if it is a JSON-encoded memo string and it matches the input memo string, then validation is successful
+               jsonObject := make(map[string]interface{})
+               err := json.Unmarshal([]byte(elem), &jsonObject)
+               if err != nil {
+                       _, ok := jsonObject[elem]
+                       if ok {
+                               delete(jsonObject, elem)
+                       }
+               } else {
+                       dst := &bytes.Buffer{}
+                       json.Compact(dst, []byte(elem))
+                       allowedMemo := dst.String()
+
+                       if memo == allowedMemo {
+                               return nil
+                       }
                }
        }

[george-aj]

I agree that supporting the mixing of the two types in a single authorization is not needed.

Assuming we still want to keep the current functionality of including packet data keys in AllowedPacketData then we could check in ValidateBasic whether all the elements of AllowedPacketData are JSON-encoded strings or not (i.e. we wouldn't support mixing JSON-encoded memo strings and packet data keys in AllowedPacketData)

However, for our purposes most of the time the memo string will be JSON. If I'm reading your code right, all three examples (the two in the write up and the one below) would all be parsed as JSON and packet data key validation run on them. For the two examples in the write up that is desired behavior, while for the below one it is not.

{ 
    "autopilot": {
          "receiver": "strideXXX", 
          "stakeibc": {
               "action": "LiquidStake",
          }
    }
}

What about having the memo string validation run first and if it fails then the packet data key validation run?

[crodriguezvega]

What about having the memo string validation run first and if it fails then the packet data key validation run?

So you mean doing something like this instead?

if err != nil {
  dst := &bytes.Buffer{}
  json.Compact(dst, []byte(elem))
  allowedMemo := dst.String()

  if memo == allowedMemo {
    return nil
  }
} else {
  _, ok := jsonObject[elem]
  if ok {
    delete(jsonObject, elem)
  }
}

Does the order actually matter? If in ValidateBasic we had made sure that AllowedPacketData contains either keys or JSON strings, then will validate the memo string based on one or the other. Does this sound correct?

[george-aj]

Does the order actually matter? If in ValidateBasic we had made sure that AllowedPacketData contains either keys or JSON strings, then will validate the memo string based on one or the other. Does this sound correct?

As long as both validations (the packet key and the packet memo against the list of stored memos) are both run and any match is found that let's things move forward, you are right it probably doesn't matter which is run first.

[crodriguezvega]

Discussed with @womensrights and we will follow @srdtrk's suggestion of dropping support of keys. We will support only memo strings.

[crodriguezvega]

@george-aj We have a PR open for this issue, in case you want to take a look. I will probably merge today, but if you have any feedback, please comment on the PR, and I will address it separately.