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
test: remove unused constants
test: update tests in light of new contract logic
  • Loading branch information
@PaulRBerg
PaulRBerg committed Jun 22, 2023
1 parent 9ad9ae0 commit 38c06e9
Show file tree
Hide file tree
Showing 12 changed files with 35 additions and 159 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
55 changes: 9 additions & 46 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
});
emit DeployProxy({ origin: tx.origin, 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
});
emit DeployProxy({ origin: tx.origin, 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 origin, 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
13 changes: 2 additions & 11 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 @@ -131,8 +122,8 @@ abstract contract Base_Test is Assertions, Events, StdCheats, StdUtils {
//////////////////////////////////////////////////////////////////////////*/

/// @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
30 changes: 4 additions & 26 deletions test/registry/deploy-and-execute/deployAndExecute.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ contract DeployAndExecute_Test is Registry_Test {
function testFuzz_DeployAndExecute_ProxyAddress(address origin, address owner) external whenOwnerDoesNotHaveProxy {
changePrank({ txOrigin: origin, 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");
}

Expand All @@ -47,21 +47,6 @@ contract DeployAndExecute_Test is Registry_Test {
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
Expand All @@ -73,7 +58,7 @@ contract DeployAndExecute_Test is Registry_Test {
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");
}

Expand All @@ -87,14 +72,7 @@ contract DeployAndExecute_Test is Registry_Test {
changePrank({ txOrigin: origin, 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({ origin: origin, operator: owner, owner: owner, proxy: IPRBProxy(computeProxyAddress(owner)) });
registry.deployAndExecute(target, data);
}

Expand All @@ -107,7 +85,7 @@ contract DeployAndExecute_Test is Registry_Test {
{
changePrank({ txOrigin: origin, 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
24 changes: 3 additions & 21 deletions test/registry/deploy-for/deployFor.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ contract DeployFor_Test is Registry_Test {
{
changePrank({ txOrigin: origin, msgSender: operator });
address actualProxy = address(registry.deployFor(owner));
address expectedProxy = computeProxyAddress(origin, SEED_ZERO);
address expectedProxy = computeProxyAddress(owner);
assertEq(actualProxy, expectedProxy, "deployed proxy address mismatch");
}

Expand All @@ -52,22 +52,6 @@ contract DeployFor_Test is Registry_Test {
assertEq(actualOwner, expectedOwner, "proxy owner mismatch");
}

function testFuzz_DeployFor_UpdateNextSeeds(
address origin,
address operator,
address owner
)
external
whenOwnerDoesNotHaveProxy
{
changePrank({ txOrigin: origin, msgSender: operator });
registry.deployFor(owner);

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

function testFuzz_DeployFor_UpdateProxies(
address origin,
address operator,
Expand All @@ -80,7 +64,7 @@ contract DeployFor_Test is Registry_Test {
registry.deployFor(owner);

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

Expand All @@ -99,9 +83,7 @@ contract DeployFor_Test is Registry_Test {
origin: origin,
operator: operator,
owner: owner,
seed: SEED_ZERO,
salt: keccak256(abi.encode(origin, SEED_ZERO)),
proxy: IPRBProxy(computeProxyAddress(origin, SEED_ZERO))
proxy: IPRBProxy(computeProxyAddress(owner))
});
registry.deployFor(owner);
}
Expand Down
1 change: 0 additions & 1 deletion test/registry/deploy-for/deployFor.tree
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,5 @@ deployFor.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 emit a {DeployProxy} event
22 changes: 3 additions & 19 deletions test/registry/deploy/deploy.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ contract Deploy_Test is Registry_Test {
function testFuzz_Deploy_ProxyAddress(address origin, address owner) external whenOwnerDoesNotHaveProxy {
changePrank({ txOrigin: origin, msgSender: owner });
address actualProxy = address(registry.deploy());
address expectedProxy = computeProxyAddress({ origin: origin, seed: SEED_ZERO });
address expectedProxy = computeProxyAddress(owner);
assertEq(actualProxy, expectedProxy, "deployed proxy address mismatch");
}

Expand All @@ -38,34 +38,18 @@ contract Deploy_Test is Registry_Test {
assertEq(actualOwner, expectedOwner, "proxy owner mismatch");
}

function testFuzz_Deploy_UpdateNextSeeds(address origin, address owner) external whenOwnerDoesNotHaveProxy {
changePrank({ txOrigin: origin, msgSender: owner });
registry.deploy();

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

function testFuzz_Deploy_UpdateProxies(address origin, address owner) external whenOwnerDoesNotHaveProxy {
changePrank({ txOrigin: origin, msgSender: owner });
registry.deploy();
address actualProxy = address(registry.getProxy(owner));
address expectedProxy = computeProxyAddress({ origin: origin, seed: SEED_ZERO });
address expectedProxy = computeProxyAddress({ owner: owner });
assertEq(actualProxy, expectedProxy, "proxy mapping mismatch");
}

function testFuzz_Deploy_Event(address origin, address owner) external whenOwnerDoesNotHaveProxy {
changePrank({ txOrigin: origin, 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: origin, seed: SEED_ZERO }))
});
emit DeployProxy({ origin: origin, operator: owner, owner: owner, proxy: IPRBProxy(computeProxyAddress(owner)) });
registry.deploy();
}
}
1 change: 0 additions & 1 deletion test/registry/deploy/deploy.tree
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,5 @@ deploy.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 emit a {DeployProxy} event
Loading

0 comments on commit 38c06e9

Please sign in to comment.