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

feat: rollapp ibc state validation upon state update #641

Merged
merged 12 commits into from
Mar 15, 2024

Conversation

srene
Copy link
Contributor

@srene srene commented Mar 9, 2024

Description


The objective of this PR is to prevent potential vector attacks that utilises other cosmos chains with the same chain id of an existing rollappid to enable malicious ibc-transfers to other chains that are not the rollapp itself.

For that, we need to prevent that any ibc-transfer can use the chain id of an existing rollapp, without coming from the real rollapp, authenticating the rollapp id. In the following we list 3 potential solutions:

Solution 1: Reject channels creation with same id of rollapp id

Main drawback: it is not possible to prevent channel creations with the chain id equal to a rollappid, that is not yet registered in the hub, but it can be known beforehand by an attacker.

Solution 2: Reject light clients update for a channel with a rollapp id equal to a registered rollapp id, when it does not come from the registered sequencer after the rollapp is created. By doing that, ibc will fail to accept packets from a channel that does not match rollapp sequencer info.

Main drawback: Headers received from an attacker channel until the first state info update from the valid rollapp sequencer, will be accepted as valid, being a vulnerability.

Solution 3: Checking the next validator hash in light client headers for the channel, corresponds to the same next validator hash created with a single validator, which is the sequencer registered for the rollapp in the state info.

Main drawback: The solution requires authenticating the rollapp id per ibc packet.

In this PR the solution 3 has been implemented, since even though is not the most efficient, it does not have the vulnerability issues of the first two.

Note that this is a short-term solution built with the assumptions of having a single honest sequencer for a rollapp.

Closes #643

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.

PR review checkboxes:

I have...

  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Targeted PR against the correct branch
  • included the correct type prefix in the PR title
  • Linked to the GitHub issue with discussion and accepted design
  • Targets only one GitHub issue
  • Wrote unit and integration tests
  • Wrote relevant migration scripts if necessary
  • All CI checks have passed
  • Added relevant godoc comments
  • Updated the scripts for local run, e.g genesis_config_commands.sh if the PR changes parameters
  • Add an issue in the e2e-tests repo if necessary

SDK Checklist

  • Import/Export Genesis
  • Registered Invariants
  • Registered Events
  • Updated openapi.yaml
  • No usage of go map
  • No usage of time.Now()
  • Used fixed point arithmetic and not float arithmetic
  • Avoid panicking in Begin/End block as much as possible
  • No unexpected math Overflow
  • Used sendCoin and not SendCoins
  • Out-of-block compute is bounded
  • No serialized ID at the end of store keys
  • UInt to byte conversion should use BigEndian

Full security checklist here

----;

For Reviewer:

  • Confirmed the correct type prefix in the PR title
  • Reviewers assigned
  • Confirmed all author checklist items have been addressed

---;

After reviewer approval:

  • In case the PR targets the main branch, PR should not be squash merge in order to keep meaningful git history.
  • In case the PR targets a release branch, PR must be rebased.

@srene srene requested a review from a team as a code owner March 9, 2024 01:35
Copy link

codecov bot commented Mar 9, 2024

Codecov Report

Attention: Patch coverage is 38.93130% with 80 lines in your changes are missing coverage. Please review.

Project coverage is 30.50%. Comparing base (9c77e99) to head (17b31ef).

Files Patch % Lines
x/delayedack/keeper/keeper.go 38.80% 28 Missing and 13 partials ⚠️
x/delayedack/ibc_middleware.go 13.33% 11 Missing and 2 partials ⚠️
testutil/mockpv/mockpv.go 45.00% 10 Missing and 1 partial ⚠️
testutil/keeper/delayedack.go 10.00% 9 Missing ⚠️
x/sequencer/types/sequencer.go 66.66% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #641      +/-   ##
==========================================
+ Coverage   29.87%   30.50%   +0.63%     
==========================================
  Files         225      226       +1     
  Lines       31861    31984     +123     
==========================================
+ Hits         9517     9756     +239     
+ Misses      20855    20669     -186     
- Partials     1489     1559      +70     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@omritoptix omritoptix left a comment

Choose a reason for hiding this comment

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

if you fix what I wrote the tests don't pass

@srene srene marked this pull request as draft March 9, 2024 11:00
@mtsitrin
Copy link
Contributor

  • I think u referenced the wrong issue here.
  • Why running this code per IBC packets, and not on the stateUpdate itself?
    The validation itself is not packet specific, no reason to run it per packet IMO

@omritoptix
Copy link
Contributor

omritoptix commented Mar 10, 2024

@mtsitrin is correct. better running it per UpdateState.
I believe this could be handled using anteHandler if no better way found.
In case this check fails the updateState should be rejected which should also protect against malicious actors trying to pretend to be a rollapp chain.

@omritoptix omritoptix changed the title feat: rollapp id validation for ibc packets feat: rollapp ibc state validation upon state update Mar 10, 2024
@omritoptix omritoptix linked an issue Mar 10, 2024 that may be closed by this pull request
@srene srene force-pushed the srene/258-ibc-rollapp-validation branch from f6709d1 to 74c085d Compare March 11, 2024 08:55
@srene
Copy link
Contributor Author

srene commented Mar 11, 2024

added right issue number in the description

@srene srene force-pushed the srene/258-ibc-rollapp-validation branch from 0fe8389 to 63ec364 Compare March 15, 2024 11:22
@srene srene marked this pull request as ready for review March 15, 2024 11:51
@omritoptix omritoptix merged commit 70aa5b6 into main Mar 15, 2024
7 of 12 checks passed
@omritoptix omritoptix deleted the srene/258-ibc-rollapp-validation branch March 15, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate rollapp IBC state update against current rollapp state
3 participants