Skip to content

Commit

Permalink
Rename arrayLengthPointer to arrayLengthOffset and add changeset (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
ernestognw authored Dec 13, 2024
1 parent a71f79f commit 535b54d
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 8 deletions.
5 changes: 5 additions & 0 deletions .changeset/seven-insects-taste.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': patch
---

`ERC7579Utils`: Add ABI decoding checks on calldata bounds within `decodeBatch`
16 changes: 8 additions & 8 deletions contracts/account/utils/draft-ERC7579Utils.sol
Original file line number Diff line number Diff line change
Expand Up @@ -182,28 +182,28 @@ library ERC7579Utils {
if (bufferLength < 32) revert ERC7579DecodingError();

// Get the offset of the array (pointer to the array length).
uint256 arrayLengthPointer = uint256(bytes32(executionCalldata[0:32]));
uint256 arrayLengthOffset = uint256(bytes32(executionCalldata[0:32]));

// The array length (at arrayLengthPointer) should be 32 bytes long. We check that this is within the
// The array length (at arrayLengthOffset) should be 32 bytes long. We check that this is within the
// buffer bounds. Since we know bufferLength is at least 32, we can subtract with no overflow risk.
if (arrayLengthPointer > bufferLength - 32) revert ERC7579DecodingError();
if (arrayLengthOffset > bufferLength - 32) revert ERC7579DecodingError();

// Get the array length. arrayLengthPointer + 32 is bounded by bufferLength so it does not overflow.
uint256 arrayLength = uint256(bytes32(executionCalldata[arrayLengthPointer:arrayLengthPointer + 32]));
// Get the array length. arrayLengthOffset + 32 is bounded by bufferLength so it does not overflow.
uint256 arrayLength = uint256(bytes32(executionCalldata[arrayLengthOffset:arrayLengthOffset + 32]));

// Check that the buffer is long enough to store the array elements as "offset pointer":
// - each element of the array is an "offset pointer" to the data.
// - each "offset pointer" (to an array element) takes 32 bytes.
// - validity of the calldata at that location is checked when the array element is accessed, so we only
// need to check that the buffer is large enough to hold the pointers.
//
// Since we know bufferLength is at least arrayLengthPointer + 32, we can subtract with no overflow risk.
// Since we know bufferLength is at least arrayLengthOffset + 32, we can subtract with no overflow risk.
// Solidity limits length of such arrays to 2**64-1, this guarantees `arrayLength * 32` does not overflow.
if (arrayLength > type(uint64).max || bufferLength - arrayLengthPointer - 32 < arrayLength * 32)
if (arrayLength > type(uint64).max || bufferLength - arrayLengthOffset - 32 < arrayLength * 32)
revert ERC7579DecodingError();

assembly ("memory-safe") {
executionBatch.offset := add(add(executionCalldata.offset, arrayLengthPointer), 32)
executionBatch.offset := add(add(executionCalldata.offset, arrayLengthOffset), 32)
executionBatch.length := arrayLength
}
}
Expand Down

0 comments on commit 535b54d

Please sign in to comment.