Add test for RLP when encoding a payload with length >= 2**64#6343
Add test for RLP when encoding a payload with length >= 2**64#6343ernestognw wants to merge 1 commit intoOpenZeppelin:masterfrom
Conversation
|
The latest updates on your security scan. Learn more about OpenZeppelin Platform.
|
|
WalkthroughA new test case was added to Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/utils/RLP.test.js`:
- Around line 208-216: The test's description ("length >= 2**64") doesn't match
the value used (MAX_UINT64 == 2**64 - 1) in the RLP.test.js case; either update
the forged length to test the >= 2**64 boundary by using MAX_UINT64 + 1 (so the
encoded length is >= 2**64) in the data construction that uses MAX_UINT64, or
keep the current numeric value and change the it(...) title/description to
accurately state "length == 2**64 - 1" or "length close to 2**64"; locate the
test by the it(...) string and the usage of MAX_UINT64 in the data variable to
apply the fix.
| it('reverts out of gas when encoding payload with length >= 2**64', async function () { | ||
| const data = ethers.concat([ | ||
| this.mock.interface.getFunction('$encode(bytes)').selector, | ||
| ethers.AbiCoder.defaultAbiCoder().encode(['uint256'], ['0x20']), // offset to bytes | ||
| ethers.toBeHex(MAX_UINT64, 32), // forged length | ||
| ethers.AbiCoder.defaultAbiCoder().encode(['uint256'], [56]), // long-form path (length > 55) | ||
| ]); | ||
| await expect(this.mock.runner.sendTransaction({ to: this.mock.target, data })).to.be.revertedWithoutReason(); | ||
| }); |
There was a problem hiding this comment.
Value mismatch: MAX_UINT64 is 2^64 - 1, not >= 2^64.
The test description and PR title reference "length >= 2**64", but MAX_UINT64 equals 2^64 - 1, which is strictly less than 2^64. If the intent is to test the boundary condition at >= 2^64, consider using MAX_UINT64 + 1n:
- ethers.toBeHex(MAX_UINT64, 32), // forged length
+ ethers.toBeHex(MAX_UINT64 + 1n, 32), // forged length >= 2**64If MAX_UINT64 is the intended value (testing the max 64-bit length), then the test description should be updated to reflect "length close to 2^64" or "length == 2^64 - 1".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('reverts out of gas when encoding payload with length >= 2**64', async function () { | |
| const data = ethers.concat([ | |
| this.mock.interface.getFunction('$encode(bytes)').selector, | |
| ethers.AbiCoder.defaultAbiCoder().encode(['uint256'], ['0x20']), // offset to bytes | |
| ethers.toBeHex(MAX_UINT64, 32), // forged length | |
| ethers.AbiCoder.defaultAbiCoder().encode(['uint256'], [56]), // long-form path (length > 55) | |
| ]); | |
| await expect(this.mock.runner.sendTransaction({ to: this.mock.target, data })).to.be.revertedWithoutReason(); | |
| }); | |
| it('reverts out of gas when encoding payload with length >= 2**64', async function () { | |
| const data = ethers.concat([ | |
| this.mock.interface.getFunction('$encode(bytes)').selector, | |
| ethers.AbiCoder.defaultAbiCoder().encode(['uint256'], ['0x20']), // offset to bytes | |
| ethers.toBeHex(MAX_UINT64 + 1n, 32), // forged length >= 2**64 | |
| ethers.AbiCoder.defaultAbiCoder().encode(['uint256'], [56]), // long-form path (length > 55) | |
| ]); | |
| await expect(this.mock.runner.sendTransaction({ to: this.mock.target, data })).to.be.revertedWithoutReason(); | |
| }); |
🤖 Prompt for AI Agents
In `@test/utils/RLP.test.js` around lines 208 - 216, The test's description
("length >= 2**64") doesn't match the value used (MAX_UINT64 == 2**64 - 1) in
the RLP.test.js case; either update the forged length to test the >= 2**64
boundary by using MAX_UINT64 + 1 (so the encoded length is >= 2**64) in the data
construction that uses MAX_UINT64, or keep the current numeric value and change
the it(...) title/description to accurately state "length == 2**64 - 1" or
"length close to 2**64"; locate the test by the it(...) string and the usage of
MAX_UINT64 in the data variable to apply the fix.
| this.mock.interface.getFunction('$encode(bytes)').selector, | ||
| ethers.AbiCoder.defaultAbiCoder().encode(['uint256'], ['0x20']), // offset to bytes | ||
| ethers.toBeHex(MAX_UINT64, 32), // forged length | ||
| ethers.AbiCoder.defaultAbiCoder().encode(['uint256'], [56]), // long-form path (length > 55) |
There was a problem hiding this comment.
Technically, this would be a valid input that would correspond to a buffer of size 264, and not 264-1.
| this.mock.interface.getFunction('$encode(bytes)').selector, | |
| ethers.AbiCoder.defaultAbiCoder().encode(['uint256'], ['0x20']), // offset to bytes | |
| ethers.toBeHex(MAX_UINT64, 32), // forged length | |
| ethers.AbiCoder.defaultAbiCoder().encode(['uint256'], [56]), // long-form path (length > 55) | |
| this.mock.interface.getFunction('$encode(bytes)').selector, | |
| ethers.toBeHex('0x20, 32), // offset to bytes | |
| ethers.toBeHex(MAX_UINT64 + 1n, 32), // forged length |
Would still not properly test the function, because it OOG in the calldata to memory copy BEFORE the any of the encode(bytes) internal logic is reached.
|
Closing. After discussing we agreed that the test is not relevant for the code we want to test because the function is not reached and Solidity reverts copying the calldata to memory. |
2 participants
Fixes #????
PR Checklist
npx changeset add)