Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add list of allowed packet data keys to Allocation of TransferAuthorization #5169

Closed
crodriguezvega opened this issue Nov 24, 2023 · 7 comments · Fixed by #5280
Closed

Add list of allowed packet data keys to Allocation of TransferAuthorization #5169

crodriguezvega opened this issue Nov 24, 2023 · 7 comments · Fixed by #5280
Assignees
Labels
20-transfer change: state machine breaking Issues or PRs that break consensus (need to be released in at least a new minor version) type: feature New features, sub-features or integrations
Milestone

Comments

@crodriguezvega
Copy link
Contributor

crodriguezvega commented Nov 24, 2023

Thanks @colin-axner for the idea.


Currently a transfer grantee can submit a MsgTransfer with a non-empty memo field and any packet data. We should give the granter the possibility to specify what packet data keys they allow.

Add here a constant string that holds the value that we expect if a granter wishes to allow any packet data key in the memo field of MsgTransfer:

// AllowAllPacketDataKeys holds the string key that allows all packet data keys in authz transfer messages
AllowAllPacketDataKeys = "*"

Add an extra field to message Allocation in authz.proto that may hold the keys allowed to be used in the memo field:

// allow list of packet data keys, an empty list.
// an empty list prohibits all packet data keys; a list only with "*" permits any packet data key
repeated string allow_packet_data_list = 5; 

Implement function to check that the memo field contains only allowed packet data keys:

func areAllowedPacketDataKeys(
  ctx sdk.Context,
  memo string,
  allowePacketDataList []string
) (bool, error) {
  // if the allow list is empty, then the memo must be an empty string
  if len(allowePacketDataList) == 0 {
    return len(strings.TrimSpace(memo)) == 0, nil
  }

  if len(allowePacketDataList) == 1 && allowePacketDataList[0] == AllowAllPacketDataKeys {
    return true, nil
  }

  jsonObject := make(map[string]interface{})
  err := json.Unmarshal([]byte(memo), &jsonObject)
  if err != nil {
    return false, err
  }

  gasCostPerIteration := ctx.KVGasConfig().IterNextCostFlat
  for k := range jsonObject {
    delete(jsonObject, k)
  }

  for key := range jsonObject {
    ctx.GasMeter().ConsumeGas(gasCostPerIteration, "transfer authorization")

    if !slices.Contains(allowePacketDataList, key) {
      return false, fmt.Errorf("not allowed packet data key: %s", key)
    }
  }

  return true, nil
}

Use the above function here:

if _, err := areAllowedPacketDataKeys(ctx, msgTransfer.Memo, allocation.AllowPacketDataList); err != nil {
  return authz.AcceptResponse{}, errorsmod.Wrapf(ErrInvalidMemo, "error: %v", err)
}
@crodriguezvega crodriguezvega added 20-transfer type: feature New features, sub-features or integrations change: state machine breaking Issues or PRs that break consensus (need to be released in at least a new minor version) labels Nov 24, 2023
@damiannolan
Copy link
Member

Why delete all keys from the map?

@crodriguezvega
Copy link
Contributor Author

Why delete all keys from the map?

That was a leftover! I corrected now.

@GNaD13
Copy link
Contributor

GNaD13 commented Nov 25, 2023

Hi @crodriguezvega, can I take this issue?

@crodriguezvega
Copy link
Contributor Author

Hi @crodriguezvega, can I take this issue?

@GNaD13 Thanks for volunteering. Let us have an internal discussion about the above proposal tomorrow (Monday) and if we all agree on it, then it's all yours. :)

@crodriguezvega
Copy link
Contributor Author

@GNaD13 We discussed this issue and updated the description based on the team's feedback. It would be very nice, if you could have a PR open by the end of the week or early next week. Would that be possible? Give me a signal if you're still interested and I will assign then the issue to you. Thanks!

@GNaD13
Copy link
Contributor

GNaD13 commented Nov 27, 2023

Yeah sir @crodriguezvega , let me work on this issue.

@crodriguezvega
Copy link
Contributor Author

Thank you, @GNaD13!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
20-transfer change: state machine breaking Issues or PRs that break consensus (need to be released in at least a new minor version) type: feature New features, sub-features or integrations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants