Repository navigation
fix(crypto): harden shielded burn ovk encryption - #24
Federico2014 wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughAdds a 96-byte burn record (80-byte AEAD cipher + 12-byte nonce + 4 reserved), updates NoteEncryption to emit/consume explicit nonces, changes calldata encoding to embed the 96-byte record, makes Wallet parse and require the nonce (trigger inputs reject legacy 80-byte ciphers), and adds tests for round-trip, uniqueness, legacy, and log handling. ChangesBurn Message Nonce-Based Encryption
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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
framework/src/main/java/org/tron/core/Wallet.java (1)
4048-4049: ⚡ Quick winDerive the nonce offset from the shared burn layout constants.
Line 4048 hardcodes
144even thoughNoteEncryption.Encryptionnow owns the burn record sizes. Recomputing this fromBURN_CIPHER_LENkeeps the log parser aligned if the record layout changes again.Suggested change
- byte[] cipher = ByteArray.subArray(logData, 64, 144); - byte[] nonce = ByteArray.subArray(logData, 144, - 144 + NoteEncryption.Encryption.BURN_NONCE_LEN); + final int burnCipherOffset = 64; + final int burnNonceOffset = + burnCipherOffset + NoteEncryption.Encryption.BURN_CIPHER_LEN; + byte[] cipher = ByteArray.subArray(logData, burnCipherOffset, burnNonceOffset); + byte[] nonce = ByteArray.subArray(logData, burnNonceOffset, + burnNonceOffset + NoteEncryption.Encryption.BURN_NONCE_LEN);🤖 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 4048 - 4049, The code in Wallet.java extracts the burn nonce using a hardcoded offset (144) which can drift when NoteEncryption.Encryption record sizes change; replace the magic number by deriving the start offset from the shared burn layout constants (e.g., use NoteEncryption.Encryption.BURN_CIPHER_LEN to compute the offset) and then call ByteArray.subArray(logData, computedOffset, computedOffset + NoteEncryption.Encryption.BURN_NONCE_LEN) so the nonce extraction remains correct if the record layout changes.framework/src/test/java/org/tron/core/zksnark/NoteEncDecryTest.java (1)
203-327: ⚡ Quick winAdd Wallet-level regression coverage for the rollout compatibility paths.
These tests validate
NoteEncryptionwell, but the risky behavior change is inWallet: parsing a 96-byte burn log and padding a legacy 80-byte trigger input back to 96 bytes. A focused test around those two paths would catch the integration regressions this PR is trying to avoid.🤖 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/test/java/org/tron/core/zksnark/NoteEncDecryTest.java` around lines 203 - 327, Add Wallet-level regression tests that exercise the two rollout-compatibility paths: parsing a full 96-byte burn log and handling an 80-byte legacy burn trigger by padding it to 96 bytes before processing. Write tests that construct a 96-byte record and an 80-byte legacy record, call the Wallet method(s) that parse/process burn logs (use the Wallet class methods that accept burn records / trigger inputs), and assert the resulting parsed amount/address match the original plaintext and that the 80-byte input is correctly padded to 96 bytes (or produces the same parsed output as the 96-byte record). Locate test targets based on Wallet and the wallet method names used to parse burn records (e.g., any method that consumes a byte[] burn record or trigger input) and add assertions for both successful parsing and failure cases (wrong nonce) to mirror existing NoteEncDecryTest expectations.
🤖 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 296-299: The current decryptBurnMessageByOvk method silently
replaces any wrong-length nonce with an all-zero nonce; instead, in
NoteEncryption.decryptBurnMessageByOvk you should only accept a null or empty
nonce to trigger the legacy all-zero behavior and reject any non-null nonce
whose length != BURN_NONCE_LEN by returning Optional.empty(); update the nonce
validation logic to check nonce == null || nonce.length == 0 (legacy) or
nonce.length == BURN_NONCE_LEN (use provided), otherwise return Optional.empty()
so malformed/truncated nonces are not silently accepted.
---
Nitpick comments:
In `@framework/src/main/java/org/tron/core/Wallet.java`:
- Around line 4048-4049: The code in Wallet.java extracts the burn nonce using a
hardcoded offset (144) which can drift when NoteEncryption.Encryption record
sizes change; replace the magic number by deriving the start offset from the
shared burn layout constants (e.g., use
NoteEncryption.Encryption.BURN_CIPHER_LEN to compute the offset) and then call
ByteArray.subArray(logData, computedOffset, computedOffset +
NoteEncryption.Encryption.BURN_NONCE_LEN) so the nonce extraction remains
correct if the record layout changes.
In `@framework/src/test/java/org/tron/core/zksnark/NoteEncDecryTest.java`:
- Around line 203-327: Add Wallet-level regression tests that exercise the two
rollout-compatibility paths: parsing a full 96-byte burn log and handling an
80-byte legacy burn trigger by padding it to 96 bytes before processing. Write
tests that construct a 96-byte record and an 80-byte legacy record, call the
Wallet method(s) that parse/process burn logs (use the Wallet class methods that
accept burn records / trigger inputs), and assert the resulting parsed
amount/address match the original plaintext and that the 80-byte input is
correctly padded to 96 bytes (or produces the same parsed output as the 96-byte
record). Locate test targets based on Wallet and the wallet method names used to
parse burn records (e.g., any method that consumes a byte[] burn record or
trigger input) and add assertions for both successful parsing and failure cases
(wrong nonce) to mirror existing NoteEncDecryTest expectations.
🪄 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: 9f2aa1ed-d7b3-4c93-9482-ec97de2f94db
📒 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/zksnark/NoteEncDecryTest.java
da67899 to
8622a5e
Compare
8622a5e to
290cbd4
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/src/main/java/org/tron/core/zen/note/NoteEncryption.java (1)
290-309:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the legacy 2-arg burn decrypt overload.
The new nonce-aware overload is here, but the public
decryptBurnMessageByOvk(byte[] ovk, byte[] ciphertext)entry point is no longer in the class. That turns the rollout into a source-compatible break for any downstream caller still decoding historical 80-byte burn records via the old API.Suggested fix
+ public static Optional<byte[]> decryptBurnMessageByOvk(byte[] ovk, byte[] ciphertext) + throws ZksnarkException { + return decryptBurnMessageByOvk(ovk, ciphertext, new byte[BURN_NONCE_LEN]); + } + public static Optional<byte[]> decryptBurnMessageByOvk(byte[] ovk, byte[] ciphertext, byte[] nonce) throws ZksnarkException { byte[] outPlaintext = new byte[64]; if (JLibsodium.cryptoAeadChacha20poly1305IetfDecrypt(new Chacha20poly1305IetfDecryptParams( outPlaintext, null, null, ciphertext, BURN_CIPHER_LEN, null, 0, nonce, ovk)) != 0) { return Optional.empty(); } return Optional.of(outPlaintext); }🤖 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 290 - 309, Add back the legacy two-arg overload decryptBurnMessageByOvk(byte[] ovk, byte[] ciphertext) that simply creates an all-zero nonce of length BURN_NONCE_LEN and delegates to the existing decryptBurnMessageByOvk(byte[] ovk, byte[] ciphertext, byte[] nonce), preserving the throws ZksnarkException signature; this restores source compatibility for callers decoding historical burn records (the underlying decrypt uses BURN_CIPHER_LEN and JLibsodium.cryptoAeadChacha20poly1305IetfDecrypt).
🧹 Nitpick comments (2)
framework/src/test/java/org/tron/core/zksnark/NoteEncDecryTest.java (2)
326-350: ⚡ Quick winAssert the burn ciphertext layout, not just
notNull.These two tests still pass if the wallet takes the wrong branch or serializes the burn record incorrectly. Please inspect the returned trigger input and verify the burn slot is 96 bytes, preserves the 96-byte input as-is, and zero-pads the nonce/reserved tail when the input is only 80 bytes.
🤖 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/test/java/org/tron/core/zksnark/NoteEncDecryTest.java` around lines 326 - 350, Update the two tests (testGetTriggerInputBurn96ByteCipher and testGetTriggerInputBurn80ByteCipher) to validate the actual burn ciphertext layout instead of only asserting non-null: call wallet.getTriggerInputForShieldedTRC20Contract(req), extract the trigger input bytes from the returned GrpcAPI.BytesMessage, locate the burn slot bytes within that serialized trigger input (the same slot produced by buildBurnTrc20Params), and assert that the burn slot length equals Encryption.BURN_CIPHER_RECORD_SIZE (96); for the 96-byte case assert the returned 96 bytes exactly match the original burnCipher, and for the 80-byte legacy case assert the first Encryption.BURN_CIPHER_LEN (80) bytes match legacyCipher and the trailing bytes up to Encryption.BURN_CIPHER_RECORD_SIZE are zero (zero-padded nonce/reserved tail).
380-413: ⚡ Quick winVerify the decoded fields on the burn-log round trip.
isPresent()only proves decryption succeeded. It will miss regressions wheregetNoteTxFromLogListByOvkpopulates the wrong amount or transparent address after decrypting. Please assert the populated result/builder matches the injectedamountandtoAddress.🤖 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/test/java/org/tron/core/zksnark/NoteEncDecryTest.java` around lines 380 - 413, After confirming Optional is present, unwrap the Optional returned from Wallet.getNoteTxFromLogListByOvk (or inspect the populated GrpcAPI.DecryptNotesTRC20.NoteTx.Builder) and assert that the decoded amount and transparent address equal the original amount and toAddress used to build logData; specifically verify the NoteTx amount field matches BigInteger value (or its byte/uint representation) and the transparent address bytes (or address string) match the injected toAddress bytes (skip prefix byte if the function stores the 20-byte payload) to catch incorrect population of those fields.
🤖 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.
Outside diff comments:
In `@framework/src/main/java/org/tron/core/zen/note/NoteEncryption.java`:
- Around line 290-309: Add back the legacy two-arg overload
decryptBurnMessageByOvk(byte[] ovk, byte[] ciphertext) that simply creates an
all-zero nonce of length BURN_NONCE_LEN and delegates to the existing
decryptBurnMessageByOvk(byte[] ovk, byte[] ciphertext, byte[] nonce), preserving
the throws ZksnarkException signature; this restores source compatibility for
callers decoding historical burn records (the underlying decrypt uses
BURN_CIPHER_LEN and JLibsodium.cryptoAeadChacha20poly1305IetfDecrypt).
---
Nitpick comments:
In `@framework/src/test/java/org/tron/core/zksnark/NoteEncDecryTest.java`:
- Around line 326-350: Update the two tests (testGetTriggerInputBurn96ByteCipher
and testGetTriggerInputBurn80ByteCipher) to validate the actual burn ciphertext
layout instead of only asserting non-null: call
wallet.getTriggerInputForShieldedTRC20Contract(req), extract the trigger input
bytes from the returned GrpcAPI.BytesMessage, locate the burn slot bytes within
that serialized trigger input (the same slot produced by buildBurnTrc20Params),
and assert that the burn slot length equals Encryption.BURN_CIPHER_RECORD_SIZE
(96); for the 96-byte case assert the returned 96 bytes exactly match the
original burnCipher, and for the 80-byte legacy case assert the first
Encryption.BURN_CIPHER_LEN (80) bytes match legacyCipher and the trailing bytes
up to Encryption.BURN_CIPHER_RECORD_SIZE are zero (zero-padded nonce/reserved
tail).
- Around line 380-413: After confirming Optional is present, unwrap the Optional
returned from Wallet.getNoteTxFromLogListByOvk (or inspect the populated
GrpcAPI.DecryptNotesTRC20.NoteTx.Builder) and assert that the decoded amount and
transparent address equal the original amount and toAddress used to build
logData; specifically verify the NoteTx amount field matches BigInteger value
(or its byte/uint representation) and the transparent address bytes (or address
string) match the injected toAddress bytes (skip prefix byte if the function
stores the 20-byte payload) to catch incorrect population of those fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 758b6518-9354-40d6-b538-655bb3757cd3
📒 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/zksnark/NoteEncDecryTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/src/main/java/org/tron/core/zen/ShieldedTRC20ParametersBuilder.java
- framework/src/main/java/org/tron/core/Wallet.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/src/main/java/org/tron/core/Wallet.java (1)
4057-4065:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate the decrypted address prefix before returning it.
The integrity check only covers the last 20 bytes of the address. A burn record with the right raw TVM address but a wrong prefix byte still passes, and Line 4063 then returns that unvalidated prefix in
transparentToAddress. Rebuild the 21-byte address from the node prefix +logToAddress, or reject the record ifplaintext[32]does not match the expected prefix.Suggested fix
if (logAmount.equals(decryptedAmount) && Hex.toHexString(logToAddress) .equals(Hex.toHexString(decryptedAddress))) { byte[] addressWithPrefix = new byte[21]; - System.arraycopy(plaintext, 32, addressWithPrefix, 0, 21); + addressWithPrefix[0] = Wallet.getAddressPreFixByte(); + System.arraycopy(logToAddress, 0, addressWithPrefix, 1, 20); + if (plaintext[32] != addressWithPrefix[0]) { + return Optional.empty(); + } builder.setToAmount(logAmount.toString(10)) .setTransparentToAddress(ByteString.copyFrom(addressWithPrefix)); return Optional.of(builder.build()); }🤖 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 4057 - 4065, The code currently accepts the 21-byte prefix from plaintext[32] without validating it, allowing a mismatched prefix to be returned via builder.setTransparentToAddress; modify the logic in Wallet (around the plaintext/amountArray/decryptedAddress check) to either (a) reconstruct addressWithPrefix by prepending the expected node prefix byte to logToAddress (i.e., use the known prefix constant + logToAddress) before calling builder.setTransparentToAddress, or (b) validate that plaintext[32] equals the expected prefix byte and reject the record if it does not; update the branch that compares logAmount and logToAddress (and uses addressWithPrefix) to perform one of these fixes so the returned transparentToAddress cannot carry an unvalidated prefix.
🧹 Nitpick comments (1)
framework/src/test/java/org/tron/core/zksnark/NoteEncDecryTest.java (1)
384-422: ⚡ Quick winAdd a wallet-level legacy burn-log compatibility case.
This block verifies the new 96-byte record path, but it never exercises
Wallet.getNoteTxFromLogListByOvkwith a pre-upgrade log payload (80B cipher + 16 zero bytes). Since the parser now slices the first 12 bytes of that tail as the nonce, a dedicated regression test here would protect the actual compatibility path, not just the lower-level decrypt helper.🤖 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/test/java/org/tron/core/zksnark/NoteEncDecryTest.java` around lines 384 - 422, Add a legacy-compatibility subcase in testGetNoteTxFromLogListByOvkBurnRoundTrip that constructs a pre-upgrade tail (80-byte cipher + 16 zero bytes) and verifies Wallet.getNoteTxFromLogListByOvk still decodes it: after you build the existing 96-byte random-nonce record, create a legacyTail byte[] of length Encryption.BURN_CIPHER_RECORD_SIZE, copy the first 80 bytes from record into legacyTail (leaving the final 16 bytes as zeros), copy legacyTail into logData at the same offset (System.arraycopy(..., 64, ...)), invoke the private method via reflection exactly as with the random-nonce case (Wallet.getNoteTxFromLogListByOvk) and Assert that the returned Optional isPresent(), ensuring the legacy path is exercised alongside the new path.
🤖 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.
Outside diff comments:
In `@framework/src/main/java/org/tron/core/Wallet.java`:
- Around line 4057-4065: The code currently accepts the 21-byte prefix from
plaintext[32] without validating it, allowing a mismatched prefix to be returned
via builder.setTransparentToAddress; modify the logic in Wallet (around the
plaintext/amountArray/decryptedAddress check) to either (a) reconstruct
addressWithPrefix by prepending the expected node prefix byte to logToAddress
(i.e., use the known prefix constant + logToAddress) before calling
builder.setTransparentToAddress, or (b) validate that plaintext[32] equals the
expected prefix byte and reject the record if it does not; update the branch
that compares logAmount and logToAddress (and uses addressWithPrefix) to perform
one of these fixes so the returned transparentToAddress cannot carry an
unvalidated prefix.
---
Nitpick comments:
In `@framework/src/test/java/org/tron/core/zksnark/NoteEncDecryTest.java`:
- Around line 384-422: Add a legacy-compatibility subcase in
testGetNoteTxFromLogListByOvkBurnRoundTrip that constructs a pre-upgrade tail
(80-byte cipher + 16 zero bytes) and verifies Wallet.getNoteTxFromLogListByOvk
still decodes it: after you build the existing 96-byte random-nonce record,
create a legacyTail byte[] of length Encryption.BURN_CIPHER_RECORD_SIZE, copy
the first 80 bytes from record into legacyTail (leaving the final 16 bytes as
zeros), copy legacyTail into logData at the same offset (System.arraycopy(...,
64, ...)), invoke the private method via reflection exactly as with the
random-nonce case (Wallet.getNoteTxFromLogListByOvk) and Assert that the
returned Optional isPresent(), ensuring the legacy path is exercised alongside
the new path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 905f3074-16d0-41a3-9fed-05d90c811b19
📒 Files selected for processing (3)
framework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/zen/note/NoteEncryption.javaframework/src/test/java/org/tron/core/zksnark/NoteEncDecryTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- framework/src/main/java/org/tron/core/zen/note/NoteEncryption.java
What does this PR do?
Hardens the
out_ciphertext(ovk-encrypted note) used by shielded TRC-20burn:NoteEncryption.encryptBurnMessageByOvkgenerates a fresh CSPRNG nonce per call and emits a 96-byte record laid out ascipher(80) || nonce(12) || reserved(4). The 2^-96 all-zero draw is rerolled so the nonce field never collides with the pre-upgrade zero padding at the same offset.decryptBurnMessageByOvk(ovk, ciphertext, nonce)requires a 12-byte nonce; null or wrong-length input is rejected via a fail-fast precondition. Pre-upgrade burn logs carry an all-zero byte field at the nonce offset and decrypt via the same path.ShieldedTRC20ParametersBuilderemits the 96-byteburnCiphertext.Wallet.getNoteTxFromLogListByOvkparses the nonce slice from on-chain logs and feeds it into the new decrypt API.Wallet.getTriggerInputForShieldedTRC20Contractaccepts only the 96-byte record. Stale clients passing an 80-byte cipher are rejected with a clear error message guiding them to rebuild against the new encoder; padding stale ciphertext to a zero-nonce record would otherwise extend the (ovk, zero-nonce) reuse surface forward in time.Why are these changes required?
The previous burn AEAD reused a fixed all-zero nonce under the same
ovkfor every burn, violating RFC 8439 §4 (key, nonce) uniqueness. Two burns under the sameovkrecover the keystream and the Poly1305 one-time key, which is sufficient to forge AEAD ciphertexts attributed to that ovk.Compatibility matrix
create+ newscanscancreate+ oldscanburnCiphertextfed into the newgetTriggerInputForShieldedTRC20ContractWallet.java:4291) with an explicit error directing the caller to rebuild againstencryptBurnMessageByOvk. Padding stale ciphertext to a zero-nonce record would extend the (ovk, zero-nonce) reuse window forward in time.getTriggerInputForShieldedTRC20ContractThis PR has been tested by:
NoteEncDecryTest: round-trip with random nonce, uniqueness across calls, pre-upgrade zero-nonce decode, malformed nonce rejection, short-log rejection, 80-byte trigger-input rejection, 96-byte trigger-input round-trip.Follow up
A scanner-side activation height (
BURN_NONCE_ACTIVATION_HEIGHT, defaultLong.MAX_VALUE, switchable via aChainParametersproposal) is planned as a separate change. Once activated,getNoteTxFromLogListByOvkwill refuse to decrypt any zero-nonce burn whose log height is ≥ H₀, closing the residual phantom-burn injection path (Finding 1) that bypasses the encoder API.Note: the gate is a hard reject and cannot distinguish injected phantom burns from legitimate burns produced by un-upgraded clients. Activating it therefore requires an ecosystem migration window: every producer that bypasses
getTriggerInputForShieldedTRC20Contract(legacy SDKs / wallets / third-party scripts) must be on the 96-byte encoder before H₀, otherwise their post-H₀ burns become unscannable. Tracking in the design doc.Extra details
Summary by CodeRabbit
Bug Fixes
New Features
Tests