fix(security): bind burn cipher nonce to nullifier#25
Open
Federico2014 wants to merge 1 commit into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements v2 burn-message encryption for shielded TRC-20 using a nonce derived from the note nullifier (nf), updates encrypt/decrypt signatures to accept nf and a nonce-from-log, integrates OVK into the parameters builder, propagates pending nullifiers during Wallet log scanning, and adds comprehensive unit tests. ChangesShielded TRC-20 V2 Burn-Message Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
framework/src/main/java/org/tron/core/Wallet.java (2)
3660-3677:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
ovklength before the burn AEAD path.These burn branches only reject an empty
ovk, but they now passovkstraight intoencryptBurnMessageByOvk(...). A non-32-byte value can still reach the AEAD call here, especially in the without-ask flow, which has no later length check.Suggested fix
byte[] ask = request.getAsk().toByteArray(); byte[] nsk = request.getNsk().toByteArray(); byte[] ovk = request.getOvk().toByteArray(); if ((ArrayUtils.isEmpty(ask) || ArrayUtils.isEmpty(nsk) || ArrayUtils.isEmpty(ovk))) { throw new ContractValidateException("No shielded TRC-20 ask, nsk or ovk"); } + if (ovk.length != 32) { + throw new ContractValidateException("ovk must be 32 bytes"); + } @@ byte[] ak = request.getAk().toByteArray(); byte[] nsk = request.getNsk().toByteArray(); byte[] ovk = request.getOvk().toByteArray(); if ((ArrayUtils.isEmpty(ak) || ArrayUtils.isEmpty(nsk) || ArrayUtils.isEmpty(ovk))) { throw new ContractValidateException("No shielded TRC-20 ak, nsk or ovk"); } + if (ovk.length != 32) { + throw new ContractValidateException("ovk must be 32 bytes"); + }Also applies to: 3788-3802
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/tron/core/Wallet.java` around lines 3660 - 3677, The ovk byte array is only checked for emptiness but not length before being set and passed into encryptBurnMessageByOvk; add a strict length check (ovk.length == 32) where ovk is read and before builder.setOvk(...) and before any branch that calls encryptBurnMessageByOvk(...) (the block around builder.setOvk and the similar block at the other occurrence) and throw a ContractValidateException with a clear message if the length is not 32 so a malformed ovk cannot reach the AEAD call.
4317-4323:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep legacy 80-byte burn ciphertexts accepted here.
Line 4317 makes the trigger-input reconstruction path v2-only. If a client still carries the legacy 80-byte burn ciphertext, this now hard-fails even though the rest of the PR preserves v1 burn compatibility. Accepting
BURN_CIPHER_LENhere and zero-extending it to the 96-byte record would avoid breaking older offline-signing flows.Suggested fix
if (parametersBuilder.getShieldedTRC20ParametersType() == ShieldedTRC20ParametersType.BURN) { byte[] burnCiper = ByteArray.fromHexString(shieldedTRC20Parameters.getTriggerContractInput()); - if (!ArrayUtils.isEmpty(burnCiper) - && burnCiper.length == NoteEncryption.Encryption.BURN_CIPHER_RECORD_SIZE) { - parametersBuilder.setBurnCiphertext(burnCiper); + if (!ArrayUtils.isEmpty(burnCiper)) { + if (burnCiper.length == NoteEncryption.Encryption.BURN_CIPHER_RECORD_SIZE) { + parametersBuilder.setBurnCiphertext(burnCiper); + } else if (burnCiper.length == NoteEncryption.Encryption.BURN_CIPHER_LEN) { + byte[] legacyRecord = new byte[NoteEncryption.Encryption.BURN_CIPHER_RECORD_SIZE]; + System.arraycopy( + burnCiper, 0, legacyRecord, 0, NoteEncryption.Encryption.BURN_CIPHER_LEN); + parametersBuilder.setBurnCiphertext(legacyRecord); + } else { + throw new ZksnarkException( + "invalid shielded TRC-20 contract parameters for burn trigger input"); + } } else { throw new ZksnarkException( "invalid shielded TRC-20 contract parameters for burn trigger input"); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/tron/core/Wallet.java` around lines 4317 - 4323, The current check only accepts NoteEncryption.Encryption.BURN_CIPHER_RECORD_SIZE (96) which rejects legacy 80-byte burnCiper; update the logic in Wallet.java where burnCiper is handled so it also accepts NoteEncryption.Encryption.BURN_CIPHER_LEN (80) and when burnCiper.length == BURN_CIPHER_LEN create a new 96-byte buffer, copy the 80 bytes into it and zero-fill the remaining bytes, then call parametersBuilder.setBurnCiphertext(...) with the 96-byte buffer; keep the existing behavior when burnCiper.length == BURN_CIPHER_RECORD_SIZE by passing it through unchanged and throw ZksnarkException for any other lengths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/src/main/java/org/tron/core/Wallet.java`:
- Around line 4043-4069: The burn-log branch (logType == 4) assumes logData has
the full fixed-size payload (Encryption.BURN_LOG_TO_ADDRESS_OFFSET ..
BURN_NONCE_LEN) before slicing and decrypting; add a defensive length check at
the start of that branch to ensure logData.length >=
(Encryption.BURN_LOG_RESERVED_OFFSET or compute 32+32+80+12+4 using the BURN_*
offsets/constants) and return Optional.empty() if too short, so subsequent
ByteArray.subArray calls and decryptBurnMessageByOvk/
decryptBurnMessageByOvk(ovk, cipher, nonceFromLog, nf) are never invoked on
truncated data; keep the existing behavior for pendingNf null check and preserve
use of logAmountArray, cipher, nonceFromLog, and decryptedText.
---
Outside diff comments:
In `@framework/src/main/java/org/tron/core/Wallet.java`:
- Around line 3660-3677: The ovk byte array is only checked for emptiness but
not length before being set and passed into encryptBurnMessageByOvk; add a
strict length check (ovk.length == 32) where ovk is read and before
builder.setOvk(...) and before any branch that calls
encryptBurnMessageByOvk(...) (the block around builder.setOvk and the similar
block at the other occurrence) and throw a ContractValidateException with a
clear message if the length is not 32 so a malformed ovk cannot reach the AEAD
call.
- Around line 4317-4323: The current check only accepts
NoteEncryption.Encryption.BURN_CIPHER_RECORD_SIZE (96) which rejects legacy
80-byte burnCiper; update the logic in Wallet.java where burnCiper is handled so
it also accepts NoteEncryption.Encryption.BURN_CIPHER_LEN (80) and when
burnCiper.length == BURN_CIPHER_LEN create a new 96-byte buffer, copy the 80
bytes into it and zero-fill the remaining bytes, then call
parametersBuilder.setBurnCiphertext(...) with the 96-byte buffer; keep the
existing behavior when burnCiper.length == BURN_CIPHER_RECORD_SIZE by passing it
through unchanged and throw ZksnarkException for any other lengths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5dc7e685-5d4e-4b76-9757-4ec800c74b28
📒 Files selected for processing (4)
framework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/zen/ShieldedTRC20ParametersBuilder.javaframework/src/main/java/org/tron/core/zen/note/NoteEncryption.javaframework/src/test/java/org/tron/core/zen/note/BurnCipherV2Test.java
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="framework/src/main/java/org/tron/core/zen/note/NoteEncryption.java">
<violation number="1" location="framework/src/main/java/org/tron/core/zen/note/NoteEncryption.java:372">
P2: Treating `null` as all-zero silently falls back to insecure v1 decryption. If a caller fails to extract the nonce from the log and passes `null`, the nonce verification is bypassed entirely — undermining the replay protection this fix adds. Consider returning `false` for null (or throwing) so that a missing nonce is never silently accepted as a v1 record.</violation>
</file>
<file name="framework/src/test/java/org/tron/core/zen/note/BurnCipherV2Test.java">
<violation number="1" location="framework/src/test/java/org/tron/core/zen/note/BurnCipherV2Test.java:86">
P2: This test should assert rejection for the bad address prefix. As written, it asserts decryption succeeds and only checks the raw byte value, so it would still pass if invalid prefixes were accepted.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Federico2014
force-pushed
the
feature/burn-nonce-nf-binding
branch
5 times, most recently
from
460ea93 to
a8442b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
framework/src/main/java/org/tron/core/zen/note/NoteEncryption.java (1)
257-296: ⚡ Quick winDeduplicate the two
encryptBurnMessageByOvkoverloads.The two overloads share ~90% of their body (plaintext layout, AEAD call, error path). Either delegate the legacy overload to the new one, or extract a private helper that takes the nonce as a parameter. As-is, future changes to the layout (e.g., 32+21 packing) must be applied in two places.
♻️ Proposed refactor
public static Optional<byte[]> encryptBurnMessageByOvk(byte[] ovk, BigInteger toAmount, byte[] transparentToAddress) throws ZksnarkException { - byte[] plaintext = new byte[64]; - byte[] amountArray = ByteUtil.bigIntegerToBytes(toAmount, 32); - byte[] zeroNonce = new byte[BURN_NONCE_LEN]; - byte[] cipher = new byte[BURN_CIPHER_LEN]; - System.arraycopy(amountArray, 0, plaintext, 0, 32); - System.arraycopy(transparentToAddress, 0, plaintext, 32, 21); - - if (JLibsodium.cryptoAeadChacha20Poly1305IetfEncrypt(new Chacha20Poly1305IetfEncryptParams( - cipher, null, plaintext, - 64, null, 0, null, zeroNonce, ovk)) != 0) { - return Optional.empty(); - } - - return Optional.of(cipher); + return encryptBurnMessageByOvkWithNonce(ovk, toAmount, transparentToAddress, + new byte[BURN_NONCE_LEN]); } public static Optional<byte[]> encryptBurnMessageByOvk(byte[] ovk, BigInteger toAmount, byte[] transparentToAddress, byte[] nf) throws ZksnarkException { - byte[] plaintext = new byte[64]; - byte[] amountArray = ByteUtil.bigIntegerToBytes(toAmount, 32); - byte[] nonce = deriveNonceFromNf(nf); - byte[] cipher = new byte[BURN_CIPHER_LEN]; - System.arraycopy(amountArray, 0, plaintext, 0, 32); - System.arraycopy(transparentToAddress, 0, plaintext, 32, 21); - - if (JLibsodium.cryptoAeadChacha20Poly1305IetfEncrypt(new Chacha20Poly1305IetfEncryptParams( - cipher, null, plaintext, - 64, null, 0, null, nonce, ovk)) != 0) { - return Optional.empty(); - } - - return Optional.of(cipher); + return encryptBurnMessageByOvkWithNonce(ovk, toAmount, transparentToAddress, + deriveNonceFromNf(nf)); + } + + private static Optional<byte[]> encryptBurnMessageByOvkWithNonce(byte[] ovk, BigInteger toAmount, + byte[] transparentToAddress, byte[] nonce) + throws ZksnarkException { + byte[] plaintext = new byte[64]; + byte[] amountArray = ByteUtil.bigIntegerToBytes(toAmount, 32); + byte[] cipher = new byte[BURN_CIPHER_LEN]; + System.arraycopy(amountArray, 0, plaintext, 0, 32); + System.arraycopy(transparentToAddress, 0, plaintext, 32, 21); + if (JLibsodium.cryptoAeadChacha20Poly1305IetfEncrypt(new Chacha20Poly1305IetfEncryptParams( + cipher, null, plaintext, + 64, null, 0, null, nonce, ovk)) != 0) { + return Optional.empty(); + } + return Optional.of(cipher); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/tron/core/zen/note/NoteEncryption.java` around lines 257 - 296, The two overloads of encryptBurnMessageByOvk share almost identical logic; extract the shared body into one private helper (e.g., encryptBurnMessageByOvkWithNonce(byte[] ovk, BigInteger toAmount, byte[] transparentToAddress, byte[] nonce)) that builds the 64-byte plaintext (using ByteUtil.bigIntegerToBytes for 32 bytes and copying the 21-byte transparentToAddress), chooses a zero nonce when passed null (or accept the explicit zeroNonce = new byte[BURN_NONCE_LEN]), and performs the JLibsodium.cryptoAeadChacha20Poly1305IetfEncrypt call with BURN_CIPHER_LEN; then have the current public overloads (encryptBurnMessageByOvk(...), and encryptBurnMessageByOvk(..., byte[] nf)) delegate to that helper (for the nf variant call deriveNonceFromNf(nf) and pass it).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/src/main/java/org/tron/core/zen/note/NoteEncryption.java`:
- Around line 279-303: deriveNonceFromNf currently passes a null or malformed nf
into Hash.sha3 causing an NPE and the v2 encrypt path (encryptBurnMessageByOvk)
propagates that; add explicit input validation: in deriveNonceFromNf call
Objects.requireNonNull(nf, "nf must not be null") and validate nf.length > 0 (or
== expected length if you have a constant) and throw an IllegalArgumentException
with a clear message on bad length; additionally, in encryptBurnMessageByOvk
(and any v2 encrypt overload that calls deriveNonceFromNf) perform a
precondition check and convert invalid input into a ZksnarkException (or throw a
ZksnarkException with a descriptive message) before calling deriveNonceFromNf so
callers get a fast, meaningful domain error rather than an NPE.
In `@framework/src/test/java/org/tron/core/zen/note/BurnCipherTest.java`:
- Around line 131-154: The test name is misleading —
testDecryptBurnWrongAddressPrefixRejected actually checks that
Encryption.encryptBurnMessageByOvk and Encryption.decryptBurnMessageByOvk
round-trip an address prefix byte of 0x00, it does not exercise Wallet prefix
rejection; rename the test to something like
testDecryptBurnRoundTripsInvalidPrefix (or similar) to reflect it verifies
round-trip behavior of Encryption.encryptBurnMessageByOvk and
Encryption.decryptBurnMessageByOvk, and add a new sibling test that
constructs/validates an address with a 0x00 prefix and asserts the Wallet code
path that enforces prefix rules (the Wallet prefix validation/constructor code
where Wallet.getAddressPreFixByte() is referenced) actually rejects it.
---
Nitpick comments:
In `@framework/src/main/java/org/tron/core/zen/note/NoteEncryption.java`:
- Around line 257-296: The two overloads of encryptBurnMessageByOvk share almost
identical logic; extract the shared body into one private helper (e.g.,
encryptBurnMessageByOvkWithNonce(byte[] ovk, BigInteger toAmount, byte[]
transparentToAddress, byte[] nonce)) that builds the 64-byte plaintext (using
ByteUtil.bigIntegerToBytes for 32 bytes and copying the 21-byte
transparentToAddress), chooses a zero nonce when passed null (or accept the
explicit zeroNonce = new byte[BURN_NONCE_LEN]), and performs the
JLibsodium.cryptoAeadChacha20Poly1305IetfEncrypt call with BURN_CIPHER_LEN; then
have the current public overloads (encryptBurnMessageByOvk(...), and
encryptBurnMessageByOvk(..., byte[] nf)) delegate to that helper (for the nf
variant call deriveNonceFromNf(nf) and pass it).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a17d0d0c-2322-4517-a522-eb24b461e74a
📒 Files selected for processing (4)
framework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/zen/ShieldedTRC20ParametersBuilder.javaframework/src/main/java/org/tron/core/zen/note/NoteEncryption.javaframework/src/test/java/org/tron/core/zen/note/BurnCipherTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/src/main/java/org/tron/core/Wallet.java
- framework/src/main/java/org/tron/core/zen/ShieldedTRC20ParametersBuilder.java
Federico2014
force-pushed
the
feature/burn-nonce-nf-binding
branch
3 times, most recently
from
5de2e31 to
39f6829
Compare
Federico2014
changed the title
Federico2014
force-pushed
the
feature/burn-nonce-nf-binding
branch
3 times, most recently
from
fa0bc20 to
9b2c9ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@framework/src/main/java/org/tron/core/zen/ShieldedTRC20ParametersBuilder.java`:
- Around line 298-310: Validate transparentToAddress before copying in the OVK
burn-encrypt path: inside the block guarded by ovk != null (the code that
creates addr21 and calls Encryption.encryptBurnMessageByOvk), check that
transparentToAddress is non-null and has length >= 20 (same guard as
ArrayUtils.isEmpty(transparentToAddress) used elsewhere); if it fails, throw a
clear ZksnarkException (or skip encryption explicitly) so a meaningful error is
raised instead of letting System.arraycopy throw
NullPointerException/ArrayIndexOutOfBoundsException. Ensure the check is placed
before creating addr21 or calling Encryption.encryptBurnMessageByOvk so the
error context remains tied to build()/ovk handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29c7f520-6cb7-4e57-a0ea-02eeddd7b8ae
📒 Files selected for processing (4)
framework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/zen/ShieldedTRC20ParametersBuilder.javaframework/src/main/java/org/tron/core/zen/note/NoteEncryption.javaframework/src/test/java/org/tron/core/zen/note/BurnCipherTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- framework/src/main/java/org/tron/core/Wallet.java
Federico2014
force-pushed
the
feature/burn-nonce-nf-binding
branch
2 times, most recently
from
c9b5c16 to
7f7e289
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
framework/src/main/java/org/tron/core/zen/ShieldedTRC20ParametersBuilder.java (1)
298-310:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
transparentToAddressbefore the arraycopy on line 303.
encryptBurnMessageByOvkdoes checktransparentToAddress.length != 21, but that runs afterSystem.arraycopy(transparentToAddress, 0, addr21, 1, 20)on line 303, so anullor< 20byte input throwsNullPointerException/ArrayIndexOutOfBoundsExceptionfirst and the surroundingtry/catchrewraps it into a generic"build the shielded TRC-20 parameters error: ...", masking the real misconfiguration. The siblingburnParamsToHexStringalready guards this at lines 496-498 withArrayUtils.isEmpty(transparentToAddress).🛡️ Suggested guard
byte[] nf = spendDescription.getNullifier().toByteArray(); if (ovk != null) { + if (ArrayUtils.isEmpty(transparentToAddress) || transparentToAddress.length < 20) { + throw new ZksnarkException("invalid transparentToAddress for burn encryption"); + } byte[] addr21 = new byte[21]; addr21[0] = Wallet.getAddressPreFixByte(); System.arraycopy(transparentToAddress, 0, addr21, 1, 20);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/tron/core/zen/ShieldedTRC20ParametersBuilder.java` around lines 298 - 310, In ShieldedTRC20ParametersBuilder (the method that builds the shielded TRC-20 parameters surrounding the shown snippet), add an input guard before the System.arraycopy that verifies transparentToAddress is non-null and has at least 20 bytes (e.g., same check used in burnParamsToHexString / ArrayUtils.isEmpty and length >= 20); if the check fails, throw a ZksnarkException with a clear message like "invalid transparentToAddress: null or too short" so we fail fast and avoid NPE/ArrayIndexOutOfBounds before calling Encryption.encryptBurnMessageByOvk.
🧹 Nitpick comments (1)
framework/src/main/java/org/tron/core/zen/note/NoteEncryption.java (1)
310-321: 💤 Low valueConfirm the v1 legacy path intentionally skips nf-binding verification even when
nfis supplied.When
isAllZero(nonceFromLog)is true the code proceeds straight to AEAD decryption regardless of whethernfwas provided, so a caller passing a realnffor a v1-record cannot detect tampering via the nonce-binding check (it has to rely solely on AEAD integrity overovk). This is consistent with the PR's documented residual risk ("injection via direct calldata construction with zero-nonce remains until an activation-height gate is added in a separate PR"), but a one-line comment here pointing at the activation-gate follow-up would make the intent explicit for future readers and prevent the legacy branch from being mistakenly relied upon as a verified path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/tron/core/zen/note/NoteEncryption.java` around lines 310 - 321, The v1 legacy branch intentionally skips nf-binding verification when isAllZero(nonceFromLog) is true; add a one-line comment above that branch (near isAllZero(nonceFromLog) / nonceFromLog / nf / deriveNonceFromNf) stating this is deliberate for v1 records and that nf-binding will be enforced by the planned activation-height gate in a follow-up PR (so callers should not rely on this branch for nf verification). Ensure the comment mentions the residual injection risk and references the activation-height gate follow-up to make intent explicit for future readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@framework/src/main/java/org/tron/core/zen/ShieldedTRC20ParametersBuilder.java`:
- Around line 298-310: In ShieldedTRC20ParametersBuilder (the method that builds
the shielded TRC-20 parameters surrounding the shown snippet), add an input
guard before the System.arraycopy that verifies transparentToAddress is non-null
and has at least 20 bytes (e.g., same check used in burnParamsToHexString /
ArrayUtils.isEmpty and length >= 20); if the check fails, throw a
ZksnarkException with a clear message like "invalid transparentToAddress: null
or too short" so we fail fast and avoid NPE/ArrayIndexOutOfBounds before calling
Encryption.encryptBurnMessageByOvk.
---
Nitpick comments:
In `@framework/src/main/java/org/tron/core/zen/note/NoteEncryption.java`:
- Around line 310-321: The v1 legacy branch intentionally skips nf-binding
verification when isAllZero(nonceFromLog) is true; add a one-line comment above
that branch (near isAllZero(nonceFromLog) / nonceFromLog / nf /
deriveNonceFromNf) stating this is deliberate for v1 records and that nf-binding
will be enforced by the planned activation-height gate in a follow-up PR (so
callers should not rely on this branch for nf verification). Ensure the comment
mentions the residual injection risk and references the activation-height gate
follow-up to make intent explicit for future readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4137961-0538-4576-bd02-001c2de7b1be
📒 Files selected for processing (4)
framework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/zen/ShieldedTRC20ParametersBuilder.javaframework/src/main/java/org/tron/core/zen/note/NoteEncryption.javaframework/src/test/java/org/tron/core/zen/note/BurnCipherTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/src/main/java/org/tron/core/Wallet.java
- framework/src/test/java/org/tron/core/zen/note/BurnCipherTest.java
Federico2014
force-pushed
the
feature/burn-nonce-nf-binding
branch
2 times, most recently
from
f08121e to
2648e05
Compare
Federico2014
force-pushed
the
feature/burn-nonce-nf-binding
branch
from
2648e05 to
682d8d5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Hardens the shielded TRC-20 burn encryption by binding the AEAD nonce to the spend nullifier, and embedding the nonce in the log record for backward-compatible scanner detection.
Why are these changes required?
The original implementation used a static (all-zero) nonce for every burn cipher under the same long-lived
ovk. This PR makes each burn use a unique per-spend nonce derived from the nullifier, restoring the AEAD (key, nonce) uniqueness guarantee.Design
nf-derived nonce with domain separation
The fixed domain tag prevents the derivation from colliding with any other SHA3-of-nf usage. Each burn gets a distinct (key, nonce) pair since the nullifier is globally unique per spend.
96-byte record layout
encryptBurnMessageByOvkreturns a 96B record. The 16-byte trailing region of the legacy log already existed as zero padding, so total calldata length is unchanged:Scanner backward compatibility
decryptBurnMessageByOvk(ovk, cipher, nonceFromLog, nf)reads the nonce from the log record:nonce == SHA3(domain || nf)[:12]before decrypt.ovk) provides authenticity. Used when the scanner cannot pair aNoteSpentlog within the same tx.Encryption moved into builder
Encryption now runs inside
ShieldedTRC20ParametersBuilder.build()aftergenerateSpendProofwhere the nullifier is available. Callers justsetOvk(ovk); if not set, the builder falls back tospend.expsk.getOvk().Compatibility
addr 32 + value 32 + cipher 80 + trailing 16); the trailing 16 B is now interpreted asnonce(12) + reserved(4)instead of pure paddingtriggerContractInputscanShieldedTRC20NotesByOvkAPIallowShieldedTransactionApidefaults to false on mainnetderiveNonceFromNfisprivateand not exposed via any public surfaceResidual note: historical burn ciphertexts encrypted under the old zero-nonce scheme remain on chain. Their keystream leakage is permanent and out of scope for this PR — only future burns benefit from the nf-binding.
Key changes
NoteEncryption.javaencryptBurnMessageByOvkreturns 96 B record;decryptBurnMessageByOvktakes 4 params with legacy/strict/relaxed paths; input length validation; privatederiveNonceFromNfwithZtron_BurnNoncedomain separationShieldedTRC20ParametersBuilder.javaburnCiphertext; encryption inlined in BURN branch after spend proof;ovkfalls back tospend.expsk.getOvk()when not setWallet.javaNoteSpent(bytes32)log support (logType=5); scanner extracts nonce from log;pendingNfcursor for same-tx pairing; logData length guard; 80 B-only cipher rejected on the trigger-input pathBurnCipherTest.javaNoteEncDecryTest.javaTesting
./gradlew :framework:test --tests "org.tron.core.zen.note.BurnCipherTest"— 16/16 pass./gradlew :framework:test --tests "org.tron.core.zksnark.NoteEncDecryTest"— 8/8 pass (6 new + 2 pre-existing)./gradlew :framework:checkstyleMain :framework:checkstyleTest— passSummary by CodeRabbit
New Features
Bug Fixes
Tests