Validate long item length before loading lengthLength

[Amxx]

Fixes L- 22

The RLP library provides decoding helpers such as decodeBytes() and readBytes(), which all rely on _decodeLength() to interpret an RLP item prefix.

For long strings and long lists, _decodeLength() computes lengthLength and immediately loads lenChunk from item.load(1) (long string, long list). If the slice is too short to hold 1 + lengthLength bytes (for example, a single-byte 0xb8 or 0xf8), Memory.Slice.load() triggers a revert via Panic.ARRAY_OUT_OF_BOUNDS before the subsequent require(..., RLPInvalidEncoding()) checks execute. Consumers that treat RLPInvalidEncoding() as a recoverable validation failure (for example, via try/catch) may instead receive an unexpected Panic revert, increasing denial-of-service surface.

Consider validating itemLength before reading the length-of-length field (for example, require(itemLength > lengthLength, RLPInvalidEncoding()) before item.load(1)) in both long branches, so malformed inputs consistently fail with RLPInvalidEncoding().

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

⚠️ No Changeset found

Latest commit: a6630d8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[coderabbitai]

Walkthrough

The pull request adds a validity check in the _decodeLength function of the RLP library. Specifically, it introduces a require(itemLength > 1, RLPInvalidEncoding()); statement in both the "long string (>55 bytes)" and "long list" branches. This check ensures the encoded item is sufficiently long before attempting to access the length chunk at offset 1.

Possibly related PRs

• Fix byte lookup when decoding length of RLP structure #6051: Modifies RLP._decodeLength validations to guard the length-byte access at offset 1 in the same function

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately reflects the main change: adding validation for long item length before accessing the lengthLength field in RLP decoding.
Description check	✅ Passed	The description clearly explains the issue (L-22), the root cause, and the proposed fix, all directly related to the changeset.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

✅ Unit Test PR creation complete.

• Create PR with unit tests
• Commit unit tests in branch fix/L-22

📝 Coding Plan

• Generate coding plan for human review comments

---

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

[coderabbitai]

🧹 Nitpick comments (2)

contracts/utils/RLP.sol (2)

455-466: Consider using require(itemLength > lengthLength, ...) for a more precise check.

The added check require(itemLength > 1, ...) at line 458 correctly prevents the panic when itemLength = 1 by ensuring offset 1 is valid before item.load(1). However, this check is conservative and partially redundant with the check at line 460.

Since lengthLength is computed at line 456, you could use require(itemLength > lengthLength, ...) instead of require(itemLength > 1, ...). This would:

• Prevent panics for all insufficient length cases (not just when itemLength = 1)
• Eliminate redundancy with the itemLength > lengthLength check at line 460
• Align with the PR objectives which mention "require(itemLength > lengthLength, RLPInvalidEncoding())"

♻️ More precise validation

             // Case: Long string (>55 bytes)
             uint256 lengthLength = prefix - SHORT_OFFSET - SHORT_THRESHOLD;

-            require(itemLength > 1, RLPInvalidEncoding());
+            require(itemLength > lengthLength, RLPInvalidEncoding());
             bytes32 lenChunk = item.load(1);
