From 49182f4b5c4fd3a179052c1cc9d77bfab32be02a Mon Sep 17 00:00:00 2001 From: Jun Kimura Date: Mon, 30 Sep 2024 00:34:32 +0900 Subject: [PATCH 1/2] improve ICS20 tests Signed-off-by: Jun Kimura --- tests/foundry/src/ICS20.t.sol | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/foundry/src/ICS20.t.sol b/tests/foundry/src/ICS20.t.sol index b0792506..1b5cb6ce 100644 --- a/tests/foundry/src/ICS20.t.sol +++ b/tests/foundry/src/ICS20.t.sol @@ -106,6 +106,18 @@ contract TestICS20 is Test { // This should not revert if the input is not a valid hex string. ICS20LibTestHelper.hexStringToAddress(any); } + + function testParseUint256String() public { + bytes memory maxAmountStr = bytes("115792089237316195423570985008687907853269984665640564039457584007913129639935\""); + (uint256 amount, uint256 pos) = ICS20LibTestHelper.parseUint256String(maxAmountStr, 0); + assertEq(amount, 115792089237316195423570985008687907853269984665640564039457584007913129639935); + assertEq(pos, maxAmountStr.length); + + bytes memory minAmountStr = bytes("1\""); + (amount, pos) = ICS20LibTestHelper.parseUint256String(minAmountStr, 0); + assertEq(amount, 1); + assertEq(pos, minAmountStr.length); + } } library ICS20LibTestHelper { @@ -128,4 +140,8 @@ library ICS20LibTestHelper { function isEscapedJSONString(string calldata s) public pure returns (bool) { return ICS20Lib.isEscapedJSONString(s); } + + function parseUint256String(bytes calldata bz, uint256 pos) public pure returns (uint256, uint256) { + return ICS20Lib.parseUint256String(bz, pos); + } } From 7d449f65ef232406a19e87dede3e94ccf1aaf5b9 Mon Sep 17 00:00:00 2001 From: Jun Kimura Date: Mon, 30 Sep 2024 00:35:19 +0900 Subject: [PATCH 2/2] remove unnecessary `unchecked` and improve code docs Signed-off-by: Jun Kimura --- contracts/apps/20-transfer/ICS20Bank.sol | 2 + contracts/apps/20-transfer/ICS20Lib.sol | 113 ++++++++++++------ contracts/core/02-client/IBCClientLib.sol | 34 +++--- .../core/04-channel/IBCChannelUpgrade.sol | 2 + contracts/core/24-host/IBCHostLib.sol | 30 +++-- 5 files changed, 114 insertions(+), 67 deletions(-) diff --git a/contracts/apps/20-transfer/ICS20Bank.sol b/contracts/apps/20-transfer/ICS20Bank.sol index a71aae67..02557e52 100644 --- a/contracts/apps/20-transfer/ICS20Bank.sol +++ b/contracts/apps/20-transfer/ICS20Bank.sol @@ -41,6 +41,7 @@ contract ICS20Bank is Context, AccessControl, IICS20Bank, IICS20Errors { revert ICS20InsufficientBalance(from, fromBalance, amount); } unchecked { + // SAFETY: balance is checked above _balances[denom][from] = fromBalance - amount; } _balances[denom][to] += amount; @@ -94,6 +95,7 @@ contract ICS20Bank is Context, AccessControl, IICS20Bank, IICS20Errors { revert ICS20InsufficientBalance(account, accountBalance, amount); } unchecked { + // SAFETY: balance is checked above _balances[denom][account] = accountBalance - amount; } } diff --git a/contracts/apps/20-transfer/ICS20Lib.sol b/contracts/apps/20-transfer/ICS20Lib.sol index b24cc2a9..4833e8af 100644 --- a/contracts/apps/20-transfer/ICS20Lib.sol +++ b/contracts/apps/20-transfer/ICS20Lib.sol @@ -20,16 +20,16 @@ library ICS20Lib { bytes internal constant FAILED_ACKNOWLEDGEMENT_JSON = bytes('{"error":"failed"}'); bytes32 internal constant KECCAK256_SUCCESSFUL_ACKNOWLEDGEMENT_JSON = keccak256(SUCCESSFUL_ACKNOWLEDGEMENT_JSON); - uint256 private constant CHAR_DOUBLE_QUOTE = 0x22; - uint256 private constant CHAR_SLASH = 0x2f; + uint256 private constant CHAR_DOUBLE_QUOTE = 0x22; // '"' + uint256 private constant CHAR_SLASH = 0x2f; // "/" uint256 private constant CHAR_BACKSLASH = 0x5c; - uint256 private constant CHAR_F = 0x66; - uint256 private constant CHAR_R = 0x72; - uint256 private constant CHAR_N = 0x6e; - uint256 private constant CHAR_B = 0x62; - uint256 private constant CHAR_T = 0x74; - uint256 private constant CHAR_CLOSING_BRACE = 0x7d; - uint256 private constant CHAR_M = 0x6d; + uint256 private constant CHAR_F = 0x66; // "f" + uint256 private constant CHAR_R = 0x72; // "r" + uint256 private constant CHAR_N = 0x6e; // "n" + uint256 private constant CHAR_B = 0x62; // "b" + uint256 private constant CHAR_T = 0x74; // "t" + uint256 private constant CHAR_CLOSING_BRACE = 0x7d; // "}" + uint256 private constant CHAR_M = 0x6d; // "m" bytes16 private constant HEX_DIGITS = "0123456789abcdef"; @@ -47,6 +47,11 @@ library ICS20Lib { /** * @dev marshalJSON marshals PacketData into JSON bytes with escaping. + * @param escapedDenom is the denom string escaped. + * @param amount is the amount field. + * @param escapedSender is the sender string escaped. + * @param escapedReceiver is the receiver string escaped. + * @param escapedMemo is the memo string escaped. */ function marshalJSON( string memory escapedDenom, @@ -72,6 +77,10 @@ library ICS20Lib { /** * @dev marshalJSON marshals PacketData into JSON bytes with escaping. + * @param escapedDenom is the denom string escaped. + * @param amount is the amount field. + * @param escapedSender is the sender string escaped. + * @param escapedReceiver is the receiver string escaped. */ function marshalJSON( string memory escapedDenom, @@ -94,12 +103,16 @@ library ICS20Lib { /** * @dev unmarshalJSON unmarshals JSON bytes into PacketData. + * @param bz the JSON bytes to unmarshal. It must be either of the following JSON formats. It is assumed that string fields are escaped. + * 1. {"amount":"","denom":"","memo":"","receiver":"","sender":""} + * 2. {"amount":"","denom":"","receiver":"","sender":""} */ function unmarshalJSON(bytes calldata bz) internal pure returns (PacketData memory) { PacketData memory pd; uint256 pos = 0; unchecked { + // SAFETY: `pos` never overflow because it is always less than `bz.length`. if (bytes32(bz[pos:pos + 11]) != bytes32('{"amount":"')) { revert IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32('{"amount":"'), bytes32(bz[pos:pos + 11])); } @@ -135,35 +148,51 @@ library ICS20Lib { } /** - * @dev parseUint256String parses `bz` from a position `pos` to produce a uint256. + * @dev parseUint256String parses `bz` from a position `pos` to produce a uint256 value. + * The parse will stop parsing when it encounters a non-digit character. + * @param bz the byte array to parse. + * @param pos the position to start parsing. + * @return ret the parsed uint256 value. + * @return pos the new position after parsing. */ function parseUint256String(bytes calldata bz, uint256 pos) internal pure returns (uint256, uint256) { uint256 ret = 0; - unchecked { - for (; pos < bz.length; pos++) { - uint256 c = uint256(uint8(bz[pos])); - if (c < 48 || c > 57) { - break; - } - ret = ret * 10 + (c - 48); + uint256 bzLen = bz.length; + for (; pos < bzLen; pos++) { + uint256 c = uint256(uint8(bz[pos])); + if (c < 48 || c > 57) { + break; } - if (pos >= bz.length || uint256(uint8(bz[pos])) != CHAR_DOUBLE_QUOTE) { - revert IICS20Errors.ICS20JSONStringClosingDoubleQuoteNotFound(pos, bz[pos]); + unchecked { + // SAFETY: we assume that the amount is uint256, so `ret` never overflows. + ret = ret * 10 + (c - 48); } + } + if (uint256(uint8(bz[pos])) != CHAR_DOUBLE_QUOTE) { + revert IICS20Errors.ICS20JSONStringClosingDoubleQuoteNotFound(pos, bz[pos]); + } + unchecked { + // SAFETY: `pos` is always less than `bz.length`. return (ret, pos + 1); } } /** * @dev parseString parses `bz` from a position `pos` to produce a string. + * @param bz the byte array to parse. + * @param pos the position to start parsing. + * @return parsedStr the parsed string. + * @return position the new position after parsing. */ function parseString(bytes calldata bz, uint256 pos) internal pure returns (string memory, uint256) { + uint256 bzLen = bz.length; unchecked { - for (uint256 i = pos; i < bz.length; i++) { + // SAFETY: i + 1 <= bzLen <= type(uint256).max + for (uint256 i = pos; i < bzLen; i++) { uint256 c = uint256(uint8(bz[i])); if (c == CHAR_DOUBLE_QUOTE) { return (string(bz[pos:i]), i + 1); - } else if (c == CHAR_BACKSLASH && i + 1 < bz.length) { + } else if (c == CHAR_BACKSLASH && i + 1 < bzLen) { i++; c = uint256(uint8(bz[i])); if ( @@ -178,14 +207,19 @@ library ICS20Lib { revert IICS20Errors.ICS20JSONStringUnclosed(bz, pos); } + /** + * @dev isEscapedJSONString checks if a string is escaped JSON. + */ function isEscapedJSONString(string calldata s) internal pure returns (bool) { bytes memory bz = bytes(s); - unchecked { - for (uint256 i = 0; i < bz.length; i++) { + uint256 bzLen = bz.length; + for (uint256 i = 0; i < bzLen; i++) { + unchecked { uint256 c = uint256(uint8(bz[i])); if (c == CHAR_DOUBLE_QUOTE) { return false; - } else if (c == CHAR_BACKSLASH && i + 1 < bz.length) { + } else if (c == CHAR_BACKSLASH && i + 1 < bzLen) { + // SAFETY: i + 1 <= bzLen <= type(uint256).max i++; c = uint256(uint8(bz[i])); if ( @@ -200,13 +234,16 @@ library ICS20Lib { return true; } + /** + * @dev isEscapeNeededString checks if a given string needs to be escaped. + * @param bz the byte array to check. + */ function isEscapeNeededString(bytes memory bz) internal pure returns (bool) { - unchecked { - for (uint256 i = 0; i < bz.length; i++) { - uint256 c = uint256(uint8(bz[i])); - if (c == CHAR_DOUBLE_QUOTE) { - return true; - } + uint256 bzLen = bz.length; + for (uint256 i = 0; i < bzLen; i++) { + uint256 c = uint256(uint8(bz[i])); + if (c == CHAR_DOUBLE_QUOTE) { + return true; } } return false; @@ -214,6 +251,8 @@ library ICS20Lib { /** * @dev addressToHexString converts an address to a hex string. + * @param addr the address to convert. + * @return the hex string. */ function addressToHexString(address addr) internal pure returns (string memory) { uint256 localValue = uint256(uint160(addr)); @@ -221,8 +260,9 @@ library ICS20Lib { buffer[0] = "0"; buffer[1] = "x"; unchecked { - for (int256 i = 41; i >= 2; --i) { - buffer[uint256(i)] = HEX_DIGITS[localValue & 0xf]; + // SAFETY: `i` is always greater than or equal to 1. + for (uint256 i = 41; i >= 2; --i) { + buffer[i] = HEX_DIGITS[localValue & 0xf]; localValue >>= 4; } } @@ -231,6 +271,8 @@ library ICS20Lib { /** * @dev hexStringToAddress converts a hex string to an address. + * @param addrHexString the hex string to convert. It must be 42 characters long and start with "0x". + * @return the address and a boolean indicating whether the conversion was successful. */ function hexStringToAddress(string memory addrHexString) internal pure returns (address, bool) { bytes memory addrBytes = bytes(addrHexString); @@ -240,9 +282,10 @@ library ICS20Lib { return (address(0), false); } uint256 addr = 0; - unchecked { - for (uint256 i = 2; i < 42; i++) { - uint256 c = uint256(uint8(addrBytes[i])); + for (uint256 i = 2; i < 42; i++) { + uint256 c = uint256(uint8(addrBytes[i])); + unchecked { + // SAFETY: we assume that the address is a valid ethereum addrress, so `addr` never overflows. if (c >= 48 && c <= 57) { addr = addr * 16 + (c - 48); } else if (c >= 97 && c <= 102) { diff --git a/contracts/core/02-client/IBCClientLib.sol b/contracts/core/02-client/IBCClientLib.sol index 3bdc18f0..3e3c7c25 100644 --- a/contracts/core/02-client/IBCClientLib.sol +++ b/contracts/core/02-client/IBCClientLib.sol @@ -8,27 +8,29 @@ library IBCClientLib { * - clientType must be in the form of `^[a-z][a-z0-9-]*[a-z0-9]$` */ function validateClientType(bytes memory clientTypeBytes) internal pure returns (bool) { - if (clientTypeBytes.length == 0) { + uint256 byteLength = clientTypeBytes.length; + if (byteLength == 0) { return false; } - unchecked { - for (uint256 i = 0; i < clientTypeBytes.length; i++) { - uint256 c = uint256(uint8(clientTypeBytes[i])); - if (0x61 <= c && c <= 0x7a) { - // a-z - continue; - } else if (c == 0x2d) { - // hyphen cannot be the first or last character - if (i == 0 || i == clientTypeBytes.length - 1) { + for (uint256 i = 0; i < byteLength; i++) { + uint256 c = uint256(uint8(clientTypeBytes[i])); + if (0x61 <= c && c <= 0x7a) { + // a-z + continue; + } else if (c == 0x2d) { + // hyphen cannot be the first or last character + unchecked { + // SAFETY: `byteLength` is greater than 0 + if (i == 0 || i == byteLength - 1) { return false; } - continue; - } else if (0x30 <= c && c <= 0x39) { - // 0-9 - continue; - } else { - return false; } + continue; + } else if (0x30 <= c && c <= 0x39) { + // 0-9 + continue; + } else { + return false; } } return true; diff --git a/contracts/core/04-channel/IBCChannelUpgrade.sol b/contracts/core/04-channel/IBCChannelUpgrade.sol index fd66f879..796bd7ae 100644 --- a/contracts/core/04-channel/IBCChannelUpgrade.sol +++ b/contracts/core/04-channel/IBCChannelUpgrade.sol @@ -340,6 +340,7 @@ abstract contract IBCChannelUpgradeBase is function toString(UpgradeHandshakeError err) internal pure returns (string memory) { bytes memory result = new bytes(1); unchecked { + // SAFETY: `err` is always less than or equal to 6, so overflow never occurs result[0] = bytes1(uint8(err) + 48); } return string(result); @@ -434,6 +435,7 @@ contract IBCChannelUpgradeInitTryAck is IBCChannelUpgradeBase, IIBCChannelUpgrad // and abort their out-of-sync upgrade without aborting our own since // the error receipt sequence is lower than ours and higher than the counterparty. unchecked { + // SAFETY: `msg_.counterpartyUpgradeSequence` is always greater than 0, so underflow never occurs writeErrorReceipt( msg_.portId, msg_.channelId, expectedUpgradeSequence - 1, UpgradeHandshakeError.OutOfSync ); diff --git a/contracts/core/24-host/IBCHostLib.sol b/contracts/core/24-host/IBCHostLib.sol index 3758a750..7c14ca6f 100644 --- a/contracts/core/24-host/IBCHostLib.sol +++ b/contracts/core/24-host/IBCHostLib.sol @@ -17,22 +17,20 @@ library IBCHostLib { if (portIdLength < 2 || portIdLength > 128) { return false; } - unchecked { - for (uint256 i = 0; i < portIdLength; i++) { - uint256 c = uint256(uint8(portId[i])); - // return false if the character is not in one of the following categories: - // a-z - // 0-9 - // A-Z - // ".", "_", "+", "-" - // "#", "[", "]", "<", ">" - if ( - !(c >= 0x61 && c <= 0x7A) && !(c >= 0x30 && c <= 0x39) && !(c >= 0x41 && c <= 0x5A) - && !(c == 0x2E || c == 0x5F || c == 0x2B || c == 0x2D) - && !(c == 0x23 || c == 0x5B || c == 0x5D || c == 0x3C || c == 0x3E) - ) { - return false; - } + for (uint256 i = 0; i < portIdLength; i++) { + uint256 c = uint256(uint8(portId[i])); + // return false if the character is not in one of the following categories: + // a-z + // 0-9 + // A-Z + // ".", "_", "+", "-" + // "#", "[", "]", "<", ">" + if ( + !(c >= 0x61 && c <= 0x7A) && !(c >= 0x30 && c <= 0x39) && !(c >= 0x41 && c <= 0x5A) + && !(c == 0x2E || c == 0x5F || c == 0x2B || c == 0x2D) + && !(c == 0x23 || c == 0x5B || c == 0x5D || c == 0x3C || c == 0x3E) + ) { + return false; } } return true;