Skip to content

fix(abstract-eth): verify ERC-7984 wrap/unwrap/finalizeUnwrap on multisig (CHALO-1957) - #10067

Merged
Venkat-Annavazzala merged 1 commit into
masterfrom
prabhsharansingh/chalo-1957-eth-erc7984-intent-verify
Oct 26, 2026
Merged

Venkat-Annavazzala merged 1 commit into
masterfrom
prabhsharansingh/chalo-1957-eth-erc7984-intent-verify

Conversation

@0xPrabh

@0xPrabh 0xPrabh commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

Summary

Part 1 of CHALO-1957, split from #10063 so the ETH-ecosystem change can be reviewed on its own.

Onchain-multisig verifyTransaction only exempted wrapApprove from its txParams.recipients requirement. wrap, unwrap and finalizeUnwrap legs therefore always ended in missing params. They are built server-side from the intent or the journey, so no client recipients exist for them.

Changes

  • New multisig path for the three intent types, content-based on the sendMultiSig args decoded from txHex:
    • unwrap — strict: target is the registered wrapper for unwrapParams.tokenName, args self-directed to the wallet base, non-empty ciphertext and proof
    • wrap — strict when wrapParams is present, bounded otherwise (registered wrapper target, mint recipient is the wallet base, positive amount)
    • finalizeUnwrap — bounded: registered wrapper target and well-formed request fields
  • Wrapper registry scoped to the wallet's network, resolved the same way for all three types. A testnet wrapper is rejected on a mainnet wallet and the reverse.
  • Signer-aligned recipient check. Eth.signTransaction builds the operation hash from txPrebuild.recipients[0], not from txHex. The prebuild must now carry exactly one recipient whose address, amount and data equal the decoded sendMultiSig args. This also applies to the existing wrapApprove path, which previously skipped data and amount.

Tests

Unit tests in sdk-coin-eth/test/unit/eth.ts cover every accept and reject branch, the network scoping, and the strict recipient check.

Behaviour change to note

The existing wrapApprove multisig path now rejects a prebuild with no recipient, several recipients, missing data, or a non-zero amount. Real prebuilds already carry the matching recipient.

Follow-up

Part 2 is the sdk-core change that strips stored recipients on the PA-approve rebuild for these intent types (#10063, to be rebased on this PR once it merges).

Ticket: CHALO-1957

…isig (CHALO-1957)

Onchain multisig verifyTransaction only exempted wrapApprove from the
txParams.recipients requirement, so wrap, unwrap and finalizeUnwrap legs
could never pass verification and ended in 'missing params'.

Add an explicit multisig path for the three intent types. It is
content-based on the sendMultiSig args in txHex: wrapper target resolved
from a registry scoped to the wallet's network, self-directed base
address rules, and well-formed calldata fields.

The user key signs the operation hash over txPrebuild.recipients[0], not
over txHex, so the prebuild must carry exactly one recipient whose
address, amount and data equal the decoded sendMultiSig args. The same
strict check now also applies to the existing wrapApprove path.

Ticket: CHALO-1957
@0xPrabh
0xPrabh requested a review from a team as a code owner October 26, 2026 13:54

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

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

@linear-code

linear-code Bot commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

CHALO-1957

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Unit tests are failing on Node 26.x (Current release line, non-blocking). This is not an LTS version yet, so it does not block merge, but it signals an incompatibility to fix before Node 26.x becomes LTS.

View run

@Venkat-Annavazzala Venkat-Annavazzala left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@Venkat-Annavazzala
Venkat-Annavazzala merged commit 1c574e5 into master Oct 26, 2026
26 checks passed
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.

2 participants