-            require(itemLength > lengthLength && bytes1(lenChunk) != 0x00, RLPInvalidEncoding());
+            require(bytes1(lenChunk) != 0x00, RLPInvalidEncoding());

             uint256 len = uint256(lenChunk) >> (256 - 8 * lengthLength);
             require(len > SHORT_THRESHOLD && itemLength > lengthLength + len, RLPInvalidEncoding());

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@contracts/utils/RLP.sol` around lines 455 - 466, Replace the conservative
check require(itemLength > 1, RLPInvalidEncoding()) with a length-aware
validation that ensures the buffer is long enough to read the length bytes: use
require(itemLength > lengthLength, RLPInvalidEncoding()). This keeps the
existing subsequent checks (the later require(itemLength > lengthLength &&
bytes1(lenChunk) != 0x00, RLPInvalidEncoding()) and require(len >
SHORT_THRESHOLD && itemLength > lengthLength + len, RLPInvalidEncoding()))
intact, but prevents the earlier potential panic from item.load(1) by validating
against the computed lengthLength first; update the check in the long-string
branch (where lengthLength is calculated from prefix, SHORT_OFFSET and
SHORT_THRESHOLD) and keep RLPInvalidEncoding as the error.

---

475-486: Consider using require(itemLength > lengthLength, ...) for a more precise check.

Similar to the long string case, the added check require(itemLength > 1, ...) at line 478 correctly prevents the panic when itemLength = 1. However, using require(itemLength > lengthLength, ...) would be more precise and eliminate redundancy with line 480.

♻️ More precise validation

             // Case: Long list
             uint256 lengthLength = prefix - LONG_OFFSET - SHORT_THRESHOLD;

-            require(itemLength > 1, RLPInvalidEncoding());
+            require(itemLength > lengthLength, RLPInvalidEncoding());
             bytes32 lenChunk = item.load(1);
-            require(itemLength > lengthLength && bytes1(lenChunk) != 0x00, RLPInvalidEncoding());
+            require(bytes1(lenChunk) != 0x00, RLPInvalidEncoding());

             uint256 len = uint256(lenChunk) >> (256 - 8 * lengthLength);
             require(len > SHORT_THRESHOLD && itemLength > lengthLength + len, RLPInvalidEncoding());

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@contracts/utils/RLP.sol` around lines 475 - 486, Replace the broad sanity
check require(itemLength > 1, RLPInvalidEncoding()) with a precise bounds check
require(itemLength > lengthLength, RLPInvalidEncoding()) before loading lenChunk
so we ensure the prefix actually contains the full length-of-length bytes; then
remove the redundant itemLength > lengthLength part from the subsequent
require(...) that currently also checks bytes1(lenChunk) != 0x00, keeping the
existing RLPInvalidEncoding() error path and leaving variables lengthLength,
itemLength, lenChunk and the return of ItemType.List unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@contracts/utils/RLP.sol`:
- Around line 455-466: Replace the conservative check require(itemLength > 1,
RLPInvalidEncoding()) with a length-aware validation that ensures the buffer is
long enough to read the length bytes: use require(itemLength > lengthLength,
RLPInvalidEncoding()). This keeps the existing subsequent checks (the later
require(itemLength > lengthLength && bytes1(lenChunk) != 0x00,
RLPInvalidEncoding()) and require(len > SHORT_THRESHOLD && itemLength >
lengthLength + len, RLPInvalidEncoding())) intact, but prevents the earlier
potential panic from item.load(1) by validating against the computed
lengthLength first; update the check in the long-string branch (where
lengthLength is calculated from prefix, SHORT_OFFSET and SHORT_THRESHOLD) and
keep RLPInvalidEncoding as the error.
- Around line 475-486: Replace the broad sanity check require(itemLength > 1,
RLPInvalidEncoding()) with a precise bounds check require(itemLength >
lengthLength, RLPInvalidEncoding()) before loading lenChunk so we ensure the
prefix actually contains the full length-of-length bytes; then remove the
redundant itemLength > lengthLength part from the subsequent require(...) that
currently also checks bytes1(lenChunk) != 0x00, keeping the existing
RLPInvalidEncoding() error path and leaving variables lengthLength, itemLength,
lenChunk and the return of ItemType.List unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 7c87bc88-2493-4431-9cb0-10dc0e6f89a9

📥 Commits

Reviewing files that changed from the base of the PR and between 45f032d and 6693d66.

📒 Files selected for processing (1)

• contracts/utils/RLP.sol

[coderabbitai]

Note

Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it.

---

Generating unit tests... This may take up to 20 minutes.

[coderabbitai]

Request timed out after 900000ms (requestId=dffe8aea-a338-4806-bb56-e294dd6501a3)