fix(sdk-core): disable assertSignableConsistency call pending chain-ID fix (WCI-1398)

[Marzooqa]

Summary

• Removes the assertSignableConsistency call in ecdsaMPCv2.ts that was causing false-positive tamper errors for BSC, BSC:BUSD, and TXDC sendMany withdrawals (introduced in fix(abstract-eth,sdk-core): enforce signableHex/serializedTxHex consistency for BSC and XDC (WCI-1398) #9526, shipped in beta.2169)
• The interface and method remain in place — a follow-up PR will re-enable the call with the correct Common object (using getCustomChainCommon(this.getChainId())) so the chain ID is derived from the coin rather than defaulting to mainnet

Root cause

Unsigned EVM transactions have v=0 — no chain ID in the serialized bytes. fromSerializedData without an explicit Common defaults to mainnet (chain ID 1), while the server computes signableHex with the correct chain ID (e.g. 97 for BSC testnet). This caused a derivation mismatch and a false-positive tamper error on every signing flow.

Test plan

• CI passes
• No unit tests affected (BSC/XDC assertSignableConsistency tests call the method directly on the coin, not through this call site)

Fixes: WCI-1398

[linear-code]

WCI-1398

[lokesh-bitgo]

@claude Please do a thorough review of this PR.

Focus especially on:

• any bugs or incorrect logic in the changes
• whether these changes could impact existing flows or cause regressions
• whether the implementation is aligned with the requirement/design
• whether the changes are correct and complete
• missing edge cases, validation gaps, and test coverage gaps

Please call out concrete issues clearly and separate them by severity if possible.

[github-actions]

Claude finished @lokesh-bitgo's task —— View job

---

PR Review: Disable assertSignableConsistency Call

Review Tasks:

• Read repository guidelines (CLAUDE.md)
• Examine PR diff to understand exact changes
• Read modified file for context and impact analysis
• Analyze root cause explanation and fix alignment
• Check for potential regressions and edge cases
• Review test coverage and related test files
• Look for similar patterns that might be affected
• Provide comprehensive review feedback with severity categorization

---

Review Summary

This PR removes 8 lines from ecdsaMPCv2.ts:958-965 that implemented a security check introduced in #9526. The change is a temporary fix to address false-positive tamper errors.

🟢 LOW SEVERITY ISSUES

1. Code Quality: Unused Import

• Issue: The import isCoinWithSignableConsistency on line 55 is now unused after removing the function call
• Location: modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts:55
• Fix: Remove the unused import to keep code clean

2. Documentation Gap

• Issue: The extensive comment explaining "Gap 2 fix (WCI-1398)" remains but the implementation is removed, which could confuse future developers
• Location: Lines 958-963 in the PR diff
• Suggestion: Consider updating/removing the comment to reflect that this is temporarily disabled

🟡 MEDIUM SEVERITY ISSUES

3. Test Coverage Alignment

• Issue: Existing unit tests for assertSignableConsistency in BSC/XDC modules call the method directly on coins, not through this code path. This means the removal may not be caught by existing tests.
• Files:
  • modules/sdk-coin-bsc/test/unit/bsc.ts:254
  • modules/sdk-coin-xdc/test/unit/xdc.ts:198
• Impact: The disabled security check won't be tested in the full signing flow context
• Recommendation: Consider adding integration tests that verify the full signing flow still works for affected coins

4. Temporary Nature Clarity

• Issue: While the PR description states this is temporary, there's no mechanism (TODO comment, tracking issue, etc.) in the code to ensure the follow-up PR happens
• Recommendation: Add a TODO comment with the issue reference to make the temporary nature explicit

🟢 POSITIVE ASPECTS

1. Root Cause Analysis is Sound

• The explanation of unsigned EVM transactions having v=0 and fromSerializedData defaulting to mainnet chain ID (1) while server uses correct chain ID is technically accurate
• The chain ID mismatch (mainnet vs BSC testnet chain ID 97) causing derivation differences is the correct root cause

2. Surgical Fix

• The change is minimal and targeted, only removing the problematic check without affecting other functionality
• The interface and method implementations remain intact for future re-enablement

3. Affected Scope is Limited

• Only affects coins with both:
  • CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX (BSC, XDC, potentially others)
  • assertSignableConsistency implementation
• The check condition shouldVerifyWithSerializedTxHex(this.baseCoin) && isCoinWithSignableConsistency(this.baseCoin) was appropriately restrictive

🔍 EDGE CASES & POTENTIAL IMPACTS

1. Security Regression Risk

• Low Risk: The removed check was designed to prevent "Gap 2" attacks where server provides inconsistent serializedTxHex and signableHex
• Mitigation: The underlying verifyTransaction calls still occur and provide transaction validation, just without the cross-consistency check

2. Chain Coverage

• Analysis: Only affects EVM-like chains with the specific feature flag combination
• Current Impact: BSC and XDC (TXDC) as mentioned in PR description
• Future: Any new chains implementing both features would be affected

🔧 IMPLEMENTATION CORRECTNESS

The changes correctly address the stated problem:

• Removes the problematic assertSignableConsistency call that was causing false positives
• Preserves all other verification logic
• Maintains the conditional structure for future re-enablement
• Does not affect non-EVM or different transaction types

✅ RECOMMENDATIONS

1. Immediate: Remove unused import isCoinWithSignableConsistency
2. Short-term: Add TODO comment referencing WCI-1398 and need for follow-up PR
3. Follow-up: Ensure the planned re-enablement PR includes proper Common object with getCustomChainCommon(this.getChainId())

Overall Assessment: ✅ APPROVED

This is a well-reasoned temporary fix that addresses a legitimate issue without introducing significant risks. The surgical nature of the change and thorough root cause analysis support approval.

---

Job: https://github.com/BitGo/BitGoJS/actions/runs/...