[prajwalu142]

Problem

A compromised or tampered platform prebuild could supply a dataToSign authorizing an arbitrary multisig transfer (e.g. full wallet drain to an attacker address) while txHex still showed the user's intended recipient. signTransaction signed the platform-supplied bytes blindly and verifyTransaction never checked them — the two artifacts are independent inputs from the same untrusted prebuild.

Goal

No user-key signature may ever be produced over a payload that is not derived from the transfer encoded in the verified txHex.

Fix

• Recompute the packed multisig payload locally with packGenericMultisigDataToSign (@taquito/michel-codec's packData; the package was already resolved transitively via @taquito/local-forging, now declared directly) and require a byte-for-byte match against the supplied dataToSign in both signTransaction and verifyTransaction.
• An absent dataToSign is unaffected (fee-account / fully-signed paths keep working).
• Regenerated the sign fixtures consistently and switched the sign tests to the key derived from the already-published test seed (the [SECRET:...] placeholder has no resolver outside CI).

Testing

Full sdk-coin-xtz suite: 137 passing / 0 failing. The tamper tests fail when the check is stashed out (regression proven). tsc build and eslint clean. Local packData verified byte-identical to the node's /pack_data output against the repo's own fixtures.

Ticket: CSHLD-2227

🤖 Generated with Claude Code

[linear-code]

CSHLD-2227

[github-actions]

⚠️ Compatibility checks are failing on Node.js 27-nightly (experimental, non-blocking). This does not block merge, but it signals an incompatibility to fix before this release becomes supported.

View run

[prajwalu142]

@claude review for logical correctness, implementation completeness and regressions

[claude]

I reviewed this PR and didn't find any bugs. It changes the XTZ multisig signing and verification path and adds a dependency, so a human from the module's code owners should still look at it.

What was reviewed:

• The new check in signTransaction and verifyTransaction: it runs only when dataToSign is present, and it compares against contents[0], which is the operation the builder signs dataToSign for.
• assertDataToSignMatchesTransfer: it rejects operations with no parameters or no counter, compares hex ignoring case, and repacks locally with packData without calling the node.
• I didn't confirm that the local packData output is byte-identical to the node's pack_data output. The PR description says it is, but checking that needs a test run.

Extended reasoning...

The change touches the XTZ coin module's transaction signing (signTransaction) and prebuild verification (verifyTransaction). It adds helpers in src/lib/utils.ts and declares @ taquito/michel-codec as a direct dependency. This is a security-sensitive surface: the check decides whether a user key signs a multisig spend authorization. CODEOWNERS assigns modules/sdk-coin-xtz/ to a specific team and package.json changes to SDK reviewers. The signing-path sensitivity and those ownership rules decided against auto-approval, even though the logic is small and has tests.

[claude]

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.}

[mukeshsp]

@claude review

[github-actions]

Claude finished @mukeshsp's task in 25s —— View job

---

Review of #10160: bind Xtz dataToSign to verified txHex

No blocking issues found. I read the diff and the surrounding builder code, but I did not run the tests or build, so the 137-passing result is the PR author's claim.

Logic

• The check recomputes the packed payload from the transfer decoded out of txHex, then requires an exact match with the supplied dataToSign. That covers the stated threat.
• It checks contents[0] only. overrideDataToSign({ dataToSign }) defaults to index 0, and buildImplementation signs the override for that index, so the verified operation and the signed operation are the same one.
• The check fails closed when the operation has no parameters or no counter, and when the builder throws on a malformed transfer.
• signTransaction runs the check before any key use, so the user key never signs mismatched bytes. This holds even if a caller skips verifyTransaction.
• An absent dataToSign skips the check, as described. The fee-account and fully-signed paths are unaffected.
• The hex comparison lowercases only the supplied value. That is fine because Buffer.toString('hex') always returns lowercase.

Minor suggestions (non-blocking)

1. Skipped check on a falsy dataToSign. Both call sites use if (dataToSign), so an empty string skips the check. In signTransaction the override is skipped under the same condition, so nothing is signed over an unchecked payload. Using !== undefined would be a more explicit fail-closed choice.
2. Missing test for a batched txHex. Tests for a multi-operation txHex would document that only operation 0 is bound. They would also show what happens if the builder later supports overrides at other indices, where this check would silently under-cover.
3. Duplicated decode in verifyTransaction. It now builds the transaction from txHex twice, once for the existing decode and once in assertDataToSignBoundToTxHex. That is harmless but could share the decoded result.
4. Dependency pin. @taquito/michel-codec is ^23.0.3 while its siblings are ^23.0.2. Confirm that yarn.lock resolves a single version, because a mismatch with local-forging could duplicate the package.
5. Node-parity claim. The PR says local packData is byte-identical to the node's /pack_data. The author verified this against the repo fixtures, but I did not re-check it. Covering more address types (originated contract destination, implicit tz1/tz2/tz3) in the unit tests would back the claim.

Process
This is a signing-path change with a new dependency, so it needs a review from the sdk-coin-xtz code owners as well.

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

[mukeshsp]

approve with minor comments

The fix does what it describes. dataToSign is recomputed locally from the transfer decoded from txHex and compared byte for byte, in both signTransaction and verifyTransaction.

I traced the binding through the code:

• The recipient, amount and counter come from getMultisigTransferDataFromOperation. That is the same source verifyTransaction uses for outputs, so what the user verifies is what gets bound.
• The wallet contract address is bound as well.
• The signing override targets index 0 and addTransferSignature writes to contents[0]. The new check also reads contents[0], so the checked operation is the signed one.
• Skipping the check when dataToSign is absent keeps the fee-account and fully-signed paths working.
• yarn.lock already has the @taquito/michel-codec@^23.0.3 entry, so frozen-lockfile installs are fine.

Non-blocking comments

1. Reveal as first operation (xtz.ts, assertDataToSignBoundToTxHex). If contents[0] is a reveal, the check throws "not a multisig transfer" even when a valid transfer sits at index 1. That fails closed, and the signing path already assumes the transfer is at index 0. Still, a reveal-first prebuild would now be rejected at verifyTransaction with a confusing error. Can you confirm the platform never emits reveal-first multisig prebuilds, or locate the first transaction op explicitly?
2. Dead branch in the test (test/unit/util.ts, "should reject a non-multisig operation"). The fixture has no singlesig op, so the singlesigOperation === undefined branch is always taken and the second assert.throws never runs. Please drop the conditional and use the synthetic operation directly.
3. Missing positive sign test. The new signTransaction tests only cover tampered dataToSign. A case with a matching dataToSign that signs successfully would guard against false rejects.
4. Double decode. verifyTransaction now builds the transaction twice. It's cheap, but the first build's toJson() could be reused.
5. Dependency range. The ^23.0.3 for michel-codec differs from the neighbouring ^23.0.2 entries. This is harmless, but you could align them.