ProxyFactory create2 and blackholing of funds

[mikeshultz]

This is a bit of an edge case, but does actually waste money. If a user attempts to deploy a proxy (with the same proxyfactory nonce) when one already exists, it dies. When it dies, it continues running the initializer (wrapped transaction) to 0x0000000000000000000000000000000000000000. Of course this isn't an actual contract, and if the the transaction carried any value(ETH), that value would be sent to the black hole that is 0x00..00. Example tx:

0x8a9650f337466d6d4ecda51ec95fd500d914ad5b3e5abb50eb253eea4240f4fc

1 event was fired:

topics[0] 0xa38789425dbeee0239e16ff2d2567e31720127fbc6430758c1a4efc6aef29f80 // ProxyCreation
topics[1] 0000000000000000000000000000000000000000000000000000000000000000

I propose adding a check to see if the address returned by create2 is zero to both createProxyWithNonce() and createProxyWithSenderNonce(). If it is, revert. Absolutely do not run this initializer.

origin/packages/contracts/contracts/proxy/ProxyFactory.s

Lines 76 to 82 in 42aa0be

	if (initializer.length > 0) {
	uint256 value = msg.value;
	// solium-disable-next-line security/no-inline-assembly
	assembly {
	if eq(call(gas, proxy, value, add(initializer, 0x20), mload(initializer), 0, 0), 0) { revert(0,0) }
	}
	}

I think uprading will be clean. We will need to add support for multiple ProxyFactory contracts for anything that tracks events(e.g. listener). Already deployed user proxies are not affected.

[franckc]

CC @DanielVF @nick

[DanielVF]

I'm in favor. It's quite painful the way it fails now - it basically fails silently. Not to mention wasting a pile of gas.

[nick]

I think this works. Just need to verify that the predicted proxy address doesn't change if we start using a new proxy factory.

eg address of deployed proxy via factory 1 == deployed proxy via factory 2

[mikeshultz]

Ah, that's a good note, @nick. It will undoubtedly change the address...

origin/packages/graphql/src/utils/proxy.js

Lines 35 to 37 in d7c40d5

	const create2hash = web3.utils
	.soliditySha3('0xff', contracts.ProxyFactory._address, salt, creationHash)
	.slice(-40)

I guess we'd have to iterate through each ProxyFactory address, compute the predicted proxy address, and check. That is a drawback, but not sure a deal breaker here.

[mikeshultz]

There seems to be more agreement that this isn't urgent, dropping to a P2.