Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: Remove unused fields verifyingContract & salt from EIP712 domain separator #75

Merged
merged 6 commits into from
Sep 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,10 @@ jobs:
go.sum
- uses: golangci/golangci-lint-action@v3.3.1
with:
# Required: the version of golangci-lint is required and must be specified without patch version: we always use the latest patch version.
version: latest
# Required: the version of golangci-lint is required and must be
# specified without patch version: Newer versions are currently not
# passing.
version: v1.59
args: --timeout 10m
github-token: ${{ secrets.github_token }}
# Check only if there are differences in the source code
Expand Down
4 changes: 1 addition & 3 deletions ethereum/eip712/msg_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,7 @@ func TestExtractMsgTypes(t *testing.T) {
"EIP712Domain": [
{ "name": "name", "type": "string" },
{ "name": "version", "type": "string" },
{ "name": "chainId", "type": "uint256" },
{ "name": "verifyingContract", "type": "string" },
{ "name": "salt", "type": "string" }
{ "name": "chainId", "type": "uint256" }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expected EIP712Domain has the two fields removed now

],
"Fee": [
{ "name": "amount", "type": "Coin[]" },
Expand Down
32 changes: 19 additions & 13 deletions ethereum/eip712/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,25 @@

func getTypedDataDomain(chainID uint64) apitypes.TypedDataDomain {
return apitypes.TypedDataDomain{
Name: "Kava Cosmos",
Version: "1.0.0",
ChainId: math.NewHexOrDecimal256(int64(chainID)),
VerifyingContract: "kavaCosmos",
Salt: "0",
Name: "Kava Cosmos",
Version: "1.0.0",
ChainId: math.NewHexOrDecimal256(int64(chainID)),

Check failure

Code scanning / gosec

Potential integer overflow by integer type conversion Error

Potential integer overflow by integer type conversion

// Fields below are not used signature verification so they are
// explicitly set empty to exclude them from the hash to be signed.

// Salt in most cases is not used, other chains sometimes set the
// chainID as the salt instead of using the chainId field and not
// together.
// Discussion on salt usage:
// https://github.com/OpenZeppelin/openzeppelin-contracts/issues/4318
Salt: "",

// VerifyingContract is empty as there is no contract that is verifying
// the signature. Signature verification is done in the ante handler.
// Smart contracts that handle EIP712 signatures will include their own
// address in the domain separator.
VerifyingContract: "",
}
}

Expand All @@ -30,14 +44,6 @@
Name: "chainId",
Type: "uint256",
},
{
Name: "verifyingContract",
Type: "string",
},
{
Name: "salt",
Type: "string",
},
},
"Tx": {
{Name: "account_number", Type: "string"},
Expand Down
27 changes: 27 additions & 0 deletions ethereum/eip712/types_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package eip712

import (
"testing"

"github.com/stretchr/testify/require"
)

func TestTypedDataDomain(t *testing.T) {
domain := getTypedDataDomain(1234)

domainMap := domain.Map()

// Verify both len and expected contents in order to assert that no other
// fields are present
require.Len(t, domainMap, 3)
require.Contains(t, domainMap, "chainId")
require.Contains(t, domainMap, "name")
require.Contains(t, domainMap, "version")

// Extra check to ensure that the fields that are not used for signature
// verification are not present in the map. Should be in conjunction with
// the checks above to ensure there isn't a different variant of these
// fields present, e.g. different casing.
require.NotContains(t, domainMap, "verifyingContract")
require.NotContains(t, domainMap, "salt")
}
Loading