Skip to content

feat(crypto): shielded transaction API security enhancement#6694

Open
Federico2014 wants to merge 1 commit intotronprotocol:developfrom
Federico2014:fix/shielded-api-security
Open

feat(crypto): shielded transaction API security enhancement#6694
Federico2014 wants to merge 1 commit intotronprotocol:developfrom
Federico2014:fix/shielded-api-security

Conversation

@Federico2014
Copy link
Copy Markdown
Collaborator

What does this PR do?

Enhances the security of shielded transaction APIs by adding overflow protection, tightening default configuration, and cleaning up unused artifacts.

  1. Added integer overflow protection in ZenTransactionBuilder and ShieldedTRC20ParametersBuilder: replaced raw +=/-= on valueBalance with StrictMathWrapper.addExact()/subtractExact(), which throw ArithmeticException on overflow. Overflow validation is performed before mutating builder state (spends/receives lists), ensuring the builder remains consistent if an overflow is detected.
  2. Wrapped createShieldedTransaction, createShieldedTransactionWithoutSpendAuthSig, createShieldedContractParameters, and createShieldedContractParametersWithoutAsk in try/catch ArithmeticException to convert overflow to ZksnarkException("shielded amount overflow")
  3. Changed the default value of allowShieldedTransactionApi from true to false to reduce exposure by default, since some shielded transaction APIs require passing private keys as parameters
  4. Updated config.conf with an explicit warning advising users to only invoke these APIs locally
  5. Removed sprout-verifying.key from the repository root — the file was not referenced by any code
  6. Updated test fixtures to explicitly enable allowShieldedTransactionApi and properly save/restore the flag

Why are these changes required?

  • valueBalance was accumulated via raw +=/-= without overflow checks, allowing crafted inputs to silently wrap around long range and produce invalid balances
  • Shielded transaction APIs accept private keys as parameters — enabling them by default on public-facing nodes risks private key leakage
  • sprout-verifying.key was an unreferenced artifact that added unnecessary repository bloat

⚠️ Breaking Changes:

  • allowShieldedTransactionApi default changed from true to false — nodes that previously relied on the default will find shielded transaction APIs disabled after upgrade. Fix: explicitly set allowShieldedTransactionApi = true in config.conf.

This PR has been tested by:

  • Unit Tests — ShieldedTransferActuatorTest, ArgsTest, ShieldedTRC20BuilderTest, CreateSpendAuthSigServletTest, MerkleContainerTest

Follow up

No consensus logic or on-chain state transitions affected.

Extra details
Close #6616

@github-actions github-actions Bot requested a review from 3for April 18, 2026 02:29
@halibobo1205 halibobo1205 added this to the GreatVoyage-v4.8.2 milestone Apr 18, 2026
@3for
Copy link
Copy Markdown
Collaborator

3for commented Apr 19, 2026

  1. Changed the default value of allowShieldedTransactionApi from true to false to reduce exposure by default, since some shielded transaction APIs require passing private keys as parameters

The default value of allowShieldedTransactionApi has been changed from true to false. This introduces a breaking change for nodes that relied on the previous default behavior.

After the upgrade, approximately 26 Wallet/gRPC/HTTP endpoints are disabled. However, many of these endpoints do not involve private key handling, for example:

  • getMerkleTreeVoucherInfo
  • getShieldTransactionHash
  • getTriggerInputForShieldedTRC20Contract
  • getDiversifier

Additionally, several read-only / scanning APIs are implicitly disabled, including:

  • getPaymentAddress(ivk)
  • scanNoteByIvk(ivk, …)
  • scanNoteByOvk(ovk, …)
  • scanShieldedTRC20NotesByIvk(ivk, …)
  • scanShieldedTRC20NotesByOvk(ovk, …)

Disabling these endpoints has significant side effects:

  1. Wallets that previously relied on public RPC nodes to synchronize shielded balances (via IVK/OVK scanning) will stop functioning on nodes where this flag is false.
  2. If most public nodes switch to false by default, users will be forced to rely on a small number of nodes that keep it enabled, introducing centralization risks — effectively leading to a “few-node monopoly” over shielded services.

The TRON developer documentation already recommends constructing shielded transaction parameters locally. The ecosystem tool wallet-cli also provides local shielded key derivation that does not depend on node APIs and is therefore unaffected by this flag.

However, when allowShieldedTransactionApi is disabled:

  • wallet-cli currently cannot provide a fully local workflow for shielded transaction construction and signing.
  • This leaves a gap where users must either run a full node or rely on centralized RPC providers.

Suggestions

  1. Introduce granular control instead of a global switch
    Rather than disabling all shielded APIs, consider:

    • Disabling only high-risk endpoints (e.g., those accepting private keys)
    • Keeping safe, read-only endpoints (e.g., scanning/query APIs) enabled
    • Adding a clear startup log indicating which categories of APIs are disabled
  2. Add explicit warnings for risky endpoints
    If a node enables endpoints that accept private keys:

    • Return a visible warning in API responses
    • Clearly inform users about the associated security risks
  3. Provide a lightweight local alternative
    Compared to requiring users to run a full node, a better approach would be:

    • Enhance ecosystem tools such as toolkit or wallet-cli
    • Provide fully local shielded transaction construction and signing
    • Enable users to handle all private key–related operations offline

Overall, the current change improves security posture, but the all-or-nothing behavior introduces usability and decentralization trade-offs that could be mitigated with finer-grained controls and better local tooling support.

@mufasacubana8-pixel
Copy link
Copy Markdown

class request {
justo=this.name
}

@Federico2014
Copy link
Copy Markdown
Collaborator Author

Federico2014 commented Apr 20, 2026

@3for Thanks for the thorough analysis. A few points in response:

On the breaking change: The clearParam: false change means nodes that have never explicitly configured this flag will see it disabled after upgrade. This is intentional — shielded transactions are a niche feature that is not actively promoted on TRON, and a conservative default better reflects the expected usage pattern for most node operators.

On the API categorization: Your observation that some APIs under this flag (e.g., getMerkleTreeVoucherInfo, getDiversifier, IVK/OVK scanning) don't involve spending keys is technically correct. However, even viewing keys (IVK/OVK) passed to a remote node carry privacy risk — the node learns which notes belong to the user. Given the low usage of shielded features overall, we think a single flag is an appropriate trade-off for now.

On centralization risk: We acknowledge the concern, but in practice shielded transaction activity on TRON is extremely limited. On-chain data shows very low usage for shielded TRC20 contracts (e.g., contract 1, contract 2), suggesting the real-world impact on users is minimal. Operators who specifically support shielded use cases can explicitly set allowShieldedTransactionApi = true.

We're open to revisiting granular controls if shielded transaction usage grows significantly, but at this stage the added complexity isn't warranted.

Comment thread framework/src/main/java/org/tron/core/Wallet.java Outdated
@Federico2014 Federico2014 force-pushed the fix/shielded-api-security branch from f301e6f to 8d20e0e Compare April 21, 2026 03:43
@halibobo1205 halibobo1205 assigned 3for and waynercheung and unassigned 3for and waynercheung Apr 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Shielded transaction API security enhancement

6 participants