diff --git a/.changeset/brown-jokes-applaud.md b/.changeset/brown-jokes-applaud.md new file mode 100644 index 00000000000..69c14ee3046 --- /dev/null +++ b/.changeset/brown-jokes-applaud.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`ERC7579Utils`: Add in-depth sanity check when `executionCalldata` is not the last buffer in calldata. diff --git a/contracts/account/utils/draft-ERC7579Utils.sol b/contracts/account/utils/draft-ERC7579Utils.sol index 38809116cbb..7bca17f5f4b 100644 --- a/contracts/account/utils/draft-ERC7579Utils.sol +++ b/contracts/account/utils/draft-ERC7579Utils.sol @@ -177,7 +177,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 < 0x20) revert ERC7579DecodingError(); @@ -204,9 +209,38 @@ library ERC7579Utils { revert ERC7579DecodingError(); assembly ("memory-safe") { - executionBatch.offset := add(add(executionCalldata.offset, arrayLengthOffset), 0x20) + executionBatch.offset := add(add(bufferPtr, arrayLengthOffset), 0x20) executionBatch.length := arrayLength } + + _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; + + uint256 itemEnd; + uint256 itemCalldataEnd; + assembly ("memory-safe") { + itemEnd := add(item, 0x60) + itemCalldataEnd := add(itemCalldata.offset, itemCalldata.length) + } + if (itemEnd > bound || itemCalldataEnd > bound) revert ERC7579DecodingError(); + } } } diff --git a/test/account/utils/draft-ERC7579Utils.t.sol b/test/account/utils/draft-ERC7579Utils.t.sol index bd7b9ad826e..a3f7d0fad39 100644 --- a/test/account/utils/draft-ERC7579Utils.t.sol +++ b/test/account/utils/draft-ERC7579Utils.t.sol @@ -300,39 +300,52 @@ 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)); - - // GOOD - this.callDecodeBatch(abi.encode(32, 0)); + // BAD: offset is out of bounds + function testDecodeBatchOffsetOutOfBound() public { + _testDecodeBatch(abi.encode(1), TEST_DECODE | FAIL_DECODE); + } - // BAD: reported array length extends beyond bounds - vm.expectRevert(ERC7579Utils.ERC7579DecodingError.selector); - this.callDecodeBatch(abi.encode(32, 1)); + // GOOD + function testDecodeBatchEmptyArray() public { + _testDecodeBatch(abi.encode(32, 0), TEST_DECODE); + } - // GOOD - this.callDecodeBatch(abi.encode(32, 1, 0)); + // BAD: reported array length extends beyond bounds + function testDecodeBatchLengthExtendsOutOfBound() public { + _testDecodeBatch(abi.encode(32, 1), TEST_DECODE | FAIL_DECODE); + } - // GOOD - // + // GOOD + function testDecodeBatchFirstBytes() public view { // 0000000000000000000000000000000000000000000000000000000000000020 (32) offset // 0000000000000000000000000000000000000000000000000000000000000001 ( 1) array length // 0000000000000000000000000000000000000000000000000000000000000020 (32) element 0 offset @@ -347,21 +360,43 @@ contract ERC7579UtilsTest is Test { abi.encode(32, 1, 32, _recipient1, 42, 96, 12, bytes12("Hello World!")) ) ); + } + // GOOD at first level, BAD when dereferencing + 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 // 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); + } - // 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 @@ -370,26 +405,65 @@ 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 fail. - 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 { + 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(); + this.callDecodeBatchWithCalldata(encoded, extraData); + } + + 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, extraData); + } + + 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, extraData); + } } 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(ERC4337Utils.ENTRYPOINT_V07)); }