Skip to content

Conversation

@Amxx
Copy link
Collaborator

@Amxx Amxx commented Nov 4, 2025

No description provided.

@Amxx Amxx added this to the 5.6 milestone Nov 4, 2025
@Amxx Amxx requested a review from a team as a code owner November 4, 2025 13:18
@changeset-bot
Copy link

changeset-bot bot commented Nov 4, 2025

🦋 Changeset detected

Latest commit: 3e96235

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Patch

Not sure what this means? Click here to learn what changesets are.

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 4, 2025

Walkthrough

The changes modify RLP decoding logic in the RLP.sol utility contract. In the long-string validity check, the memory offset accessed for the zero-value guard is adjusted from position 0 to position 1. Similarly, in the long-list validity check, two separate validation conditions are consolidated into a single combined check that also reads from memory offset 1 instead of 0. The overall effect is a change in which memory offset is evaluated and a unification of boundary checks into a single require statement for both cases.

Suggested labels

bug

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive No description was provided by the author, making it impossible to assess whether the description relates to the changeset. Add a pull request description explaining the purpose of the RLP decoding changes and why the byte lookup offset adjustment is necessary.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Fix byte lookup when decoding length of RLP structure' accurately describes the main change: adjusting which memory offset is read during RLP length validation.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 04e683b and d3c84f5.

📒 Files selected for processing (1)
  • contracts/utils/RLP.sol (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: Redirect rules - solidity-contracts
  • GitHub Check: Header rules - solidity-contracts
  • GitHub Check: slither
  • GitHub Check: coverage
  • GitHub Check: tests-upgradeable
  • GitHub Check: tests-foundry
  • GitHub Check: tests
  • GitHub Check: Pages changed - solidity-contracts
  • GitHub Check: halmos
🔇 Additional comments (2)
contracts/utils/RLP.sol (2)

354-354: Correct fix: validating the first length byte instead of the prefix.

This change properly validates that the first byte of the length encoding (at position 1) is not 0x00, preventing non-canonical RLP with leading zeros in the length field. The previous check at position 0 was incorrect because it examined the prefix byte, which can never be 0x00 in the long-string case (prefix range: 0xB8-0xBF). This fix improves RLP specification compliance by ensuring canonical encoding.


372-372: Correct fix: validating the first length byte instead of the prefix.

This change mirrors the long-string fix, properly checking that the first byte of the length encoding (at position 1) is not 0x00. The previous check at position 0 was incorrect because the prefix byte can never be 0x00 in the long-list case (prefix range: 0xF8-0xFF). Additionally, consolidating the two separate checks into a single combined require statement improves gas efficiency while maintaining the same validation logic.


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

'openzeppelin-solidity': patch
---

`RLP`: Fix RLP encoding validity check when decoding long lists or strings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe include in a ### Bug fixes section of the changelog?

@Amxx Amxx merged commit 3f37ada into OpenZeppelin:master Nov 10, 2025
21 checks passed
@Amxx Amxx deleted the fix/RLP-decode-length-check branch November 10, 2025 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants