Skip to content

Commit

Permalink
fix: use owner as CREATE2 salt
Browse files Browse the repository at this point in the history 
docs: improve writing in README
refactor: delete "nextSeeds"
refactor: delete stale CREATE2 seeds
refactor: remove "origin" in "DeployProxy" event
test: remove unused constants
test: update tests in light of new contract logic
test: remove stale "origin" fuzzing
  • Loading branch information
@PaulRBerg
PaulRBerg committed Jun 24, 2023
1 parent 9ad9ae0 commit 231b87f
Show file tree
Hide file tree
Showing 12 changed files with 64 additions and 239 deletions.
9 changes: 4 additions & 5 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,11 @@ Solidity compiler and new EVM OPCODES, as well as the introduction of more user-

Enter PRBProxy, the modern successor to DSProxy; a "DSProxy 2.0", if you will. It improves upon DSProxy in several ways:

1. PRBProxy is deployed with [CREATE2][eip-1014], which allows clients to know the proxy contract's address in advance.
2. `CREATE2` seeds are generated in a way that eliminates the risk of front-running.
3. The proxy owner cannot be changed during the `DELEGATECALL` operation.
1. PRBProxy is deployed with [CREATE2][eip-1014], which allows clients to pre-compute the proxy contract's address.
2. The `CREATE2` salts are generated in a way that eliminates the risk of front-running.
3. The proxy owner is immutable, and so it cannot be changed during any `DELEGATECALL`.
4. PRBProxy uses high-level Solidity code that is easier to comprehend and less prone to errors.
5. A minimum gas reserve is stored in the proxy to prevent it from becoming unusable if future EVM opcode gas costs change.
6. PRBProxy offers more features than DSProxy.
5. PRBProxy offers more features than DSProxy.

