hedera-improvement-proposal icon indicating copy to clipboard operation
hedera-improvement-proposal copied to clipboard

Consistent ability to remove admin & privileged keys

Open justynspooner opened this issue 2 years ago • 6 comments

This HIP proposes a modification to the TokenUpdateTransaction and ContractUpdateTransactionBody calls to allow the admin key to sign an update to remove itself and/or any other key (Wipe, KYC, Freeze, Pause, Supply, Fee Schedule) from a Token or the Admin key from a Contract.

justynspooner avatar Aug 05 '22 17:08 justynspooner

Deploy Preview for hedera-hips ready!

Name Link
Latest commit ac9a931e48f1206c37d9cf08974ca3247b915407
Latest deploy log https://app.netlify.com/sites/hedera-hips/deploys/63c88274f675400008c95f74
Deploy Preview https://deploy-preview-540--hedera-hips.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Aug 05 '22 17:08 netlify[bot]

For cases that an NFT with no admin keys but access to other privileged keys nullable options should still be possible, perhaps based on the owner of the collection?

mattsmithies avatar Aug 10 '22 08:08 mattsmithies

Thanks for the comments. I have created a discussion doc here to make it easier to process: https://docs.google.com/document/d/1P-qeYaXOcPIKCkTvfqFSbuVWfgrwvf0pwNMbGfurNjI/edit?usp=sharing

justynspooner avatar Aug 10 '22 10:08 justynspooner

For cases that an NFT with no admin keys but access to other privileged keys nullable options should still be possible, perhaps based on the owner of the collection?

