[mukeshsp]

Summary

Fixes a "what you see is not what you sign" gap in ALGO: explainTransaction never reported reKeyTo, and signTransaction signed the rebuilt transaction including the rekey. An attacker who controls the removable-drive file could swap in a pay/axfer that keeps the displayed sender/recipient/amount but rekeys the account, getting operators to approve a hidden transfer of account authority.

• Transaction.toJson() and TxData now emit reKeyTo (encoded address) for any decoded transaction that carries it.
• Algo.explainTransaction surfaces reKeyTo in the explanation and displayOrder for pay, axfer, and keyreg transactions, and now prefers halfSigned.txHex over txHex so it explains the exact bytes signTransaction will sign. (acfg/afrz already fail closed with NotSupported in the builder factory.)
• Algo.signTransaction throws InvalidTransactionError naming the rekey target when the decoded transaction sets reKeyTo, unless the caller passes allowReKeyTo with the exact approved rekey address (new string option on algo's SignTransactionOptions); a mismatched address also throws. Covers the half-signed path. Flows that never rekey (including recover() unsigned sweeps) are unaffected.
• Keyreg builders (buildOnlineKeyRegTransaction, buildOfflineKeyRegTransaction) now pass this._reKeyTo so rebuilt bytes preserve the rekey.
• Unit tests: explain surfaces the rekey target for crafted pay, axfer, and keyreg, and prefers the halfSigned hex over txHex when both are present; sign rejects a rekeyed tx, rejects a mismatched approved address, and succeeds with the matching address.

Follow-up (separate repo, offline-vault-console): display explanation.reKeyTo prominently on the OVC review screen. The sign-side guard already makes the OVC flow fail closed without it.

Ticket: https://linear.app/bitgo/issue/CSHLD-2171/high-bitgojs-algo-explaintransaction-omits-rekeyto-so-ovc-signs-a

Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com

[linear-code]

CSHLD-2171

[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

[mukeshsp]

@claude review

[claude]

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

[mukeshsp]

@claude re-review

[mukeshsp]

@claude review

[claude]

Still open from earlier reviews (1):

• Unresolved: 1 minor or pre-existing.

[mukeshsp]

@claude review and share verdict

[claude]

The latest push fixes both points from my earlier review, and this pass found no new bugs. Because the change sits on the ALGO signing path, a human from the module's code owners should still look at it.

What was reviewed:

• allowReKeyTo is now a string compared against encodeAddress(txn.reKeyTo.publicKey) from the exact hex being signed. A mismatched or missing value throws.
• Explain and sign now both prefer halfSigned.txHex, so the operator sees the same bytes that get signed.
• Both online and offline keyreg builders now pass this._reKeyTo, which from() fills in. The rekey therefore survives the rebuild in both explain and sign.

Extended reasoning...

The change touches the ALGO coin module: explainTransaction, signTransaction, Transaction.toJson, the keyreg builders and the unit tests. Its security-sensitive surface is the signing path: a new guard that refuses to sign rekeyed transactions unless the caller passes the approved rekey address. The latest commit addresses the two earlier inline findings (keyreg dropping the rekey, and the boolean opt-in plus the explain/sign hex mismatch). New tests cover rejecting a rekeyed transaction, rejecting a mismatched address and signing with the matching one. I deferred instead of approving because the signing guard is security-critical and /modules/sdk-coin-algo/ has a CODEOWNERS entry.

Still open from earlier reviews (1):

• Unresolved: 1 minor or pre-existing.

[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]

Review verdict: Approve (static review of the diff; tests not run locally, CI is green on 1220edf61e).

• reKeyTo is surfaced in explainTransaction for pay, axfer and keyreg, and added to toJson() / TxData.
• Explain and sign now both prefer halfSigned.txHex, so the operator sees the exact bytes that get signed.
• signTransaction throws unless allowReKeyTo equals the exact rekey address decoded from the hex being signed, so a swapped rekey target is rejected.
• Keyreg builders now pass this._reKeyTo, so rebuilt bytes keep the rekey.
• Tests cover explain for pay, axfer and keyreg, the halfSigned precedence, and sign rejected, approved and mismatched.

Non-blocking follow-ups:

• closeRemainderTo (and the axfer close-out field) has the same hidden-authority shape: it is not shown in explain and not guarded on sign. This is tracked in CSHLD-2166, but that ticket's acceptance criteria only cover wallet-platform. Please add an SDK-side criterion or sub-ticket so the explain and sign guard has an owner.
• This guards Algo.signTransaction only. Callers driving TransactionBuilder.sign directly are not covered, as the new code comment notes.

A human code owner should still sign off, since this touches the ALGO signing path.