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

Zero out client state before processing upgraded client proof #292

Closed
3 tasks
colin-axner opened this issue Jul 22, 2021 · 3 comments
Closed
3 tasks

Zero out client state before processing upgraded client proof #292

colin-axner opened this issue Jul 22, 2021 · 3 comments
Assignees
Labels
07-tendermint type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.

Comments

@colin-axner
Copy link
Contributor

Summary

In 07-tendermint, we check the proof against the submitted upgraded client state. If the relayer submits the zeroed out client state wrong, it fails. We should zero out the client state before checking the proof. If the proof works for the zeroed out client state, no need to prevent an upgrade because a relayer implementation was incorrect


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@crodriguezvega crodriguezvega added 07-tendermint type: refactor Architecture, code or CI improvements that may or may not tackle technical debt. labels Jan 3, 2022
@crodriguezvega crodriguezvega moved this to Backlog in ibc-go Jan 3, 2022
faddat pushed a commit to notional-labs/ibc-go that referenced this issue Mar 1, 2022
faddat pushed a commit to notional-labs/ibc-go that referenced this issue Mar 1, 2022
* WIP contextual codecs

* Push changes

* WIP

* Merge PR cosmos#319: Remove GetSignBytes usage

* Merge PR cosmos#314: Fix timeout determination

* fix mutex

* fix timeout bug

* fix GetSignBytes

* address other calls to GetSignBytes

Co-authored-by: Jack Zampolin <jack.zampolin@gmail.com>

* Working version

* Update retries for better errors

* Update import paths and address cosmos#292

* Address cosmos#293

* Address cosmos#295

* Address cosmos#315

* Add defensive key checks per cosmos#297

* Add flags for timeout offsets

* Update retries to have better and more explainatory errors

* WIP acks

* Update to remove replace

* update to sdk master

* Update sdk and make changes

* WIP timeouts and acks, strategy refactor

* WIP cleanup

* update to v0.40.0-rc2

* Refactor path generation and status

* Merge PR cosmos#274: feat: allow building a shared library

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Co-authored-by: Michael FIG <mfig@agoric.com>
@crodriguezvega crodriguezvega added this to the Q2-2022-backlog milestone Mar 31, 2022
@crodriguezvega
Copy link
Contributor

crodriguezvega commented May 18, 2022

So do we need to do something like this?

// Verify client proof
zeroedUpgradedClient := upgradedClient.ZeroCustomFields()
bz, err := cdc.MarshalInterface(zeroedUpgradedClient)
if err != nil {
	return nil, nil, sdkerrors.Wrapf(clienttypes.ErrInvalidClient, "could not marshal client state: %v", err)
}
// construct clientState Merkle path
upgradeClientPath := constructUpgradeClientMerklePath(cs.UpgradePath, lastHeight)
if err := merkleProofClient.VerifyMembership(cs.ProofSpecs, consState.GetRoot(), upgradeClientPath, bz); err != nil {
	return nil, nil, sdkerrors.Wrapf(err, "client state proof failed. Path: %s", upgradeClientPath.Pretty())
}

In this part of the code?

@colin-axner
Copy link
Contributor Author

Correct, this should be done with the 02-client refactor

@crodriguezvega crodriguezvega moved this from Backlog to Todo in ibc-go May 19, 2022
@chatton chatton self-assigned this Jun 13, 2022
@crodriguezvega crodriguezvega moved this from Todo to In review in ibc-go Jun 13, 2022
@seantking seantking moved this from In review to On hold in ibc-go Jun 27, 2022
@crodriguezvega crodriguezvega moved this from On hold to In review in ibc-go Jul 4, 2022
@colin-axner
Copy link
Contributor Author

closed by #1674

Repository owner moved this from In review to Done in ibc-go Jul 12, 2022
CosmosCar pushed a commit to caelus-labs/ibc-go that referenced this issue Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
07-tendermint type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants