check that target master copy has code

[auryn-macmillan]

This PR adds a check to our proxy factory to ensure that the target mastercopy has code deployed before deploying a proxy.

[cristovaoth]

Awesome!

[nginnever]

lgtm!

[auryn-macmillan]

@nginnever, do you think this change invalidates the audit?
Given that it just adds an error, I tend to think it's fine.

It will obviously change the address of the factory though.
That being the case, any other changes that we might want to make while we're at it?

[cristovaoth]

@nginnever, do you think this change invalidates the audit? Given that it just adds an error, I tend to think it's fine.

It's trivial to see that the change introduces no vulnerability, therefore it's fine.

However it would be nevertheless nice to have the audit reflect latest sol state.

[nginnever]

@auryn-macmillan well this opcode has a lot of controversy around it, but I don't think there is a risk with this use case. The only risk I have seen with extcodesize would be if you are using this feature to check if a contract or EOA is making a call. The constructor returning 0 has always made that assumption a risk and with aa it will only get harder.

But this is just a security feature to help ensure that an incorrect address isn't provided. Maybe the EVM stops supporting this opcode one day, but I haven't seen any discussion around removing extcodesize and eth core is careful about not breaking contracts when they do remove anything.

[auryn-macmillan]

Yeah, I'd be surprised if they made a breaking change with this opcode.

Nevertheless, do you see any other ways to accomplish this same kind of check in this context?
Essentially, we just want to avoid a scenario where people create a proxy that points to an address that currently has no code deployed but could later have code deployed to it.

[nginnever]

I've tried thinking about other approaches (could always use codehash but thats the same)... but I think this still the best option. The hard part is enforcing that "there is no code", but enforcing that "there is code" shouldn't be a problem.

[auryn-macmillan]

Before merging this, I guess we should generate a new vanity address for it.

[auryn-macmillan]

Alrighty, I went ahead and found a new vanity address for the factory. It has 12 leading zeros! 🎉

@nginnever and @cristovaoth, could you please review the deployed code and confirm that it corresponds to the changes in this PR before we merge?

https://goerli.etherscan.io/address/0x000000000000aDdB49795b0f9bA5BC298cDda236#code