Fix invalid 'to' field on contract deploy transactions

[benjamincburns]

It seems somebody has launched a new tutorial with MEW and Ganache, because I'm getting numerous issue reports that contracts can't be deployed to Ganache via MEW. The error occurs in Ganache transaction validation. MEW submits contract deploy transactions with an empty string in the to param. Last I checked, empty strings aren't valid hex encoded addresses.

I've changed Ganache to treat empty string transaction parameters as if they weren't defined, but I figured it's best to fix up MEW as well. By my testing this PR fixes the problem. That said, I didn't see some huge cache of automated tests to run (just app/tests/index.html), so I can't promise that I didn't break something here.

[tayvano]

I have a branch locally trying to fix this without breaking anything else. The problem is ethFuncs.sanitizeHex is used so widely that I'm scared there is going to be some unintended consequence and, as you said, the tests are lacking severely.

A worst-case scenario I could imagine is having users send to 0x0000000000 or something. I'm not sure how that could happen but with a function that is engrained so deeply, I'm shit terrified.

[benjamincburns]

The problem is ethFuncs.sanitizeHex is used so widely that I'm scared there is going to be some unintended consequence and, as you said, the tests are lacking severely.

I understand exactly how this came to be, as I've lived it myself on numerous projects. However that sounds like a fairly scary position to be in, especially given the that trust people place in MEW.

A worst-case scenario I could imagine is having users send to 0x0000000000 or something.

I don't see how that's possible here. The modifications don't assign any variant of 0 to anything, undefined doesn't coerce to 0 (see example below), and any "zero address" apart from '0x0000000000000000000000000000000000000000' is meant to be rejected by clients as invalid, as it won't meet the address size requirement.

> 0 == undefined
false
> undefined == 0
false
> console.log(Number(undefined))
NaN

[benjamincburns]

I think if you want some extra safety it might be a good idea to change isValidHexString to return false on empty string. This will cause obvious fixable errors in any other places where empty string values are being used as hex values.

[benjamincburns]

@tayvano also if you want to protect against sending to the null address, you could add the following to the first line of this function:

if ((txData.to && new BigNumber(txData.to).eq(0)) || txData.to === 0) return false;

Edit: Fixed for 0 falsyness. (grumble grumble javascript)

[benjamincburns]

Though strictly speaking this change is incorrect, as technically sending to '0x0000000000000000000000000000000000000000' is a valid action. I don't know what warning mechanisms exist in MEW, but maybe it's best to pop up a "are you sure about that?" box in this case.

[benjamincburns]

Hi @tayvano - do you need anything from me in order to be able to merge this? Your last comment made it sound like things were in a bit of gridlock - not sure how best to move forward.

[kvhnuke]

Hey let me take a look at this and add it to our next release

[benjamincburns]

@kvhnuke did you need me to fix up merge conflicts, or will you all handle that?

[gamalielhere]

@benjamincburns you can remove those conflicting files from your commit since the /app folder gets compiled anyway.