HAL-05 lack of validation of ethereum address formats

[juniuszhou]

Description

In several code paths in the Sifchain codebase, the HexToAddress function from the go-ethereum library is used to parse Ethereum addresses. This function does not return an error when the format of the address passed to it is incorrect. Indeed, the function will accept any number of bytes and return the right-most 20 bytes. (see the following reference). For this reason, it is unsuitable for use as a function to validate Ethereum addresses. The use of this function may be misleading for developers unfamiliar with the go-ethereum library, as it appears to perform more thorough validation of Ethereum addresses than it actually does.

code location

x/ tokenregistry / client / cli / metadata .go
2 79: tokenAddress := common . HexToAddress (tokenAddressRaw ) 34 x/ ethbridge / types / ethereum .go
5 16: return EthereumAddress ( gethCommon . HexToAddress ( address ))
67 x/ ethbridge / types / msgs_test .go
8 25: EthereumReceiver : common . HexToAddress ("0xa98cea040E91e28D71b883b88d6c6445b486124D ") ,
9 26: TokenAddress : common . HexToAddress ("0xC62C770B3223E7ABeD54B4026ad972C84e9a424b ") ,
10
11 x/ ethbridge / types / msgs .go
12 282: gethCommon . HexToAddress ( ethereumReceiver ) ,
283: gethCommon . HexToAddress ( tokenAddress ) ,
14
15 cmd / ebrelayer / main .go
16 255: contractAddress := common . HexToAddress (contractAddressString )
17 455: contractAddress := common . HexToAddress (contractAddressString )
18
19 cmd / ebrelayer / relayer / cosmos_test .go
20 36: registryContractAddress := common . HexToAddress (contractAddress )
21
22 cmd / ebrelayer / txs / registry .go
23 26: var bridgeBankAddress = common . HexToAddress (nullAddress )
24 27: var cosmosBridgeAddress = common . HexToAddress (nullAddress )
25
26 cmd / ebrelayer / txs / signature_test .go
27 54: signerAddr := common . HexToAddress (TestAddrHex )
28
29 cmd / ebrelayer / txs / parser_test .go
30 138: falseRes := isZeroAddress ( common . HexToAddress (TestOtherAddress ) )
31 141: trueRes := isZeroAddress ( common . HexToAddress (TestNullAddress ))
32
33 cmd / ebrelayer / txs / prophecy .go
34 80: ethereumReceiver = common . HexToAddress ( val )
35
36 cmd / ebrelayer / txs / relayToEthereum .go
37 99: EthereumReceiver : common . HexToAddress (prophecyInfo . EthereumReceiver ) ,
38 100: TokenAddress : common . HexToAddress (prophecyInfo . TokenContractAddress ) ,
39 121: Signer : common . HexToAddress (address ) ,
40
41 cmd / ebrelayer / txs / parser .go
42 151: ethereumSender = common . HexToAddress (val )
43 218: return address == common . HexToAddress ( nullAddress )
44
45 cmd / ebrelayer / txs / test_common .go
46 51: testBridgeContractAddress := common . HexToAddress (TestBridgeContractAddress )
47 58: testEthereumSender := common . HexToAddress (TestEthereumAddress1 )
48 60: testTokenAddress := common . HexToAddress (TestEthTokenAddress )
49 82: testEthereumReceiver := common . HexToAddress (TestEthereumAddress1 )
50 83: testValidatorAddress := common . HexToAddress (TestEthereumAddress2 )
51 84: testTokenAddress := common . HexToAddress (TestEthTokenAddress )
52 150: Value : [] byte ( common . HexToAddress (TestEthereumAddress1 ). Hex () ) 
53 180: Value : [] byte ( common . HexToAddress (TestEthereumAddress1 ). Hex () ) 
54 210: Value : [] byte ( common . HexToAddress (TestEthereumAddress1 ). Hex () )

Recommendation

Consider writing a helper function or struct to validate Ethereum addresses in a more robust way. This function could make use of go ethereum’s IsHexAddress function or alternatively use a regular expression or other means to verify valid Ethereum addresses. This function could additionally be used to filter out addresses that could lead to errors, for example the zero-address (0x00..00).

[juniuszhou]

I have checked all the HexToAddress uages in source code, there is the IsHexAddress called before. We don't have IsHexAddress in the *_test.go, but I think it is fine, the Ethereum address in test code is hard coded with correct one. I suggest we won't fix it.

[juniuszhou]

close it if one more person agree with no fix. @banshee @smartyalgo @Brando753

[smartyalgo]

I think we'll need to add that validation for TokenMetadataAddRequest.ValidateBasic(). Please verify, not sure if IBC flow uses the same code

[smartyalgo]

The rest are fine, that's the only one outstanding

[juniuszhou]

will prepare a PR to add the IsHexAddress check in TokenMetadataAddRequest.ValidateBasic()

[juniuszhou]

PR #3289 created.

[juniuszhou]

close it after #3289 merged.