diff --git a/.github/setup/action.yml b/.github/setup/action.yml index 0061068..cdc7c51 100644 --- a/.github/setup/action.yml +++ b/.github/setup/action.yml @@ -7,8 +7,6 @@ runs: uses: "actions/checkout@v4" - name: "Install Foundry" uses: "foundry-rs/foundry-toolchain@v1" - with: - version: nightly-f8aa4afec04cc0b7d364a5d78f0cde9e64fd14bf - name: "Install Bun" uses: "oven-sh/setup-bun@v2" - name: "Install the Node.js dependencies" diff --git a/.github/workflows/foundry.yml b/.github/workflows/foundry.yml index c45fffa..d9a3e57 100644 --- a/.github/workflows/foundry.yml +++ b/.github/workflows/foundry.yml @@ -80,8 +80,10 @@ jobs: uses: ./.github/setup - name: "Run coverage" - run: "bash scripts/checks/coverage.sh" + run: forge coverage --report lcov - uses: codecov/codecov-action@v4 + with: + exclude: test env: - CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} \ No newline at end of file + CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} diff --git a/.solhint.json b/.solhint.json index d11bf3f..2d5ddc4 100644 --- a/.solhint.json +++ b/.solhint.json @@ -9,6 +9,7 @@ "named-parameters-mapping": "warn", "no-console": "off", "not-rely-on-time": "off", - "one-contract-per-file": "off" + "one-contract-per-file": "off", + "gas-custom-errors": "off" } } diff --git a/foundry.toml b/foundry.toml index b9d2939..8fdfb10 100644 --- a/foundry.toml +++ b/foundry.toml @@ -5,7 +5,7 @@ block_timestamp = 1_680_220_800 # March 31, 2023 at 00:00 GMT bytecode_hash = "none" evm_version = "cancun" - fuzz = { runs = 1_000_00 } + fuzz = { runs = 100_000 } gas_reports = ["*"] optimizer = true optimizer_runs = 10_000 diff --git a/scripts/E2ETestDeploy.s.sol b/scripts/E2ETestDeploy.s.sol index 91ad363..5a7e63e 100644 --- a/scripts/E2ETestDeploy.s.sol +++ b/scripts/E2ETestDeploy.s.sol @@ -5,7 +5,7 @@ pragma solidity ^0.8.28; This script is used for end-to-end testing with SP1_PROVER=network. */ -// solhint-disable gas-custom-errors,custom-errors +// solhint-disable custom-errors import { stdJson } from "forge-std/StdJson.sol"; import { Script } from "forge-std/Script.sol"; diff --git a/scripts/checks/coverage.sh b/scripts/checks/coverage.sh deleted file mode 100644 index 4bd9569..000000000 --- a/scripts/checks/coverage.sh +++ /dev/null @@ -1,10 +0,0 @@ -#!/usr/bin/env bash - -set -euo pipefail - -# Foundry coverage -forge coverage --report lcov -# Remove zero hits -sed -i '/,0/d' lcov.info - -# Reports are then uploaded to Codecov automatically by workflow, and merged. diff --git a/src/ICS02Client.sol b/src/ICS02Client.sol index ab561d1..c040aea 100644 --- a/src/ICS02Client.sol +++ b/src/ICS02Client.sol @@ -23,9 +23,7 @@ contract ICS02Client is IICS02Client, IICS02ClientErrors, Ownable { /// @param clientType The client type /// @return The next client identifier function getNextClientId(string calldata clientType) private returns (string memory) { - if (!IBCIdentifiers.validateClientType(clientType)) { - revert IBCInvalidClientType(clientType); - } + require(IBCIdentifiers.validateClientType(clientType), IBCInvalidClientType(clientType)); uint32 seq = nextClientSeq[clientType]; nextClientSeq[clientType] = seq + 1; @@ -35,9 +33,7 @@ contract ICS02Client is IICS02Client, IICS02ClientErrors, Ownable { /// @inheritdoc IICS02Client function getCounterparty(string calldata clientId) public view returns (CounterpartyInfo memory) { CounterpartyInfo memory counterpartyInfo = counterpartyInfos[clientId]; - if (bytes(counterpartyInfo.clientId).length == 0) { - revert IBCCounterpartyClientNotFound(clientId); - } + require(bytes(counterpartyInfo.clientId).length != 0, IBCCounterpartyClientNotFound(clientId)); return counterpartyInfo; } @@ -45,9 +41,7 @@ contract ICS02Client is IICS02Client, IICS02ClientErrors, Ownable { /// @inheritdoc IICS02Client function getClient(string calldata clientId) public view returns (ILightClient) { ILightClient client = clients[clientId]; - if (client == ILightClient(address(0))) { - revert IBCClientNotFound(clientId); - } + require(address(client) != address(0), IBCClientNotFound(clientId)); return client; } diff --git a/src/ICS20Transfer.sol b/src/ICS20Transfer.sol index 03be1ee..d91a1e9 100644 --- a/src/ICS20Transfer.sol +++ b/src/ICS20Transfer.sol @@ -42,9 +42,7 @@ contract ICS20Transfer is IIBCApp, IICS20Transfer, IICS20Errors, Ownable, Reentr /// @inheritdoc IICS20Transfer function sendTransfer(SendTransferMsg calldata msg_) external override returns (uint32) { - if (msg_.amount == 0) { - revert ICS20InvalidAmount(msg_.amount); - } + require(msg_.amount > 0, ICS20InvalidAmount(msg_.amount)); // we expect the denom to be an erc20 address address contractAddress = ICS20Lib.mustHexStringToAddress(msg_.denom); @@ -83,19 +81,16 @@ contract ICS20Transfer is IIBCApp, IICS20Transfer, IICS20Errors, Ownable, Reentr // The packet sender has to be the contract itself. // Because of the packetData massaging we do in sendTransfer to convert the amount to sdkCoin, we don't allow // this function to be called by anyone else. They could end up transferring a larger amount than intended. - if (msg_.sender != address(this)) { - revert ICS20UnauthorizedPacketSender(msg_.sender); - } + require(msg_.sender == address(this), ICS20UnauthorizedPacketSender(msg_.sender)); - if (keccak256(bytes(msg_.payload.version)) != keccak256(bytes(ICS20Lib.ICS20_VERSION))) { - revert ICS20UnexpectedVersion(ICS20Lib.ICS20_VERSION, msg_.payload.version); - } + require( + keccak256(bytes(msg_.payload.version)) == keccak256(bytes(ICS20Lib.ICS20_VERSION)), + ICS20UnexpectedVersion(ICS20Lib.ICS20_VERSION, msg_.payload.version) + ); ICS20Lib.PacketDataJSON memory packetData = ICS20Lib.unmarshalJSON(msg_.payload.value); - if (packetData.amount == 0) { - revert ICS20InvalidAmount(packetData.amount); - } + require(packetData.amount > 0, ICS20InvalidAmount(packetData.amount)); address sender = ICS20Lib.mustHexStringToAddress(packetData.sender); @@ -137,7 +132,6 @@ contract ICS20Transfer is IIBCApp, IICS20Transfer, IICS20Errors, Ownable, Reentr return ICS20Lib.errorAck(abi.encodePacked("invalid receiver: ", packetData.receiver)); } - // TODO: Implement escrow balance tracking (#6) if (originatorChainIsSource) { // sender is source, so we mint vouchers // NOTE: getReceiveTokenContractAndSource has already created a contract if it didn't exist @@ -202,9 +196,10 @@ contract ICS20Transfer is IIBCApp, IICS20Transfer, IICS20Errors, Ownable, Reentr uint256 expectedEndingBalance = ourStartingBalance + amount; // a very strange ERC20 may trigger this condition, if we didn't have this we would // underflow, so it's mostly just an error message printer - if (actualEndingBalance <= ourStartingBalance || actualEndingBalance != expectedEndingBalance) { - revert ICS20UnexpectedERC20Balance(expectedEndingBalance, actualEndingBalance); - } + require( + actualEndingBalance > ourStartingBalance && actualEndingBalance == expectedEndingBalance, + ICS20UnexpectedERC20Balance(expectedEndingBalance, actualEndingBalance) + ); } /// @notice For a send packet, get the ERC20 address for the token and whether the originator chain is the source @@ -234,9 +229,7 @@ contract ICS20Transfer is IIBCApp, IICS20Transfer, IICS20Errors, Ownable, Reentr // receiving chain is source of the token, so we've received and mapped this token before string memory ibcDenom = ICS20Lib.toIBCDenom(packetData.denom); erc20Address = address(_ibcDenomContracts[ibcDenom]); - if (erc20Address == address(0)) { - revert ICS20DenomNotFound(packetData.denom); - } + require(erc20Address != address(0), ICS20DenomNotFound(packetData.denom)); } return (erc20Address, originatorChainIsSource); } diff --git a/src/ICS26Router.sol b/src/ICS26Router.sol index 908ab4b..a765598 100644 --- a/src/ICS26Router.sol +++ b/src/ICS26Router.sol @@ -39,9 +39,7 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua /// @inheritdoc IICS26Router function getIBCApp(string calldata portId) public view returns (IIBCApp) { IIBCApp app = apps[portId]; - if (app == IIBCApp(address(0))) { - revert IBCAppNotFound(portId); - } + require(address(app) != address(0), IBCAppNotFound(portId)); return app; } @@ -59,12 +57,8 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua newPortId = Strings.toHexString(app); } - if (apps[newPortId] != IIBCApp(address(0))) { - revert IBCPortAlreadyExists(newPortId); - } - if (!IBCIdentifiers.validatePortIdentifier(bytes(newPortId))) { - revert IBCInvalidPortIdentifier(newPortId); - } + require(address(apps[newPortId]) == address(0), IBCPortAlreadyExists(newPortId)); + require(IBCIdentifiers.validatePortIdentifier(bytes(newPortId)), IBCInvalidPortIdentifier(newPortId)); apps[newPortId] = IIBCApp(app); @@ -77,17 +71,15 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua /// @inheritdoc IICS26Router function sendPacket(MsgSendPacket calldata msg_) external nonReentrant returns (uint32) { // TODO: Support multi-payload packets #93 - if (msg_.payloads.length != 1) { - revert IBCMultiPayloadPacketNotSupported(); - } + require(msg_.payloads.length == 1, IBCMultiPayloadPacketNotSupported()); Payload calldata payload = msg_.payloads[0]; string memory counterpartyId = ICS02_CLIENT.getCounterparty(msg_.sourceChannel).clientId; // TODO: validate all identifiers - if (msg_.timeoutTimestamp <= block.timestamp) { - revert IBCInvalidTimeoutTimestamp(msg_.timeoutTimestamp, block.timestamp); - } + require( + msg_.timeoutTimestamp > block.timestamp, IBCInvalidTimeoutTimestamp(msg_.timeoutTimestamp, block.timestamp) + ); uint32 sequence = IBC_STORE.nextSequenceSend(msg_.sourceChannel); @@ -120,19 +112,18 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua /// @inheritdoc IICS26Router function recvPacket(MsgRecvPacket calldata msg_) external nonReentrant { // TODO: Support multi-payload packets #93 - if (msg_.packet.payloads.length != 1) { - revert IBCMultiPayloadPacketNotSupported(); - } + require(msg_.packet.payloads.length == 1, IBCMultiPayloadPacketNotSupported()); Payload calldata payload = msg_.packet.payloads[0]; IICS02ClientMsgs.CounterpartyInfo memory cInfo = ICS02_CLIENT.getCounterparty(msg_.packet.destChannel); - if (keccak256(bytes(cInfo.clientId)) != keccak256(bytes(msg_.packet.sourceChannel))) { - revert IBCInvalidCounterparty(cInfo.clientId, msg_.packet.sourceChannel); - } - - if (msg_.packet.timeoutTimestamp <= block.timestamp) { - revert IBCInvalidTimeoutTimestamp(msg_.packet.timeoutTimestamp, block.timestamp); - } + require( + keccak256(bytes(cInfo.clientId)) == keccak256(bytes(msg_.packet.sourceChannel)), + IBCInvalidCounterparty(cInfo.clientId, msg_.packet.sourceChannel) + ); + require( + msg_.packet.timeoutTimestamp > block.timestamp, + IBCInvalidTimeoutTimestamp(msg_.packet.timeoutTimestamp, block.timestamp) + ); bytes memory commitmentPath = ICS24Host.packetCommitmentPathCalldata(msg_.packet.sourceChannel, msg_.packet.sequence); @@ -157,9 +148,7 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua relayer: _msgSender() }) ); - if (acks[0].length == 0) { - revert IBCAsyncAcknowledgementNotSupported(); - } + require(acks[0].length != 0, IBCAsyncAcknowledgementNotSupported()); writeAcknowledgement(msg_.packet, acks); @@ -173,21 +162,21 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua /// @inheritdoc IICS26Router function ackPacket(MsgAckPacket calldata msg_) external nonReentrant { // TODO: Support multi-payload packets #93 - if (msg_.packet.payloads.length != 1) { - revert IBCMultiPayloadPacketNotSupported(); - } + require(msg_.packet.payloads.length == 1, IBCMultiPayloadPacketNotSupported()); Payload calldata payload = msg_.packet.payloads[0]; IICS02ClientMsgs.CounterpartyInfo memory cInfo = ICS02_CLIENT.getCounterparty(msg_.packet.sourceChannel); - if (keccak256(bytes(cInfo.clientId)) != keccak256(bytes(msg_.packet.destChannel))) { - revert IBCInvalidCounterparty(cInfo.clientId, msg_.packet.destChannel); - } + require( + keccak256(bytes(cInfo.clientId)) == keccak256(bytes(msg_.packet.destChannel)), + IBCInvalidCounterparty(cInfo.clientId, msg_.packet.destChannel) + ); // this will revert if the packet commitment does not exist bytes32 storedCommitment = IBC_STORE.deletePacketCommitment(msg_.packet); - if (storedCommitment != ICS24Host.packetCommitmentBytes32(msg_.packet)) { - revert IBCPacketCommitmentMismatch(storedCommitment, ICS24Host.packetCommitmentBytes32(msg_.packet)); - } + require( + storedCommitment == ICS24Host.packetCommitmentBytes32(msg_.packet), + IBCPacketCommitmentMismatch(storedCommitment, ICS24Host.packetCommitmentBytes32(msg_.packet)) + ); bytes memory commitmentPath = ICS24Host.packetAcknowledgementCommitmentPathCalldata(msg_.packet.destChannel, msg_.packet.sequence); @@ -224,21 +213,21 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua /// @inheritdoc IICS26Router function timeoutPacket(MsgTimeoutPacket calldata msg_) external nonReentrant { // TODO: Support multi-payload packets #93 - if (msg_.packet.payloads.length != 1) { - revert IBCMultiPayloadPacketNotSupported(); - } + require(msg_.packet.payloads.length == 1, IBCMultiPayloadPacketNotSupported()); Payload calldata payload = msg_.packet.payloads[0]; IICS02ClientMsgs.CounterpartyInfo memory cInfo = ICS02_CLIENT.getCounterparty(msg_.packet.sourceChannel); - if (keccak256(bytes(cInfo.clientId)) != keccak256(bytes(msg_.packet.destChannel))) { - revert IBCInvalidCounterparty(cInfo.clientId, msg_.packet.destChannel); - } + require( + keccak256(bytes(cInfo.clientId)) == keccak256(bytes(msg_.packet.destChannel)), + IBCInvalidCounterparty(cInfo.clientId, msg_.packet.destChannel) + ); // this will revert if the packet commitment does not exist bytes32 storedCommitment = IBC_STORE.deletePacketCommitment(msg_.packet); - if (storedCommitment != ICS24Host.packetCommitmentBytes32(msg_.packet)) { - revert IBCPacketCommitmentMismatch(storedCommitment, ICS24Host.packetCommitmentBytes32(msg_.packet)); - } + require( + storedCommitment == ICS24Host.packetCommitmentBytes32(msg_.packet), + IBCPacketCommitmentMismatch(storedCommitment, ICS24Host.packetCommitmentBytes32(msg_.packet)) + ); bytes memory receiptPath = ICS24Host.packetReceiptCommitmentPathCalldata(msg_.packet.destChannel, msg_.packet.sequence); @@ -250,9 +239,10 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua }); uint256 counterpartyTimestamp = ICS02_CLIENT.getClient(msg_.packet.sourceChannel).membership(nonMembershipMsg); - if (counterpartyTimestamp < msg_.packet.timeoutTimestamp) { - revert IBCInvalidTimeoutTimestamp(msg_.packet.timeoutTimestamp, counterpartyTimestamp); - } + require( + counterpartyTimestamp >= msg_.packet.timeoutTimestamp, + IBCInvalidTimeoutTimestamp(msg_.packet.timeoutTimestamp, counterpartyTimestamp) + ); getIBCApp(payload.sourcePort).onTimeoutPacket( IIBCAppCallbacks.OnTimeoutPacketCallback({ diff --git a/src/utils/IBCStore.sol b/src/utils/IBCStore.sol index 839503c..25908d9 100644 --- a/src/utils/IBCStore.sol +++ b/src/utils/IBCStore.sol @@ -35,26 +35,26 @@ contract IBCStore is IIBCStore, IICS24HostErrors, Ownable { /// @inheritdoc IIBCStore function commitPacket(IICS26RouterMsgs.Packet memory packet) public onlyOwner { bytes32 path = ICS24Host.packetCommitmentKeyCalldata(packet.sourceChannel, packet.sequence); - if (commitments[path] != 0) { - revert IBCPacketCommitmentAlreadyExists( + require( + commitments[path] == 0, + IBCPacketCommitmentAlreadyExists( ICS24Host.packetCommitmentPathCalldata(packet.sourceChannel, packet.sequence) - ); - } + ) + ); bytes32 commitment = ICS24Host.packetCommitmentBytes32(packet); - emit PacketCommitted(path, commitment); commitments[path] = commitment; + emit PacketCommitted(path, commitment); } /// @inheritdoc IIBCStore function deletePacketCommitment(IICS26RouterMsgs.Packet memory packet) public onlyOwner returns (bytes32) { bytes32 path = ICS24Host.packetCommitmentKeyCalldata(packet.sourceChannel, packet.sequence); bytes32 commitment = commitments[path]; - if (commitment == 0) { - revert IBCPacketCommitmentNotFound( - ICS24Host.packetCommitmentPathCalldata(packet.sourceChannel, packet.sequence) - ); - } + require( + commitment != 0, + IBCPacketCommitmentNotFound(ICS24Host.packetCommitmentPathCalldata(packet.sourceChannel, packet.sequence)) + ); delete commitments[path]; return commitment; @@ -63,11 +63,12 @@ contract IBCStore is IIBCStore, IICS24HostErrors, Ownable { /// @inheritdoc IIBCStore function setPacketReceipt(IICS26RouterMsgs.Packet memory packet) public onlyOwner { bytes32 path = ICS24Host.packetReceiptCommitmentKeyCalldata(packet.destChannel, packet.sequence); - if (commitments[path] != 0) { - revert IBCPacketReceiptAlreadyExists( + require( + commitments[path] == 0, + IBCPacketReceiptAlreadyExists( ICS24Host.packetReceiptCommitmentPathCalldata(packet.destChannel, packet.sequence) - ); - } + ) + ); commitments[path] = ICS24Host.PACKET_RECEIPT_SUCCESSFUL_KECCAK256; } @@ -75,11 +76,12 @@ contract IBCStore is IIBCStore, IICS24HostErrors, Ownable { /// @inheritdoc IIBCStore function commitPacketAcknowledgement(IICS26RouterMsgs.Packet memory packet, bytes[] memory acks) public onlyOwner { bytes32 path = ICS24Host.packetAcknowledgementCommitmentKeyCalldata(packet.destChannel, packet.sequence); - if (commitments[path] != 0) { - revert IBCPacketAcknowledgementAlreadyExists( + require( + commitments[path] == 0, + IBCPacketAcknowledgementAlreadyExists( ICS24Host.packetAcknowledgementCommitmentPathCalldata(packet.destChannel, packet.sequence) - ); - } + ) + ); bytes32 commitment = ICS24Host.packetAcknowledgementCommitmentBytes32(acks); emit AckCommitted(path, commitment); diff --git a/src/utils/ICS20Lib.sol b/src/utils/ICS20Lib.sol index abd4208..aa8da93 100644 --- a/src/utils/ICS20Lib.sol +++ b/src/utils/ICS20Lib.sol @@ -106,40 +106,43 @@ library ICS20Lib { uint256 pos = 0; unchecked { - if (bytes32(bz[pos:pos + 10]) != bytes32("{\"denom\":\"")) { - revert IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32("{\"denom\":\""), bytes32(bz[pos:pos + 10])); - } + require( + bytes32(bz[pos:pos + 10]) == bytes32("{\"denom\":\""), + IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32("{\"denom\":\""), bytes32(bz[pos:pos + 10])) + ); (pd.denom, pos) = parseString(bz, pos + 10); - if (bytes32(bz[pos:pos + 11]) != bytes32(",\"amount\":\"")) { - revert IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32("{\"amount\":\""), bytes32(bz[pos:pos + 11])); - } + require( + bytes32(bz[pos:pos + 11]) == bytes32(",\"amount\":\""), + IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32("{\"amount\":\""), bytes32(bz[pos:pos + 11])) + ); (pd.amount, pos) = parseUint256String(bz, pos + 11); - if (bytes32(bz[pos:pos + 11]) != bytes32(",\"sender\":\"")) { - revert IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32(",\"sender\":\""), bytes32(bz[pos:pos + 11])); - } + require( + bytes32(bz[pos:pos + 11]) == bytes32(",\"sender\":\""), + IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32(",\"sender\":\""), bytes32(bz[pos:pos + 11])) + ); (pd.sender, pos) = parseString(bz, pos + 11); - if (bytes32(bz[pos:pos + 13]) != bytes32(",\"receiver\":\"")) { - revert IICS20Errors.ICS20JSONUnexpectedBytes( - pos, bytes32(",\"receiver\":\""), bytes32(bz[pos:pos + 13]) - ); - } + require( + bytes32(bz[pos:pos + 13]) == bytes32(",\"receiver\":\""), + IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32(",\"receiver\":\""), bytes32(bz[pos:pos + 13])) + ); (pd.receiver, pos) = parseString(bz, pos + 13); // check if the memo field is present, if not, we leave it empty if (pos != bz.length - 1 && uint256(uint8(bz[pos + 2])) == CHAR_M) { - if (bytes32(bz[pos:pos + 9]) != bytes32(",\"memo\":\"")) { - // solhint-disable-next-line max-line-length - revert IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32(",\"memo\":\""), bytes32(bz[pos:pos + 9])); - } + require( + bytes32(bz[pos:pos + 9]) == bytes32(",\"memo\":\""), + IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32(",\"memo\":\""), bytes32(bz[pos:pos + 9])) + ); (pd.memo, pos) = parseString(bz, pos + 9); } - if (pos != bz.length - 1 || uint256(uint8(bz[pos])) != CHAR_CLOSING_BRACE) { - revert IICS20Errors.ICS20JSONClosingBraceNotFound(pos, bz[pos]); - } + require( + pos == bz.length - 1 && uint256(uint8(bz[pos])) == CHAR_CLOSING_BRACE, + IICS20Errors.ICS20JSONClosingBraceNotFound(pos, bz[pos]) + ); } return pd; @@ -160,9 +163,10 @@ library ICS20Lib { } ret = ret * 10 + (c - 48); } - if (pos >= bz.length || uint256(uint8(bz[pos])) != CHAR_DOUBLE_QUOTE) { - revert IICS20Errors.ICS20JSONStringClosingDoubleQuoteNotFound(pos, bz[pos]); - } + require( + pos < bz.length && uint256(uint8(bz[pos])) == CHAR_DOUBLE_QUOTE, + IICS20Errors.ICS20JSONStringClosingDoubleQuoteNotFound(pos, bz[pos]) + ); return (ret, pos + 1); } } @@ -181,58 +185,17 @@ library ICS20Lib { } else if (c == CHAR_BACKSLASH && i + 1 < bz.length) { i++; c = uint256(uint8(bz[i])); - if ( - c != CHAR_DOUBLE_QUOTE && c != CHAR_SLASH && c != CHAR_BACKSLASH && c != CHAR_F && c != CHAR_R - && c != CHAR_N && c != CHAR_B && c != CHAR_T - ) { - revert IICS20Errors.ICS20JSONInvalidEscape(i, bz[i]); - } + require( + c == CHAR_DOUBLE_QUOTE || c == CHAR_SLASH || c == CHAR_BACKSLASH || c == CHAR_F || c == CHAR_R + || c == CHAR_N || c == CHAR_B || c == CHAR_T, + IICS20Errors.ICS20JSONInvalidEscape(i, bz[i]) + ); } } } revert IICS20Errors.ICS20JSONStringUnclosed(bz, pos); } - /// @notice isEscapedJSONString checks if a string is escaped JSON. - /// @param s string - /// @return true if the string is escaped JSON - function isEscapedJSONString(string calldata s) private pure returns (bool) { - bytes memory bz = bytes(s); - unchecked { - for (uint256 i = 0; i < bz.length; i++) { - uint256 c = uint256(uint8(bz[i])); - if (c == CHAR_DOUBLE_QUOTE) { - return false; - } else if (c == CHAR_BACKSLASH && i + 1 < bz.length) { - i++; - c = uint256(uint8(bz[i])); - if ( - c != CHAR_DOUBLE_QUOTE && c != CHAR_SLASH && c != CHAR_BACKSLASH && c != CHAR_F && c != CHAR_R - && c != CHAR_N && c != CHAR_B && c != CHAR_T - ) { - return false; - } - } - } - } - return true; - } - - /// @notice isEscapeNeededString checks if a string needs to be escaped. - /// @param bz bytes - /// @return true if the string needs to be escaped - function isEscapeNeededString(bytes memory bz) private 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; - } - } - } - return false; - } - /// @notice hexStringToAddress converts a hex string to an address. /// @param addrHexString hex address string /// @return address value @@ -267,9 +230,7 @@ library ICS20Lib { /// @return address the converted address function mustHexStringToAddress(string memory addrHexString) internal pure returns (address) { (address addr, bool success) = hexStringToAddress(addrHexString); - if (!success) { - revert IICS20Errors.ICS20InvalidAddress(addrHexString); - } + require(success, IICS20Errors.ICS20InvalidAddress(addrHexString)); return addr; } diff --git a/src/utils/ICS24Host.sol b/src/utils/ICS24Host.sol index 2adede6..62ee2ce 100644 --- a/src/utils/ICS24Host.sol +++ b/src/utils/ICS24Host.sol @@ -167,9 +167,7 @@ library ICS24Host { /// @param path The path to append /// @return The prefixed path function prefixedPath(bytes[] memory merklePrefix, bytes memory path) internal pure returns (bytes[] memory) { - if (merklePrefix.length == 0) { - revert IICS24HostErrors.InvalidMerklePrefix(merklePrefix); - } + require(merklePrefix.length > 0, IICS24HostErrors.InvalidMerklePrefix(merklePrefix)); merklePrefix[merklePrefix.length - 1] = abi.encodePacked(merklePrefix[merklePrefix.length - 1], path); return merklePrefix; diff --git a/test/ICS20LibTest.t.sol b/test/ICS20LibTest.t.sol index 8980aca..7d33e64 100644 --- a/test/ICS20LibTest.t.sol +++ b/test/ICS20LibTest.t.sol @@ -5,6 +5,7 @@ pragma solidity ^0.8.28; import { Test } from "forge-std/Test.sol"; import { ICS20Lib } from "../src/utils/ICS20Lib.sol"; +import { IICS20Errors } from "../src/errors/IICS20Errors.sol"; contract ICS20LibTest is Test { struct IBCDenomTestCase { @@ -34,7 +35,7 @@ contract ICS20LibTest is Test { } } - function test_unmarshalJSON() public view { + function test_unmarshalJSON() public { // ICS20Lib marshalled json with memo bytes memory jsonBz = ICS20Lib.marshalJSON("denom", 42, "sender", "receiver", "memo"); ICS20Lib.PacketDataJSON memory packetData = this.unmarshalJSON(jsonBz); @@ -54,8 +55,9 @@ contract ICS20LibTest is Test { assertEq(packetData.memo, ""); // Test with a manual JSON string with memo - jsonBz = - "{\"denom\":\"denom3\",\"amount\":\"43\",\"sender\":\"sender3\",\"receiver\":\"receiver3\",\"memo\":\"memo3\"}"; + jsonBz = bytes( + "{\"denom\":\"denom3\",\"amount\":\"43\",\"sender\":\"sender3\",\"receiver\":\"receiver3\",\"memo\":\"memo3\"}" + ); packetData = this.unmarshalJSON(jsonBz); assertEq(packetData.denom, "denom3"); assertEq(packetData.amount, 43); @@ -64,13 +66,18 @@ contract ICS20LibTest is Test { assertEq(packetData.memo, "memo3"); // Test with a manual JSON string without memo - jsonBz = "{\"denom\":\"denom3\",\"amount\":\"43\",\"sender\":\"sender3\",\"receiver\":\"receiver3\"}"; + jsonBz = bytes("{\"denom\":\"denom3\",\"amount\":\"43\",\"sender\":\"sender3\",\"receiver\":\"receiver3\"}"); packetData = this.unmarshalJSON(jsonBz); assertEq(packetData.denom, "denom3"); assertEq(packetData.amount, 43); assertEq(packetData.sender, "sender3"); assertEq(packetData.receiver, "receiver3"); assertEq(packetData.memo, ""); + + // Test with a broken JSON string without memo + jsonBz = bytes("{\"denom\":\"denom3\",\"amount\":\"43\",\"sender\":\"sender3\\,\"receiver\":\"receiver3\"}"); + vm.expectRevert(abi.encodeWithSelector(IICS20Errors.ICS20JSONInvalidEscape.selector, 50, bytes1(0x2c))); + packetData = this.unmarshalJSON(jsonBz); } function unmarshalJSON(bytes calldata bz) external pure returns (ICS20Lib.PacketDataJSON memory) { diff --git a/test/IbcIdentifiersTest.t.sol b/test/IbcIdentifiersTest.t.sol index be60be0..075acbe 100644 --- a/test/IbcIdentifiersTest.t.sol +++ b/test/IbcIdentifiersTest.t.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.28; -// solhint-disable gas-custom-errors,max-line-length +// solhint-disable max-line-length import { Test } from "forge-std/Test.sol"; import { IBCIdentifiers } from "../src/utils/IBCIdentifiers.sol";