Using CREATE2 eliminates the risk of a [chain reorg](https://en.bitcoin.it/wiki/Chain_Reorganization) overriding the proxy contract owner, making
PRBProxy a more secure alternative to DSProxy. With DSProxy, users must wait for several blocks to be mined before assuming the contract is secure.
Expand Down
59 changes: 11 additions & 48 deletions src/PRBProxyRegistry.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,6 @@ contract PRBProxyRegistry is IPRBProxyRegistry {
/// @inheritdoc IPRBProxyRegistry
ConstructorParams public override constructorParams;

/// @inheritdoc IPRBProxyRegistry
mapping(address origin => bytes32 seed) public override nextSeeds;

/*//////////////////////////////////////////////////////////////////////////
INTERNAL STORAGE
//////////////////////////////////////////////////////////////////////////*/
Expand Down Expand Up @@ -139,37 +136,20 @@ contract PRBProxyRegistry is IPRBProxyRegistry {
noProxy(msg.sender)
returns (IPRBProxy proxy)
{
// Load the next seed.
bytes32 seed = nextSeeds[tx.origin];

// Prevent front-running the salt by hashing the concatenation of "tx.origin" and the user-provided seed.
bytes32 salt = keccak256(abi.encode(tx.origin, seed));
// Use the address of the owner as the CREATE2 salt.
address owner = msg.sender;
bytes32 salt = bytes32(abi.encodePacked(owner));

// Deploy the proxy with CREATE2, and execute the delegate call in the constructor.
address owner = msg.sender;
constructorParams = ConstructorParams({ owner: owner, target: target, data: data });
proxy = new PRBProxy{ salt: salt }();
delete constructorParams;

// Associate the the owner with the proxy in the mapping.
// Associate the owner and the proxy.
_proxies[owner] = proxy;

// Increment the seed.
// Using unchecked arithmetic here because this cannot realistically overflow, ever.
unchecked {
nextSeeds[tx.origin] = bytes32(uint256(seed) + 1);
}

// Log the proxy via en event.
// forgefmt: disable-next-line
emit DeployProxy({
origin: tx.origin,
operator: msg.sender,
owner: owner,
seed: seed,
salt: salt,
proxy: proxy
});
// Log the creation of the proxy.
emit DeployProxy({ operator: msg.sender, owner: owner, proxy: proxy });
}

/// @inheritdoc IPRBProxyRegistry
Expand Down Expand Up @@ -247,35 +227,18 @@ contract PRBProxyRegistry is IPRBProxyRegistry {

/// @dev See the documentation for the user-facing functions that call this internal function.
function _deploy(address owner) internal returns (IPRBProxy proxy) {
// Load the next seed.
bytes32 seed = nextSeeds[tx.origin];

// Prevent front-running the salt by hashing the concatenation of "tx.origin" and the user-provided seed.
bytes32 salt = keccak256(abi.encode(tx.origin, seed));
// Use the address of the owner as the CREATE2 salt.
bytes32 salt = bytes32(abi.encodePacked(owner));

// Deploy the proxy with CREATE2.
constructorParams.owner = owner;
proxy = new PRBProxy{ salt: salt }();
delete constructorParams;

// Associate the the owner with the proxy in the mapping.
// Associate the owner and the proxy.
_proxies[owner] = proxy;

// Increment the seed.
// We're using unchecked arithmetic here because this cannot realistically overflow, ever.
unchecked {
nextSeeds[tx.origin] = bytes32(uint256(seed) + 1);
}

// Log the proxy via en event.
// forgefmt: disable-next-line
emit DeployProxy({
origin: tx.origin,
operator: msg.sender,
owner: owner,
seed: seed,
salt: salt,
proxy: proxy
});
// Log the creation of the proxy.
emit DeployProxy({ operator: msg.sender, owner: owner, proxy: proxy });
}
}
27 changes: 8 additions & 19 deletions src/interfaces/IPRBProxyRegistry.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,7 @@ interface IPRBProxyRegistry {
//////////////////////////////////////////////////////////////////////////*/

/// @notice Emitted when a new proxy is deployed.
event DeployProxy(
address indexed origin,
address indexed operator,
address indexed owner,
bytes32 seed,
bytes32 salt,
IPRBProxy proxy
);
event DeployProxy(address indexed operator, address indexed owner, IPRBProxy proxy);

/// @notice Emitted when a plugin is installed.
event InstallPlugin(address indexed owner, IPRBProxy indexed proxy, IPRBProxyPlugin indexed plugin);
Expand Down Expand Up @@ -122,15 +115,11 @@ interface IPRBProxyRegistry {
/// @param owner The user address to make the query for.
function getProxy(address owner) external view returns (IPRBProxy proxy);

/// @notice The seed that will be used to deploy the next proxy for the provided origin.
/// @param origin The externally owned account (EOA) that is part of the CREATE2 salt.
function nextSeeds(address origin) external view returns (bytes32 seed);

/*//////////////////////////////////////////////////////////////////////////
NON-CONSTANT FUNCTIONS
//////////////////////////////////////////////////////////////////////////*/

/// @notice Deploys a new proxy with CREATE2 by setting the caller as the owner.
/// @notice Deploys a new proxy with CREATE2, using the caller as the owner.
///
/// @dev Emits a {DeployProxy} event.
///
Expand All @@ -140,8 +129,9 @@ interface IPRBProxyRegistry {
/// @return proxy The address of the newly deployed proxy contract.
function deploy() external returns (IPRBProxy proxy);

/// @notice Deploys a new proxy via CREATE2 by setting the caller as the owner, and delegate calls to the provided
/// target contract by forwarding the data. It returns the data it gets back, and bubbles up any potential revert.
/// @notice Deploys a new proxy via CREATE2, using the caller as the owner. It delegate calls to the provided
/// target contract by forwarding the data. Then, it returns the data it gets back, and bubbles up any potential
/// revert.
///
/// @dev Emits a {DeployProxy} and an {Execute} event.
///
Expand Down Expand Up @@ -170,9 +160,8 @@ interface IPRBProxyRegistry {
/// @dev Emits an {InstallPlugin} event.
///
/// Notes:
/// - Installing a plugin is a potentially dangerous operation, because anyone will be able to run the plugin.
/// - Plugin methods that have the same selector as {PRBProxy.execute} will be installed, but they will never be run
/// by the proxy.
/// - Installing a plugin is a potentially dangerous operation, because anyone can run the plugin.
/// - Plugin methods that have the same selector as {PRBProxy.execute} can be installed, but they can never be run.
///
/// Requirements:
/// - The caller must have a proxy.
Expand All @@ -193,7 +182,7 @@ interface IPRBProxyRegistry {
/// Requirements:
/// - The caller must have a proxy.
///
/// @param envoy The address of the envoy account.
/// @param envoy The address of the account given permission to call the target contract.
/// @param target The address of the target contract.
/// @param permission The boolean permission to set.
function setPermission(address envoy, address target, bool permission) external;
Expand Down
15 changes: 3 additions & 12 deletions test/Base.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,15 +60,6 @@ abstract contract Base_Test is Assertions, Events, StdCheats, StdUtils {
address payable eve;
}

/*//////////////////////////////////////////////////////////////////////////
CONSTANTS
//////////////////////////////////////////////////////////////////////////*/

uint256 internal constant DEFAULT_MIN_GAS_RESERVE = 5000;
bytes32 internal constant SEED_ONE = bytes32(uint256(0x01));
bytes32 internal constant SEED_TWO = bytes32(uint256(0x02));
bytes32 internal constant SEED_ZERO = bytes32(uint256(0x00));

/*//////////////////////////////////////////////////////////////////////////
VARIABLES
//////////////////////////////////////////////////////////////////////////*/
Expand Down Expand Up @@ -123,16 +114,16 @@ abstract contract Base_Test is Assertions, Events, StdCheats, StdUtils {
deployRegistryConditionally();

// Make Alice both the caller and the origin.
vm.startPrank({ msgSender: users.alice, txOrigin: users.alice });
vm.startPrank({ msgSender: users.alice });
}

/*//////////////////////////////////////////////////////////////////////////
HELPERS
//////////////////////////////////////////////////////////////////////////*/

/// @dev Computes the proxy address without deploying it.
function computeProxyAddress(address origin, bytes32 seed) internal returns (address) {
bytes32 salt = keccak256(abi.encode(origin, seed));
function computeProxyAddress(address owner) internal returns (address) {
bytes32 salt = bytes32(abi.encodePacked(owner));
bytes32 creationBytecodeHash = keccak256(getProxyBytecode());
// Use the Create2 utility from Forge Std.
return computeCreate2Address({ salt: salt, initcodeHash: creationBytecodeHash, deployer: address(registry) });
Expand Down
68 changes: 14 additions & 54 deletions test/registry/deploy-and-execute/deployAndExecute.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,82 +32,42 @@ contract DeployAndExecute_Test is Registry_Test {
_;
}

function testFuzz_DeployAndExecute_ProxyAddress(address origin, address owner) external whenOwnerDoesNotHaveProxy {
changePrank({ txOrigin: origin, msgSender: owner });
function testFuzz_DeployAndExecute_ProxyAddress(address owner) external whenOwnerDoesNotHaveProxy {
changePrank({ msgSender: owner });
IPRBProxy actualProxy = registry.deployAndExecute(target, data);
address expectedProxy = computeProxyAddress(origin, SEED_ZERO);
address expectedProxy = computeProxyAddress(owner);
assertEq(address(actualProxy), expectedProxy, "deployed proxy address mismatch");
}

function testFuzz_DeployAndExecute_ProxyOwner(address origin, address owner) external whenOwnerDoesNotHaveProxy {
changePrank({ txOrigin: origin, msgSender: owner });
function testFuzz_DeployAndExecute_ProxyOwner(address owner) external whenOwnerDoesNotHaveProxy {
changePrank({ msgSender: owner });
IPRBProxy proxy = registry.deployAndExecute(target, data);
address actualOwner = proxy.owner();
address expectedOwner = owner;
assertEq(actualOwner, expectedOwner, "proxy owner mismatch");
}

function testFuzz_DeployAndExecute_UpdateNextSeeds(
address origin,
address owner
)
external
whenOwnerDoesNotHaveProxy
{
changePrank({ txOrigin: origin, msgSender: owner });
registry.deployAndExecute(target, data);

bytes32 actualNextSeed = registry.nextSeeds(origin);
bytes32 expectedNextSeed = SEED_ONE;
assertEq(actualNextSeed, expectedNextSeed, "next seed mismatch");
}

function testFuzz_DeployAndExecute_UpdateProxies(
address origin,
address owner
)
external
whenOwnerDoesNotHaveProxy
{
changePrank({ txOrigin: origin, msgSender: owner });
function testFuzz_DeployAndExecute_UpdateProxies(address owner) external whenOwnerDoesNotHaveProxy {
changePrank({ msgSender: owner });
registry.deployAndExecute(target, data);

address actualProxyAddress = address(registry.getProxy(owner));
address expectedProxyAddress = computeProxyAddress(origin, SEED_ZERO);
address expectedProxyAddress = computeProxyAddress(owner);
assertEq(actualProxyAddress, expectedProxyAddress, "proxy address mismatch");
}

function testFuzz_DeployAndExecute_Event_DeployProxy(
address origin,
address owner
)
external
whenOwnerDoesNotHaveProxy
{
changePrank({ txOrigin: origin, msgSender: owner });
function testFuzz_DeployAndExecute_Event_DeployProxy(address owner) external whenOwnerDoesNotHaveProxy {
changePrank({ msgSender: owner });

vm.expectEmit({ emitter: address(registry) });
emit DeployProxy({
origin: origin,
operator: owner,
owner: owner,
seed: SEED_ZERO,
salt: keccak256(abi.encode(origin, SEED_ZERO)),
proxy: IPRBProxy(computeProxyAddress(origin, SEED_ZERO))
});
emit DeployProxy({ operator: owner, owner: owner, proxy: IPRBProxy(computeProxyAddress(owner)) });
registry.deployAndExecute(target, data);
}

function testFuzz_DeployAndExecute_Event_Execute(
address origin,
address owner
)
external
whenOwnerDoesNotHaveProxy
{
changePrank({ txOrigin: origin, msgSender: owner });
function testFuzz_DeployAndExecute_Event_Execute(address owner) external whenOwnerDoesNotHaveProxy {
changePrank({ msgSender: owner });

vm.expectEmit({ emitter: computeProxyAddress({ origin: origin, seed: SEED_ZERO }) });
vm.expectEmit({ emitter: computeProxyAddress(owner) });
emit Execute({ target: address(targets.echo), data: data, response: abi.encode(input) });
registry.deployAndExecute(target, data);
}
Expand Down
1 change: 0 additions & 1 deletion test/registry/deploy-and-execute/deployAndExecute.tree
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ deployAndExecute.t.sol
└── when the owner does not have a proxy
├── it should generate the correct address
├── it should initialize the owner
├── it should update the next seeds mapping
├── it should update the proxies mapping
├── it should delegate call to the target contract
├── it should emit a {DeployProxy} event
Expand Down
Loading

0 comments on commit 231b87f

Please sign in to comment.