fix: validate untrusted input before indexing, asserting or dereferencing - #297
Conversation
…cing
Wave 1 of the repository review: twelve findings that share one defect
class — data from the node, from a keystore file or from a caller
reaching an index, a type assertion or a pointer dereference with no
check.
pkg/client, node responses:
C8 ParseTRC20StringProperty took the ABI string length from contract
return data and evaluated 2*int(l), which overflows to a negative
value and passed the bounds check before panicking on the slice.
A >64-bit length was also silently truncated by Uint64 to its low
bits, returning "" with a nil error. Reject non-uint64 lengths and
bound by division so no multiplication can overflow.
W11 Four TRC20 queries indexed GetConstantResult()[0] unguarded; a node
may report success with no entries. Routed through one helper, which
also requires a full 32-byte ABI word, matching callForAddress.
W12 TRC20 transfer, approve and transferFrom encoded amounts with
LeftPadBytes: nil panicked, a negative amount became a real positive
transfer because big.Int.Bytes returns the absolute value, and a
>256-bit value was appended whole because LeftPadBytes does not
truncate. All three now use the validated trc20enc.PadUint256, which
is byte-identical for every previously valid amount.
W27 DeployContractCtx assigned to tx.Transaction.RawData without
checking the result code, so a node-side rejection panicked instead
of reporting why the deployment failed.
W28 UpdateAccountPermissionCtx made eight unchecked assertions on
caller-supplied maps. A missing key or an int where int64 was meant
panicked mid-update.
W43 GetTransactionInfoByIDCtx dereferenced a possibly-nil response.
W44 WithdrawExpireUnfreeze, DelegateResource and UnDelegateResource
skipped the result-code check every sibling performs, so rejections
surfaced only after signing and broadcast.
Five further sites of the same class, found while reviewing the above:
triggerConstantContract, triggerContract, DeployContract and their
callers in contracts.go, plus TRC20CallCtx, all read tx.Result.Code
directly rather than through the nil-safe accessor, so a response with
no Result panicked. TRC20CallCtx gates all four W11 queries, and
triggerContract assigned through tx.Transaction.RawData with no nil
check — the same shape as W27. All now use GetResult()/GetTransaction().
pkg/common/numeric:
W37 NewDecFromString used an unanchored regex, so "1e5x" matched and
Atoi("5x") failed silently, returning 1 with a nil error. Anchored,
and both errors now propagate.
W38 NewDecFromHex let a nil big.Int from SetString reach Mul, panicking.
It also accepted a sign, because big.Int.SetString does, yielding a
negative Dec out of a hex parser; signs are now rejected.
pkg/keystore:
W10 getKDFKey asserted salt and prf to string and every numeric
parameter to float64 with no comma-ok, and passed n, r and p to
scrypt with no upper bound — all before the MAC is verified, so a
crafted file could panic the process or force an unbounded
derivation. Bounds leave room for StandardScryptN and pbkdf2
defaults.
W26 NewKeyForDirectICAP retried until an address began with "0x00",
which a TRON address never does, so every call recursed until the
stack overflowed. Removed; ICAP has no meaning on TRON.
W66 Negative-case coverage for the numeric parsers, which is why W37 and
W38 survived.
W42 (nil g.Client before Start) is deliberately not here: it is a
mechanical sweep of 80 call sites across 13 files and does not belong in
the same diff as these targeted guards.
Tests assert both that the input is rejected and that nothing panics.
The TRC20 length cases were verified against the original code to
confirm which panic and which returned a silent empty string. Branches
that a real gRPC round trip cannot produce — a nil TransactionInfo, a
nil Result — are reached by stubbing the exported Client field, which is
also how an SDK consumer can produce them.
Note for the release notes: W27 and W44 change observable behaviour.
Callers that previously received a TransactionExtention for a request
the node rejected now receive an error.
|
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:
📝 WalkthroughWalkthroughThe PR hardens client RPC response handling, permission parsing, TRC20 ABI processing, numeric conversion, and keystore decryption. It also removes the exported Direct ICAP key-generation helper and adds malformed-input and node-response tests. ChangesClient input and response validation
Numeric parser validation
Keystore input and key generation changes
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #297 +/- ##
==========================================
+ Coverage 80.92% 82.77% +1.85%
==========================================
Files 76 77 +1
Lines 6201 6333 +132
==========================================
+ Hits 5018 5242 +224
+ Misses 844 785 -59
+ Partials 339 306 -33
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@pkg/client/trc20.go`:
- Around line 98-103: Update the rejected-result branch in the relevant TRC20
contract method to return a nil result together with the formatted error,
matching triggerContract and DeployContractCtx. Preserve returning the node
result only on successful execution so TRC20Send, TRC20Approve, and
TRC20TransferFrom never expose a rejected TransactionExtention alongside an
error.
In `@pkg/common/numeric/numeric.go`:
- Around line 672-676: In pkg/common/numeric/numeric.go:672-676, validate the
parsed exponent before calling Pow, rejecting values outside the established
safe bound, including the minimum signed int, and return the existing
invalid-exponent error path. In
pkg/common/numeric/parse_malformed_test.go:15-28, add malformed-input cases
covering the minimum signed int exponent and an exponent above the safe limit,
asserting both are rejected without invoking unbounded exponentiation.
In `@pkg/keystore/passphrase.go`:
- Around line 394-397: Update the kdfInt validation for "dklen" in
getKDFKey/DecryptDataV3 to use a minimum of 32, matching the AES and MAC key
slices derived from the result. Preserve the existing maximum bound and error
propagation, and add a kdfparams test case covering a dklen below 32 alongside
the existing upper-bound test.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5dbedf8-93a5-4f61-a253-6b4616f38c46
📒 Files selected for processing (14)
pkg/client/account.gopkg/client/bank.gopkg/client/contracts.gopkg/client/network.gopkg/client/resources.gopkg/client/trc20.gopkg/client/trc20_test.gopkg/client/untrusted_input_test.gopkg/common/numeric/numeric.gopkg/common/numeric/parse_malformed_test.gopkg/keystore/kdfparams_test.gopkg/keystore/key.gopkg/keystore/key_test.gopkg/keystore/passphrase.go
💤 Files with no reviewable changes (2)
- pkg/keystore/key_test.go
- pkg/keystore/key.go
…rejects Three findings from review of the previous commit. NewDecFromString accepted any exponent the anchored regex matched, and Atoi accepts the whole int range. Pow raises 10 to it, which panics with "Int overflow" past 10^76 because Dec is capped at 255+DecimalPrecisionBits bits over an 18-decimal scale; and Pow negates a negative exponent, so math.MinInt stays negative and recurses until the stack overflows. "1e-9223372036854775808" was a fatal stack overflow from a plain string. Exponents are now bounded to the representable range, verified empirically as 10^76 representable and 10^77 panicking. getKDFKey allowed dklen down to 1, but the decrypt paths read derivedKey[:16] for the AES key and derivedKey[16:32] for the MAC. That does not panic today only because scrypt.Key and pbkdf2.Key return a slice with capacity 32 even when dklen is 1, so the expression reads past len into spare capacity. Require the V3 spec's 32 and stop depending on that. TRC20CallCtx returned the node's rejection payload alongside the error, unlike triggerContract and DeployContractCtx which return nil. TRC20Send, TRC20Approve and TRC20TransferFrom pass that value straight through, so a caller checking the result before the error could sign and broadcast a rejected transaction. Returns nil now.
|
All three addressed in ✅ Exponent bound — confirmed, and worse than reportedReal, and it is a hard crash from a plain string. Your analysis of the mechanism is exactly right: There is a second overflow you did not mention, which I hit because my first bound was still too loose. I picked ±1000 and my own test case Bounded to ±76. The original input now returns
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/common/numeric/parse_malformed_test.go (2)
102-106: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert exact decoded values.
IsZero()only proves the result is non-zero; an incorrect value would still pass. Compare both0xffandffagainst the exact decimal result.🤖 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 `@pkg/common/numeric/parse_malformed_test.go` around lines 102 - 106, Update TestNewDecFromHex_Valid to assert that both numeric.NewDecFromHex("0xff") and numeric.NewDecFromHex("ff") equal the exact decimal value 255, while preserving the existing NotPanics coverage.
29-38: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd regressions for allowed-but-unrepresentable inputs.
Cover
9e76and a long syntactically valid hex value such asstrings.Repeat("f", 128). Both should be rejected without panic after the implementation fix.Also applies to: 77-100
🤖 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 `@pkg/common/numeric/parse_malformed_test.go` around lines 29 - 38, Extend the malformed-input regression cases to include the allowed-but-unrepresentable decimal input 9e76 and a syntactically valid 128-character hexadecimal value generated with strings.Repeat. Ensure both cases are asserted as rejected and processed without panicking, preserving the existing malformed-input test behavior.
🤖 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 `@pkg/common/numeric/numeric.go`:
- Around line 692-695: Validate the total numeric magnitude before fixed-point
arithmetic in the scientific-notation parsing path around numeric.go:692-695,
rejecting values such as 9e76 before Mul or Pow can overflow. In the hexadecimal
parsing path at numeric.go:731-738, reject or short-circuit inputs whose length
would make Pow overflow. Add 9e76 to parse_malformed_test.go:29-38 as a no-panic
rejection case and add an oversized valid-hex regression case in
parse_malformed_test.go:77-100.
---
Nitpick comments:
In `@pkg/common/numeric/parse_malformed_test.go`:
- Around line 102-106: Update TestNewDecFromHex_Valid to assert that both
numeric.NewDecFromHex("0xff") and numeric.NewDecFromHex("ff") equal the exact
decimal value 255, while preserving the existing NotPanics coverage.
- Around line 29-38: Extend the malformed-input regression cases to include the
allowed-but-unrepresentable decimal input 9e76 and a syntactically valid
128-character hexadecimal value generated with strings.Repeat. Ensure both cases
are asserted as rejected and processed without panicking, preserving the
existing malformed-input test behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c825e7be-f330-457d-bb1b-f2a6dd32ceb5
📒 Files selected for processing (5)
pkg/client/trc20.gopkg/common/numeric/numeric.gopkg/common/numeric/parse_malformed_test.gopkg/keystore/kdfparams_test.gopkg/keystore/passphrase.go
The exponent bound added in the previous commit is necessary but not sufficient. Magnitude depends on the mantissa as well, so 1e76 is representable while 9e76 and 99e75 sit inside the bound and still panic with "Int overflow". NewDecFromHex has the same problem by length: a 64-character string — an ordinary ABI uint256 word — exceeds Dec's range and panicked, as did anything longer. Deciding in advance whether a parsed value fits means reproducing Dec's 255+DecimalPrecisionBits cap in the parser, which is where the previous two attempts went wrong. Instead both parsers recover the documented "Int overflow" panic and convert it to ErrOutOfRange, re-raising anything else so real bugs still surface. That covers every arithmetic path exactly rather than approximating it. The exponent bound stays: it stops math.MinInt reaching Pow, where the negation of the minimum signed int is still negative and recurses until the stack overflows, which no recover can turn into an error. NewDecFromHex keeps its signature and degrades to zero, consistent with how it already reports other invalid input.
|
Both confirmed and fixed in
|
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)
pkg/common/numeric/numeric.go (1)
732-732: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate non-scientific values against
Dec’s range.This fallback only constructs a
big.Int, sorecoverIntOverflowsees no panic. A plain value such asstrings.Repeat("9", 77)is accepted with no error despite its precision-scaled representation exceedingDec’s range; later arithmetic can still panic. Validate this branch (preferably centrally inNewDecFromStr) and add a regression assertingErrOutOfRange.🤖 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 `@pkg/common/numeric/numeric.go` at line 732, The non-scientific fallback around NewDecFromStr accepts values whose precision-scaled representation exceeds Dec’s range. Update NewDecFromStr or the fallback caller to validate the constructed value against Dec bounds and return ErrOutOfRange, ensuring scientific and non-scientific inputs share the same validation; add a regression covering an oversized repeated-9 input.
🤖 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 `@pkg/common/numeric/numeric.go`:
- Line 732: The non-scientific fallback around NewDecFromStr accepts values
whose precision-scaled representation exceeds Dec’s range. Update NewDecFromStr
or the fallback caller to validate the constructed value against Dec bounds and
return ErrOutOfRange, ensuring scientific and non-scientific inputs share the
same validation; add a regression covering an oversized repeated-9 input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 408bee2c-e848-4c24-8bbb-436674442917
📒 Files selected for processing (2)
pkg/common/numeric/numeric.gopkg/common/numeric/parse_malformed_test.go
…t tests Require non-nil RawData after a success code on Delegate, UnDelegate and WithdrawExpireUnfreeze so a hollow TransactionExtention cannot be signed. Standardise GetCode() != 0, restore UpdateAccountPermissionCtx godoc, and document TRC20CallCtx's constant-path Result tolerance. Extend untrusted-input tests for rejection and success-without-tx on those builders plus TriggerContract/TRC20Send. Add DecryptKey compatibility cases for in-the-wild KDF params and the intentional short-dklen rejection.
Introduce requireTxExtension for every write builder so SUCCESS responses without RawData cannot be returned as signable transactions. Reject plain decimal overflow in NewDecFromStr and scientific underflows that collapse to zero at fixed-point precision. Cap keystore KDF salt length before hex-decode so unauthenticated params cannot allocate unbounded input pre-MAC.
Validate MAC, IV and ciphertext length before KDF/decrypt so crafted keystore JSON cannot panic NewCTR/CryptBlocks or force unbounded pre-MAC allocations. Reject empty estimate-energy and GetAccount responses from substituted clients, and include result codes when node rejection messages are empty.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/client/untrusted_input_test.go (1)
168-258: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that invalid responses return no transaction.
These cases discard
tx. A regression fromreturn nil, errtoreturn response, errwould pass the tests while callers receive a rejected extension. Capturetxand requirenilin each rejection and no-transaction case.Proposed test change
- _, err := c.WithdrawExpireUnfreeze(testAddrA, 1700000000000) + tx, err := c.WithdrawExpireUnfreeze(testAddrA, 1700000000000) require.ErrorContains(t, err, "node refused the request") + require.Nil(t, tx)🤖 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 `@pkg/client/untrusted_input_test.go` around lines 168 - 258, Update TestBuilders_SurfaceNodeRejection and TestDeployContract_NodeRejection to capture the returned transaction alongside err in every rejection and no-transaction subtest, then assert the transaction is nil while retaining the existing error assertions and panic guards.
🤖 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 `@pkg/client/txext.go`:
- Line 22: Update the transaction validation around tx.GetResult() to reject a
nil Result before calling GetCode(), returning an error for responses with
RawData but no Result; preserve the existing nonzero-code validation and add a
regression test covering the absent-Result case.
In `@pkg/keystore/passphrase.go`:
- Around line 273-275: Update EncryptDataV3 to reject plaintext or resulting
payloads exceeding the existing 1,024-byte V3 ciphertext limit before performing
KDF work, matching DecryptDataV3’s maxCiphertextLen validation. Document this
limit alongside the V3 implementation and add boundary tests covering exactly
the maximum and one byte over it.
---
Nitpick comments:
In `@pkg/client/untrusted_input_test.go`:
- Around line 168-258: Update TestBuilders_SurfaceNodeRejection and
TestDeployContract_NodeRejection to capture the returned transaction alongside
err in every rejection and no-transaction subtest, then assert the transaction
is nil while retaining the existing error assertions and panic guards.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1878ec26-94b5-4c23-900e-a3cf153b96c0
📒 Files selected for processing (20)
pkg/client/account.gopkg/client/assets.gopkg/client/bank.gopkg/client/contracts.gopkg/client/exchange.gopkg/client/proposal.gopkg/client/resources.gopkg/client/transfer.gopkg/client/trc20.gopkg/client/txext.gopkg/client/txext_test.gopkg/client/untrusted_input_test.gopkg/client/witnesses.gopkg/common/numeric/numeric.gopkg/common/numeric/parse_malformed_test.gopkg/keystore/cipherparams_test.gopkg/keystore/compat_test.gopkg/keystore/crypto.gopkg/keystore/kdfparams_test.gopkg/keystore/passphrase.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/client/contracts.go
- pkg/keystore/kdfparams_test.go
- pkg/client/trc20.go
Cover requireTxExtension empty-op paths, estimateEnergy rejections, SUCCESS-without-transaction across more write builders, AES IV/block alignment errors, invalid cipher hex, and kdfInt int/int64 branches so Codecov patch coverage clears the 80% target.
requireTxExtension now errors when Result is absent before GetCode(), which otherwise defaults to SUCCESS on a nil *Return. EncryptDataV3 enforces the same maxCiphertextLen as DecryptDataV3 before KDF work.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/client/untrusted_input_test.go (1)
128-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that malformed permission maps do not reach the RPC.
The assertion at Lines 150-155 accepts any error. It can pass if
AccountPermissionUpdatereceives a malformed permission and the mock RPC returns an error. Count calls toAccountPermissionUpdateFunc, then assert zero for every case.As per path instructions,
pkg/client/**requires focus on error handling and API correctness.🤖 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 `@pkg/client/untrusted_input_test.go` around lines 128 - 147, Update the malformed permission cases in the untrusted-input test to count invocations of AccountPermissionUpdateFunc and assert that each case makes zero RPC calls, rather than accepting any returned error. Keep the existing validation-error assertions while ensuring malformed active and witness permission maps are rejected before reaching the RPC.Source: Path instructions
🤖 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.
Nitpick comments:
In `@pkg/client/untrusted_input_test.go`:
- Around line 128-147: Update the malformed permission cases in the
untrusted-input test to count invocations of AccountPermissionUpdateFunc and
assert that each case makes zero RPC calls, rather than accepting any returned
error. Keep the existing validation-error assertions while ensuring malformed
active and witness permission maps are rejected before reaching the RPC.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87acb970-5f1d-4963-8424-d5f4bdfaf79d
📒 Files selected for processing (8)
pkg/client/txext.gopkg/client/txext_test.gopkg/client/untrusted_input_test.gopkg/keystore/cipherparams_test.gopkg/keystore/crypto_internal_test.gopkg/keystore/kdf_internal_test.gopkg/keystore/key_test.gopkg/keystore/passphrase.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/client/txext.go
- pkg/keystore/passphrase.go
1 participant
Wave 1 of the repository review: twelve findings that share one defect class — data from the node,
from a keystore file, or from a caller reaching an index, a type assertion, or a pointer dereference
with no check.
pkg/client— node responsesParseTRC20StringPropertytook the ABI string length from contract return data and evaluated2*int(l), which overflows to a negative value, passed the bounds check, then panicked on the slice.GetConstantResult()[0]unguarded; a node may report success with no entries.TRC20Send/Approve/TransferFromencoded amounts withLeftPadBytes.DeployContractCtxassigned throughtx.Transaction.RawDatawithout checking the result code.UpdateAccountPermissionCtxmade eight unchecked assertions on caller-supplied maps.GetTransactionInfoByIDCtxdereferenced a possibly-nil response.WithdrawExpireUnfreeze,DelegateResource,UnDelegateResourceskipped the result-code check every sibling performs.C8 had two failure modes, not one
Verified by running the original bounds check against each input:
slice bounds [:9223372036854775936]""—2*lwraps to exactly 0""—Uint64truncates to the low bitsA hostile
name()could therefore return an empty token name with a nil error, not only crash. NowIsUint64catches the truncation and the bound is expressed as a division, so no multiplication existsto overflow.
W12 encoding is unchanged for valid amounts
All three sites now use
trc20enc.PadUint256. Confirmed byte-identical toLeftPadBytesacross0,1,1e6, max uint64, 2^255−1 and max uint256 — zero mismatches, so calldata for any amount thatworked before is untouched. What changes is that
nil(panicked), negative (silently became a realpositive transfer, because
big.Int.Bytesreturns the absolute value) and >256-bit values (appendedwhole, because
LeftPadBytesdoes not truncate) are now rejected.Five further sites of the same class
Found while reviewing the above.
triggerConstantContract,triggerContract,DeployContractandtheir callers in
contracts.go, plusTRC20CallCtx, all readtx.Result.Codedirectly ratherthan through the nil-safe accessor, so a response with no
Resultpanicked.TRC20CallCtxgates allfour W11 queries — the constant-result indexing was guarded while a nil-
Resultpanic sat upstream onthe same path.
triggerContractalso assigned throughtx.Transaction.RawDatawith no nil check, thesame shape as W27.
pkg/common/numeric"1e5x"matched,Atoi("5x")failed silently, and the functionreturned
1with a nil error. Anchored; both errors propagate.*big.IntfromSetStringreachedMuland panicked. It also accepted a sign(because
big.Int.SetStringdoes), yielding a negativeDecout of a hex parser.pkg/keystoregetKDFKeyassertedsalt/prfto string and every numeric parameter tofloat64with nocomma-ok, and passed
n,r,pto scrypt with no upper bound — all before the MAC isverified. A crafted file could panic the process or force an unbounded derivation. Bounds leave room
for
StandardScryptN(256 MiB working set) and pbkdf2 defaults.NewKeyForDirectICAPretried until an address began with0x00, which a TRON address neverdoes, so every call recursed until the stack overflowed. Removed; ICAP has no meaning on TRON.
Testing
W66 adds the negative-case coverage whose absence let W37 and W38 survive.
Tests assert both that input is rejected and that nothing panics. Branches a real gRPC round trip
cannot produce — a nil
TransactionInfo, a nilResult— are reached by stubbing the exportedClientfield, which is also how an SDK consumer can produce them. (
grpc-gomaterialises a(nil, nil)serverreturn into an empty message, so a mock server cannot reach those guards; coverage confirmed the branch
was dead before this change.)
Every touched function keeps its pre-existing happy-path test. Coverage:
pkg/client83.5 → 84.7%,numeric91.7 → 92.9%,keystore84.0 → 86.1%.make lint(0 issues) ·make test(20/20 packages) ·go test ./cmd/...·make— all clean.Not included
W42 (nil
g.ClientbeforeStart) is a mechanical sweep of 80 call sites across 13 files anddoes not belong in the same diff as these targeted guards. It gets its own PR.
W27 and W44 change observable behaviour. Callers that previously received a
TransactionExtentionfor a request the node rejected now receive an error. That is the fix — the failure previously
surfaced only after signing and broadcast, or not at all — but it is breaking for anyone who was
ignoring the result code.
Summary by CodeRabbit