Audit Fixes for RLP library on Broadcaster Audit

[luiz-lvj]

PR Checklist

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

[changeset-bot]

⚠️ No Changeset found

Latest commit: e27858d

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 RLP library in contracts/utils/RLP.sol was refactored with several changes: the encode(address) function now delegates to encode(uint256) instead of using inline assembly; readAddress removed explicit length validation and delegates to readUint256; readBytes documentation clarified about length validation handling; _decodeLength return signature changed from named variables to unnamed tuples; encode(bytes[] memory) received updated documentation about pre-encoded elements; and encode(Encoder) had formatting alignment adjustments.

Possibly related PRs

• Add RLP library #5680: Directly modifies functions and signatures in contracts/utils/RLP.sol including encode(address), readAddress, and _decodeLength.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Audit Fixes for RLP library on Broadcaster Audit' directly relates to the changeset, which contains modifications to the RLP.sol utility library addressing audit findings.
Description check	✅ Passed	The description contains a PR checklist related to standard merge requirements (Tests, Documentation, Changeset entry), which is relevant to the pull request process and changeset context.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6308fdc and 55b33a0.

📒 Files selected for processing (1)

• contracts/utils/RLP.sol (4 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: coverage
• GitHub Check: tests-foundry
• GitHub Check: slither
• GitHub Check: tests-upgradeable
• GitHub Check: tests
• GitHub Check: halmos
• GitHub Check: Pages changed - solidity-contracts

[Amxx]

This encoding is not consistent with how the ethereum ecosystem does things.

If you try to use ethers.js to do encode an address that had zeros, you'll get them encoded

> ethers.encodeRlp(ethers.ZeroAddress);
'0x940000000000000000000000000000000000000000'

Note that this difference is important in the context of operations like getCreateAddress. If the address prefix zeros are removed, you'd get an invalid address prediction.

I'll add unit tests to show these issues.

[ernestognw]

I mentioned to the auditors that it makes sense to include leading zeroes in addresses because there's no definition of an address in RLP. My thinking about this is that leading zeroes in an address are not necessarily "leading" because they constitute relevant information for the address.

[Amxx]

My thinking about this is that leading zeroes in an address are not necessarily "leading" because they constitute relevant information for the address.

I agree with that!

[Amxx]

Added tests that demonstrate the breaking effect of these changes. You can see on #6107 that these tests pass on master.

---

If someone wants to encode an Address without the leading zeros, they can manually do the casting to uint256 and then call the corresponding encode function. However I believe this should not be the default encoding. In particular, predicting the addresses of contracts created using the CREATE mechanism requires the addresses to have the prefix zeros. IMO the library must offer a function that behaves that way, and I think the encode(address) is the one.

[GianfrancoBazzani]

Adding some observations about the Solidity RLP implementation that should be considered:

1. Right now there is no clear reference implementation for this library to compare against, the comments in the code state that it is inspired on succinctlabs/optimism-bedrock-contracts, while @Amxx uses ethers.js encoding as example but in both ethers.js and viem there is not special treatment for this types and they are encoded as the raw 20 bytes.

2. The specification in the Yellow Paper clearly states that scalar values with leading zeroes should be treated as invalid RLP encodings. If we take go-ethereum rlp as the reference implementation, we don’t have an address type encoding, but we do have uint256 encoding defined. In this Solidity implementation, the uint256 encoding function treats it as scalar value and is injective (as in go-ethereum), meaning that a uint256 maps to only one specific encoding without leading zero bytes. However, for decoding, Solidity’s uint256 RLP decoding is non-injective, as it accepts many different encodings with leading zero bytes that could lead to the same uint256. This is a divergence from both the spec and the a reference implementation. The bool type is also defined in the go-ethereum implementation, and this divergence is present there as well.

3. address and bytes32 types are decoded as uint256 so the non injective behavior is propagated also to that types. address in the case of the address could only happen with a certain values as the length of the rlp is constrained to 21 bytes or 1 byte.

4. There is an additional divergence between the OZ implementation and succinctlabs/optimism-bedrock-contracts on for the address type. The OZ implementation allows addresses to be decoded from a single-byte RLP encoding (for precompiles and zero addresses), while the succinctlabs/optimism-bedrock-contracts implementation treats any single-byte RLP encoding as the zero address.

Examples:

1. Example arbitrary uint256:

• Solidity RLP uint256 decoding of 0x88ab54a98ceb1f0ad2: 12345678901234567890
• Go-ethereum RLP uint256 decoding of 0x88ab54a98ceb1f0ad2: 12345678901234567890
• Solidity RLP uint256 decoding of 0x8900ab54a98ceb1f0ad2: 12345678901234567890
• Go-ethereum RLP uint256 decoding of 0x8900ab54a98ceb1f0ad2: "panic: rlp: non-canonical integer (leading zero bytes) for *uint256.Int"

2. Example of zero uint256:

• Solidity RLP uint256 decoding of 0x80: 0
• Go-ethereum RLP uint256 decoding of 0x80: 0
• Solidity RLP uint256 decoding of 0x820000: 0
• Go-ethereum RLP uint256 decoding of 0x820000: "panic: rlp: non-canonical integer (leading zero bytes) for *uint256.Int"

3. Example bool:

• Solidity RLP bool decoding of 0x01: True
• Go-ethereum RLP bool decoding of 0x01: True
• Solidity RLP bool decoding of 0x02: True
• Go-ethereum RLP bol decoding of 0x02: "panic: rlp: invalid boolean value: 2"

3. Example with address:

• OpenZeppelin RLP address decoding of 0x01: 0x0000000000000000000000000000000000000001
• OpenZeppelin RLP address decoding of 0x940000000000000000000000000000000000000001: 0x0000000000000000000000000000000000000001

4. Example with bytes32:

• OpenZeppelin RLP address decoding of 0x8204d2: 0x00000000000000000000000000000000000000000000000000000000000004d2
  OpenZeppelin RLP address decoding of 0x830004d2: 0x00000000000000000000000000000000000000000000000000000000000004d2

5. Example differences succinctlabs/optimism-bedrock-contracts with :

• OpenZeppelin RLP address decoding of 0x01: 0x0000000000000000000000000000000000000001
• optimism-bedrock RLP address decoding of 0x01: 0x0000000000000000000000000000000000000000

[GianfrancoBazzani]

@ernestognw to keep the consistency I think the following instances of memory manipulation should use same syntax.

• openzeppelin-contracts/contracts/utils/RLP.sol

  Line 144 in e27858d

  mstore(result, 1) // length of the encoded data: 1 byte

• openzeppelin-contracts/contracts/utils/RLP.sol

  Line 118 in e27858d

  mstore(result, 0x01) // length of the encoded data: 1 byte