fix(x/forwarding)!: bind token identity to forwarding address derivation by damiannolan · Pull Request #6906 · celestiaorg/celestia-app

damiannolan · 2026-03-23T14:35:53Z

Overview

This PR adds the token identifier HexAddress to the forwarding address derivation.
This hoists the trust model off-chain, preventing linear scan of global permissionless warp tokens.

Below are the outcomes associated with that design decision that affect this PR.

Adding the token_id to the forwarding address derivation:

Trusted frontends provide token identifiers for forwarding address derivation
- This means we can now only forward that token in a single MsgForward execution - as that is the token identifier committed to in the forwarding address
- No send all denom balances in one go - Relayers can still submit a single tx with n MsgForward payloads
- No partial failures - This is actually a safer design decision, no need to worry about multiple token atomicity in execution path
There will exist a forwarding address for each unique denom
- E.g. Address for TIA is different to address for USDC for forwarding to same recipient 0x...abcd on destination domain 1234

Previous issue re. poisoning account with IBC denoms is void, as the token id is provided in rpcs (less ambiguity)
If somebody sends random denom assets to a forwarding address those funds are irrecoverable by design (no poison pill, accepted user risk)

Please disregard the failing Protobuf break-check CI.

NOTE: This PR directly addresses two bug reports submitted by @loopghost. This includes CELESTIA-247 via HackenProof and another submitted privately via email.

damiannolan

Self review

claude

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review.

