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.

Sign up for GitHub

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: Use canonical hexadecimal strings for Eip155 address encoding #2120

Merged
merged 7 commits into from
Dec 6, 2024
Merged

fix: Use canonical hexadecimal strings for Eip155 address encoding #2120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
20cc19e
refactor: funtoken erc20 addr to be bytes in proto
CalicoNino Dec 3, 2024
43578e2
refactor: marshall to return address
CalicoNino Dec 3, 2024
3081cff
fix: eip55 proto encoding using hex string
CalicoNino Dec 6, 2024
d1747ab
Merge branch 'main' into nino/funtoken
CalicoNino Dec 6, 2024
7c06ddd
fix: typos
CalicoNino Dec 6, 2024
aeacfbc
--amend
CalicoNino Dec 6, 2024
9aa15f7
Update x/evm/keeper/funtoken_from_erc20_test.go
Unique-Divine Dec 6, 2024
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
9 changes: 5 additions & 4 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,22 +45,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- [#2119](https://github.com/NibiruChain/nibiru/pull/2119) - fix(evm): Guarantee
that gas consumed during any send operation of the "NibiruBankKeeper" depends
only on the "bankkeeper.BaseKeeper"'s gas consumption.
- [#2120](https://github.com/NibiruChain/nibiru/pull/2120) - fix: Use canonical hexadecimal strings for Eip155 address encoding

#### Nibiru EVM | Before Audit 2 - 2024-12-06

The codebase went through a third-party [Code4rena
Zenith](https://code4rena.com/zenith) Audit, running from 2024-10-07 until
2024-11-01 and including both a primary review period and mitigation/remission
period. This section describes code changes that occured after that audit in
period. This section describes code changes that occurred after that audit in
preparation for a second audit starting in November 2024.

- [#2068](https://github.com/NibiruChain/nibiru/pull/2068) - feat: enable wasm light clients on IBC (08-wasm)
- [#2074](https://github.com/NibiruChain/nibiru/pull/2074) - fix(evm-keeper): better utilize ERC20 metadata during FunToken creation. The bank metadata for a new FunToken mapping ties a connection between the Bank Coin's `DenomUnit` and the ERC20 contract metadata like the name, decimals, and symbol. This change brings parity between EVM wallets, such as MetaMask, and Interchain wallets like Keplr and Leap.
- [#2076](https://github.com/NibiruChain/nibiru/pull/2076) - fix(evm-gas-fees):
Use effective gas price in RefundGas and make sure that units are properly
reflected on all occurences of "base fee" in the codebase. This fixes [#2059](https://github.com/NibiruChain/nibiru/issues/2059)
reflected on all occurrences of "base fee" in the codebase. This fixes [#2059](https://github.com/NibiruChain/nibiru/issues/2059)
and the [related comments from @Unique-Divine and @berndartmueller](https://github.com/NibiruChain/nibiru/issues/2059#issuecomment-2408625724).
- [#2084](https://github.com/NibiruChain/nibiru/pull/2084) - feat(evm-forge): foundry support and template for Nibiru EVM develoment
- [#2084](https://github.com/NibiruChain/nibiru/pull/2084) - feat(evm-forge): foundry support and template for Nibiru EVM development
- [#2086](https://github.com/NibiruChain/nibiru/pull/2086) - fix(evm-precomples):
Fix state consistency in precompile execution by ensuring proper journaling of
state changes in the StateDB. This pull request makes sure that state is
Expand Down Expand Up @@ -115,7 +116,7 @@ tests for race conditions within funtoken precompile
timestamps resulting from ctx.WithBlock* don't actually correspond to the block
header information from specified blocks in the chain's history, so the oracle
exchange rates need a way to correctly retrieve this information. This change
fixes that discrepency, giving the expected block timesamp for the EVM's oracle
fixes that discrepancy, giving the expected block timestamp for the EVM's oracle
precompiled contract. The change also simplifies and corrects the code in x/oracle.

#### Nibiru EVM | Before Audit 1 - 2024-10-18
Expand Down
25 changes: 10 additions & 15 deletions eth/eip55.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package eth

import (
"encoding/json"
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -36,53 +35,49 @@
// Marshal implements the gogo proto custom type interface.
// Ref: https://github.com/cosmos/gogoproto/blob/v1.5.0/custom_types.md
func (h EIP55Addr) Marshal() ([]byte, error) {
return h.Bytes(), nil
return []byte(h.Address.Hex()), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incorrect Protobuf Serialization Format in Marshal Method

The Marshal method should return the raw bytes of the address to conform with protobuf serialization expectations. Returning the hexadecimal string representation ([]byte(h.Address.Hex())) may lead to serialization issues.

Suggested fix:

func (h EIP55Addr) Marshal() ([]byte, error) {
-	return []byte(h.Address.Hex()), nil
+	return h.Address.Bytes(), nil
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return []byte(h.Address.Hex()), nil
return h.Address.Bytes(), nil

}

// MarshalJSON returns the [EIP55Addr] as JSON bytes.
// Implements the gogo proto custom type interface.
// Ref: https://github.com/cosmos/gogoproto/blob/v1.5.0/custom_types.md
func (h EIP55Addr) MarshalJSON() ([]byte, error) {
return json.Marshal(h.String())
return []byte(h.String()), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Invalid JSON Encoding in MarshalJSON Method

Directly returning []byte(h.String()) bypasses JSON encoding and may result in invalid JSON output. The MarshalJSON method should use json.Marshal(h.String()) to ensure proper JSON formatting and escaping.

Proposed fix:

func (h EIP55Addr) MarshalJSON() ([]byte, error) {
-	return []byte(h.String()), nil
+	return json.Marshal(h.String())
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return []byte(h.String()), nil
return json.Marshal(h.String())

}

// MarshalTo serializes a EIP55Addr directly into a pre-allocated byte slice ("data").
// MarshalTo implements the gogo proto custom type interface.
// Implements the gogo proto custom type interface.
// Ref: https://github.com/cosmos/gogoproto/blob/v1.5.0/custom_types.md
func (h *EIP55Addr) MarshalTo(data []byte) (n int, err error) {
copy(data, h.Bytes())
copy(data, []byte(h.Address.Hex()))

Check warning on line 53 in eth/eip55.go

View check run for this annotation

Codecov / codecov/patch

eth/eip55.go#L53 

Added line #L53 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incorrect Serialization in MarshalTo Method

The MarshalTo method should copy the raw bytes of the address into the data slice to conform with protobuf serialization requirements. Copying the hexadecimal string representation ([]byte(h.Address.Hex())) is inappropriate and may cause serialization issues.

Suggested fix:

func (h *EIP55Addr) MarshalTo(data []byte) (n int, err error) {
-	copy(data, []byte(h.Address.Hex()))
+	copy(data, h.Address.Bytes())
	return h.Size(), nil
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
copy(data, []byte(h.Address.Hex()))
copy(data, h.Address.Bytes())

return h.Size(), nil
}

// Unmarshal implements the gogo proto custom type interface.
// Ref: https://github.com/cosmos/gogoproto/blob/v1.5.0/custom_types.md
func (h *EIP55Addr) Unmarshal(data []byte) error {
addr := gethcommon.BytesToAddress(data)
*h = EIP55Addr{Address: addr}
addr, err := NewEIP55AddrFromStr(string(data))
if err != nil {
return err
}

Check warning on line 63 in eth/eip55.go

View check run for this annotation

Codecov / codecov/patch

eth/eip55.go#L62-L63 

Added lines #L62 - L63 were not covered by tests
*h = addr
Comment on lines +60 to +64
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incorrect Deserialization in Unmarshal Method

The Unmarshal method should reconstruct the address from the raw bytes provided, not from a string representation. Converting data to a string and parsing it may result in incorrect deserialization, especially if the original data is not a valid string.

Suggested fix:

func (h *EIP55Addr) Unmarshal(data []byte) error {
-	addr, err := NewEIP55AddrFromStr(string(data))
-	if err != nil {
-		return err
-	}
-	*h = addr
+	if len(data) != gethcommon.AddressLength {
+		return fmt.Errorf("invalid EIP55Addr length: %d", len(data))
+	}
+	h.Address = gethcommon.BytesToAddress(data)
	return nil
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
addr, err := NewEIP55AddrFromStr(string(data))
if err != nil {
return err
}
*h = addr
if len(data) != gethcommon.AddressLength {
return fmt.Errorf("invalid EIP55Addr length: %d", len(data))
}
h.Address = gethcommon.BytesToAddress(data)
return nil

return nil
}

// UnmarshalJSON implements the gogo proto custom type interface.
// Ref: https://github.com/cosmos/gogoproto/blob/v1.5.0/custom_types.md
func (h *EIP55Addr) UnmarshalJSON(bz []byte) error {
text := new(string)
if err := json.Unmarshal(bz, text); err != nil {
return err
}

addr, err := NewEIP55AddrFromStr(*text)
addr, err := NewEIP55AddrFromStr(string(bz))
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incorrect JSON Decoding in UnmarshalJSON Method

The UnmarshalJSON method should use json.Unmarshal to correctly parse the JSON-encoded data. Directly converting bz to a string and parsing it may fail to handle JSON encoding properly, especially with quotation marks.

Proposed fix:

func (h *EIP55Addr) UnmarshalJSON(bz []byte) error {
-	addr, err := NewEIP55AddrFromStr(string(bz))
+	var addrStr string
+	if err := json.Unmarshal(bz, &addrStr); err != nil {
+		return err
+	}
+	addr, err := NewEIP55AddrFromStr(addrStr)
	if err != nil {
		return err
	}
	*h = addr
	return nil
}

Committable suggestion skipped: line range outside the PR's diff.

if err != nil {
return err
}

*h = addr

return nil
}

// Size implements the gogo proto custom type interface.
// Ref: https://github.com/cosmos/gogoproto/blob/v1.5.0/custom_types.md
func (h EIP55Addr) Size() int {
return len(h.Bytes())
return len([]byte(h.Address.Hex()))
CalicoNino marked this conversation as resolved.
Show resolved Hide resolved
}
32 changes: 27 additions & 5 deletions eth/eip55_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,15 +116,15 @@ func (s *EIP55AddrSuite) TestProtobufEncoding() {
}{
{
input: threeValidAddrs[0],
expectedJson: "\"0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed\"",
expectedJson: "0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed",
},
{
input: threeValidAddrs[1],
expectedJson: "\"0xAe967917c465db8578ca9024c205720b1a3651A9\"",
expectedJson: "0xAe967917c465db8578ca9024c205720b1a3651A9",
},
{
input: threeValidAddrs[2],
expectedJson: "\"0x1111111111111111111112222222222223333323\"",
expectedJson: "0x1111111111111111111112222222222223333323",
},
} {
s.Run(strconv.Itoa(tcIdx), func() {
Expand All @@ -140,15 +140,16 @@ func (s *EIP55AddrSuite) TestProtobufEncoding() {

bz, err := tc.input.Marshal()
s.NoError(err)
s.Equal(tc.input.Bytes(), bz,
s.Equal(tc.expectedJson, string(bz),
"Marshaling to bytes gives different value than the test case specifies. test case #%d", tcIdx)

err = eip55Addr.Unmarshal(bz)
s.NoError(err)
s.Equal(tc.input.Address, eip55Addr.Address,
"Given -> Marshal -> Unmarshal returns a different value than the given when it should be an identity operation (no-op). test case #%d", tcIdx)

s.Equal(len(tc.input.Bytes()), tc.input.Size())
s.Equal(len([]byte(tc.input.Hex())), tc.input.Size())
s.Equal(len(tc.input.Hex()), tc.input.Size())
})
}
}
Expand All @@ -173,3 +174,24 @@ type EIP55AddrSuite struct {
func TestEIP55AddrSuite(t *testing.T) {
suite.Run(t, new(EIP55AddrSuite))
}

func (s *EIP55AddrSuite) TestStringEncoding() {
addrHex := "0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed"
addr := new(eth.EIP55Addr)
err := addr.Unmarshal([]byte(addrHex))
s.NoError(err)
s.Equal(addrHex, addr.Address.Hex())

addrBytes, err := addr.Marshal()
s.NoError(err)
s.Equal(addrHex, string(addrBytes))

bz, err := addr.MarshalJSON()
s.NoError(err)
s.Equal(addrHex, string(bz))

addrb := new(eth.EIP55Addr)
err = addrb.UnmarshalJSON([]byte(addrHex))
s.NoError(err)
s.EqualValues(addrb, addr)
}
CalicoNino marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion x/evm/keeper/grpc_query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,7 @@ const DefaultGethTraceTimeout = 5 * time.Second
// Configures a Nibiru EVM tracer that is used to "trace" and analyze
// the execution of transactions within a given block. Block information is read
// from the context (goCtx). [TraceBlock] is responsible iterates over each Eth
// transacion message and calls [TraceEthTxMsg] on it.
// transaction message and calls [TraceEthTxMsg] on it.
func (k Keeper) TraceBlock(
goCtx context.Context, req *evm.QueryTraceBlockRequest,
) (*evm.QueryTraceBlockResponse, error) {
Expand Down
Loading