[dpkjnr]

What

A narrower alternative to #10134 that hardens verifyTssTransaction only, assuming the caller passes correct txParams. Signing routing, sdk-core plumbing, the legacy EvmCoin verifier, FLR and statics are untouched.

Payments (transfer, and stored payment treated the same way)

• Single native: destination and value match, and calldata is empty.
• Single ERC-20: a canonical transfer(address,uint256) whose recipient and amount match, with zero native value, sent to the declared token contract (resolved from statics; a token coin declares itself).
• Explicit calldata (dApp / WalletConnect): to, value and data must match exactly. Nonzero native value is allowed.
• Native batch: sent to the network batcher, with batch(address[],uint256[]) arrays matching the intent and value equal to their sum.
• Token batch: batchTransferFrom(address,address[],uint256[]) with matching arrays, zero value and the declared token contract. Top-level tokenName is not required.

Consolidation (verification.consolidationToBaseAddress): the existing base-address checks stay. An ERC-20 sweep must also be canonical, carry zero native value and call the declared token contract; a native sweep must carry no calldata. Sweep amounts are not compared.

Warnings only: a token that statics can't resolve, a network with no batcher configured, ERC-20 calldata where no token is declared (recipient, amount and value are still enforced), ERC-7984 payments, and intent types with no verifier (including contractCall / customTx). A warning never skips the other checks.

Deliberately not in this PR

• Rejecting contractCall / customTx: existing upstream tests send customTx through this verifier and expect it to pass.
• Legacy / type-0 signing-preimage routing, binding serialized bytes to signableHex on ECDSA, the EvmCoin legacy bypass, FLR atomic handling, and the Katana batcher address in statics.
• Plumbing that separates the stored intent from caller params.

Test changes

• New: sdk-coin-eth/test/unit/tssPaymentVerification.ts, which builds real hteth bytes and covers matches and definite mismatches for every rule above. It also checks that a known payment with the wrong function fails, that a missing batcher does not hide wrong batch amounts, and that explicit calldata may carry native value.
• sdk-coin-xdc/test/unit/xdcToken.ts: the fixture used mainnet USDC's contract for txdc:tmt, so it now uses the token's contract from statics.
• sdk-coin-bsc/test/unit/bsc.ts: a tampered WalletConnect payload is now rejected by the exact-calldata check, so the expected message changed.

Verification

• Node 24.22.1 unit suites: abstract-eth 188, sdk-coin-eth 549 (33 new), sdk-coin-evm 40, sdk-coin-bsc 43, sdk-coin-xdc 65, sdk-coin-polygon 71, sdk-coin-flr 61, sdk-coin-arbeth 56, all passing.
• No SDK replay against production data was run for this branch. Exact explicit-calldata matching relies on the earlier field-level SQL validation, which found no mismatches.

Ticket: CECHO-2460

🤖 Generated with Claude Code

[linear-code]

CECHO-2460

