Deal with create2 failures in ProxyFactory

[mikeshultz]

Description:

revert on create2 failure in ProxyFactory proxy creation methods.

Ref #3392

Deployment Considerations

Actually using this will require some alterations to dapp/graphql logic. Will need to support multiple ProxyFactory addresses when verifying and checking predicted proxy addresses. AFAIK, there's no way to make a second ProxyFactory deploy create the same addresses as the last. The checks are probably relatively cheap, except for anything that does eth_getCode. It will mostly be localized to the dapp proxy utils, though there may be some others out there(e.g. listener) I don't think there's any way around this, however.

Checklist:

• Test your work and double-check to confirm that you didn't break anything
• Wrap any new text/strings with fbt for translation
• If there are any new/changed strings to translate, npm run translate
• Update any relevant READMEs and docs

[mikeshultz]

I don't think the Crytic stuff is gonna finish, so opening this up for eyeballs now. Did run slither, it's mostly style and efficiency stuff and nothing relevant to my changes. I don't think it would be appropriate to fix any of that in this PR. If anyone thinks it needs to be addressed, it can be done in another PR before we actually deploy it.

[nick]

LGTM

[franckc]

Thanks for looking into this !

It's unfortunate we are going to have to add extra complexity in the dapp/graphql/listener to handle this new version of the ProxyFactory contract...

I just want to make sure we weight the pros/cons of our options.

What if as opposed to changing the ProxyFactory contract we add extra checks on the client side to verify whether there is already a proxy ?

[franckc]

Ah also if we decide to go ahead and update the ProxyFactory contract, it would be nice to run it through Slither. Just in case it finds a security issue that we should fix at the same time...

[mikeshultz]

What if as opposed to changing the ProxyFactory contract we add extra checks on the client side to verify whether there is already a proxy ?

We're already checking that now. Not working so hot. Never seems to fail that users somehow find a way to rapid fire transactions at the relayer, and I'm not confident we'll ever fully get rid of these. This has been seen to burn money and I think that's worth mounting a defense against that at the contract level.

Ah also if we decide to go ahead and update the ProxyFactory contract, it would be nice to run it through Slither.

See my previous comment.

[franckc]

Sorry I had missed the fact you ran Slither. Thanks for doing that.

I'm still not thrilled about adding yet another contract version and extra complexity to our code but if you feel it's the best way to go then LGTM.

One question before you proceed: should we we call this contract V01_ProxyFactory.s and rename the old one V00_ProxyFractory.s ?

[mikeshultz]

I'm not thrilled by this myself. But I'm less thrilled by wasting money of users and I'm not confident we can 100% prevent this on the client-side.

I'm not sure we need to keep separate versions of the contract, though I'll defer to everyone else on that. I think keeping a configuration with the previous ProxyFactory address would be necessary, though for address prediction.

[nick]

I think we should rename this V01_ProxyFactory and keep a copy of the old one so it's easy to refer to what code is deployed an in use on mainnet without having to refer to git history.

Perhaps we should hold off on deploying a copy and actually using the new ProxyFactory until some of the other fixes we have going on are in place, such as the Dai repair I hope to get merged soon. Even if we don't end up using a new ProxyFactory immediately we should keep this code around for when we eventually do a new deploy.

One potential other change I'm thinking it'd be nice to get in before deploying is having the same predicted address regardless of which factory deployed the identity.

[mikeshultz]

Yeah, I'm not really in a rush to deploy this since the impact has so far been low(that we know of), but I still think burning money to 0x00..00 is a pretty big deal.

One potential other change I'm thinking it'd be nice to get in before deploying is having the same predicted address regardless of which factory deployed the identity.

I don't think that's possible since the addresses are bassed upon the calling contract's address. Here's a snippet for create2 from the solidity assembly docs:

create new contract with code mem[p…(p+n)) at address keccak256(0xff . this . s . keccak256(mem[p…(p+n))) and send v wei and return the new address, where 0xff is a 8 byte value, this is the current contract’s address as a 20 byte value and s is a big-endian 256-bit value

And since ProxyFactory isn't in-place upgradable through selfdestruct + create2 I think we're out of luck.