[0xbigapple]

What does this PR do?

This PR hardens externally reachable API inputs before expensive or ambiguous processing:

• rejects an absent, blank, or malformed address consistently on getReward and getBrokerage
• renders address fields longer than 21 bytes as hex instead of Base58Check in visible=true responses, so broadcasthex, which echoes the submitted transaction even when it rejects it, no longer spends quadratic time encoding an oversized address such as raw_data.auths[].account.address
• renders addresses in log and exception messages through the new WalletUtil.getAddressString, which Base58Check-encodes only 20- and 21-byte values and renders any other length as invalid-address(<hex of at most 21 bytes>,len=N), instead of Base58Check-encoding the whole value
• limits integer fields read through Util.getJsonLongValue (contract deploy and trigger values, exchange and proposal IDs), in string or numeric form, to 64 characters before BigDecimal conversion, and rejects values that are not exact 64-bit integers
• accepts Permission_id only as a JSON integer or a decimal integer string within the 32-bit range
• limits shielded TRC20 amount strings before BigInteger conversion, and reports an unusable amount or trigger input as a ContractValidateException
• requires one or two spend and receive descriptions for a shielded TRC20 transfer, checked before the serialization loops
• adds InvalidHttpParameterException for HTTP parameter validation and lets the error sanitizer pass its message through, without echoing the rejected value

Why are these changes required?

Malformed inputs could previously trigger disproportionate CPU work, silently truncate or wrap numeric values, or return plausible results for structurally invalid addresses.

These changes reject invalid inputs before expensive conversion, encoding, storage access, or repeated buffer merging.

This PR has been tested by:

• Unit Tests
• Manual Testing

Compatibility

Breaking, limited to requests with missing or invalid input; requests with valid input keep their existing behavior.

• breaking: getReward and getBrokerage no longer substitute a default for an address they never received. Util.getAddress now either returns a valid address or throws, so a request that carries no address at all is rejected like any other unusable one: GET /wallet/getReward without the parameter answered {"reward": 0} and now answers {"Error":"INVALID address"}. The same applies to getBrokerage and to both endpoints on /walletsolidity and the PBFT mirror, six in total. A caller that relied on the zero default has to send an address.
• with visible=true, an address field longer than 21 bytes is rendered as hex instead of Base58Check
• Permission_id values other than a 32-bit JSON integer or decimal integer string are rejected, including booleans, fractions, overflowing integers, explicit null, floating-point forms such as 1.0 or 1e2, and strings such as "1,000" or "1.0"; true used to be applied as permission 1, 1.0 and "1.0" as 1, 1e2 as 100, and "1,000" as 1000
• shielded TRC20, over HTTP and gRPC, only when the existing node.allowShieldedTransactionApi flag is enabled (disabled by default):
  • an amount string longer than 80 characters is rejected before conversion in createShieldedContractParameters, createShieldedContractParametersWithoutAsk and getTriggerInputForShieldedTRC20Contract; such a value already exceeded the 256-bit range unless padded with leading zeros
  • getTriggerInputForShieldedTRC20Contract rejects a transfer with no or more than two receive descriptions
• validation failures that answered internal server error, such as key [x] does not exist, an overflowing integer, or an unusable shielded TRC20 trigger input, now answer the specific message
• no protocol, protobuf, storage, dependency, configuration, or HTTP status-code changes are introduced

Follow up

Extra details

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 0a1aed30-45e5-48d0-bc5f-74004a883841

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cubic-dev-ai]

1 issue found across 15 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/test/java/org/tron/core/services/interfaceOnSolidity/http/RewardBrokerageAddressValidationTest.java">

<violation number="1" location="framework/src/test/java/org/tron/core/services/interfaceOnSolidity/http/RewardBrokerageAddressValidationTest.java:42">
P3: The Solidity and PBFT test files duplicate the entire harness (reflection-backed wallet factory, request builder, inject helper, and the reward/brokerage rejection test) differing only in the wallet type and injected field name. Extract a shared base class or parameterized harness to avoid maintaining the same assertions in two places.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P3: The Solidity and PBFT test files duplicate the entire harness (reflection-backed wallet factory, request builder, inject helper, and the reward/brokerage rejection test) differing only in the wallet type and injected field name. Extract a shared base class or parameterized harness to avoid maintaining the same assertions in two places.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/core/services/interfaceOnSolidity/http/RewardBrokerageAddressValidationTest.java, line 42:

