From a727fe5d63ac8b7241e30d10d7564640050b466b Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 23 Dec 2024 18:32:20 +0100 Subject: [PATCH 1/7] Perform sanity check if executionCalldata is not at the end of allocated calldata --- .../account/utils/draft-ERC7579Utils.sol | 41 +++++++ test/account/utils/draft-ERC7579Utils.t.sol | 115 +++++++++++++----- 2 files changed, 126 insertions(+), 30 deletions(-) diff --git a/contracts/account/utils/draft-ERC7579Utils.sol b/contracts/account/utils/draft-ERC7579Utils.sol index 8be094e649d..b235ef6715e 100644 --- a/contracts/account/utils/draft-ERC7579Utils.sol +++ b/contracts/account/utils/draft-ERC7579Utils.sol @@ -206,6 +206,47 @@ library ERC7579Utils { executionBatch.offset := add(add(executionCalldata.offset, arrayLengthOffset), 32) executionBatch.length := arrayLength } + + // [ Sanity check ] + // Solidity performs "lazy" verification that all calldata objects are valid, by checking that they are + // within calldatasize. This check is performed when objects are dereferenced. If the `executionCalldata` + // is not the last element (buffer) in msg.data, this check will not detect potentially ill-formed objects + // that point to the memory space between the end of the `executionCalldata` buffer. + // If we are in a situation where the lazy checks are not sufficient, we do an in-depth traversal of the + // array, checking that everything is valid. + uint256 bufferEnd; + assembly ("memory-safe") { + bufferEnd := add(executionCalldata.offset, bufferLength) + } + + if (bufferEnd < msg.data.length) { + uint256 arrayPtr = arrayLengthOffset + 32; + for (uint256 i = 0; i < arrayLength; ++i) { + // location of the element pointer in the array + uint256 elementPtr = arrayPtr + i * 32; + + // location of the element data (arrayPtr + offset stored at elementPtr) + uint256 elementDataPtr = arrayPtr + uint256(bytes32(executionCalldata[elementPtr:elementPtr + 32])); + + // check that element data fits in the buffer (element has length 0x60 = 96) + if (bufferLength < elementDataPtr + 96) revert ERC7579DecodingError(); + + // location of the element inner calldata (elementDataPtr + offset stored as elementDataPtr + 64) + uint256 elementContentPtr = elementDataPtr + + uint256(bytes32(executionCalldata[elementDataPtr + 64:elementDataPtr + 96])); + + // check that at length of element inner calldata fits in the buffer + if (bufferLength < elementContentPtr + 32) revert ERC7579DecodingError(); + + // length of the element inner calldata + uint256 elementContentLength = uint256( + bytes32(executionCalldata[elementContentPtr:elementContentPtr + 32]) + ); + + // check that all the data of the element inner calldata fits in the buffer + if (bufferLength < elementContentPtr + 32 + elementContentLength) revert ERC7579DecodingError(); + } + } } } diff --git a/test/account/utils/draft-ERC7579Utils.t.sol b/test/account/utils/draft-ERC7579Utils.t.sol index fdd4edf5958..f88397d7269 100644 --- a/test/account/utils/draft-ERC7579Utils.t.sol +++ b/test/account/utils/draft-ERC7579Utils.t.sol @@ -287,38 +287,57 @@ contract ERC7579UtilsTest is Test { _collectAndPrintLogs(true); } - function testDecodeBatch() public { - // BAD: buffer empty - vm.expectRevert(ERC7579Utils.ERC7579DecodingError.selector); - this.callDecodeBatch(""); + uint256 private constant TEST_DECODE = 0x01; + uint256 private constant TEST_GETFIRST = 0x02; + uint256 private constant TEST_GETFIRSTBYTES = 0x04; + uint256 private constant FAIL_DECODE = 0x10; + uint256 private constant FAIL_GETFIRST = 0x20; + uint256 private constant FAIL_GETFIRSTBYTES = 0x40; + uint256 private constant FAIL_ANY = FAIL_DECODE | FAIL_GETFIRST | FAIL_GETFIRSTBYTES; + + // BAD: buffer empty + function testDecodeBatchEmptyBuffer() public { + _testDecodeBatch("", TEST_DECODE | FAIL_DECODE); + } + + // BAD: buffer too short + function testDecodeBatchBufferTooShort() public { + _testDecodeBatch(abi.encodePacked(uint128(0)), TEST_DECODE | FAIL_DECODE); + } - // BAD: buffer too short - vm.expectRevert(ERC7579Utils.ERC7579DecodingError.selector); - this.callDecodeBatch(abi.encodePacked(uint128(0))); + // GOOD + function testDecodeBatchZero() public { + _testDecodeBatch(abi.encode(0), TEST_DECODE); - // GOOD - this.callDecodeBatch(abi.encode(0)); // Note: Solidity also supports this even though it's odd. Offset 0 means array is at the same location, which // is interpreted as an array of length 0, which doesn't require any more data // solhint-disable-next-line var-name-mixedcase uint256[] memory _1 = abi.decode(abi.encode(0), (uint256[])); _1; + } - // BAD: offset is out of bounds - vm.expectRevert(ERC7579Utils.ERC7579DecodingError.selector); - this.callDecodeBatch(abi.encode(1)); + // BAD: offset is out of bounds + function testDecodeBatchOffsetOutOfBound() public { + _testDecodeBatch(abi.encode(1), TEST_DECODE | FAIL_DECODE); + } - // GOOD - this.callDecodeBatch(abi.encode(32, 0)); + // GOOD + function testDecodeBatchEmptyArray() public { + _testDecodeBatch(abi.encode(32, 0), TEST_DECODE); + } - // BAD: reported array length extends beyond bounds - vm.expectRevert(ERC7579Utils.ERC7579DecodingError.selector); - this.callDecodeBatch(abi.encode(32, 1)); + // BAD: reported array length extends beyond bounds + function testDecodeBatchLengthExtendsOutOfBound() public { + _testDecodeBatch(abi.encode(32, 1), TEST_DECODE | FAIL_DECODE); + } - // GOOD - this.callDecodeBatch(abi.encode(32, 1, 0)); + // GOOD at first level, BAD when dereferencing + function testDecodeBatchTopLevelCheck() public { + _testDecodeBatch(abi.encode(32, 1, 0), TEST_DECODE | TEST_GETFIRST | FAIL_GETFIRST); + } - // GOOD + // GOOD + function testDecodeBatchFirstBytes() public view { // // 0000000000000000000000000000000000000000000000000000000000000020 (32) offset // 0000000000000000000000000000000000000000000000000000000000000001 ( 1) array length @@ -334,7 +353,10 @@ contract ERC7579UtilsTest is Test { abi.encode(32, 1, 32, _recipient1, 42, 96, 12, bytes12("Hello World!")) ) ); + } + // GOOD at first level, BAD when dereferencing + function testDecodeBatchTopLevelCheckBis() public { // This is invalid, the first element of the array points is out of bounds // but we allow it past initial validation, because solidity will validate later when the bytes field is accessed // @@ -342,11 +364,10 @@ contract ERC7579UtilsTest is Test { // 0000000000000000000000000000000000000000000000000000000000000001 ( 1) array length // 0000000000000000000000000000000000000000000000000000000000000020 (32) element 0 offset // - bytes memory invalid = abi.encode(32, 1, 32); - this.callDecodeBatch(invalid); - vm.expectRevert(); - this.callDecodeBatchAndGetFirst(invalid); + _testDecodeBatch(abi.encode(32, 1, 32), TEST_DECODE | TEST_GETFIRST | FAIL_GETFIRST); + } + function testDecodeBatchDeepOutOfBound() public { // this is invalid: the bytes field of the first element of the array is out of bounds // but we allow it past initial validation, because solidity will validate later when the bytes field is accessed // @@ -357,26 +378,60 @@ contract ERC7579UtilsTest is Test { // 000000000000000000000000000000000000000000000000000000000000002a (42) value for element #0 // 0000000000000000000000000000000000000000000000000000000000000060 (96) offset to calldata for element #0 // - bytes memory invalidDeeply = abi.encode(32, 1, 32, _recipient1, 42, 96); - this.callDecodeBatch(invalidDeeply); - // Note that this is ok because we don't return the value. Returning it would introduce a check that would fails. - this.callDecodeBatchAndGetFirst(invalidDeeply); - vm.expectRevert(); - this.callDecodeBatchAndGetFirstBytes(invalidDeeply); + _testDecodeBatch( + abi.encode(32, 1, 32, _recipient1, 42, 96), + TEST_DECODE | TEST_GETFIRST | TEST_GETFIRSTBYTES | FAIL_GETFIRSTBYTES + ); + } + + function _testDecodeBatch(bytes memory encoded, uint256 test) private { + if (test & TEST_DECODE > 0) { + if (test & FAIL_DECODE > 0) vm.expectRevert(ERC7579Utils.ERC7579DecodingError.selector); + this.callDecodeBatch(encoded); + if (test & FAIL_ANY > 0) vm.expectRevert(ERC7579Utils.ERC7579DecodingError.selector); + this.callDecodeBatchWithCalldata(encoded, new bytes(256)); + } + if (test & TEST_GETFIRST > 0) { + if (test & FAIL_GETFIRST > 0) vm.expectRevert(); + this.callDecodeBatchAndGetFirst(encoded); + if (test & FAIL_ANY > 0) vm.expectRevert(); + this.callDecodeBatchAndGetFirstWithCalldata(encoded, new bytes(256)); + } + if (test & TEST_GETFIRSTBYTES > 0) { + if (test & FAIL_GETFIRSTBYTES > 0) vm.expectRevert(); + this.callDecodeBatchAndGetFirstBytes(encoded); + if (test & FAIL_ANY > 0) vm.expectRevert(); + this.callDecodeBatchAndGetFirstBytesWithCalldata(encoded, new bytes(256)); + } } function callDecodeBatch(bytes calldata executionCalldata) public pure { ERC7579Utils.decodeBatch(executionCalldata); } + function callDecodeBatchWithCalldata(bytes calldata executionCalldata, bytes calldata) public pure { + ERC7579Utils.decodeBatch(executionCalldata); + } + function callDecodeBatchAndGetFirst(bytes calldata executionCalldata) public pure { ERC7579Utils.decodeBatch(executionCalldata)[0]; } + function callDecodeBatchAndGetFirstWithCalldata(bytes calldata executionCalldata, bytes calldata) public pure { + ERC7579Utils.decodeBatch(executionCalldata)[0]; + } + function callDecodeBatchAndGetFirstBytes(bytes calldata executionCalldata) public pure returns (bytes calldata) { return ERC7579Utils.decodeBatch(executionCalldata)[0].callData; } + function callDecodeBatchAndGetFirstBytesWithCalldata( + bytes calldata executionCalldata, + bytes calldata + ) public pure returns (bytes calldata) { + return ERC7579Utils.decodeBatch(executionCalldata)[0].callData; + } + function hashUserOperation(PackedUserOperation calldata useroperation) public view returns (bytes32) { return useroperation.hash(address(ENTRYPOINT), block.chainid); } From 315bb5528efc06e7832c611438b70b71bb9e5235 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 23 Dec 2024 22:50:41 +0100 Subject: [PATCH 2/7] refactor math for safety --- .../account/utils/draft-ERC7579Utils.sol | 74 ++++++++++++------- 1 file changed, 46 insertions(+), 28 deletions(-) diff --git a/contracts/account/utils/draft-ERC7579Utils.sol b/contracts/account/utils/draft-ERC7579Utils.sol index b235ef6715e..d3c287006e1 100644 --- a/contracts/account/utils/draft-ERC7579Utils.sol +++ b/contracts/account/utils/draft-ERC7579Utils.sol @@ -176,7 +176,12 @@ library ERC7579Utils { /// NOTE: This function runs some checks and will throw a {ERC7579DecodingError} if the input is not properly formatted. function decodeBatch(bytes calldata executionCalldata) internal pure returns (Execution[] calldata executionBatch) { unchecked { - uint256 bufferLength = executionCalldata.length; + uint256 bufferPtr; + uint256 bufferLength; + assembly ("memory-safe") { + bufferPtr := executionCalldata.offset + bufferLength := executionCalldata.length + } // Check executionCalldata is not empty. if (bufferLength < 32) revert ERC7579DecodingError(); @@ -203,7 +208,7 @@ library ERC7579Utils { revert ERC7579DecodingError(); assembly ("memory-safe") { - executionBatch.offset := add(add(executionCalldata.offset, arrayLengthOffset), 32) + executionBatch.offset := add(add(bufferPtr, arrayLengthOffset), 32) executionBatch.length := arrayLength } @@ -214,37 +219,50 @@ library ERC7579Utils { // that point to the memory space between the end of the `executionCalldata` buffer. // If we are in a situation where the lazy checks are not sufficient, we do an in-depth traversal of the // array, checking that everything is valid. - uint256 bufferEnd; - assembly ("memory-safe") { - bufferEnd := add(executionCalldata.offset, bufferLength) - } - - if (bufferEnd < msg.data.length) { + if (bufferPtr + bufferLength < msg.data.length) { uint256 arrayPtr = arrayLengthOffset + 32; - for (uint256 i = 0; i < arrayLength; ++i) { - // location of the element pointer in the array - uint256 elementPtr = arrayPtr + i * 32; - // location of the element data (arrayPtr + offset stored at elementPtr) - uint256 elementDataPtr = arrayPtr + uint256(bytes32(executionCalldata[elementPtr:elementPtr + 32])); - - // check that element data fits in the buffer (element has length 0x60 = 96) - if (bufferLength < elementDataPtr + 96) revert ERC7579DecodingError(); - - // location of the element inner calldata (elementDataPtr + offset stored as elementDataPtr + 64) - uint256 elementContentPtr = elementDataPtr + - uint256(bytes32(executionCalldata[elementDataPtr + 64:elementDataPtr + 96])); - - // check that at length of element inner calldata fits in the buffer - if (bufferLength < elementContentPtr + 32) revert ERC7579DecodingError(); + for (uint256 i = 0; i < arrayLength; ++i) { + // Location of the element pointer in the array ... + // ... and offset stored at that location + // + // Cannot overflow: arrayPtr + arrayLength * 32 is bounded by bufferLength + uint256 elementPtrPtr = arrayPtr + i * 32; + uint256 elementOffset = uint256(bytes32(executionCalldata[elementPtrPtr:elementPtrPtr + 32])); + + // Check that element data fits in the buffer (element has length 0x60 = 96) + // We know that arrayPtr <= bufferLength, so "bufferLength - arrayPtr" is safe + if (elementOffset > type(uint64).max || bufferLength - arrayPtr < elementOffset + 96) + revert ERC7579DecodingError(); + + // Location of the element data (arrayPtr + offset stored at elementPtrPtr) ... + // ... and offset stored at that location + // + // Cannot overflow: arrayPtr + elementOffset + 96 is bounded by bufferLength + uint256 elementPtr = arrayPtr + elementOffset; + uint256 elementCalldataOffset = uint256( + bytes32(executionCalldata[elementPtr + 64:elementPtr + 96]) + ); - // length of the element inner calldata - uint256 elementContentLength = uint256( - bytes32(executionCalldata[elementContentPtr:elementContentPtr + 32]) + // Check that at length of element inner calldata fits in the buffer + // We know that elementPtr + 96 <= bufferLength, so "bufferLength - elementPtr" is safe + if ( + elementCalldataOffset > type(uint64).max || + bufferLength - elementPtr < elementCalldataOffset + 32 + ) revert ERC7579DecodingError(); + + // Location of the element inner calldata (elementPtr + offset stored as elementPtr + 64) ... + // ... and length of the calldata stored at the location + // + // Cannot overflow: elementPtr + elementCalldataOffset + 32 is bounded by bufferLength + uint256 elementCalldataPtr = elementPtr + elementCalldataOffset; + uint256 elementCalldataLength = uint256( + bytes32(executionCalldata[elementCalldataPtr:elementCalldataPtr + 32]) ); - // check that all the data of the element inner calldata fits in the buffer - if (bufferLength < elementContentPtr + 32 + elementContentLength) revert ERC7579DecodingError(); + // Check that all the data of the element inner calldata fits in the buffer + // We know that elementCalldataOffset + 32 <= bufferLength, so "bufferLength - elementCalldataPtr" - 32 is safe + if (bufferLength - elementCalldataPtr - 32 < elementCalldataLength) revert ERC7579DecodingError(); } } } From a93ca4d1a99d9e8f40f52a64fde8b5dd2969eb03 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 23 Dec 2024 22:52:37 +0100 Subject: [PATCH 3/7] cleanup --- contracts/account/utils/draft-ERC7579Utils.sol | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/contracts/account/utils/draft-ERC7579Utils.sol b/contracts/account/utils/draft-ERC7579Utils.sol index d3c287006e1..829c4c3c09a 100644 --- a/contracts/account/utils/draft-ERC7579Utils.sol +++ b/contracts/account/utils/draft-ERC7579Utils.sol @@ -223,9 +223,7 @@ library ERC7579Utils { uint256 arrayPtr = arrayLengthOffset + 32; for (uint256 i = 0; i < arrayLength; ++i) { - // Location of the element pointer in the array ... - // ... and offset stored at that location - // + // Location of the element pointer in the array, and offset stored at that location // Cannot overflow: arrayPtr + arrayLength * 32 is bounded by bufferLength uint256 elementPtrPtr = arrayPtr + i * 32; uint256 elementOffset = uint256(bytes32(executionCalldata[elementPtrPtr:elementPtrPtr + 32])); @@ -235,9 +233,7 @@ library ERC7579Utils { if (elementOffset > type(uint64).max || bufferLength - arrayPtr < elementOffset + 96) revert ERC7579DecodingError(); - // Location of the element data (arrayPtr + offset stored at elementPtrPtr) ... - // ... and offset stored at that location - // + // Location of the element data, and offset stored at that location // Cannot overflow: arrayPtr + elementOffset + 96 is bounded by bufferLength uint256 elementPtr = arrayPtr + elementOffset; uint256 elementCalldataOffset = uint256( @@ -251,9 +247,7 @@ library ERC7579Utils { bufferLength - elementPtr < elementCalldataOffset + 32 ) revert ERC7579DecodingError(); - // Location of the element inner calldata (elementPtr + offset stored as elementPtr + 64) ... - // ... and length of the calldata stored at the location - // + // Location of the element inner calldata, and length of the calldata stored at the location // Cannot overflow: elementPtr + elementCalldataOffset + 32 is bounded by bufferLength uint256 elementCalldataPtr = elementPtr + elementCalldataOffset; uint256 elementCalldataLength = uint256( From 3b01a7d8f264ac110ae3970de1c0cbbc1b58b1b0 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 23 Dec 2024 22:59:03 +0100 Subject: [PATCH 4/7] up --- test/account/utils/draft-ERC7579Utils.t.sol | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/test/account/utils/draft-ERC7579Utils.t.sol b/test/account/utils/draft-ERC7579Utils.t.sol index f88397d7269..e14d4f79812 100644 --- a/test/account/utils/draft-ERC7579Utils.t.sol +++ b/test/account/utils/draft-ERC7579Utils.t.sol @@ -385,23 +385,27 @@ contract ERC7579UtilsTest is Test { } function _testDecodeBatch(bytes memory encoded, uint256 test) private { + bytes memory extraData = new bytes(256); + if (test & TEST_DECODE > 0) { if (test & FAIL_DECODE > 0) vm.expectRevert(ERC7579Utils.ERC7579DecodingError.selector); this.callDecodeBatch(encoded); if (test & FAIL_ANY > 0) vm.expectRevert(ERC7579Utils.ERC7579DecodingError.selector); - this.callDecodeBatchWithCalldata(encoded, new bytes(256)); + this.callDecodeBatchWithCalldata(encoded, extraData); } + if (test & TEST_GETFIRST > 0) { - if (test & FAIL_GETFIRST > 0) vm.expectRevert(); + if (test & FAIL_GETFIRST > 0) vm.expectRevert(); // solidity failure without data this.callDecodeBatchAndGetFirst(encoded); - if (test & FAIL_ANY > 0) vm.expectRevert(); - this.callDecodeBatchAndGetFirstWithCalldata(encoded, new bytes(256)); + if (test & FAIL_ANY > 0) vm.expectRevert(ERC7579Utils.ERC7579DecodingError.selector); + this.callDecodeBatchAndGetFirstWithCalldata(encoded, extraData); } + if (test & TEST_GETFIRSTBYTES > 0) { - if (test & FAIL_GETFIRSTBYTES > 0) vm.expectRevert(); + if (test & FAIL_GETFIRSTBYTES > 0) vm.expectRevert(); // solidity failure without data this.callDecodeBatchAndGetFirstBytes(encoded); - if (test & FAIL_ANY > 0) vm.expectRevert(); - this.callDecodeBatchAndGetFirstBytesWithCalldata(encoded, new bytes(256)); + if (test & FAIL_ANY > 0) vm.expectRevert(ERC7579Utils.ERC7579DecodingError.selector); + this.callDecodeBatchAndGetFirstBytesWithCalldata(encoded, extraData); } } From 85b30a27b0ac7e4c74beb8a8c9ae5009dd39421c Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 24 Dec 2024 15:54:44 +0100 Subject: [PATCH 5/7] improve coverage --- test/account/utils/draft-ERC7579Utils.t.sol | 36 ++++++++++++++------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/test/account/utils/draft-ERC7579Utils.t.sol b/test/account/utils/draft-ERC7579Utils.t.sol index e14d4f79812..96f81a544c1 100644 --- a/test/account/utils/draft-ERC7579Utils.t.sol +++ b/test/account/utils/draft-ERC7579Utils.t.sol @@ -331,14 +331,8 @@ contract ERC7579UtilsTest is Test { _testDecodeBatch(abi.encode(32, 1), TEST_DECODE | FAIL_DECODE); } - // GOOD at first level, BAD when dereferencing - function testDecodeBatchTopLevelCheck() public { - _testDecodeBatch(abi.encode(32, 1, 0), TEST_DECODE | TEST_GETFIRST | FAIL_GETFIRST); - } - // GOOD function testDecodeBatchFirstBytes() public view { - // // 0000000000000000000000000000000000000000000000000000000000000020 (32) offset // 0000000000000000000000000000000000000000000000000000000000000001 ( 1) array length // 0000000000000000000000000000000000000000000000000000000000000020 (32) element 0 offset @@ -356,9 +350,19 @@ contract ERC7579UtilsTest is Test { } // GOOD at first level, BAD when dereferencing - function testDecodeBatchTopLevelCheckBis() public { + function testDecodeBatchDeepOutOfBound1() public { + // This is invalid, the first element of the array points is out of bounds + // + // 0000000000000000000000000000000000000000000000000000000000000020 (32) offset + // 0000000000000000000000000000000000000000000000000000000000000001 ( 1) array length + // 0000000000000000000000000000000000000000000000000000000000000000 ( 0) element 0 offset + // + _testDecodeBatch(abi.encode(32, 1, 0), TEST_DECODE | TEST_GETFIRST | FAIL_GETFIRST); + } + + // GOOD at first level, BAD when dereferencing + function testDecodeBatchDeepOutOfBound2() public { // This is invalid, the first element of the array points is out of bounds - // but we allow it past initial validation, because solidity will validate later when the bytes field is accessed // // 0000000000000000000000000000000000000000000000000000000000000020 (32) offset // 0000000000000000000000000000000000000000000000000000000000000001 ( 1) array length @@ -367,9 +371,19 @@ contract ERC7579UtilsTest is Test { _testDecodeBatch(abi.encode(32, 1, 32), TEST_DECODE | TEST_GETFIRST | FAIL_GETFIRST); } - function testDecodeBatchDeepOutOfBound() public { - // this is invalid: the bytes field of the first element of the array is out of bounds - // but we allow it past initial validation, because solidity will validate later when the bytes field is accessed + function testDecodeBatchDeepOutOfBound3() public { + // This is invalid: the first item is partly out of bounds. + // + // 0000000000000000000000000000000000000000000000000000000000000020 (32) offset + // 0000000000000000000000000000000000000000000000000000000000000001 ( 1) array length + // 0000000000000000000000000000000000000000000000000000000000000020 (32) element 0 offset + // 000000000000000000000000xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx (recipient) target for element #0 + // + _testDecodeBatch(abi.encode(32, 1, 32, _recipient1), TEST_DECODE | TEST_GETFIRST | FAIL_GETFIRST); + } + + function testDecodeBatchDeepOutOfBound4() public { + // This is invalid: the bytes field of the first element of the array is out of bounds // // 0000000000000000000000000000000000000000000000000000000000000020 (32) offset // 0000000000000000000000000000000000000000000000000000000000000001 ( 1) array length From 6706d8cda7bcf5168eb7452590bb508504ed4a7f Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 24 Dec 2024 15:56:50 +0100 Subject: [PATCH 6/7] add changeset --- .changeset/brown-jokes-applaud.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/brown-jokes-applaud.md diff --git a/.changeset/brown-jokes-applaud.md b/.changeset/brown-jokes-applaud.md new file mode 100644 index 00000000000..8d9b543845a --- /dev/null +++ b/.changeset/brown-jokes-applaud.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`ERC7579Utils`: Add in-depth sanity check when `executionCalldata` is the last buffer in calldata. From 36dcfb78147d2ef04bf4a66fd190ef8dcdc93362 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Sat, 28 Dec 2024 15:09:09 +0100 Subject: [PATCH 7/7] minimize assembly use --- .../account/utils/draft-ERC7579Utils.sol | 71 +++++++------------ test/account/utils/draft-ERC7579Utils.t.sol | 10 +-- 2 files changed, 31 insertions(+), 50 deletions(-) diff --git a/contracts/account/utils/draft-ERC7579Utils.sol b/contracts/account/utils/draft-ERC7579Utils.sol index 829c4c3c09a..0e830966ffb 100644 --- a/contracts/account/utils/draft-ERC7579Utils.sol +++ b/contracts/account/utils/draft-ERC7579Utils.sol @@ -212,52 +212,33 @@ library ERC7579Utils { executionBatch.length := arrayLength } - // [ Sanity check ] - // Solidity performs "lazy" verification that all calldata objects are valid, by checking that they are - // within calldatasize. This check is performed when objects are dereferenced. If the `executionCalldata` - // is not the last element (buffer) in msg.data, this check will not detect potentially ill-formed objects - // that point to the memory space between the end of the `executionCalldata` buffer. - // If we are in a situation where the lazy checks are not sufficient, we do an in-depth traversal of the - // array, checking that everything is valid. - if (bufferPtr + bufferLength < msg.data.length) { - uint256 arrayPtr = arrayLengthOffset + 32; - - for (uint256 i = 0; i < arrayLength; ++i) { - // Location of the element pointer in the array, and offset stored at that location - // Cannot overflow: arrayPtr + arrayLength * 32 is bounded by bufferLength - uint256 elementPtrPtr = arrayPtr + i * 32; - uint256 elementOffset = uint256(bytes32(executionCalldata[elementPtrPtr:elementPtrPtr + 32])); - - // Check that element data fits in the buffer (element has length 0x60 = 96) - // We know that arrayPtr <= bufferLength, so "bufferLength - arrayPtr" is safe - if (elementOffset > type(uint64).max || bufferLength - arrayPtr < elementOffset + 96) - revert ERC7579DecodingError(); - - // Location of the element data, and offset stored at that location - // Cannot overflow: arrayPtr + elementOffset + 96 is bounded by bufferLength - uint256 elementPtr = arrayPtr + elementOffset; - uint256 elementCalldataOffset = uint256( - bytes32(executionCalldata[elementPtr + 64:elementPtr + 96]) - ); - - // Check that at length of element inner calldata fits in the buffer - // We know that elementPtr + 96 <= bufferLength, so "bufferLength - elementPtr" is safe - if ( - elementCalldataOffset > type(uint64).max || - bufferLength - elementPtr < elementCalldataOffset + 32 - ) revert ERC7579DecodingError(); - - // Location of the element inner calldata, and length of the calldata stored at the location - // Cannot overflow: elementPtr + elementCalldataOffset + 32 is bounded by bufferLength - uint256 elementCalldataPtr = elementPtr + elementCalldataOffset; - uint256 elementCalldataLength = uint256( - bytes32(executionCalldata[elementCalldataPtr:elementCalldataPtr + 32]) - ); - - // Check that all the data of the element inner calldata fits in the buffer - // We know that elementCalldataOffset + 32 <= bufferLength, so "bufferLength - elementCalldataPtr" - 32 is safe - if (bufferLength - elementCalldataPtr - 32 < elementCalldataLength) revert ERC7579DecodingError(); + _validateCalldataBound(executionBatch, bufferPtr + bufferLength); + } + } + + /** @dev Calldata sanity check + * + * Solidity performs "lazy" verification that all calldata objects are valid, by checking that they are + * within calldatasize. This check is performed when objects are dereferenced. If the `executionCalldata` + * is not the last element (buffer) in msg.data, this check will not detect potentially ill-formed objects + * that point to the memory space between the end of the `executionCalldata` buffer. + * If we are in a situation where the lazy checks are not sufficient, we do an in-depth traversal of the + * array, checking that everything is valid. + */ + function _validateCalldataBound(Execution[] calldata executionBatch, uint256 bound) private pure { + if (bound < msg.data.length) { + for (uint256 i = 0; i < executionBatch.length; ++i) { + Execution calldata item = executionBatch[i]; + bytes calldata itemCalldata = item.callData; + + bool outOfBound; + assembly ("memory-safe") { + outOfBound := or( + gt(add(item, 0x60), bound), + gt(add(itemCalldata.offset, itemCalldata.length), bound) + ) } + if (outOfBound) revert ERC7579DecodingError(); } } } diff --git a/test/account/utils/draft-ERC7579Utils.t.sol b/test/account/utils/draft-ERC7579Utils.t.sol index 96f81a544c1..27e27100615 100644 --- a/test/account/utils/draft-ERC7579Utils.t.sol +++ b/test/account/utils/draft-ERC7579Utils.t.sol @@ -404,21 +404,21 @@ contract ERC7579UtilsTest is Test { if (test & TEST_DECODE > 0) { if (test & FAIL_DECODE > 0) vm.expectRevert(ERC7579Utils.ERC7579DecodingError.selector); this.callDecodeBatch(encoded); - if (test & FAIL_ANY > 0) vm.expectRevert(ERC7579Utils.ERC7579DecodingError.selector); + if (test & FAIL_ANY > 0) vm.expectRevert(); this.callDecodeBatchWithCalldata(encoded, extraData); } if (test & TEST_GETFIRST > 0) { - if (test & FAIL_GETFIRST > 0) vm.expectRevert(); // solidity failure without data + if (test & FAIL_GETFIRST > 0) vm.expectRevert(); this.callDecodeBatchAndGetFirst(encoded); - if (test & FAIL_ANY > 0) vm.expectRevert(ERC7579Utils.ERC7579DecodingError.selector); + if (test & FAIL_ANY > 0) vm.expectRevert(); this.callDecodeBatchAndGetFirstWithCalldata(encoded, extraData); } if (test & TEST_GETFIRSTBYTES > 0) { - if (test & FAIL_GETFIRSTBYTES > 0) vm.expectRevert(); // solidity failure without data + if (test & FAIL_GETFIRSTBYTES > 0) vm.expectRevert(); this.callDecodeBatchAndGetFirstBytes(encoded); - if (test & FAIL_ANY > 0) vm.expectRevert(ERC7579Utils.ERC7579DecodingError.selector); + if (test & FAIL_ANY > 0) vm.expectRevert(); this.callDecodeBatchAndGetFirstBytesWithCalldata(encoded, extraData); } }