[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

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

[dpkjnr]

@claude review

[github-actions]

Claude finished @dpkjnr's task in 29s —— View job

---

Review of #10136: harden EVM TSS payment and consolidation checks

I read the abstractEthLikeNewCoins.ts diff in full. I did not read the new test file, and I did not run the test suites. The approach is sound and the scope matches the PR description. The canonical-ABI re-encode check is a good way to catch trailing bytes, dirty padding and odd offsets. I found no blocking bugs, only the items below.

Should fix

1. Consolidation warning prints "undefined". In the consolidation ERC-20 branch (abstractEthLikeNewCoins.ts ~L4030), resolveTssToken(txParams.tokenName) returns { isToken: false } with no reason when no token is declared and the coin is a base coin. The warning then reads undefined: the consolidation token contract was not verified. The single-payment and batch paths already fall back with token.reason ?? 'payment declares no token'. Use the same fallback here, for example token.reason ?? 'consolidation declares no token'.

2. A consolidation with ERC-20 calldata passes without a contract check if no token is declared. The base-address and canonical checks still apply, but to can be any contract. This is the same trade-off you accept for payments. It is worth a test that pins the behavior (a warning, not a pass or a throw) so a later change doesn't alter it unnoticed.

Worth a look

3. Unparseable amounts throw a raw error. In verifyTssBatchPayment (the reduce over recipients), BigInt(String(recipient.amount)) throws a bare SyntaxError for a malformed amount such as "1.5" or "". Every other mismatch goes through throwRecipientMismatch. tssValuesEqual already wraps this in try/catch, so a small helper that returns a mismatch on a parse failure would keep the error behavior consistent.

4. A token batch with a native recipient is not rejected. In the token-batch path, a recipient with isToken: false just produces the "declares no token" warning. A mixed batch is therefore not flagged. If a batch moves exactly one asset, as the comment says, consider rejecting recipients that resolve to the native asset inside a batchTransferFrom.

5. Warning noise. warnTssVerificationIncomplete now fires for every non-dedicated intent type, and for transfer with no recipients, on every verify call. That is fine for rollout visibility, but console.warn per transaction can be noisy in the express server. Consider routing it through whatever logger the module uses elsewhere.

Notes and nits

• The explicit-calldata branch skips the token and contract checks and allows native value. The PR description states this. The safety rests on the caller's txParams being correct, and the PR defers stored-intent separation.
• The BSC and XDC test edits make sense: the fixture used the wrong token contract, and the changed error message comes from the stricter check.
• The PR says no replay against production data was run. For a change that now rejects transactions it used to accept (native consolidation with calldata, non-canonical ERC-20 encodings, payment now verified like transfer), a replay or a staged rollout would give more confidence than the SQL validation of explicit calldata alone.

[dpkjnr]

@claude review again

[github-actions]

Claude finished @dpkjnr's task in 36s —— View job

---

Re-review of #10136 (second commit d4235db)

I read the diff of HEAD~2..HEAD for abstract-eth and sdk-core. I did not run the test suites. I could not diff against origin/master because the checkout is shallow and has no merge base.

Resolved: ERC-7984 approvals from a base-coin wallet

The new chainName argument on resolveEffectiveTxParams in ecdsaMPCv2.ts (L994/L1004) fixes the issue raised in the earlier inline comment.

• mpcUtils.ts:251 sets amount.symbol to the chain name for native sends and to the token name otherwise.
• On a stored-request approval, a token recipient now gets tokenName from amount.symbol. A confidential token therefore reaches the token.confidential warning path and no longer fails with the "not a native send or ERC-20 transfer" error.
• Native sends still produce no tokenName, because the symbol equals the chain.
• The PR's three updated test stubs add getChain, which matches the code change.

Still open from the first review

1. Consolidation warning can print "undefined". At about L4036, `${token.reason}: the consolidation token contract was not verified` is unchanged. resolveTssToken returns { isToken: false } with no reason when no token is declared. Use token.reason ?? 'consolidation declares no token', as the payment paths do.
2. Raw SyntaxError on a malformed amount. In the native-batch branch of verifyTssBatchPayment, BigInt(String(recipient.amount)) inside the reduce throws a bare error for an amount like "1.5" or "". Every other mismatch goes through throwRecipientMismatch. Parse once up front and report a mismatch on failure.
3. A token batch with a native recipient is only a warning. Take a batchTransferFrom where one recipient resolves to isToken: false. The code treats it as "declares no token" and warns. The comment says a batch moves exactly one asset, so that recipient should be rejected.

New observations

4. MPCv1 ECDSA is not updated. ecdsa/ecdsa.ts:835 and :842 still call resolveEffectiveTxParams without chainName. If any EVM wallet signs through MPCv1, the same payment mapping can drop the token identity there. If MPCv1 is out of scope for EVM, a one-line note in the PR description would say so.
5. Pin the ERC-7984 behavior with a test. Nothing in the diff covers a stored payment intent with a confidential token name that reaches verifyTssTransaction and produces a warning instead of a throw. That is the regression the earlier comment found, so a test belongs next to the chainName change. The same applies to a consolidation with ERC-20 calldata and no declared token, which should warn.
6. Warning volume. console.warn fires on every verify of a non-dedicated intent type and on transfer with no recipients. ERC-7984 builds send empty recipients by design, so each of them warns. Consider the module's logger or a once-per-reason throttle.

Summary: The second commit fixes the blocking regression for MPCv2. The remaining items are minor (1–3) or test and coverage gaps (4–6). The earlier note stands: no replay against production data was run. Because this change rejects transactions that used to sign, a staged rollout would add confidence.
· branch CECHO-2460-evm-tss-verify-lean

[claude]

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

Still open from earlier reviews (1):

• 🔴 modules/abstract-eth/src/abstractEthLikeNewCoins.ts:3699 — Approvers of ERC-7984 confidential sends from a base-coin EVM TSS wallet now get a signing rejection when they approve…

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

[claude]

🟡 (optional) Approvers of stored native EVM sends whose intent symbol isn't the exact statics name of the signing chain now get a hard signing rejection. On the base branch these sign. With getChain() now passed at ecdsaMPCv2.ts:994/1004, any symbol other than getChain() becomes tokenName. resolveTssToken then returns isToken: true for a name statics can't resolve (:3636), and the native path throws 'token payment carries no ERC-20 transfer calldata'. Fix: treat a token name statics can't resolve as unverified on every path, so it only warns, as the ERC-20 path already does. The same goes for the native-batch tokens.some(isToken) rejection.

Why this was flagged

This applies only to stored payment intents whose recipient amount.symbol differs from baseCoin.getChain(). Examples are a different case ('ETH'), an alias, or a symbol written by a non-SDK client. From this checkout I could not list which producers do that. mpcUtils.ts:251 only covers SDK-built intents. On approval, resolveEffectiveTxParams (recipientUtils.ts:143-145) now gets chainName from ecdsaMPCv2.ts:994/1004 and sets tokenName = symbol. coins.getOrUndefined (statics map.ts:285) is an exact, case-sensitive lookup, so resolveTssToken returns { isToken: true, reason } at abstractEthLikeNewCoins.ts:3636. verifyTssSinglePayment then rejects a plain native send with empty calldata at :3682. verifyTssBatchPayment likewise rejects a native batch through tokens.some((token) => token.isToken). On the base branch tokenName was never derived on MPCv2, and native transfers checked only value and destination, so these approvals signed. An unresolvable token on the ERC-20 path only warns, so the two paths are inconsistent.

Verification: Trigger: a stored payment intent has a native recipient whose amount.symbol is not baseCoin.getChain() and not a statics name, id or alias of the native coin. ecdsaMPCv2.ts:994/1004 now pass this.baseCoin.getChain(), so recipientUtils.ts sets tokenName = symbol. resolveTssToken then returns { isToken: true } and the native path rejects the signing. On the base branch these signed.

[claude]

🟣 pre-existing, not blocking: Pre-existing, security-relevant: when a payment declares an ERC-7984 token, any contract, calldata and native value still pass with just a warning. The PR describes this as warning-only, but the branch returns before every other check. resolveTssToken (:3638) leaves out the contract it just resolved, and :3695 returns without checking to or value. Fix: return the contract for ERC-7984 tokens too, then require to equal to it, zero native value, and canonical calldata. Also require the selector to be confidentialTransferWithProofMethodId with the recipient decoded by decodeDirectConfidentialTransferCalldata matching, and warn only about the encrypted amount.
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

The trigger is a single-recipient transfer/payment whose recipient.tokenName (or txParams.tokenName, or amount.symbol on MPCv2 stored approvals) names an Erc7984Coin, such as hteth:ctest1. resolveTssToken returns { isToken: true, confidential: true, reason } at :3639 and drops the contract it validated at :3634-3635. In verifyTssSinglePayment, any data that does not start with transfer(address,uint256) hits :3695-3697: it logs a warning and returns. to and value are never compared, so a prebuild with approve(attacker, max) on another token, or any call carrying native value, gets signed. The PR description says "A warning never skips the other checks", but this branch does. The base was just as open for transfer (non-0xa9059cbb data was never checked) and skipped payment entirely, so this is a gap the hardening leaves open, not a regression.

Verification: pre-existing (security-relevant). In verifyTssSinglePayment, when !data.startsWith(transferMethodId) and token.confidential (:3694-3697), the code logs a warning and returns. to, value and the calldata are never compared. So any contract, any calldata and any native value pass for a declared ERC-7984 payment. The base fails the same way by the same route, which makes this pre-existing and not a regression.

[dpkjnr]

@claude review again

[claude]

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

Still open from earlier reviews (2):

• Unresolved: 2 minor or pre-existing.