<comment>The Solidity and PBFT test files duplicate the entire harness (reflection-backed wallet factory, request builder, inject helper, and the reward/brokerage rejection test) differing only in the wallet type and injected field name. Extract a shared base class or parameterized harness to avoid maintaining the same assertions in two places.</comment>

<file context>
@@ -0,0 +1,57 @@
+  }
+
+  @Test
+  public void rewardAndBrokerageMirrorsRejectMalformedAddresses() throws Exception {
+    GetRewardOnSolidityServlet reward = new GetRewardOnSolidityServlet();
+    inject(reward, "walletOnSolidity", walletOnSolidity());
</file context>

[cubic-dev-ai]

1 issue found across 17 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="framework/src/main/java/org/tron/core/services/http/Util.java">

<violation number="1" location="framework/src/main/java/org/tron/core/services/http/Util.java:705">
P2: For multibyte UTF-8 bodies, this check counts characters instead of request bytes and buffers well beyond `httpMaxMessageSize` before rejecting. Count bytes from the input stream while reading so the early bound matches the configured limit.</violation>
</file>

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: For multibyte UTF-8 bodies, this check counts characters instead of request bytes and buffers well beyond httpMaxMessageSize before rejecting. Count bytes from the input stream while reading so the early bound matches the configured limit.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/services/http/Util.java, line 705:

<comment>For multibyte UTF-8 bodies, this check counts characters instead of request bytes and buffers well beyond `httpMaxMessageSize` before rejecting. Count bytes from the input stream while reading so the early bound matches the configured limit.</comment>

<file context>
@@ -673,14 +684,33 @@ private static String checkGetParam(HttpServletRequest request, String key) thro
+        new InputStreamReader(request.getInputStream()))) {
+      int read;
+      while ((read = reader.read(buffer)) != -1) {
+        if (sb.length() + read > limit) {
+          throw new Exception("body size is too big, the limit is " + limit);
+        }
</file context>

[cubic-dev-ai]

3 issues found across 40 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="framework/src/main/java/org/tron/core/services/http/JsonFormat.java">

<violation number="1" location="framework/src/main/java/org/tron/core/services/http/JsonFormat.java:864">
P3: `escapeBytesSelfType` hex-renders the entire oversized ByteString (2 hex chars per byte), so a 64KB attacker-controlled address field is echoed as a ~128KB hex string in visible-mode responses — larger than the base58 rendering it replaces and contrary to the PR's stated goal of avoiding echoing oversized attacker-controlled values. The log-side helper introduced in the same change truncates to a 4-byte head; this response path does not.</violation>

<violation number="2" location="framework/src/main/java/org/tron/core/services/http/JsonFormat.java:1708">
P2: This guard still accepts non-empty 1–20-byte hex values as addresses, while the visible Base58 path rejects any length other than 21 bytes. Reject non-empty lengths other than `Constant.TRON_ADDRESS_SIZE` here, preserving empty optional address fields.</violation>
</file>

<file name="framework/src/test/java/org/tron/core/WalletMockTest.java">

<violation number="1" location="framework/src/test/java/org/tron/core/WalletMockTest.java:910">
P3: This transfer test passes with a completely empty ReceiveDescription (zero-length commitment, value, epk, zkproof, c_enc, c_out), so it only proves the new count check (1-2 receives) succeeds. The PR claims unusable inputs are rejected with ContractValidateException, but a fully empty receive description still produces a trigger input here. Either use a populated ReceiveDescription or add assertions that empty/invalid receive descriptions are rejected, so the hardened validation path is actually exercised.</violation>
</file>

<sub>Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: This guard still accepts non-empty 1–20-byte hex values as addresses, while the visible Base58 path rejects any length other than 21 bytes. Reject non-empty lengths other than Constant.TRON_ADDRESS_SIZE here, preserving empty optional address fields.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/services/http/JsonFormat.java, line 1708:

<comment>This guard still accepts non-empty 1–20-byte hex values as addresses, while the visible Base58 path rejects any length other than 21 bytes. Reject non-empty lengths other than `Constant.TRON_ADDRESS_SIZE` here, preserving empty optional address fields.</comment>

<file context>
@@ -1701,6 +1704,10 @@ public ByteString consumeByteString(final String fieldName, boolean selfType)
         if (!selfType) {
           result = unescapeBytes(escaped);
+          if (HttpSelfFormatFieldName.isAddressFormat(fieldName)
+              && result.size() > Constant.TRON_ADDRESS_SIZE) {
+            throw new InvalidEscapeSequence("invalid address for field: " + fieldName);
+          }
</file context>

Suggested change

	&& result.size() > Constant.TRON_ADDRESS_SIZE) {
	&& result.size() != 0
	&& result.size() != Constant.TRON_ADDRESS_SIZE) {

[cubic-dev-ai]

P3: This transfer test passes with a completely empty ReceiveDescription (zero-length commitment, value, epk, zkproof, c_enc, c_out), so it only proves the new count check (1-2 receives) succeeds. The PR claims unusable inputs are rejected with ContractValidateException, but a fully empty receive description still produces a trigger input here. Either use a populated ReceiveDescription or add assertions that empty/invalid receive descriptions are rejected, so the hardened validation path is actually exercised.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/core/WalletMockTest.java, line 910:

<comment>This transfer test passes with a completely empty ReceiveDescription (zero-length commitment, value, epk, zkproof, c_enc, c_out), so it only proves the new count check (1-2 receives) succeeds. The PR claims unusable inputs are rejected with ContractValidateException, but a fully empty receive description still produces a trigger input here. Either use a populated ReceiveDescription or add assertions that empty/invalid receive descriptions are rejected, so the hardened validation path is actually exercised.</comment>

<file context>
@@ -907,6 +907,7 @@ public void testGetTriggerInputForShieldedTRC20Contract1()
     GrpcAPI.ShieldedTRC20Parameters shieldedTRC20Parameters =
         GrpcAPI.ShieldedTRC20Parameters.newBuilder()
             .addSpendDescription(spendDescription)
+            .addReceiveDescription(ShieldContract.ReceiveDescription.getDefaultInstance())
             .setParameterType("transfer")
             .build();
</file context>

[cubic-dev-ai]

P3: escapeBytesSelfType hex-renders the entire oversized ByteString (2 hex chars per byte), so a 64KB attacker-controlled address field is echoed as a ~128KB hex string in visible-mode responses — larger than the base58 rendering it replaces and contrary to the PR's stated goal of avoiding echoing oversized attacker-controlled values. The log-side helper introduced in the same change truncates to a 4-byte head; this response path does not.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/services/http/JsonFormat.java, line 864:

<comment>`escapeBytesSelfType` hex-renders the entire oversized ByteString (2 hex chars per byte), so a 64KB attacker-controlled address field is echoed as a ~128KB hex string in visible-mode responses — larger than the base58 rendering it replaces and contrary to the PR's stated goal of avoiding echoing oversized attacker-controlled values. The log-side helper introduced in the same change truncates to a 4-byte head; this response path does not.</comment>

<file context>
@@ -861,6 +861,9 @@ static String escapeBytes(ByteString input, final String fieldName, boolean self
   static String escapeBytesSelfType(ByteString input, final String fieldName) {
     //Address
     if (HttpSelfFormatFieldName.isAddressFormat(fieldName)) {
+      if (input.size() > Constant.TRON_ADDRESS_SIZE) {
+        return ByteArray.toHexString(input.toByteArray());
+      }
</file context>

[cubic-dev-ai]

1 existing issue remains and 3 new issues found across 30 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="framework/src/main/java/org/tron/core/services/http/GetBrokerageServlet.java">

<violation number="1" location="framework/src/main/java/org/tron/core/services/http/GetBrokerageServlet.java:22">
P3: This catch-all around `Util.getAddress` routes every exception into the debug-level client-error path, but `getAddress` already converts all expected request-input failures to `InvalidHttpParameterException` (blank/missing address, malformed hex or base58, unparseable JSON body). Anything else that escapes it — e.g. an `IOException` from reading the POST body in `getRequestValue`, or an unexpected runtime failure — is a genuine server-side fault, and `Util.processError` now hides it at DEBUG while reporting it to the client as a parameter error. Per the new `Util.processServerError` contract, server-side failures should be logged at ERROR. Catch `InvalidHttpParameterException` in the parsing block and let unexpected exceptions hit the second catch (single try block with both catch clauses). `GetRewardServlet` has the identical issue.</violation>
</file>

<file name="framework/src/main/java/org/tron/core/services/http/TriggerSmartContractServlet.java">

<violation number="1" location="framework/src/main/java/org/tron/core/services/http/TriggerSmartContractServlet.java:39">
P3: These throws are still swallowed by the generic `catch (Exception e)` below, which formats the response as `e.getClass() + " : " + errString`. The only visible effect of swapping the type is that the OTHER_ERROR message now exposes the internal class name `org.tron.core.exception.InvalidHttpParameterException`, which contradicts this PR's handling of the same exception in `Util.processError`/`clientMessage` (clean message, no class prefix) used by the other affected servlets. Add an explicit catch for `InvalidHttpParameterException` before the generic one, returning just `e.getMessage()`, so the validation message is surfaced without the internal FQN.</violation>
</file>

<file name="framework/src/test/java/org/tron/core/WalletContractAddressLoggingTest.java">

<violation number="1" location="framework/src/test/java/org/tron/core/WalletContractAddressLoggingTest.java:37">
P3: The test only proves Base58 is never touched; it never asserts the log output that the PR's safety property is about. `assertNull(…getContract(…))` and `getContractInfo` are true on any null-account path, so this test passes even if the account-missing log line is deleted entirely, and it also passes if `getAddressString` regressed to logging the full oversized value (e.g. `ByteArray.toHexString(address)` for all 32 KB) — the same log-bloat/DoS the change is meant to prevent. Capture the logged message (e.g. a `ListAppender` on the Wallet logger) and assert it is bounded and contains the `invalid-address(...len=...)` form, or capture the string returned per call and assert bound + absence of the raw value.</violation>
</file>

<sub>Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.

View guided diff | Turn on auto-fix | Re-trigger cubic</sub>

[cubic-dev-ai]

P3: This catch-all around Util.getAddress routes every exception into the debug-level client-error path, but getAddress already converts all expected request-input failures to InvalidHttpParameterException (blank/missing address, malformed hex or base58, unparseable JSON body). Anything else that escapes it — e.g. an IOException from reading the POST body in getRequestValue, or an unexpected runtime failure — is a genuine server-side fault, and Util.processError now hides it at DEBUG while reporting it to the client as a parameter error. Per the new Util.processServerError contract, server-side failures should be logged at ERROR. Catch InvalidHttpParameterException in the parsing block and let unexpected exceptions hit the second catch (single try block with both catch clauses). GetRewardServlet has the identical issue.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At framework/src/main/java/org/tron/core/services/http/GetBrokerageServlet.java, line 22:

<comment>This catch-all around `Util.getAddress` routes every exception into the debug-level client-error path, but `getAddress` already converts all expected request-input failures to `InvalidHttpParameterException` (blank/missing address, malformed hex or base58, unparseable JSON body). Anything else that escapes it — e.g. an `IOException` from reading the POST body in `getRequestValue`, or an unexpected runtime failure — is a genuine server-side fault, and `Util.processError` now hides it at DEBUG while reporting it to the client as a parameter error. Per the new `Util.processServerError` contract, server-side failures should be logged at ERROR. Catch `InvalidHttpParameterException` in the parsing block and let unexpected exceptions hit the second catch (single try block with both catch clauses). `GetRewardServlet` has the identical issue.</comment>

<file context>
@@ -17,18 +16,19 @@ public class GetBrokerageServlet extends RateLimiterServlet {
+    byte[] address;
+    try {
+      address = Util.getAddress(request);
+    } catch (Exception e) {
+      Util.processError(e, response);
+      return;
</file context>

[cubic-dev-ai]

P3: These throws are still swallowed by the generic catch (Exception e) below, which formats the response as e.getClass() + " : " + errString. The only visible effect of swapping the type is that the OTHER_ERROR message now exposes the internal class name org.tron.core.exception.InvalidHttpParameterException, which contradicts this PR's handling of the same exception in Util.processError/clientMessage (clean message, no class prefix) used by the other affected servlets. Add an explicit catch for InvalidHttpParameterException before the generic one, returning just e.getMessage(), so the validation message is surfaced without the internal FQN.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At framework/src/main/java/org/tron/core/services/http/TriggerSmartContractServlet.java, line 39:

<comment>These throws are still swallowed by the generic `catch (Exception e)` below, which formats the response as `e.getClass() + " : " + errString`. The only visible effect of swapping the type is that the OTHER_ERROR message now exposes the internal class name `org.tron.core.exception.InvalidHttpParameterException`, which contradicts this PR's handling of the same exception in `Util.processError`/`clientMessage` (clean message, no class prefix) used by the other affected servlets. Add an explicit catch for `InvalidHttpParameterException` before the generic one, returning just `e.getMessage()`, so the validation message is surfaced without the internal FQN.</comment>

<file context>
@@ -36,10 +36,10 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) {
     JSONObject jsonObject = JSONObject.parseObject(contract);
     if (StringUtil.isNullOrEmpty(jsonObject.getString(Util.OWNER_ADDRESS))) {
-      throw new InvalidParameterException(Util.OWNER_ADDRESS + " isn't set.");
+      throw new InvalidHttpParameterException(Util.OWNER_ADDRESS + " isn't set.");
     }
     if (StringUtil.isNullOrEmpty(jsonObject.getString(Util.CONTRACT_ADDRESS))) {
</file context>

[cubic-dev-ai]

P3: The test only proves Base58 is never touched; it never asserts the log output that the PR's safety property is about. assertNull(…getContract(…)) and getContractInfo are true on any null-account path, so this test passes even if the account-missing log line is deleted entirely, and it also passes if getAddressString regressed to logging the full oversized value (e.g. ByteArray.toHexString(address) for all 32 KB) — the same log-bloat/DoS the change is meant to prevent. Capture the logged message (e.g. a ListAppender on the Wallet logger) and assert it is bounded and contains the invalid-address(...len=...) form, or capture the string returned per call and assert bound + absence of the raw value.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At framework/src/test/java/org/tron/core/WalletContractAddressLoggingTest.java, line 37:

<comment>The test only proves Base58 is never touched; it never asserts the log output that the PR's safety property is about. `assertNull(…getContract(…))` and `getContractInfo` are true on any null-account path, so this test passes even if the account-missing log line is deleted entirely, and it also passes if `getAddressString` regressed to logging the full oversized value (e.g. `ByteArray.toHexString(address)` for all 32 KB) — the same log-bloat/DoS the change is meant to prevent. Capture the logged message (e.g. a `ListAppender` on the Wallet logger) and assert it is bounded and contains the `invalid-address(...len=...)` form, or capture the string returned per call and assert bound + absence of the raw value.</comment>

<file context>
@@ -0,0 +1,41 @@
+      try (MockedStatic<Base58> base58 = mockStatic(Base58.class)) {
+        assertNull(wallet.getContract(message));
+        assertNull(wallet.getContractInfo(message));
+        base58.verifyNoInteractions();
+      }
+    }
</file context>

[github-actions]

❌ Math Usage Detection Results

Found forbidden usage of java.lang.Math in the following files:

./chainbase/src/main/java/org/tron/common/utils/WalletUtil.java

Please review if this usage is intended.

Caution

Note: You should use org.tron.common.math.StrictMathWrapper.
If you need to use java.lang.Math, please provide a justification.