Configuration-based mechanism for 'freezing' ledger keys

[dmkozh]

One of the first remediation steps for the data corruption incident that has occurred in protocol 23 has been the 'freeze' of the corrupted ledger entries at the overlay layer in order to prevent further data corruption until the protocol has been fixed. Any transaction that has accessed any one of the corrupted keys (detectable via footprint) was rejected by the validators without being added to the mempool.

This was a bespoke change that has been released in a separate Core build. The 'freeze' has only become fully active when every tier 1 validator has updated to the new build, and even then it was not 100% effective (e.g. a validator could roll back the build). Also notably this kind of change can only be easily done for the Soroban entries - it would be much trickier to do quickly in case if any classic accounts or trustlines have been corrupted for whatever reason.

While the Stellar Core team puts a lot of effort to reduce the probability of the similar data corruption issues from ever occurring again, there is always a non-zero risk that something goes wrong. There also may be a possibility of using a similar mechanism for remediation of other issues beyond the protocol bugs, such as freezing the data entries that are known to be vulnerable in some way, e.g. are known to be hacked.

With that motivation in mind, the following protocol change sketch is proposed:

• Add a new type of configuration setting that contains a list of 'frozen' keys
• Add a way to upgrade that configuration setting in a similar way to the current network configuration upgrade mechanism (https://github.com/stellar/stellar-protocol/blob/master/core/cap-0046-09.md), potentially with an additional multiplexing mechanism that works around the contract data entry size limitation.
• Make any transaction that accesses a key in the 'frozen' set invalid
  • The caveat here is that for the Soroban transactions the check is very easy - as long as the 'frozen' key is in the footprint, the transaction is invalid. However, for the 'classic' transaction the check might be trickier as there is no footprint
  • In order to simplify the proposal, it could be narrowed to only Account and Trustline classic entries being eligible for being 'frozen'. That should simplify the classic transaction detection mechanism. These are also the classic entries that hold the actual value.

The proposal has the following advantages over overlay-level bans:

• Frozen keys are guaranteed to actually be frozen after the vote goes through and can only be unfrozen by another validator vote
• Only SCP consensus of tier 1 validators is required to make the upgrade go through (unlike every validator in case of the overlay-level bans)
• Frozen keys are clearly observable by anyone

There are also some downsides of the proposal:

• This is still rather slow, which might be problematic in a context where a very fast action is required.
  • However, it's not clear if a faster solution even exists
• This is a rather complex mechanism for a feature that might never actually get used
• This creates a relatively simple way for validators to censor arbitrary network traffic
  • However, this requires validator consensus and there are ways for validators to censor traffic via modified Core anyways. The latter is also hard to detect, unlike the protocol vote.

This discussion is open to anyone to add any more ideas or raise any concerns.

[tupui]

If such mechanism is in place, I think we will need some clear rules on how to add an entry and also have a record for the reason, potentially some timeout, context info, etc. Would the ban be on every operations or would you add an option to allow some operations?

[dmkozh]

I think we will need some clear rules on how to add an entry and also have a record for the reason, potentially some timeout, context info, etc

I would like to avoid introducing more complexity into protocol itself, the proposal is pretty complex even without the additional metadata and timeouts. But I agree that generally protocol should not be changed without any justification or history trace. For the network limit configuration settings we already have the SLP process (https://github.com/stellar/stellar-protocol/blob/master/limits/README.md) that is meant to document and justify the decisions. I think a similar process would be used here, with a caveat that sometimes the external comms would happen after the fix has been voted for by tier 1, as this feature is meant to be used in the context of the critical bugs and vulnerabilities. The exact document format can be iterated on, but in general I agree that it would have all the items that you've listed, i.e. the reason for ban, potentially analysis of the impacted keys, timeline for unfreezing the keys etc.

Would the ban be on every operations or would you add an option to allow some operations?

As the proposal states, all the operations will be banned unconditionally, but the ledger keys will likely be restricted to the contract data, contract code, classic account, and classic trustlines. Allowing some operations would introduce more complexity and it also seems quite dangerous in general. E.g. in case of the recent data corruption the protocol would not be able to tell if the key is being accessed by the protocol admins that amend the data, or by a malicious actor that tries to benefit from it.

[dmkozh]

I've drafted CAP-77 for this. Properly supporting even just account and trustline entries out of all the classic entries ended up rather tricky. It's not clear to me if complexity is worth it, maybe we could limit the proposal to just the Soroban entries, but that seems rather restrictive as well.

[leighmcculloch]

stellar-protocol/core/cap-0077.md

Line 264 in 3cb90ac

The offer removal also results in adjusting the liabilities of the trustline or an account, even though it is frozen. This is one of the few changes allowed to perform for the frozen entries.

stellar-protocol/core/cap-0077.md

Line 282 in 3cb90ac

In order to avoid DEX disruption the approach described in this CAP is proposed: the offer is automatically removed and liabilities are released. This comes with the minimal DEX disruption with a tradeoff of modifying a frozen entry. However, the modification is minimal (only the liabilities are modified, the actual balance stays the same) and thus is unlikely to make data corruption worse. In the worst case, if an entry is in an arbitrarily broken state and update of liabilities would violate the protocol invariants for the liabilities, the transaction will fail with an internal error, which is equivalent to what would unconditionally happen in the 'simple' approach.

I have read the motivation, but I still don't completely understand the reasoning around why we would lean into removing the offer versus ignoring the offer. Can Core, when collecting offers, skip over offers that are frozen or exclude them in the initial query? Or if this is an optimisation, mark them to be excluded on future queries?

[dmkozh]

Can Core, when collecting offers, skip over offers that are frozen or exclude them in the initial query?

In theory, yes, it could, and that would be the 'default' option I'd take. In practice, it's the area of code that I'd prefer not to touch, as the logic for figuring out the best offer is baked quite deeply into the ledger subsystem. DEX has been in 'support-only' mode for a while, so a significant re-write for the sake of an edge case feature seems like a pretty significant (and risky) investment to me. So the current proposal is rather a complexity compromise (removing the offers is straightforward because it's what the DEX does normally).
That said, I might be missing a simpler option here - I'll be happy to update this if there is one.

Or if this is an optimisation, mark them to be excluded on future queries?

That's an additional benefit, but it's not the main motivator.

[leighmcculloch]

a significant re-write for the sake of an edge case feature seems like a pretty significant (and risky) investment to me. So the current proposal is rather a complexity compromise (removing the offers is straightforward because it's what the DEX does normally).

Got it, thanks. That seems like a good trade-off.