I disagree. If the plan is to remove mint keys in the future, then the admin owner can remove the mint key then the admin key (or both at the same time) and accomplish the same thing. IIRC, once a key is removed, it can not be added, so an admin key can only facilitate rotating existing keys (or deleting them) and/or change memo symbol etc. So even with an admin key, consumers still typically base their "trust" on which keys already exist. (because you can't "add" any).

bugbytesinc avatar Aug 10 '22 13:08 bugbytesinc

Thanks for the comments. I have created a discussion doc here to make it easier to process: https://docs.google.com/document/d/1P-qeYaXOcPIKCkTvfqFSbuVWfgrwvf0pwNMbGfurNjI/edit?usp=sharing

Could you please migrate this discussion to GitHub when you get a chance? https://github.com/hashgraph/hedera-improvement-proposal/discussions/new

mgarbs avatar Aug 17 '22 16:08 mgarbs

Any reason this is taking precedent over the HIP discussed in https://github.com/hashgraph/hedera-improvement-proposal/discussions/522?

Cooper-Kunz avatar Oct 16 '22 14:10 Cooper-Kunz

Any reason this is taking precedent over the HIP discussed in #522?

No reason other than I saw it as a PR, and I'm a maintainer of the PRs. Looking at your discussion about this HIP, I'd say add yourself as an author in this HIP, and we can also update the discussion to include yours too. Does this sound reasonable @Cooper-Kunz @justynspooner ?

mgarbs avatar Nov 02 '22 14:11 mgarbs

Hello! I wanted to check and see what the status of this HIP currently is.

This is much needed functionality and would greatly empower creators.

kantorcodes avatar Mar 08 '23 11:03 kantorcodes

I agree with this. The admin key could remove and rotate another key, but not add the key if it's not currently set. This allows additional trust when the ADMIN key is set, but the SUPPLY (for example) is not set or has been removed.

Ashe-Oro avatar Mar 14 '23 19:03 Ashe-Oro

Hi all, we have discussed the best solution to move this HIP forward, which has come out of a discussion with the engineering team. Feel free to drop any questions or comments you might have about the proposed solution. The end goal is to align all community members, update the HIP content, and move it to the next stage.

Tagging previously involved community members @justynspooner @bugbytesinc @kantorcodes @mattsmithies @Cooper-Kunz @nkavian . Big thanks for your help and thoughts!

Language

First, let's address important language to set a clear distinction between "removed" and "invalid".

  • "Removed" refers to not setting a key or setting it to "no key". If a key is absent, the token is considered immutable for that key. For example, if the freeze-key is absent when the token is created, then even the admin cannot create a freeze-key later on.
  • "Invalid" refers to an invalid key, such as an all-zero key (the key is present, but no private key corresponds to it). There’s nothing magical about the all-zero key except we believe that it is difficult to find a private key that maps to an all-zero public key. So we recommend using all-zeros for all invalid keys.
  • "Lower privilege key" refers to all keys you can set for a token except for the admin key, which is a high privilege key. In other words, the KYC, freeze, pause, wipe, supply, and fee schedule keys are considered low privilege keys.

Proposed Solution

The behavior is summarized as:

  1. If you have a low-privilege key, you should be able to change that key to another valid key, or to an invalid key (such as all-zeros).

    a. But you don’t have a right to remove it, nor to set a key that is absent on the token.

  2. If you have a high-privilege key (like an admin key), then you should be able to change that key to a valid or invalid key.

    a. You also have the right to remove it. But not to set it if it is absent.

    b. You also have the right to change a low-privilege key under it to a valid or invalid key, or to remove it. But you cannot set it if it is absent.

In short: any key can be used to change itself or a lower-privilege key to a valid or invalid key. Only admin keys can remove themselves or the lower-privilege keys. No absent key can ever be set.

Why do we need a distinction between "removed" and "invalid"?

Consider the following scenario: Alice creates an AliceToken with an admin key. This AliceToken needs to be KYC-compliant, so she hires a company called AcmeKYC and provides their public key as the KYC-key.

At some time, an employee in AcmeKYC gets disgruntled and removes the KYC-key from AliceToken completely. If a token does not have a key that’s considered immutable for that property, Alice cannot reinstate the KYC-key to a valid key, even if she has the admin key on that token.

In our proposed solution, AcmeKYC will be only limited to be able to make the key to be all-zeros. But not remove the KYC-key completely, if the transaction was signed only with the current KYC-key.

Additional Questions

1. Can an admin key remove itself? Yes, because it's a high-privilege key.

2. If the admin key is removed, can lower privilege keys then remove themselves? No, it still remains a lower privilege key that doesn't have the authority to remove itself. It has the ability to change itself to a valid or invalid key. Changing itself to an invalid key, like the all-zeros key, has the same effect as removing a key.

3. If there’s no admin key set and there’s a treasury account ID set? Should the corresponding keypair for the treasury account be able to update the token to change the treasury account ID to a new one? No!

Other Considerations

  1. We will have to add a boolean flag in the Update Transaction about whether the system should check the validity of the updated key. Currently, we test the validity of all updates. With this boolean flag, the user can tell the system to avoid checking the validity of the updated key. The flag's default value will maintain the current behavior of checking the validity.

  2. We should standardize the use of the all-zeros key as an invalid key across all keys in Hedera.

michielmulders avatar May 22 '23 13:05 michielmulders

2. If the admin key is removed, can lower privilege keys then remove themselves? No, it still remains a lower privilege key that doesn't have the authority to remove itself. It has the ability to change itself to a valid or invalid key. Changing itself to an invalid key, like the all-zeros key, has the same effect as removing a key.

I'm looking for community feedback on this point. To be clear, the current proposed solution still requires an ADMIN key to be set in order for a key (ie SUPPLY) to be properly removed. If no ADMIN key is set, the only option to achieve token supply immutability would be to use the SUPPLY key to set itself to a pubic key which has no private key (0x000000...0 or 0xDEADBEEF... or 0x1111111, etc).

The general guidance would be to only use 0x00000...0 and for the Hedera ecosystem to consider this key to equate to immutability unless the ADMIN key is set (since the SUPPLY key could again be rotated).

What are your thoughts? Should there exist a path for a SUPPLY key properly remove itself when no ADMIN key is set or is setting the SUPPLY key to 0x000000 acceptable?

Here's a flow chart of "testing for immutability" as I understand it from the proposed solution.

//testing if an NFT Collection is immutable
immutable = 
  supply == null 
  || (supply == allZeros 
      && (admin == null 
          || admin == allZeros))

*the use of supply key is only used as a reference and this logic is shared by all “lower-privilege keys, not just the supply key

**the use of allZeros is in reference to any invalid key, not only to 0x0000000000000000000000000000000000000000

***the allZeros key is recommended to communicate an invalid key.

image

It should be mentioned that in either case, using an invalid key (such as 0x00000) or properly removing the key will require NFT apps to test against both paths to confidently display if an NFT Collection is immutable.

@justynspooner @HGraphPunks @bugbytesinc @rocketmay @Cooper-Kunz @mattsmithies @nkavian

Ashe-Oro avatar May 23 '23 20:05 Ashe-Oro

2. If the admin key is removed, can lower privilege keys then remove themselves? No, it still remains a lower privilege key that doesn't have the authority to remove itself. It has the ability to change itself to a valid or invalid key. Changing itself to an invalid key, like the all-zeros key, has the same effect as removing a key.

I'm looking for community feedback on this point. To be clear, the current proposed solution still requires an ADMIN key to be set in order for a key (ie SUPPLY) to be properly removed. If no ADMIN key is set, the only option to achieve token supply immutability would be to use the SUPPLY key to set itself to a pubic key which has no private key (0x000000...0 or 0xDEADBEEF... or 0x1111111, etc).

The general guidance would be to only use 0x00000...0 and for the Hedera ecosystem to consider this key to equate to immutability unless the ADMIN key is set (since the SUPPLY key could again be rotated).

What are your thoughts? Should there exist a path for a SUPPLY key properly remove itself when no ADMIN key is set or is setting the SUPPLY key to 0x000000 acceptable?

Here's a flow chart of "testing for immutability" as I understand it from the proposed solution.

//testing if an NFT Collection is immutable
immutable = 
  supply == null 
  || (supply == allZeros 
      && (admin == null 
          || admin == allZeros))

*the use of supply key is only used as a reference and this logic is shared by all “lower-privilege keys, not just the supply key

**the use of allZeros is in reference to any invalid key, not only to 0x0000000000000000000000000000000000000000

***the allZeros key is recommended to communicate an invalid key.

image

It should be mentioned that in either case, using an invalid key (such as 0x00000) or properly removing the key will require NFT apps to test against both paths to confidently display if an NFT Collection is immutable.

@justynspooner @HGraphPunks @bugbytesinc @rocketmay @Cooper-Kunz @mattsmithies @nkavian

My two cents are that we should have a pathway for any key to remove itself, but I am happy to start with the admin key being able to do that and revisiting in another hip if that means we can expedite this along.

We've used smart contract based mints and always have to think about this scenario. Having the ability to remove keys even with the admin key set, would open many doors in terms of flexibility.

kantorcodes avatar May 23 '23 23:05 kantorcodes

My two cents are that we should have a pathway for any key to remove itself, but I am happy to start with the admin key being able to do that and revisiting in another hip if that means we can expedite this along.

We've used smart contract based mints and always have to think about this scenario. Having the ability to remove keys even with the admin key set, would open many doors in terms of flexibility.

After posting this, I had a call with Engineering and they we discussed a solution which would includes a key being able to remove itself as long as it's currently has the highest priority.

Said differently, there are 2 levels of priority on a token HIGH and LOW. The ADMIN key has intrinsic high privilege and all other keys have low privilege. They key here that ONLY HIGH PRIVILEGE KEYS CAN REMOVE KEYS INCLUDING ITSELF.

If set, the ADMIN is the sole high priv key and is the only key that can remove all other keys and itself. It can also change any other key (including itself) as described in my last comment.

The difference here is that if the ADMIN key is REMOVED...all other keys become high priv and can remove themselves in addition to changing themselves. That said, there's no path for a SUPPLY key to either change or remove a FREEZE key, etc.

This empowers each key to manage itself and does not reduce the intrinsic high privilege of the ADMIN key since if it's set (even to an invalid key) then they minor keys cannot remove themselves.

One last important thing to note...if the ADMIN key is set but invalid thus unable to call the removeKey function, the other keys (SUPPLY, WIPE, etc) are able to set themselves to 0x000000...0 to establish immutability since they will never reach the high priv status. While this solution works, I recommend using the cleaner removeKey functionality if you can since it should be the first query on an NFT Collection to determine immutability or not.

tl;dr

  • Any key considered "high privilege" can remove itself.
  • Only the ADMIN key has intrinsic high privilege.
  • If the ADMIN key is removed, all other keys are considered high privilege and can remove themselves individually (see #1)
  • If a key is NOT set, it cannot be set under any circumstance.
  • Any key can change itself at any time to a valid or invalid key (ie 0x000000...0)
  • Only the ADMIN key can change any other key (ie. SUPPLY, FREEZE, WIPE, KYC, etc) to a valid or invalid key (ie 0x000000...0)

Ashe-Oro avatar May 24 '23 02:05 Ashe-Oro

I'm looking for community feedback on this point.

My feedback would be to avoid allowing child keys to pass their permissions to someone else.

A key has a responsibility to perform its core action. A freezeKey should be responsible to manage freezes and unfreezes.

The key should not be responsible to admin itself. It escalates/increases the amount of management required off-chain.

Imagine a freezeKey were stolen, and the key was allowed to reassign itself to a different malicious address.

  • The original freezeKey becomes dead, the owner can't change it to a different safe key in time.
  • If the admin is asleep then 8 hours or 2 weeks pass by with malicious activities occurring.

I understand it's not a perfect example, but the point was that it's more complicated to handle. To mitigate this and other types of issues, more off-chain tech has to be built to monitor, detect, & alert a larger set of circumstances.

For example, moving from a fee-less blockchain to a system that charged rent for wallets increased the amount of off-chain monitoring to urgently verify every single hosted wallet; to have an early warning system if a wallet was going to expire with tokens still stored in it. Then the scanning has to occur on a daily basis (does the indexer really want all this scanning logic). Then the system becomes fragile when you get rate limited or indexers, validators, & you name it has an outage.

Considering the impact on integrators is just as valuable as considering how cool it would be if a key holder could change themselves.

Sometimes the simpler solution is better.

nkavian avatar May 24 '23 04:05 nkavian

I'm looking for community feedback on this point.

My feedback would be to avoid allowing child keys to pass their permissions to someone else.

A key has a responsibility to perform its core action. A freezeKey should be responsible to manage freezes and unfreezes.

The key should not be responsible to admin itself. It escalates/increases the amount of management required off-chain.

Imagine a freezeKey were stolen, and the key was allowed to reassign itself to a different malicious address.

  • The original freezeKey becomes dead, the owner can't change it to a different safe key in time.
  • If the admin is asleep then 8 hours or 2 weeks pass by with malicious activities occurring.

I understand it's not a perfect example, but the point was that it's more complicated to handle. To mitigate this and other types of issues, more off-chain tech has to be built to monitor, detect, & alert a larger set of circumstances.

For example, moving from a fee-less blockchain to a system that charged rent for wallets increased the amount of off-chain monitoring to urgently verify every single hosted wallet; to have an early warning system if a wallet was going to expire with tokens still stored in it. Then the scanning has to occur on a daily basis (does the indexer really want all this scanning logic). Then the system becomes fragile when you get rate limited or indexers, validators, & you name it has an outage.

Considering the impact on integrators is just as valuable as considering how cool it would be if a key holder could change themselves.

Sometimes the simpler solution is better.

@nkavian When there's no admin key and the freeze key gets compromised, there's no difference between being able to remove itself or set itself to a valid or invalid key. In both cases, the key is unretrievable.

If there is an admin key, then only the admin key can remove the key. If the freeze key decides to change itself to a malicious or invalid key, the admin key can still reinstate access by changing it to a legit, valid key.

But if I understand you correctly, you don't want to give any administrative responsibilities for lower-privilege keys. In case the freeze key gets compromised, the admin key can change the freeze key to a valid key again.

michielmulders avatar May 24 '23 08:05 michielmulders

I wish this was two or more separate HIPS. One explicitly for extending existing functionality (where all lower-level key rotations already require consent of ADMIN) to include the ability to remove a key to that list of functionality.

...and One for the discussion of whether or not lower-level keys can self-admin.

...and/or One for the discussion of self-admin when ADMIN does not exist and the object should otherwise be immutable.

I am in the camp that once the ADMIN key is removed, the object becomes immutable, just like present functionality.

It seems we are getting hung up the semantics of the last two and its stalling the whole thing.

bugbytesinc avatar May 24 '23 11:05 bugbytesinc

@michielmulders I did try to point out that my example is not perfect; and I understand many people can find flaws in it. It was late.

However, my main point was about considering the effects on integrators. Hedera has already made other design choices that increased our monitoring layer and raised integration/maintenance complexity. Now we would have to add one more for this thread. I'm contrasting that with knowledge of 16 other blockchains we've integrated and 5 others we've evaluated for readiness. If child keys have semi-admin privileges, it's simply increasing that problem space (for integrators to understand; and sometimes nuanced details are buried deep in the Hedera documentation and you don't realize it until you perform integration testing and realize the actual behavior).

Developer sentiment is painfully important and can affect organic growth; when they whisper a different blockchain/graph is easier to use.

you don't want to give any administrative responsibilities for lower-privilege keys

Yes, that's my point of view.

nkavian avatar May 24 '23 17:05 nkavian

@michielmulders I did try to point out that my example is not perfect; and I understand many people can find flaws in it. It was late.

However, my main point was about considering the effects on integrators. Hedera has already made other design choices that increased our monitoring layer and raised integration/maintenance complexity. Now we would have to add one more for this thread. I'm contrasting that with knowledge of 16 other blockchains we've integrated and 5 others we've evaluated for readiness. If child keys have semi-admin privileges, it's simply increasing that problem space (for integrators to understand; and sometimes nuanced details are buried deep in the Hedera documentation and you don't realize it until you perform integration testing and realize the actual behavior).

Developer sentiment is painfully important and can affect organic growth; when they whisper a different blockchain/graph is easier to use.

you don't want to give any administrative responsibilities for lower-privilege keys

Yes, that's my point of view.

As an NFT creator, I want the ability to reduce the risk profile on my collection so that the community feels safe holding my NFTs.

Example: Bob creates an NFT Collection which will be minted into for the next 3 months. He assigns only a SUPPLY key to the Collection which gives the Collection a risk rating of LOW. Since there is no ADMIN key, his NFT community doesn't need to worry about Bob's ability to rug them by deleting the entire collection. Once Bob has minted the final NFTs, he further reduces the risk profile of the NFT Collection by removing the SUPPLY key leaving no keys defined.

If the SUPPLY key cannot remove itself, the ADMIN key would need to be set which gives the NFT Collection a risk rating of HIGH until it is removed. Bob could use the ADMIN key to remove both the SUPPLY key and the ADMIN key at the end of the 3-month NFT campaign, but during his launch the risk score would be higher than necessary.

NFT Risk Calculator: https://github.com/hashgraph/hedera-nft-utilities/tree/main/risk

Ashe-Oro avatar May 24 '23 18:05 Ashe-Oro

integrators

agree that this could be 2 separate HIPs. It's a balancing act between (more scope & fewer) vs (less scope & more frequent). I'll bring it up with Engineering and see which they prefer.

Ashe-Oro avatar May 24 '23 18:05 Ashe-Oro

NFT Risk Calculator: https://github.com/hashgraph/hedera-nft-utilities/tree/main/risk

Are there any 3rd party impartial risk scoring tools?

That's a great counter example you shared, thanks.

nkavian avatar May 24 '23 19:05 nkavian

Are there any 3rd party impartial risk scoring tools?

I'm not aware of any other risk score calculators in the Hedera ecosystem.

Ashe-Oro avatar May 24 '23 19:05 Ashe-Oro

NFT Risk Calculator: https://github.com/hashgraph/hedera-nft-utilities/tree/main/risk

Are there any 3rd party impartial risk scoring tools?

That's a great counter example you shared, thanks.

also if you disagree or have suggestions to improve the risk score manager, please ping me [email protected].

Ashe-Oro avatar May 25 '23 14:05 Ashe-Oro

UPDATE: After giving time for community feedback on this HIP and thru several conversations with the SwirldsLabs Engineering team, I believe it's best to NOT include the ability for keys to remove themselves at this time. I agree with @bugbytesinc that this functionality can (and should) be broken out into it's own HIP.

It is important to note that all NFTs which have an ADMIN key set are considered to carry HIGH risk since the entire NFT Collection can be deleted with the ADMIN key.

This means that if you want to properly remove a key in the future, you'll need to set an ADMIN key and your NFT Collection will be considered HIGH risk until it's removed.

If you choose NOT to include an ADMIN key, you have the option to use the lower priv key (ie SUPPLY) to set itself to 0x00000000..0 which essential the same as removing they key since there's no private key associated with the "long-zero" public key.

With the 0x0000 key set as a low priv key AND no ADMIN key set, the NFT Collection can be seen as immutable. Again, here's the path to check for immutability.

image

tl;dr

  1. ADMIN key can properly remove itself
  2. ADMIN key can update itself to either a valid or invalid key
  3. ADMIN key can properly remove low priv key (ie SUPPLY, WIPE, KYC, etc)
  4. Low priv key can update itself to either a valid or invalid key
  5. If a key has been removed, it CANNOT be set/defined in the future.
  6. Low priv key CANNOT properly remove itself; only update itself.

Any questions?

Ashe-Oro avatar Jun 01 '23 14:06 Ashe-Oro