Skip to content

Commit

Permalink
Refactor parseUint, parseInt and parseHexUint to check bounds (O…
Browse files Browse the repository at this point in the history
…penZeppelin#5304)

Co-authored-by: Hadrien Croubois <[email protected]>
  • Loading branch information
ernestognw and Amxx authored Nov 25, 2024
1 parent d11ed2f commit b3ce884
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 8 deletions.
53 changes: 45 additions & 8 deletions contracts/utils/Strings.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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();
Expand Down Expand Up @@ -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);
}

/**
Expand All @@ -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;
Expand Down Expand Up @@ -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));
Expand Down
23 changes: 23 additions & 0 deletions test/utils/Strings.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
15 changes: 15 additions & 0 deletions test/utils/Strings.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit b3ce884

Please sign in to comment.