[yashvanthbl137-crypto]

The wrapApprove PA-rebuild (the onchain-multisig server-leg approval) failed verification: verifyMultisigWrapApproveTransaction demanded txParams.wrapParams (tokenName + amount) — but the PA-rebuild path carries no caller wrapParams (the PA's buildParams store only the type; the intent's wrapParams live in the journey). The verify's own doc comment contemplates the skip for the pendingApproval re-sign paths; the rebuild's merged txParams accidentally triggered the full check.

Restructure verifyMultisigWrapApproveTransaction to be self-sufficient on the PA-rebuild path:

• the wrapParams cross-check demand: skipped when txParams.pendingApprovalId is present (the PA-rebuild)
• the pair resolved from the tx's own calldata: the approve's spender decoded → the reverse lookup over the registered ERC-7984 pairs on the network (by the wrapper address → the pair's tokenName)
• the bounded checks preserved: the inner target = the pair's underlying ERC-20, no native value inside sendMultiSig, the spender = the wrapper, the prebuild-recipients mirror
• the wrapParams cross-check (the amount vs the intent) runs only on the direct-sign path where the caller carries the intent

Validated in staging: the wrapApprove PA approval — the rebuild 200, the verify passed (isPaRebuild: true, hasWrapParams: false), the tx signed, the approval PUT reached WP and the policy resolved.

Ticket: CHALO-2010

[linear-code]

CHALO-2010

[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

[yashvanthbl137-crypto]

@claude review

[github-actions]

Claude finished @yashvanthbl137-crypto's task in 23s —— View job

---

Review of PR #10096

I read the diff and the surrounding getErc7984WrappersForNetwork code. I did not run the tests or the build.

The restructure is sound. Resolving the pair from the approve's spender and then running the bounded checks (inner target, zero native value, spender equals wrapper) is the right approach for the PA-rebuild path. The test coverage is good: a happy path, a tampered spender, and both direct-sign regressions.

Findings

1. The approve amount is unchecked on the PA-rebuild path. modules/abstract-eth/src/abstractEthLikeNewCoins.ts, around the new if (!isPaRebuild && approve.amount !== wrapParams?.amount) line.

   • A server-built tx could approve any amount, such as uint256.max, to a registered wrapper, and verification would pass.
   • The spender is limited to registered wrappers, so the damage is bounded. It is still a loss of the amount check on a signing path.
   • If the pending approval's intent amount can be reached from the PA data in the SDK (or passed as txParams), compare against it. Otherwise, say explicitly in the comment that the amount is enforced elsewhere (WP policy or the journey).

2. isPaRebuild is derived from a caller-supplied field. The code reads (txParams as { pendingApprovalId?: string }).pendingApprovalId.

   • Anyone calling verifyTransaction with a pendingApprovalId skips the wrapParams demand and the amount check.
   • Callers control their own txParams, so this is probably acceptable. The comment should still state the trust assumption.
   • Replace the as cast with a typed field on the txParams type. This follows the CLAUDE.md guidance on avoiding loose typing.

3. The find over byTokenName is a linear scan. It runs on every PA verify. The map is small, so this is fine. getErc7984WrappersForNetwork already returns addresses, so you could add a reverse map there.

4. The wrapParams?.tokenName fallback is confusing. coins.get(wrapParams?.tokenName ?? '') and the '${wrapParams?.tokenName}' error message only exist to satisfy the type checker. The !isPaRebuild guard above already guarantees wrapParams is present on that branch. Pull tokenName and amount into local consts after the guard to remove the ?. noise.

5. Test gaps (minor):

   • A PA-rebuild case with a non-zero inner value or a wrong inner target would show the preserved checks still fire after the reordering.
   • The tampered-spender test replaces the hex address. Asserting on the exact rejection path would make it less brittle.

6. Reordering: the tx is now decoded before the pair lookup. The behavior is equivalent, and errors on the direct path keep their original messages except for ordering. Note that an unregistered tokenName on the direct path now surfaces only after the tx build and decode succeed, not before.

Verdict: Looks good overall. I'd resolve item 1 (or document why it's safe) before merging, and items 2 and 4 are cheap cleanups.

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

[claude]

Findings marked 🟡 are optional suggestions and need no follow-up push.

[0xPrabh]

@claude review

[github-actions]

Claude finished @0xPrabh's task in 32s —— View job

---

Review of PR #10096

I read verifyMultisigWrapApproveTransaction at HEAD (c33828f) and the new tests in modules/abstract-eth/test/unit/coin.ts. I did not run the tests or the build. The checkout is shallow, so I couldn't run git diff origin/master...HEAD.

The two 🔴/🟡 inline comments from the earlier review no longer apply to this commit.

• Strictness follows wrapParams, not pendingApprovalId.
  • requested is built only when both tokenName and amount are present (abstractEthLikeNewCoins.ts:4035).
  • The code rejects partial wrapParams, or no wrapParams without a pendingApprovalId (:4039).
  • When requested exists, the tokenName lookup and the exact amount check run even if a pendingApprovalId is present (:4099-4128).
  • Tests cover this: "cross-checks wrapParams … even when a pendingApprovalId is present" and "rejects a mismatched wrapParams tokenName …".
• The rebuild path now bounds the allowance. With no wrapParams, the amount must be a decimal integer, greater than 0 and less than MAX_UINT256 (:4129). Tests cover zero and unlimited amounts.
• The other preserved checks still run on the rebuild path. These are the inner target equal to the pair's underlying ERC-20, zero native value, and spender equal to wrapper. The tests cover the wrong target, non-zero value and unregistered spender cases.

Remaining findings, all minor

1. Stale doc comment (:4011-4014). It still says the inner calldata is approve(wrapper, wrapParams.amount) exactly. Update it to describe both modes: strict when wrapParams is present, and bounded with a spender reverse-lookup on the rebuild.
2. Residual trust gap on the rebuild path. Any amount from 1 to MAX_UINT256 - 1 is accepted, to any registered wrapper. This follows from the PA carrying no intent amount. Say in the comment that the intent amount is enforced server-side (the journey and WP policy), or compare against the PA amount if the SDK can reach it.
3. Missing test: wrapParams with only one of tokenName or amount plus a pendingApprovalId should reject. The code handles it at :4039 but no test covers it.
4. Minor cleanup: the reverse lookup at :4092 is a linear scan over byTokenName. The map is small, so this is fine. If getErc7984WrappersForNetwork exposes an address-keyed map, use that.

Verdict: Looks good. The rebuild path is no longer a bypass when wrapParams is present. Item 1 is worth fixing before merge. The rest are optional.

[claude]

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