refactor: ownership redesign

[PaulRBerg]

Closes #108, addressing all of the audit report findings mentioned there.

[andreivladbrg]

I can say that the design is cleaner now.

Not sure if it's a good idea(it would slightly increase the gas) but what do you think about changing the internal _deploy function to: function _deploy(ConstructorParams calldata params). This way you will be able to remove duplicated code from _deploy and deployAndExecute.

[PaulRBerg]

Thanks for the review. I agree that the new design is clearer - and safer.

Interesting idea to pass ConstructorParams to _deploy, but I suggest we don't do it because it would increase the gas cost by a non-trivial amount (not just "slightly"). ABI-encoding and decoding structs is an expensive operation. And gas costs are very important for deployments (on Ethereum Mainnet), so I would prefer to save any little that we can.

Both functions are well-tested, so we have a pretty good idea that they work as expected.

[andreivladbrg]

Just noticed

prb-proxy/src/PRBProxyRegistry.sol

Lines 132 to 134 in 17f65af

	constructorParams.owner = owner;
	proxy = new PRBProxy{ salt: salt }();
	delete constructorParams;

Given that the only variable used here is the owner, this can be optimized to:

        constructorParams.owner = owner;
        proxy = new PRBProxy{ salt: salt }();
        constructorParams.owner = address(0);

[PaulRBerg]

Interesting optimization idea @andreivladbrg but the two implementations cost the same gas when only the owner is set. See my conversation with ChatGPT:

https://chat.openai.com/share/3187fa95-2732-4fe8-95c0-539e259fc835

Also, assigning only the owner to zero is less secure than deleting the entire struct because of the possibility of reentrancy.

[tibbarytsur]

Set the proxies[owner] to a temp value before creating the proxy to avoid reentrancy for the same owner

prb-proxy/src/PRBProxyRegistry.sol

Lines 95 to 99 in 17f65af

	proxy = new PRBProxy{ salt: salt }();
	delete constructorParams;
	// Associate the the owner with the proxy in the mapping.
	proxies[owner] = proxy;

noProxy(owner) modifier would pass otherwise when calling deployFor on the registry with the same owner creating 2 proxies with the same owner set in the proxy itself, and the first one set in the registry.

[tibbarytsur]

All constructor params should be set here (target and data to 0), not just the owner

prb-proxy/src/PRBProxyRegistry.sol

Line 132 in 17f65af

constructorParams.owner = owner;

deployAndExecute can reenter in deployFor (via a target contract) which would only change the constructorParams.owner leaving target and data intact. The PRBProxy constructor would then get the constructor params from the registry including the old target and data and execute it on the second proxy (this time doing something else like setting token approvals).

[andreivladbrg]

See my conversation with ChatGPT

I don't think that using GPT for this specific use case is the best choice, tested this in remix and it was actually a difference by few thousands.

the possibility of reentrancy.

Right, it is safer to use delete.

[PaulRBerg]

Thanks for your review, both!

noProxy(owner) modifier would pass otherwise when calling deployFor on the registry with the same owner creating 2 proxies with the same owner set in the proxy itself
...
deployAndExecute can reenter in deployFor (via a target contract) which would only change the constructorParams.owner leaving target and data intact.

As discussed on Discord, this cannot happen under the current design - happy to take a call tomorrow to walk through the low-level steps.

All constructor params should be set here (target and data to 0), not just the owner

Agree with this proposal - it would be a good precautionary step even if the attack above cannot happen.

I don't think that using GPT for this specific use case is the best choice, tested this in remix and it was actually a difference by few thousands.

You may be right about ChatGPT (the 2021 cut-off), but did you use via IR and the same number of optimizer runs in your test?

Right, it is safer to use delete.

Yep

[andreivladbrg]

but did you use ethereum/remix-project#2350 (comment) and the same number of optimizer runs in your test

Yes

[PaulRBerg]

@tibbarytsur and @andreivladbrg have reviewed this PR, but I will also wait for Zach's confirmation before merging this in.