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

SlashPackets may lead to delegators getting incorrectly slashed #643

Open
2 tasks done
mpoke opened this issue Jan 6, 2023 · 2 comments
Open
2 tasks done

SlashPackets may lead to delegators getting incorrectly slashed #643

mpoke opened this issue Jan 6, 2023 · 2 comments
Assignees
Labels
more-info-needed Further information is requested scope: cosmos-sdk Integration with Cosmos SDK type: bug Issues that need priority attention -- something isn't working

Comments

@mpoke
Copy link
Contributor

mpoke commented Jan 6, 2023

Problem

As compare to the normal behavior of SDK chains, ICS introduces an extra delay between the time an infraction is committed and the time the misbehaving validator is slashed (i.e., due to relaying). If during this period a delegator delegates to the misbehaving validator, it will get slashed although it's stake did not contribute to power the validator had when it committed the infraction.

Closing criteria

Asses whether this is indeed an issue. In case it is, figure out a way to fix it.

Problem details

Currently, the SDK accepts two types of evidence: downtime and double-signing. For both an infraction height is created that is used by the staking module to avoid slashing undelegations or redelegations that occur before the infraction was committed (i.e., it affects what unbonding stake can be slashed). Note that the infraction height doesn't have any impact on the bonded stake that is slashed. This means that if a delegator manages to delegate to a validator after it committed the infraction but before it got slashed, then that delegator will get slashed as well. The purpose of this issue is to analyze

  • If this is a current issue in the SDK.
  • If ICS makes this issue worse.

Note that, in the case of ICS, every infraction height on every consumer chain is mapped correctly to a height on the provider (see the mapping from consumer chain block heights to provider chain block heights in the spec).

Downtime

Downtime evidence is generated in slashing.BeginBlock based on the LastCommitInfo received from TM. If at height h a validator V has not voted in enough blocks, then V is slashed at height h for an infraction at height h-2, i.e., https://github.com/cosmos/cosmos-sdk/blob/47f46643affd7ec7978329c42bac47275ac7e1cc/x/slashing/keeper/infractions.go#L85.

This means that a delegator D can delegate to V at height h-1 and as a result get slashed at height h for the downtime infraction at height h-2. ICS makes this worse due to the relaying delay.

Double-singing

Double-signing evidence is passed to the SDK by TM and handled in evidence.BeginBlock. If at height h, some evidence is received for a validator V for an infraction committed at height hi, then V is slashed at height h for an infraction at height hi-1, i.e., https://github.com/cosmos/cosmos-sdk/blob/47f46643affd7ec7978329c42bac47275ac7e1cc/x/evidence/keeper/infraction.go#L101.

This means that a delegator D can delegate to V at any height in the interval [hi, h-1] and as a result get slashed at height h for the double-singing infraction at height hi-1. ICS makes this worse due to the relaying delay.

TODOs

  • Labels have been added for issue
  • Issue has been added to the ICS project
@mpoke mpoke added type: bug Issues that need priority attention -- something isn't working question scope: cosmos-sdk Integration with Cosmos SDK labels Jan 6, 2023
@mpoke
Copy link
Contributor Author

mpoke commented Jan 6, 2023

This means that a delegator D can delegate to V at any height in the interval [hi, h-1] and as a result get slashed at height h for the double-singing infraction at height hi-1. ICS makes this worse due to the relaying delay.

@sergio-mena Is there any guarantee provided by TM re. the size of the [hi, h-1] interval? In other words, how fast does TM detect double-signing infractions?

@sergio-mena
Copy link

sergio-mena commented Jan 11, 2023

A priori there is no strong guarantee on when an infraction (evidence of misbehavior) will be committed to a block. They are pooled like txs, gossiped like txs, but they take priority over tx in the block.

In practical terms, the evidence will be committed as soon as an honest proposer has received it via gossip. However, there is no upper bound on the timeliness of an evidence being committed to a block. Tendermint even has logic to disregard evidences if they are beyond the trusting period: take a look this section.

