diff --git a/contracts/utils/Strings.sol b/contracts/utils/Strings.sol index c353212927e..161a2b576d7 100644 --- a/contracts/utils/Strings.sol +++ b/contracts/utils/Strings.sol @@ -158,7 +158,7 @@ library Strings { * NOTE: This function will revert if the result does not fit in a `uint256`. */ function tryParseUint(string memory input) internal pure returns (bool success, uint256 value) { - return tryParseUint(input, 0, bytes(input).length); + return _tryParseUintUncheckedBounds(input, 0, bytes(input).length); } /** @@ -172,6 +172,18 @@ library Strings { uint256 begin, uint256 end ) internal pure returns (bool success, uint256 value) { + if (end > bytes(input).length || begin > end) return (false, 0); + return _tryParseUintUncheckedBounds(input, begin, end); + } + + /** + * @dev Variant of {tryParseUint} that does not check bounds and returns (true, 0) if they are invalid. + */ + function _tryParseUintUncheckedBounds( + string memory input, + uint256 begin, + uint256 end + ) private pure returns (bool success, uint256 value) { bytes memory buffer = bytes(input); uint256 result = 0; @@ -216,7 +228,7 @@ library Strings { * NOTE: This function will revert if the absolute value of the result does not fit in a `uint256`. */ function tryParseInt(string memory input) internal pure returns (bool success, int256 value) { - return tryParseInt(input, 0, bytes(input).length); + return _tryParseIntUncheckedBounds(input, 0, bytes(input).length); } uint256 private constant ABS_MIN_INT256 = 2 ** 255; @@ -232,10 +244,22 @@ library Strings { uint256 begin, uint256 end ) internal pure returns (bool success, int256 value) { + if (end > bytes(input).length || begin > end) return (false, 0); + return _tryParseIntUncheckedBounds(input, begin, end); + } + + /** + * @dev Variant of {tryParseInt} that does not check bounds and returns (true, 0) if they are invalid. + */ + function _tryParseIntUncheckedBounds( + string memory input, + uint256 begin, + uint256 end + ) private pure returns (bool success, int256 value) { bytes memory buffer = bytes(input); // Check presence of a negative sign. - bytes1 sign = bytes1(_unsafeReadBytesOffset(buffer, begin)); + bytes1 sign = begin == end ? bytes1(0) : bytes1(_unsafeReadBytesOffset(buffer, begin)); // don't do out-of-bound (possibly unsafe) read if sub-string is empty bool positiveSign = sign == bytes1("+"); bool negativeSign = sign == bytes1("-"); uint256 offset = (positiveSign || negativeSign).toUint(); @@ -280,7 +304,7 @@ library Strings { * NOTE: This function will revert if the result does not fit in a `uint256`. */ function tryParseHexUint(string memory input) internal pure returns (bool success, uint256 value) { - return tryParseHexUint(input, 0, bytes(input).length); + return _tryParseHexUintUncheckedBounds(input, 0, bytes(input).length); } /** @@ -294,10 +318,22 @@ library Strings { uint256 begin, uint256 end ) internal pure returns (bool success, uint256 value) { + if (end > bytes(input).length || begin > end) return (false, 0); + return _tryParseHexUintUncheckedBounds(input, begin, end); + } + + /** + * @dev Variant of {tryParseHexUint} that does not check bounds and returns (true, 0) if they are invalid. + */ + function _tryParseHexUintUncheckedBounds( + string memory input, + uint256 begin, + uint256 end + ) private pure returns (bool success, uint256 value) { bytes memory buffer = bytes(input); // skip 0x prefix if present - bool hasPrefix = bytes2(_unsafeReadBytesOffset(buffer, begin)) == bytes2("0x"); + bool hasPrefix = (begin < end + 1) && bytes2(_unsafeReadBytesOffset(buffer, begin)) == bytes2("0x"); // don't do out-of-bound (possibly unsafe) read if sub-string is empty uint256 offset = hasPrefix.toUint() * 2; uint256 result = 0; @@ -355,12 +391,13 @@ library Strings { uint256 end ) internal pure returns (bool success, address value) { // check that input is the correct length - bool hasPrefix = bytes2(_unsafeReadBytesOffset(bytes(input), begin)) == bytes2("0x"); + bool hasPrefix = (begin < end + 1) && bytes2(_unsafeReadBytesOffset(bytes(input), begin)) == bytes2("0x"); // don't do out-of-bound (possibly unsafe) read if sub-string is empty + uint256 expectedLength = 40 + hasPrefix.toUint() * 2; - if (end - begin == expectedLength) { + if (end - begin == expectedLength && end <= bytes(input).length) { // length guarantees that this does not overflow, and value is at most type(uint160).max - (bool s, uint256 v) = tryParseHexUint(input, begin, end); + (bool s, uint256 v) = _tryParseHexUintUncheckedBounds(input, begin, end); return (s, address(uint160(v))); } else { return (false, address(0)); diff --git a/test/utils/Strings.t.sol b/test/utils/Strings.t.sol index 45e11342916..fe3c90bd115 100644 --- a/test/utils/Strings.t.sol +++ b/test/utils/Strings.t.sol @@ -24,4 +24,27 @@ contract StringsTest is Test { function testParseChecksumHex(address value) external pure { assertEq(value, value.toChecksumHexString().parseAddress()); } + + function testTryParseHexUintExtendedEnd(string memory random) external pure { + uint256 length = bytes(random).length; + assembly ("memory-safe") { + mstore(add(add(random, 0x20), length), 0x3030303030303030303030303030303030303030303030303030303030303030) + } + + (bool success, ) = random.tryParseHexUint(1, length + 1); + assertFalse(success); + } + + function testTryParseAddressExtendedEnd(address random, uint256 begin) external pure { + begin = bound(begin, 3, 43); + string memory input = random.toHexString(); + uint256 length = bytes(input).length; + + assembly ("memory-safe") { + mstore(add(add(input, 0x20), length), 0x3030303030303030303030303030303030303030303030303030303030303030) + } + + (bool success, ) = input.tryParseAddress(begin, begin + 40); + assertFalse(success); + } } diff --git a/test/utils/Strings.test.js b/test/utils/Strings.test.js index 5a47d4d10de..0b2d87190d3 100644 --- a/test/utils/Strings.test.js +++ b/test/utils/Strings.test.js @@ -240,6 +240,11 @@ describe('Strings', function () { expect(await this.mock.$tryParseUint('1 000')).deep.equal([false, 0n]); }); + it('parseUint invalid range', async function () { + expect(this.mock.$parseUint('12', 3, 2)).to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar'); + expect(await this.mock.$tryParseUint('12', 3, 2)).to.deep.equal([false, 0n]); + }); + it('parseInt overflow', async function () { await expect(this.mock.$parseInt((ethers.MaxUint256 + 1n).toString(10))).to.be.revertedWithPanic( PANIC_CODES.ARITHMETIC_OVERFLOW, @@ -276,6 +281,11 @@ describe('Strings', function () { expect(await this.mock.$tryParseInt('1 000')).to.deep.equal([false, 0n]); }); + it('parseInt invalid range', async function () { + expect(this.mock.$parseInt('-12', 3, 2)).to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar'); + expect(await this.mock.$tryParseInt('-12', 3, 2)).to.deep.equal([false, 0n]); + }); + it('parseHexUint overflow', async function () { await expect(this.mock.$parseHexUint((ethers.MaxUint256 + 1n).toString(16))).to.be.revertedWithPanic( PANIC_CODES.ARITHMETIC_OVERFLOW, @@ -303,6 +313,11 @@ describe('Strings', function () { expect(await this.mock.$tryParseHexUint('1 000')).to.deep.equal([false, 0n]); }); + it('parseHexUint invalid begin and end', async function () { + expect(this.mock.$parseHexUint('0x', 3, 2)).to.be.revertedWithCustomError(this.mock, 'StringsInvalidChar'); + expect(await this.mock.$tryParseHexUint('0x', 3, 2)).to.deep.equal([false, 0n]); + }); + it('parseAddress invalid format', async function () { for (const addr of [ '0x736a507fB2881d6bB62dcA54673CF5295dC07833', // valid