feat(contract): add ability for admin to freeze diamond

[thomas-nguy]

What ❔

add more control for hyperchain's admin

[vladbochok]

Hey @thomas-nguy, currently freezing is designed to be controlled by the Governance mechanism, which means the governance can unfreeze/freeze ZK chains. Moreover, anyone can unfreeze the chain by calling the unfreeze method on the Goverance protocol if the protocol as a whole is not frozen. Due to this reason, there is no additional power to the Admin role to freeze/unfreeze chains implemented in this PR.

• https://github.com/ZKsync-Association/zk-governance/blob/master/l1-contracts/src/ProtocolUpgradeHandler.sol#L402

Let me know if you still think this change makes sense to include, otherwise I will close the PR. Thank you.

[thomas-nguy]

The goal is to implement a circuit breaker that would trigger freeze automatically in case of emergency

I don't think human intervention would be quick enough for such case.

Now I understand that initially the freeze and unfreeze were originally designed to disable fraudulous chains and isolate them from the ecosystem as funds are shared in the sharedbridge.

I respect this decision that is why I am not adding the hyperchain's "admin" permission to unfreeze the chain

• in case of emergency, hyperchain's can freeze themselves to prevent damages to be spread within the ecosystem
• to unfreeze, governance vote is needed so that malicious hyperchain's cannot unfreeze themselves

does it make sense?

[vladbochok]

The idea behind State Transition Manager is that it is trust zone between chains. All the chains use the same implementation, so the bug in one chain means the bug in all STM chains. Therefore, If a chain has emergency, all the STM chains will have emergency. On additional note, the funds are stored in the shared bridge, so the damage there means damage to the each and every ZK chain, that contract can be frozen only by the Governance protocol too.

At the same time, the decision to freeze the protocol is made by the Security Council body with low threshold and high SLA for many members, so fast reaction is expected behaviour.

[thomas-nguy]

The condition to trigger freezing may not be related to bug in the STM but protocol hack (dapps), scams or bad managements of the keys( 90% of the case?). In that case we would like to have the ability to react quickly.

I understand that some chain would not require any intervention for some cases but some others would.

I think having the ability to freeze its own chain is harmless for the ecosystems but yet could be useful for the chain operators.

I need to hear more about this security council. We had some discussion about it previously but I am quite sceptical on the speed of intervention, especially for emergency that are specifics for one hyperchain

[StanislavBreadless]

@thomas-nguy We talked a bit internally and we think it is okay to add the ability to freeze one specific chain. But it is just not a oneliner, a new storage slot needs to be added to track whether the chain locked itself also. We will also need to store whether the main STM has frozen the chain. Upon each freeze/unfreeze we'll have to decide whether to full freeze/unfreeze the chain or not, since both the chain admin and the security council should be okay with the chain being unfrozen

[thomas-nguy]

okay let me clarify the specs

• add a boolean FreezeByAdmin in ZkSyncHyperchainStorage to track if the freeze has been activated by chain admin or not
• Change implementation of the Freeze method to set the boolean FreezeByAdmin to 1 when it has been triggered by chain admin
• add a method in AdminFacet to set FreezeByAdmin back to 0 available to chain admin
• Change implementation of the Unfreeze to check additionally that boolean FreezeByAdmin is set to 0

Is that correct?

[StanislavBreadless]

@thomas-nguy yes

[StanislavBreadless]

@vladbochok this PR presumes that it will be always possible to call the unfreezeDiamond via STM. This will be the case for mainnet, but will this be the case for e.g. testnet/stage?

[thomas-nguy]

I am also assuming that the "admin facet" is not freezable

https://github.com/matter-labs/era-contracts/pull/674/files#diff-d14cd974a96b5a41e7a7064e771df7d5eae5582cd460aa8d28fcc1705e9be7cbR41

I think it is already the case, but better to be careful when upgrading facets

[StanislavBreadless]

@thomas-nguy yes, it is the case

[thomas-nguy]

@vladbochok @StanislavBreadless this PR has been hanging for a while

Beside fixing conflicts, anything else we need to discuss?

[StanislavBreadless]

Hey @thomas-nguy, we discussed with @vladbochok and we worried that with this change a malicious admin could abuse his powers and freeze the chain indefinitely to lock users away from their funds, which violate guarantees that we try to provide as rollup where users can always withdraw funds no matter what.

To prevent these cases, we started work on developing a more enhanced version of the ChainAdmin that could ensure that e.g. Era ChainAdmin could not use the functionality introduced in this PR. Here it is: #699.

So TLDR: we are fine with your change, but it should go into the branch after the new version of ChainAdmin is included (there is still some WIP things to do, but we are close)

[thomas-nguy]

Okay I think this is an acceptable concern

However from risk perspective, it is less likely to have a malicious admin trying to "lock" user funds rather than hack happening on chain.

The zkstack has been build with the assumption that admin is not malicious and there are already too much power given to the admin that I don't think giving the ability to freeze would make much difference. Trying to protect users from admin would require a lot of change imo

But I agree that we need to walk into this direction for future development

[thomas-nguy]

how do you compute the hex?

[StanislavBreadless]

https://soliditylang.org/blog/2021/04/21/custom-errors/ you can read more about custom errors and how to compute the hex here

TLDR: abi encode

[StanislavBreadless]

But if the question is whether we have some automated tooling for that, then no, not atm

[thomas-nguy]

thanks! updated