@mpoke mpoke added this to the ICS v1.1.0 milestone Jan 27, 2023
@mpoke mpoke removed this from the ICS v1.1.0 milestone Mar 5, 2023
@mpoke mpoke self-assigned this Mar 5, 2023
@mpoke mpoke removed their assignment Aug 30, 2023
@mpoke mpoke added the S: NewThings Work towards your business objectives with new products, features, or integrations label Sep 13, 2023
ThanhNhann pushed a commit to decentrio/interchain-security that referenced this issue Jan 3, 2024
* Backport commits from main to v3 release branch (cosmos#682)

* reorganize channel handshake handler (cosmos#647)

* reorganize channel handshake handler

split out channel state changes into its own function.

* readjust 27-interchain-accounts to not rely on state being set before the application callback

* add changelog and migration doc entry

* Update modules/core/04-channel/keeper/handshake.go

* docs: ICA Overview (cosmos#626)

* docs: ica overview

* fix: ordering

* add spacing

* fix: spacing

* fix: remove bulletpoints

* fix: wording

* update go mod for security vulnerabilities (cosmos#655)

* update go mod for security vulnerabilities

* update package-lock.json

* update vue dependency (cosmos#662)

* bump glob-parent version in json package (cosmos#663)

* build(deps): bump actions/setup-go from 2.1.4 to 2.1.5 (cosmos#656)

Bumps [actions/setup-go](https://github.com/actions/setup-go) from 2.1.4 to 2.1.5.
- [Release notes](https://github.com/actions/setup-go/releases)
- [Commits](actions/setup-go@v2.1.4...v2.1.5)

---
updated-dependencies:
- dependency-name: actions/setup-go
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: colin axnér <[email protected]>

* docs: begin removal of internal "spec" directories (cosmos#634)

* begin removal of spec docs within core ibc

* remove broken link

* Apply suggestions from code review

Co-authored-by: Damian Nolan <[email protected]>

* remove broken link

* remove broken links

* Apply suggestions from code review

Co-authored-by: Carlos Rodriguez <[email protected]>

Co-authored-by: Damian Nolan <[email protected]>
Co-authored-by: Carlos Rodriguez <[email protected]>

* Modify `OnChanOpenTry` application callback to perform app version negotitation (cosmos#646)

* remove NegotiateAppVersion and AppVersion gRPC (cosmos#643)

The NegotiateAppVersion callback has been removed from the IBC Application interface.
The gRPC AppVersion has been removed.
The app version negoitation will be handled by applications by returning the version in OnChanOpenTry.

* Modify `OnChanOpenTry` to return application version (cosmos#650)

* modify OnChanOpenTry to return negotiated version

modify IBCModule interface function OnChanOpenTry to return the negotiated app version. Tests have not been updated

* fix ibc_module_test.go tests

* fix tests

* Apply suggestions from code review

* add handshake test case

* add CHANGELOG and migration docs

* update documentation

* fix broken link

* fix broken link (cosmos#664)

* chore: update make build-docs, add docs build checker (cosmos#667)

* update Makefile, add docs build checker

* Update .github/workflows/check-docs.yml

Co-authored-by: Marko <[email protected]>

Co-authored-by: Marko <[email protected]>

* register ICA query server, fix panics in params query cli (cosmos#666)

Register the controller and host query servers to a chain.
Returns an error upon cli params query failure instead of panicing.

* update of roadmap with latest release (cosmos#653)

* update of roadmap with latest release and changed the way release versions are encoded

* fixed typo

Co-authored-by: Carlos Rodriguez <[email protected]>

* build(deps): bump actions/checkout from 2.3.1 to 2.4.0 (cosmos#672)

Bumps [actions/checkout](https://github.com/actions/checkout) from 2.3.1 to 2.4.0.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v2.3.1...v2.4.0)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* allow ics20 to connect to middleware (cosmos#675)

* allow ics20 to connect to middleware

Creates ics4Wrapper which allows middleware applications to only implement SendPacket to connect ics20 as an application in its middleware stack

* add changelog and migration doc

* fix migration doc spelling

* fix: register InterchainAccount as x/auth GenesisAccount (cosmos#676)

* adding GenesisAccount interface registration for InterchainAccount impl

* updating RegisterInterfaces ica godoc

* enable mergify for backports (cosmos#678)

Co-authored-by: Carlos Rodriguez <[email protected]>

Co-authored-by: Sean King <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Damian Nolan <[email protected]>
Co-authored-by: Carlos Rodriguez <[email protected]>
Co-authored-by: Marko <[email protected]>
Co-authored-by: Carlos Rodriguez <[email protected]>

* fixes evmos tests

* cleanup

* changelog and cleanup

* pr suggestions. bump chainid

Co-authored-by: colin axnér <[email protected]>
Co-authored-by: Sean King <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Damian Nolan <[email protected]>
Co-authored-by: Carlos Rodriguez <[email protected]>
Co-authored-by: Marko <[email protected]>
Co-authored-by: Carlos Rodriguez <[email protected]>
Co-authored-by: Federico Kunze Küllmer <[email protected]>
@mpoke mpoke added more-info-needed Further information is requested and removed question labels Mar 22, 2024
@mpoke mpoke self-assigned this Apr 11, 2024
@mpoke mpoke removed the S: NewThings Work towards your business objectives with new products, features, or integrations label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more-info-needed Further information is requested scope: cosmos-sdk Integration with Cosmos SDK type: bug Issues that need priority attention -- something isn't working
Projects
Status: 🤔 F1: Investigate
Development

No branches or pull requests

2 participants