_{Tip: disable this comment in your organization's Code Review settings.}

loopghost · 2026-03-24T13:16:59Z

Hi @damiannolan @rootulp @rach-id,

This PR directly addresses two distinct and independent security vulnerabilities I reported through Celestia's security program. Both were submitted with full end-to-end PoCs.

Report A (CELESTIA-247): submitted via HackenProof. @rootulp subscribed to it, moved it to triaged, then to in-review, and days later this PR was opened by the team.
Report B: submitted via email to Ismail.

These are two independent vulnerabilities with different root causes, different impacts, and different attack flows. Fixing one would not have fixed the other. Both are also distinct from CELESTIA-234, as both were identified and reported after PR #6877 was already merged.

The scope of this refactor directly corresponds to the attack vectors I disclosed, and I would appreciate acknowledgment of this. Specifically:

Reframing the PR title from refactor! to fix!, consistent with how other security-prompted changes in this module have been labeled (e.g., PR fix!: filter unsupported denoms before truncating in Forward #6877 and PR fix!: x/forwarding atomic state token sends #6881 both use fix!). Suggested: fix!(x/forwarding): bind token identity to forwarding address derivation.
Referencing CELESTIA-247 and my email submission in the PR description, consistent with how PR fix!: filter unsupported denoms before truncating in Forward #6877 references CELESTIA-234.
Restoring CELESTIA-247 to triaged status on HackenProof. The usual workflow is to move a report to triaged before beginning work on a fix. I am confused as to why this was reverted given that a fix PR was then opened.

Thank you.

damiannolan · 2026-03-24T13:26:21Z

Hi @loopghost! Thank you for dropping a comment here. I will of course update the PR title and add acknowledgements within the description. Much appreciated for your reports, they have been invaluable! cc. @rootulp for HackenProof status updates

loopghost · 2026-03-24T13:31:38Z

Thank you so much @damiannolan, really appreciate the quick response and the kind words! Super happy to know my contributions have been valuable to the team. I'll definitely keep at it. Looking forward to the updates and to @rootulp's confirmation on the HackenProof side. Let me know if you need anything else from me.

rootulp · 2026-03-24T15:22:57Z

Moved CELESTIA-247 to triaged.

rootulp

Thanks for the quick turnaround on the fix! 🙏 really appreciate it

damiannolan · 2026-03-27T14:05:47Z

@Mergifyio refresh

mergify · 2026-03-27T14:06:10Z

refresh

✅ Pull request refreshed

@loopghost

…ion (#6906) ## Overview This PR adds the token identifier `HexAddress` to the forwarding address derivation. This hoists the trust model off-chain, preventing linear scan of global permissionless warp tokens. Below are the outcomes associated with that design decision that affect this PR. **Adding the token_id to the forwarding address derivation:** - Trusted frontends provide token identifiers for forwarding address derivation - This means we can now only forward that token in a single MsgForward execution - as that is the token identifier committed to in the forwarding address - No send all denom balances in one go - Relayers can still submit a single tx with n MsgForward payloads - No partial failures - This is actually a safer design decision, no need to worry about multiple token atomicity in execution path - There will exist a forwarding address for each unique denom - E.g. Address for TIA is different to address for USDC for forwarding to same recipient 0x...abcd on destination domain 1234 Previous issue re. poisoning account with IBC denoms is void, as the token id is provided in rpcs (less ambiguity) If somebody sends random denom assets to a forwarding address those funds are irrecoverable by design (no poison pill, accepted user risk) Please disregard the failing `Protobuf break-check` CI. --- **NOTE**: This PR directly addresses two bug reports submitted by @loopghost. This includes CELESTIA-247 via HackenProof and another submitted privately via email.  --- <a href="https://app.devin.ai/review/celestiaorg/celestia-app/pull/6906" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a>  (cherry picked from commit 1c37708) # Conflicts: # go.mod # test/docker-e2e/e2e_hyperlane_test.go # x/forwarding/keeper/msg_server.go # x/forwarding/keeper/msg_server_test.go

@loopghost

…ion (backport #6906) (#6920) ## Overview This PR adds the token identifier `HexAddress` to the forwarding address derivation. This hoists the trust model off-chain, preventing linear scan of global permissionless warp tokens. Below are the outcomes associated with that design decision that affect this PR. **Adding the token_id to the forwarding address derivation:** - Trusted frontends provide token identifiers for forwarding address derivation - This means we can now only forward that token in a single MsgForward execution - as that is the token identifier committed to in the forwarding address - No send all denom balances in one go - Relayers can still submit a single tx with n MsgForward payloads - No partial failures - This is actually a safer design decision, no need to worry about multiple token atomicity in execution path - There will exist a forwarding address for each unique denom - E.g. Address for TIA is different to address for USDC for forwarding to same recipient 0x...abcd on destination domain 1234 Previous issue re. poisoning account with IBC denoms is void, as the token id is provided in rpcs (less ambiguity) If somebody sends random denom assets to a forwarding address those funds are irrecoverable by design (no poison pill, accepted user risk) Please disregard the failing `Protobuf break-check` CI. --- **NOTE**: This PR directly addresses two bug reports submitted by @loopghost. This includes CELESTIA-247 via HackenProof and another submitted privately via email. --- <a href="https://app.devin.ai/review/celestiaorg/celestia-app/pull/6906" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <hr>This is an automatic backport of pull request #6906 done by [Mergify](https://mergify.com).  --- <a href="https://app.devin.ai/review/celestiaorg/celestia-app/pull/6920" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a>  --------- Co-authored-by: Damian Nolan <[email protected]>

damiannolan added 4 commits March 23, 2026 14:49

refactor(x/forwarding): add token identifier to address derivation

c39d78c

chore: cleanup redundant code, helper func no longer required

df8fa6d

chore: cleanup integration tests

cd85407

Merge branch 'main' into damian/add-token-id-to-forwardaddr-derivation

fd023ac

damiannolan self-assigned this Mar 23, 2026

damiannolan added 3 commits March 23, 2026 15:43

chore: rm report md

9fbb640

chore: make lint-fix

a4599b7

chore: clean up events and single token forwarding

33a432c

damiannolan commented Mar 23, 2026

View reviewed changes

Comment thread tools/spam_txclient/main.go

chore: update CLAUDE.md

d977cd7

damiannolan commented Mar 23, 2026

View reviewed changes

damiannolan marked this pull request as ready for review March 23, 2026 16:04

damiannolan requested a review from a team as a code owner March 23, 2026 16:04

damiannolan requested review from ninabarbakadze and removed request for a team March 23, 2026 16:04

claude Bot reviewed Mar 23, 2026

View reviewed changes

damiannolan requested review from Manav-Aggarwal, jonas089 and rootulp March 23, 2026 16:04

chore: address self-review comments from pr

6b95cd1

This comment was marked as resolved.

Sign in to view

chore: apply devin suggestions

b0d76d7

This comment was marked as resolved.

Sign in to view

damiannolan added 2 commits March 23, 2026 17:37

chore: address devin suggestions

99c07ea

test: update expected addresses from stale vals

9d50e4b

rootulp requested review from evan-forbes and rach-id and removed request for ninabarbakadze March 23, 2026 21:28

rootulp changed the title ~~refactor(x/forwarding): add token identifier to address derivation~~ refactor(x/forwarding)!: add token identifier to address derivation Mar 23, 2026

evan-forbes previously approved these changes Mar 23, 2026

View reviewed changes

rach-id previously approved these changes Mar 24, 2026

View reviewed changes

chore: last nits from ai reviews

62a292b

damiannolan dismissed rach-id’s stale review via 62a292b March 24, 2026 12:51

rach-id previously approved these changes Mar 24, 2026

View reviewed changes

damiannolan changed the title ~~refactor(x/forwarding)!: add token id to address derivation with explicit single token forwarding~~ fix!(x/forwarding): bind token identity to forwarding address derivation Mar 24, 2026

This comment was marked as resolved.

Sign in to view

damiannolan changed the title ~~fix!(x/forwarding): bind token identity to forwarding address derivation~~ fix(x/forwarding)!: bind token identity to forwarding address derivation Mar 24, 2026

test: enable e2e hyperlane test and final touches

1b90cff

damiannolan dismissed rach-id’s stale review via 1b90cff March 24, 2026 15:50

claude Bot reviewed Mar 24, 2026

View reviewed changes

Comment thread test/docker-e2e/e2e_hyperlane_test.go

Comment thread x/forwarding/keeper/msg_server_test.go

Comment thread x/forwarding/CLAUDE.md

damiannolan added 3 commits March 25, 2026 13:24

chore: address e2e test configuration and claude nits

63cb83b

chore: claude nits

15cb835

Merge branch 'main' into damian/add-token-id-to-forwardaddr-derivation

dd62178

claude Bot reviewed Mar 25, 2026

View reviewed changes

Comment thread x/forwarding/keeper/msg_server.go

Comment thread test/docker-e2e/e2e_hyperlane_test.go

rootulp approved these changes Mar 25, 2026

View reviewed changes

Comment thread proto/celestia/forwarding/v1/event.proto

ninabarbakadze added the backport:v8.x PR will be automatically backported to the v8.x branch after merging label Mar 27, 2026

ninabarbakadze approved these changes Mar 27, 2026

View reviewed changes

damiannolan added this pull request to the merge queue Mar 27, 2026

Merged via the queue into main with commit 1c37708 Mar 27, 2026
29 of 30 checks passed

damiannolan deleted the damian/add-token-id-to-forwardaddr-derivation branch March 27, 2026 13:38

mergify Bot mentioned this pull request Mar 27, 2026

fix(x/forwarding)!: bind token identity to forwarding address derivation (backport #6906) #6920

Merged

Uh oh!

Conversation

damiannolan commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Uh oh!

Uh oh!

damiannolan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Claude Code Review

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

loopghost commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damiannolan commented Mar 24, 2026

Uh oh!

This comment was marked as resolved.

Uh oh!

loopghost commented Mar 24, 2026

Uh oh!

rootulp commented Mar 24, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rootulp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

damiannolan commented Mar 27, 2026

Uh oh!

mergify Bot commented Mar 27, 2026

✅ Pull request refreshed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

damiannolan commented Mar 23, 2026 •

edited

Loading

loopghost commented Mar 24, 2026 •

edited

Loading