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.

Sign up for GitHub

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

Jump to bottom

docs: CIP for make security related governance parameters immutable #74

Merged
merged 5 commits into from
Feb 16, 2024
Merged

docs: CIP for make security related governance parameters immutable #74

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d6de216
!D: cip-draft-msrgpi
caomingpei Feb 7, 2024
ac5ede3
!S: Apply suggestions in doc style
caomingpei Feb 8, 2024
6cdcf13
!D: fix grammar mistake
caomingpei Feb 11, 2024
130d755
!B: amend to backwards incompatibility
caomingpei Feb 14, 2024
9ab1e57
Update cips/cip-draft_msrgpi_abbrev.md
jcstein Feb 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 125 additions & 0 deletions cips/cip-draft_msrgpi_abbrev.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
---
jcstein marked this conversation as resolved.
Show resolved Hide resolved
cip: 16
title: Make Security Related Governance Parameters Immutable
description: Consensus-related parameters should not be modified via on-chain governance in the Celestia network.
author: Mingpei CAO (@caomingpei)
discussions-to: https://forum.celestia.org/t/cip-make-security-related-governance-parameters-immutable/1566
status: Draft
type: Standards Track
category: Core
created: 2024-02-07
requires: 13
---

## Abstract

This CIP suggests that the security-critical governance parameters—`consensus.evidence.MaxAgeDuration` and `consensus.evidence.MaxAgeNumBlocks`—should be immutable to on-chain governance proposals. In light of the present Celestia specifications and the details provided in [CIP-13](cip-13.md), maintaining the mutability of those parameters could open the door to future on-chain proposals that alter their values, potentially leading to an inconsistency between the protocol and implementation. This CIP also briefly analyzes the potential security risks that may arise from the parameter modifications.

## Motivation

Consensus protocols play an important role in Celestia. As a Data Availability solution, the robustness of Celestia is crucial to the performance and reliability of higher-level applications, such as rollups. [CIP-13](cip-13.md) introduces a framework for the on-chain governance parameters. Considering that governance proposals have the potential to alter parameter values, it is essential to designate certain parameters as immutable for the on-chain governance to prevent inconsistencies and mitigate security risks.

## Specification

caomingpei marked this conversation as resolved.
Show resolved Hide resolved
| Param \ Changeable via Governance | Current | **Proposed** |
|:------------------------------------:|:-------:|:-------------:|
| `consensus.evidence.MaxAgeDuration` | True | **False** |
| `consensus.evidence.MaxAgeNumBlocks` | True | **False** |

> Note: Parameters listed in this table are identical to those module parameters specified in [CIP-13](cip-13.md). The purpose of this CIP is to modify the `Changeable via Governance` attribute of those two parameters from `True` to `False`.

## Rationale

Adopting an on-chain governance method comes with inherent risks of governance attacks, particularly concerning parameters related to `consensus.evidence`.
As outlined in the [decentralized governance documentation](https://docs.celestia.org/learn/staking-governance-supply), all holders of the native token TIA can propose and vote on on-chain governance proposals. It is unrealistic to expect the majority of holders to understand the technical details of the protocol and implementation thoroughly. Consequently, on-chain governance participants (a.k.a. TIA holders) may be incentivized (or, in some cases, bribed) to vote for or against a proposal without understanding the potential impact. Any changeable parameter within the `Governance Parameter for Celestia` could be targeted for changes through on-chain governance proposals. Therefore, making security-related parameters unchangeable for the on-chain governance proposal could serve as an effective solution to alleviate introduced risks.

### Inconsistency

| Module.Parameter | Default | Summary | Changeable via Governance |
|--------------------------------------|----------------------------|-------------------------------------------------------------------------------------------------------------------------------------|---------------------------|
| `consensus.evidence.MaxAgeDuration` | 1814400000000000 (21 days) | The maximum age of evidence before it is considered invalid in nanoseconds. This value should be identical to the unbonding period. | True |
| `consensus.evidence.MaxAgeNumBlocks` | 120960 | The maximum number of blocks before evidence is considered invalid. This value will stop CometBFT from pruning block data. | True |
| ...... | ...... | ...... | ...... |
| `staking.UnbondingTime` | 1814400 (21 days) | Duration of time for unbonding in seconds. | False |

This is a part of the table introduced in [CIP-13](cip-13.md). The summary of parameter `consensus.evidence.MaxAgeDuration` states *"...This value should be identical to the unbonding period"*. Meanwhile, the parameter `staking.UnbondingTime` is NOT changeable since the `Changeable via Governance` is set to False. Suppose an on-chain governance proposal tries to modify the default value of `consensus.evidence.MaxAgeDuration` from `1814400000000000 (21 days)` to a different value. It would create an inconsistency between the description and implementation because the modified value would no longer be identical to the unbonding period.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[clarifying question] I think our specs are actually wrong here. I think the MaxAgeDuration doesn't need to be identical to the unbonding time but just >= unbonding time. For example, could the unbonding time be set to 30 days?

Update: I see the point about resource consumption below. Given evidence is usually empty, the resource consumption on nodes for a longer unboding time seems negligible to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good question. It is well known that setting evidence.maxAge shorter than the unbonding period could introduce security risks. In the previous description, setting both values as equal seems reasonable. Although a longer time will not consume too much storage space, the meaning of evidence is lost if it is longer than the unbonding period. Suppose there is an attack, even if the evidence is still valid after a period longer than the unbonding period, but the attacker has already unstaked. Different value settings in evidence expiration and unbonding periods could also lead to confusion. It is unclear which value should be chosen for the "challenge period" for users. If the evidence always equals the unbonding period, we can also simplify the Celestia description, i.e., we need to confirm the validity of consensus within the unbonding period.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your thorough response. It seems like we want evidnece.MaxAgeDuration to == unbonding period for simplicity and because the protocol doesn't need to retain evidence longer than the unbonding period because a malicious validator could unbond and exit the protocol in that time. Seems reasonable to me.

If we wanted to provide a bit more info in this section, we could analyze the MaxAgeDuration and unboding period of other Cosmos chains (like Cosmos Hub, Osmosis). Something like:

Chain MaxAgeDuration Unbonding time
Celestia 1814400000000000 (21 days) 1814400 (21 days)
Cosmos Hub TBD 21 days
Osmosis TBD 14 days

and then if they're identical (on a per-chain basis), that could be supporting evidence for making the param governance unmodifiable and exactly match the unbonding time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chain MaxAgeDuration Unbonding time
Celestia 1814400000000000 (21 days) 1814400 (21 days)
Cosmos Hub Complex Situation* 21 days
Osmosis 14 days (UnbondingTime) 14 days

The MaxAgeDuration of Osmosis is the same as its unbonding time.

However, for Cosmos Hub, the situation is complex. For the genesis, the max_evidence_age in slashing is set to 1814400000000000 (21 days), which equals to the unbonding time. Moreover, this parameter remains unchanged until the genesis of cosmoshub-4. We can check the #PR5952, and the L191 in Changelog states that "Remove parameters from x/evidence genesis and module state. The x/evidence module now solely uses Tendermint consensus parameters to determine of evidence is valid or not." In fact, there seems to still remain a problem (Inconsistency between design and implementation) in the parameter setting because after PR5952, the setting in ComosHub adopts the default setting of Tendermint. But if we check the current-parameters.js in gaia, we can find the max_age_num_blocks is 10 times than the default value in Tendermint. Besides, the average block creation interval for Comos Hub is 6 seconds (which is different from the default value). So, the final validity period for evidence in gaia is about MAX(70 days, 2 days), this is a strange situation because these two values are expected to be close.

I believe the security guarantee for Celestia should be close to that for Osmosis. This is because for Cosmos Hub, the exploit of malicious behaviour usually gains the native token, and the punishment mechanism will make it not profitable. However, the potential consensus instability risk for Osmosis or Celestia may lead to other losses. Overall, the genesis settings indicate that the maxAgeDuration equaling the unbonding time is a typical implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for your thorough investigation! Agreed the Cosmos Hub situation seems complex. @mpoke is there any rationale for the inconsistency?

Given all that info, it seems simpler to set both evidnece.MaxAgeDuration and unbonding time to the same values (21 days) like you initially proposed.


caomingpei marked this conversation as resolved.
Show resolved Hide resolved
### Security Risk

```go
func (evpool *Pool) verify(evidence types.Evidence) error {
...
// check that the evidence hasn't expired
if ageDuration > evidenceParams.MaxAgeDuration && ageNumBlocks > evidenceParams.MaxAgeNumBlocks {
...
}
...
}
```

Those two parameters are used in the function of `verify.go`, which is responsible for verifying whether evidence has expired or not. According to the `if` statement, even without modifying `consensus.evidence.MaxAgeDuration`, it is still possible to prolong the expiration time of evidence by increasing `consensus.evidence.MaxAgeNumBlocks`, which means that older evidence will be considered valid. An extended expiration time of evidence introduces the potential risk of Resource Consumption, and a detailed discussion can be found in the Security Considerations.

Additionally, suppose that an on-chain governance proposal sets the `evidence.MaxAgeDuration` and `evidence.MaxAgeNumBlocks` to extremely low values, meaning that evidence expires quickly. If a malicious validator were to engage in Duplicate Vote or Light Client Attack, it would lead to consensus instability. Given that Celestia is a solution to data availability, this consensus instability would introduce security risk to the upper-layer applications (e.g. rollup). A detailed discussion can be found in the Security Considerations.

### Summary

In summary, configuring these two parameters as immutable values that can NOT be changed via on-chain governance can mitigate the risks of inconsistency and security issues introduced by unintentional (or malicious) governance activities. Moreover, in the face of a security incident that concerns these parameters, reliance on on-chain governance may be inadequate. Implementing modifications through a hard fork represents a more resilient approach.

caomingpei marked this conversation as resolved.
Show resolved Hide resolved
## Backwards Compatibility

This CIP recommends freezing on-chain governance for two security related parameters, which introduces backward incompatibility. This incompatibility is due to any future modifications to these parameters requiring at least a hardfork.

## Reference Implementation

```diff
func (*App) BlockedParams() [][2]string {
return [][2]string{
...
// consensus.validator.PubKeyTypes
{baseapp.Paramspace, string(baseapp.ParamStoreKeyValidatorParams)},
+ // consensus.evidence.MaxAgeDuration and .MaxAgeNumBlocks
+ {baseapp.Paramspace, string(baseapp.ParamStoreKeyEvidenceParams)},
}
}
```
The above example serves as a conceptual illustration, and `MaxBytes` in `ParamStoreKeyEvidenceParams` should still remain changeable.

Besides, relevant documents should be updated accordingly, such as [Celestia App Specifications](https://github.com/celestiaorg/celestia-app/blob/main/specs/src/specs/params.md)

## Security Considerations

This CIP recommends setting those two parameters as immutable constants which are NOT allowed to change via on-chain governance proposals. Adopting this CIP means future changes for those two parameters require community coordination and hardforks. Although hardforks carry the community-divided risk, it is worth noting that many blockchain communities have successfully navigated multiple hardforks. The risks of division and disagreement can be minimized by thoroughly discussing and working towards widespread agreement before moving forward with a hardfork. Consequently, the risk is manageable and should not be a significant concern.

If the proposed two parameters have not been changed since genesis, the security impact of making them NOT changeable via on-chain governance is not significant. Besides, if modifications to those two parameters via on-chain governance are still allowed, this could not only result in the inconsistency between the protocol and implementation but also introduce the following potential security risks:

caomingpei marked this conversation as resolved.
Show resolved Hide resolved
### Consensus Instability

```
+---------------------+
·—— | Block Header (H1) |
| +---------------------+ ......
| | PayForBlobs |
| +---------------------+
+---------------------+ |
| Block Header (H0) | |
+---------------------+ <--|
| Block Body |
+---------------------+ <--|
|
|
| +---------------------+
·—— | Block Header (H1') |
+---------------------+ ......
| PayForBlobs' |
+---------------------+
```

Assuming that `evidence.MaxAgeDuration` and `evidence.MaxAgeNumBlocks` are configured with extremely low values, such a configuration implies that evidence would expire rapidly. Under these conditions, malicious validators might exploit the system by double voting on two blocks simultaneously, potentially leading to temporary network partitions. In the context of a Data Availability solution like Celestia, any instability in consensus could compromise the blob transactions. This situation risks the security and reliability of upper-layer rollups dependent on this infrastructure.

caomingpei marked this conversation as resolved.
Show resolved Hide resolved
### Resource Consumption

Resource consumption attacks targeting ComeBFT consensus algorithms aim to exploit the resource constraints of nodes within the network. The summary of `consensus.evidence.MaxAgeNumBlocks` states *"...This value will stop CometBFT from pruning block data"*. If the pruned window is set too large, nodes must store all the relevant data for the extended period, which can inundate their storage and memory capacity.

## Copyright

Copyright and related rights waived via [CC0](../LICENSE).