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

[tgrade-valset] Metadata issues #66

Closed
maurolacy opened this issue Feb 3, 2022 · 6 comments · Fixed by #84
Closed

[tgrade-valset] Metadata issues #66

maurolacy opened this issue Feb 3, 2022 · 6 comments · Fixed by #84
Assignees

Comments

@maurolacy
Copy link
Contributor

maurolacy commented Feb 3, 2022

During #42, noticed a number of relatively small issues with metadata handling in tgrade-valset:

  • Metadata validation is almost nil. Only moniker is validated, and for minimum length. Let's add validation for max length over all string fields, with some reasonable values. There will be a max length that is enforced by the vm, but that length can be very large for a string field (128 Kb or more).
  • Metadata is not validated during instantiation.
  • The sender is not validated during metadata update. That means anybody can overwrite the metadata of an operator.
  • The sender is not validated during validator key registration. That means anybody can register a public key and associated metadata for an operator. Perhaps this is the intended feature, as this registration method can only used for setting a pubkey, not updating one.
@ethanfrey
Copy link
Contributor

Let's validate all this. Good points.

@ethanfrey ethanfrey added this to the 0.6.0 Patchnet Stretch milestone Feb 3, 2022
@ueco-jb ueco-jb self-assigned this Feb 7, 2022
@ueco-jb
Copy link

ueco-jb commented Feb 7, 2022

@maurolacy

The sender is not validated during metadata update. That means anybody can overwrite the metadata of an operator.

https://github.com/confio/poe-contracts/blob/main/contracts/tgrade-valset/src/contract.rs#L239

operators().update(deps.storage, &info.sender, |info| match info {

update() searches using key, which here is info.sender. Unless I'm missing something, that means sender may update only his own metadata - if he's already in operators() list.

@maurolacy
Copy link
Contributor Author

maurolacy commented Feb 7, 2022

update() searches using, which here is info.sender. Unless I'm missing something, that means sender may update only his own metadata - if he's already in operators() list.

Good. Missed that; was looking for the typical auth pattern I guess.

@ethanfrey
Copy link
Contributor

The sender is not validated during validator key registration. That means anybody can register a public key and associated metadata for an operator. Perhaps this is the intended feature, as this registration method can only used for setting a pubkey, not updating one

Please check this. I think this is handled the same way as update (sender set their own key)

@ethanfrey
Copy link
Contributor

Metadata validation is almost nil. Only moniker is validated, and for minimum length. Let's add validation for max length over all string fields, with some reasonable values. There will be a max length that is enforced by the vm, but that length can be very large for a string field (128 Kb or more).

I think max length of 256 or so should be plenty. You can make it a bit bigger if you think for some fields

@ueco-jb
Copy link

ueco-jb commented Feb 7, 2022

Please check this. I think this is handled the same way as update (sender set their own key)

Yup.

    let pubkey: Ed25519Pubkey = pubkey.try_into()?;
    let operator = OperatorInfo {
...
    match operators().may_load(deps.storage, &info.sender)? {
        Some(_) => return Err(ContractError::OperatorRegistered {}),
        None => operators().save(deps.storage, &info.sender, &operator)?,
    };

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants