feat: use abi encoding of ICS20Transfer payload #117

gjermundgaraba · 2024-11-24T13:55:39Z

Description

closes: #116

Based off of the work done in: #102

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
Wrote unit and integration tests.
Added relevant natspec and godoc comments.
Provide a conventional commit message to follow the repository standards.
Re-reviewed Files changed in the GitHub PR explorer.
Review SonarCloud Report in the comment section below once CI passes.

codecov · 2024-11-24T14:00:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.89%. Comparing base (d952d31) to head (2efc62f).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #117      +/-   ##
==========================================
- Coverage   94.10%   93.89%   -0.21%     
==========================================
  Files           9        9              
  Lines         339      311      -28     
==========================================
- Hits          319      292      -27     
+ Misses         20       19       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…er-solidity-2

.github/workflows/foundry.yml

gjermundgaraba · 2024-11-24T16:35:46Z

abigen/go.mod

@@ -0,0 +1,42 @@
+module github.com/cosmos/solidity-ibc-eureka/abigen
+
+go 1.22.7


Using the setup as the e2e tests. But maybe we should just fully update it to go 1.23 soon...

Feel free to open a deps(e2e) pr anytime, and I'll approve fast :)

gjermundgaraba · 2024-11-24T16:38:18Z

e2e/interchaintestv8/chainconfig/chain_config.go

-					Repository: "ghcr.io/cosmos/ibc-go-wasm-simd", // FOR LOCAL IMAGE USE: Docker Image Name
-					Version:    "feat-ibc-eureka",                 // FOR LOCAL IMAGE USE: Docker Image Tag
+					Repository: "ghcr.io/cosmos/ibc-go-wasm-simd",  // FOR LOCAL IMAGE USE: Docker Image Name
+					Version:    "gjermund-7591-ics20-abi-encoding", // FOR LOCAL IMAGE USE: Docker Image Tag


This one can only be updated once cosmos/ibc-go#7592 is merged. We could wait, but I'd rather just update this in a PR so we're not blocked on ibc-go reviewing which might take time.

Hmm, should it be merged soon ahead of the security audit?

Yes, I would think so.

gjermundgaraba · 2024-11-24T16:40:52Z

justfile

@@ -39,12 +44,14 @@ generate-abi:
 	jq '.abi' out/ERC20.sol/ERC20.json > abi/ERC20.json
 	jq '.abi' out/IBCERC20.sol/IBCERC20.json > abi/IBCERC20.json
 	jq '.abi' out/IBCStore.sol/IBCStore.json > abi/IBCStore.json
-	abigen --abi abi/ICS02Client.json --pkg ics02client --type Contract --out e2e/interchaintestv8/types/ics02client/contract.go


Mostly just different ordering to have some level of separation between types generated for e2e tests only and the ones that should be public in abigen/.

That, and adding ICS20Lib.

We should also remove the files from types that have been moved to abigen.

That was done already

gjermundgaraba · 2024-11-24T19:10:52Z

A quick comment on the test coverage. I believe it went down a little bit because some of the old parsing logic was well tested... I looked over the main parts of the ICS20Lib that has some branches that are not unit tested right now, and it does not seem worth spending time on them, as they will all be replaced by new functions from OpenZeppelin in their next release (which should be early next year it looks like).

srdtrk

Lgtm. I left a couple comments

.github/workflows/foundry.yml

srdtrk · 2024-11-26T04:19:49Z

README.md

-| `multicall/recvPacket` | Receiving _back_ an `ERC20` token. | ~228,000 | ~220,000 |
-| `multicall/ackPacket` | Acknowledging an ICS20 packet. | ~141,000 | ~135,000 |
+| `multicall/recvPacket` | Receiving _back_ an `ERC20` token. | ~208,000 | ~201,000 |
+| `multicall/ackPacket` | Acknowledging an ICS20 packet. | ~121,000 | ~115,000 |


srdtrk · 2024-11-26T04:20:37Z

abigen/go.mod

@@ -0,0 +1,42 @@
+module github.com/cosmos/solidity-ibc-eureka/abigen
+
+go 1.22.7


Feel free to open a deps(e2e) pr anytime, and I'll approve fast :)

srdtrk · 2024-11-26T04:22:44Z

e2e/interchaintestv8/chainconfig/chain_config.go

-					Repository: "ghcr.io/cosmos/ibc-go-wasm-simd", // FOR LOCAL IMAGE USE: Docker Image Name
-					Version:    "feat-ibc-eureka",                 // FOR LOCAL IMAGE USE: Docker Image Tag
+					Repository: "ghcr.io/cosmos/ibc-go-wasm-simd",  // FOR LOCAL IMAGE USE: Docker Image Name
+					Version:    "gjermund-7591-ics20-abi-encoding", // FOR LOCAL IMAGE USE: Docker Image Tag


Hmm, should it be merged soon ahead of the security audit?

e2e/interchaintestv8/go.mod

srdtrk · 2024-11-26T04:25:56Z

src/utils/ICS20Lib.sol

+    /**
+     * @notice Encodes a `FungibleTokenPacketData` struct into ABI-encoded bytes.
+     *
+     * @dev This function uses `abi.encode` to convert a `FungibleTokenPacketData` struct into its ABI-encoded
+     * `bytes` representation. The resulting bytes can be transmitted or stored for decoding later.
+     *
+     * @param payload The `FungibleTokenPacketData` struct to encode.
+     * @return Encoded `bytes` representation of the input struct.
+     *
+     * @dev What this function does:
+     * - Converts the `FungibleTokenPacketData` struct into a `bytes` array using ABI encoding.
+     * - Ensures the resulting bytes are compatible with `abi.decode` for the same struct.
+     * - Preserves the field order and data structure of the input struct during encoding.
+     *
+     * @dev What this function does NOT do:
+     * - It does not validate the content of the `FungibleTokenPacketData` struct before encoding.
+     *   For example:
+     *     - Does not check if `amount` is greater than 0.
+     *     - Does not verify that `receiver` or `sender` are valid addresses or non-empty strings.
+     *     - Does not validate the format or expected content of `denom` or `memo`.
+     * - It does not ensure compatibility with external systems unless they adhere to the same ABI encoding rules.
+     *
+     * @dev Recommended validation to avoid issues:
+     * - Validate the fields of the `FungibleTokenPacketData` struct before calling this function:
+     *   - Ensure `amount > 0`.
+     *   - Check that `receiver` and `sender` are non-empty and, if required, valid address strings.
+     *   - Validate `denom` against expected formats or whitelisted values, if applicable.
+     *   - Optionally validate `memo` for length or allowed characters.
+     * - Ensure that the consumer of the encoded bytes uses the same ABI decoding standard.
+     */
+    function encodePayload(FungibleTokenPacketData memory payload) internal pure returns (bytes memory) {
+        return abi.encode(payload);
+    }
+
+    /**
+     * @notice Decodes ABI-encoded bytes into a `FungibleTokenPacketData` struct.
+     *
+     * @dev This function uses `abi.decode` to decode a `bytes` payload into a `FungibleTokenPacketData` struct.
+     * It assumes that the input data is correctly ABI-encoded and matches the structure of `FungibleTokenPacketData`.
+     *
+     * @param data ABI-encoded bytes representing a `FungibleTokenPacketData`.
+     * @return Decoded `FungibleTokenPacketData` struct.
+     *
+     * @dev What this function does:
+     * - Decodes the `bytes` payload into the expected `FungibleTokenPacketData` struct.
+     * - Ensures that the payload conforms to the ABI encoding of `FungibleTokenPacketData` (field types and order).
+     * - Reverts if the input data is not properly ABI-encoded.
+     *
+     * @dev What this function does NOT do:
+     * - Validate the logical correctness or semantic meaning of the decoded fields.
+     *   For example:
+     *     - Does not check if `amount` is greater than 0.
+     *     - Does not verify that `receiver` or `sender` are valid addresses or non-empty strings.
+     *     - Does not validate the format, length, or expected content of `denom` or `memo`.
+     * - Does not validate whether the payload matches a specific JSON schema or key order.
+     *
+     * @dev Recommended validation to avoid exploits:
+     * - After decoding, validate each field of the struct:
+     *   - Ensure `amount > 0`.
+     *   - Check that `receiver` and `sender` are non-empty and, if required, valid address strings.
+     *   - Validate `denom` against expected formats or whitelisted values, if applicable.
+     *   - Optionally validate `memo` for length or allowed characters.
+     * - Implement JSON key-order validation if strict ordering is required.
+     * - Consider using a try/catch block for decoding, or handle decoding errors explicitly to ensure
+     *   the function does not fail silently or revert without providing clear error messages.
+     */
+    function decodePayload(bytes memory data) external pure returns (FungibleTokenPacketData memory) {
+        return abi.decode(data, (FungibleTokenPacketData));
+    }


Do we need these functions given that they don't perform any validation. I also don't support making these functions external to be used in e2e tests. We should keep the number of external functions as small as possible.

That is fair actually. I'll remove them.

Looks like this actually saved us some gas, so updated the benchmarks again :)

srdtrk · 2024-11-26T04:29:27Z

justfile

@@ -39,12 +44,14 @@ generate-abi:
 	jq '.abi' out/ERC20.sol/ERC20.json > abi/ERC20.json
 	jq '.abi' out/IBCERC20.sol/IBCERC20.json > abi/IBCERC20.json
 	jq '.abi' out/IBCStore.sol/IBCStore.json > abi/IBCStore.json
-	abigen --abi abi/ICS02Client.json --pkg ics02client --type Contract --out e2e/interchaintestv8/types/ics02client/contract.go


We should also remove the files from types that have been moved to abigen.

sangier and others added 13 commits November 13, 2024 18:29

start abi-encoding

c8b932f

fix transfer tests

0d643e0

fix test, del json related code

b6f5619

rm try catch

36d0194

lint

1f7c77a

Merge branch 'main' into stefano/95-abi-encoding

01ef938

rename PacketDataJSON

13d95a1

lint

a61644d

fixtures and update benchmark

569bb1c

move generated types into abigen folder

3e2c74b

add comment on abi decoding

62d4be5

document abigen additions more

1a0be5c

remove unused ics20lib errors

9c5952c

gjermundgaraba linked an issue Nov 24, 2024 that may be closed by this pull request

Use ABI encoding in ICS20Transfer (solidity) #116

Open

gjermundgaraba added 10 commits November 24, 2024 16:52

Merge branch 'main' into gjermund/116-use-abi-encoding-in-ics20transf…

938fca4

…er-solidity-2

add lint and test to abigen CI workflow

69f2e14

clean up abigen workflow

afc5fee

clean up more in abigen workflow

3eaeb64

use yml instead of yaml

f917257

rename

5cebb4c

fix syntax

df69446

fix syntax

2f49530

generate abi

7a74b4d

Merge branch 'main' into gjermund/116-use-abi-encoding-in-ics20transf…

1da63c9

…er-solidity-2

gjermundgaraba commented Nov 24, 2024

View reviewed changes

.github/workflows/foundry.yml Outdated Show resolved Hide resolved

gjermundgaraba commented Nov 24, 2024

View reviewed changes

clean up ICS20TransferTest

07163f2

gjermundgaraba added 2 commits November 24, 2024 18:57

clean up more tests

965faec

clean up IntegrationTest

2977ae9

gjermundgaraba marked this pull request as ready for review November 24, 2024 18:54

gjermundgaraba requested a review from srdtrk as a code owner November 24, 2024 18:54

srdtrk approved these changes Nov 26, 2024

View reviewed changes

gjermundgaraba added 2 commits November 26, 2024 11:54

code review fixes

c9b073b

update abigen

2efc62f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use abi encoding of ICS20Transfer payload #117

feat: use abi encoding of ICS20Transfer payload #117

gjermundgaraba commented Nov 24, 2024 •

edited

Loading

codecov bot commented Nov 24, 2024 •

edited

Loading

gjermundgaraba Nov 24, 2024

srdtrk Nov 26, 2024

gjermundgaraba Nov 24, 2024

srdtrk Nov 26, 2024

gjermundgaraba Nov 26, 2024

gjermundgaraba Nov 24, 2024 •

edited

Loading

srdtrk Nov 26, 2024

gjermundgaraba Nov 26, 2024

gjermundgaraba commented Nov 24, 2024

srdtrk left a comment

srdtrk Nov 26, 2024

srdtrk Nov 26, 2024

srdtrk Nov 26, 2024

srdtrk Nov 26, 2024

gjermundgaraba Nov 26, 2024

gjermundgaraba Nov 26, 2024

srdtrk Nov 26, 2024

		@@ -0,0 +1,42 @@
		module github.com/cosmos/solidity-ibc-eureka/abigen

		go 1.22.7

feat: use abi encoding of ICS20Transfer payload #117

Are you sure you want to change the base?

feat: use abi encoding of ICS20Transfer payload #117

Conversation

gjermundgaraba commented Nov 24, 2024 • edited Loading

Description

codecov bot commented Nov 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjermundgaraba Nov 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjermundgaraba commented Nov 24, 2024

srdtrk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjermundgaraba commented Nov 24, 2024 •

edited

Loading

codecov bot commented Nov 24, 2024 •

edited

Loading

gjermundgaraba Nov 24, 2024 •

edited

Loading