From 113fd5fdb8ed49c2d3b041a96f45ba6670d1e56b Mon Sep 17 00:00:00 2001
From: drklee3 <github@dlee.dev>
Date: Thu, 19 Sep 2024 11:42:35 -0700
Subject: [PATCH] fix!: Remove unused fields verifyingContract & salt from
 EIP712 domain separator (#75)

verifyingContract field is validated by MetaMask to be an address, but was expected to be a string. This field is unused so it is removed.
---
 .github/workflows/lint.yml    |  6 ++++--
 ethereum/eip712/msg_test.go   |  4 +---
 ethereum/eip712/types.go      | 32 +++++++++++++++++++-------------
 ethereum/eip712/types_test.go | 27 +++++++++++++++++++++++++++
 4 files changed, 51 insertions(+), 18 deletions(-)
 create mode 100644 ethereum/eip712/types_test.go

diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
index bf7bb9977e..16e110d9f7 100644
--- a/.github/workflows/lint.yml
+++ b/.github/workflows/lint.yml
@@ -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
diff --git a/ethereum/eip712/msg_test.go b/ethereum/eip712/msg_test.go
index 00b40dbe9a..aa9f217332 100644
--- a/ethereum/eip712/msg_test.go
+++ b/ethereum/eip712/msg_test.go
@@ -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" }
 				],
 				"Fee": [
 					{ "name": "amount", "type": "Coin[]" },
diff --git a/ethereum/eip712/types.go b/ethereum/eip712/types.go
index 2cef4539a1..db3825814f 100644
--- a/ethereum/eip712/types.go
+++ b/ethereum/eip712/types.go
@@ -7,11 +7,25 @@ import (
 
 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)),
+
+		// 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: "",
 	}
 }
 
@@ -30,14 +44,6 @@ func getRootTypes() apitypes.Types {
 				Name: "chainId",
 				Type: "uint256",
 			},
-			{
-				Name: "verifyingContract",
-				Type: "string",
-			},
-			{
-				Name: "salt",
-				Type: "string",
-			},
 		},
 		"Tx": {
 			{Name: "account_number", Type: "string"},
diff --git a/ethereum/eip712/types_test.go b/ethereum/eip712/types_test.go
new file mode 100644
index 0000000000..750456a6c4
--- /dev/null
+++ b/ethereum/eip712/types_test.go
@@ -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")
+}