From 535b54da5967f0fcfe06cf4eb9b1805c5d3e8681 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Fri, 13 Dec 2024 12:46:04 -0600 Subject: [PATCH] Rename `arrayLengthPointer` to `arrayLengthOffset` and add changeset (#5371) --- .changeset/seven-insects-taste.md | 5 +++++ contracts/account/utils/draft-ERC7579Utils.sol | 16 ++++++++-------- 2 files changed, 13 insertions(+), 8 deletions(-) create mode 100644 .changeset/seven-insects-taste.md diff --git a/.changeset/seven-insects-taste.md b/.changeset/seven-insects-taste.md new file mode 100644 index 000000000..bfa8737d7 --- /dev/null +++ b/.changeset/seven-insects-taste.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`ERC7579Utils`: Add ABI decoding checks on calldata bounds within `decodeBatch` diff --git a/contracts/account/utils/draft-ERC7579Utils.sol b/contracts/account/utils/draft-ERC7579Utils.sol index c672fb279..8be094e64 100644 --- a/contracts/account/utils/draft-ERC7579Utils.sol +++ b/contracts/account/utils/draft-ERC7579Utils.sol @@ -182,14 +182,14 @@ 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. @@ -197,13 +197,13 @@ library ERC7579Utils { // - 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 } }