Skip to content

fix: make deterministic CREATE2 deployment idempotent - #1339

Open
BhariGowda wants to merge 1 commit into
aave:mainfrom
BhariGowda:fix/create2-deploy-idempotent
Open

fix: make deterministic CREATE2 deployment idempotent#1339
BhariGowda wants to merge 1 commit into
aave:mainfrom
BhariGowda:fix/create2-deploy-idempotent

Conversation

@BhariGowda

Copy link
Copy Markdown

Reported to Aave's Sherlock bug bounty program first (Medium severity, on-chain PoC
included). The submission was marked invalid with no reason given. Opening here since
the disclosure hold no longer applies once a report is closed, and the underlying fix
is still worth having before the next chain expansion.

Deployment steps that deploy through the Safe Singleton Factory revert with ContractAlreadyDeployed when the computed address already has code. Because the factory is permissionless and salts are derived from public data, a third party can occupy any of those addresses first and block the deployment. The same revert also makes a partially-completed deployment impossible to resume.

LibraryPreCompile / LiquidationLogic

scripts/LibraryPreCompile.s.sol is step 1 of the documented deploy flow and calls SpokeDeployUtils.deployLiquidationLogic(bytes32(0)). The salt is a hardcoded constant with no override path, and LiquidationLogic is a plain library whose creation code takes no constructor arguments and reads no chain state. Anyone can deploy it at that exact address on any chain, at any time, with no information about the deployment plans. After that the step reverts and the salt cannot be changed without editing the source. The same revert fires on a legitimate re-run whenever the .env guard in LibraryPreCompile is not present.

Orchestration

AaveV4DeployOrchestration._deriveSalt places the deployer address in the top 160 bits of the salt. The Safe Singleton Factory does not enforce that prefix, so the namespacing only helps while the salt is unguessable. With deployInputs.salt left at bytes32(0), which the deploy script permits with a warning only and which the repo's own deployment tests use, the whole root salt is computable in advance from the deployer address alone. Two addresses on that path are then reachable by a third party:

  • AccessManagerEnumerable, the first contract of the run. Its constructor only stores an admin, and that admin is the deployer.
  • The HubInstance implementation. create2Deploy is called with the raw compiled bytecode and no constructor arguments, so the creation code is a public constant. The child salt is keccak256(rootSalt, 'hub', label) and the labels are public.

Occupying either one blocked the entire deployment before this change. Tests covering both are included.

Note that a secret salt is not a general fix. spokeSalt / hubSalt in some namespacing schemes derive the user salt as
keccak256("chain _version "), which is fully public by design so addresses stay reproducible.

Change

create2DeployIdempotent returns the existing address instead of reverting. This is safe because a CREATE2 address commits to keccak256(creationCode): any contract at the computed address was created by this factory running exactly that creation code, so its runtime code is identical to what the call would produce. Constructor arguments, including proxy admin owner and initializer calldata, are part of that creation code. HubInstance, SpokeInstance, TreasurySpokeInstance and TokenizationSpokeInstance initializers were each checked for msg.sender, tx.origin and block.* reads and use none, so an adopted proxy is state-equivalent to a freshly deployed one.

Applied to the 13 deploy procedures under
src/deployments/procedures/deploy, to Create2Utils.proxify, and to SpokeDeployUtils.deployLiquidationLogic. create2Deploy itself is unchanged and still available where a hard failure is wanted.

Create2Utils.create2DeployIdempotent is left out of src/config-engine/libraries/TokenizationSpokeDeployer.sol on purpose. Its salts are derived from public proposal data, but the implementation constructor calls HUB.getAssetId(underlying), which reverts until addAsset runs in the same transaction, so the address cannot be occupied ahead of time. A hard revert is also the behaviour we want on a governance path rather than silently adopting a pre-existing spoke.

Since the revert previously doubled as the signal for an accidental re-run, the adoption branch emits Create2DeploymentAdopted. A fresh deployment emits nothing, so the event distinguishes a real deployment from a no-op adoption in the deployment receipt.

Testing

tests/contracts 1548 passed, 1 skipped (pre-existing). tests/deployments, tests/scripts and tests/config-engine 440 passed. No failures. Gas snapshots run, unchanged; no runtime path in src/hub or src/spoke is touched.

Related

If any future call site deploys through the permissionless factory with a fixed, publicly-derivable salt as a mandatory step, it has the same property and should route through create2DeployIdempotent rather than create2Deploy, for the same reason as proxify.

Deployment steps that deploy through the Safe Singleton Factory revert
with ContractAlreadyDeployed when the computed address already has code.
Because the factory is permissionless and salts are derived from public
data, a third party can occupy any of those addresses first and block
the deployment. The same revert also makes a partially-completed
deployment impossible to resume.

LibraryPreCompile / LiquidationLogic

scripts/LibraryPreCompile.s.sol is step 1 of the documented deploy flow
and calls SpokeDeployUtils.deployLiquidationLogic(bytes32(0)). The salt
is a hardcoded constant with no override path, and LiquidationLogic is a
plain library whose creation code takes no constructor arguments and
reads no chain state. Anyone can deploy it at that exact address on any
chain, at any time, with no information about the deployment plans.
After that the step reverts and the salt cannot be changed without
editing the source. The same revert fires on a legitimate re-run
whenever the .env guard in LibraryPreCompile is not present.

Orchestration

AaveV4DeployOrchestration._deriveSalt places the deployer address in the
top 160 bits of the salt. The Safe Singleton Factory does not enforce
that prefix, so the namespacing only helps while the salt is
unguessable. With deployInputs.salt left at bytes32(0), which the deploy
script permits with a warning only and which the repo's own deployment
tests use, the whole root salt is computable in advance from the
deployer address alone. Two addresses on that path are then reachable
by a third party:

- AccessManagerEnumerable, the first contract of the run. Its
  constructor only stores an admin, and that admin is the deployer.
- The HubInstance implementation. create2Deploy is called with the raw
  compiled bytecode and no constructor arguments, so the creation code
  is a public constant. The child salt is keccak256(rootSalt, 'hub',
  label) and the labels are public.

Occupying either one blocked the entire deployment before this change.
Tests covering both are included.

Note that a secret salt is not a general fix. spokeSalt / hubSalt in
some namespacing schemes derive the user salt as
keccak256("chain <chainid>_version <n>"), which is fully public by
design so addresses stay reproducible.

Change

create2DeployIdempotent returns the existing address instead of
reverting. This is safe because a CREATE2 address commits to
keccak256(creationCode): any contract at the computed address was
created by this factory running exactly that creation code, so its
runtime code is identical to what the call would produce. Constructor
arguments, including proxy admin owner and initializer calldata, are
part of that creation code. HubInstance, SpokeInstance,
TreasurySpokeInstance and TokenizationSpokeInstance initializers were
each checked for msg.sender, tx.origin and block.* reads and use none,
so an adopted proxy is state-equivalent to a freshly deployed one.

Applied to the 13 deploy procedures under
src/deployments/procedures/deploy, to Create2Utils.proxify, and to
SpokeDeployUtils.deployLiquidationLogic. create2Deploy itself is
unchanged and still available where a hard failure is wanted.

Create2Utils.create2DeployIdempotent is left out of
src/config-engine/libraries/TokenizationSpokeDeployer.sol on purpose.
Its salts are derived from public proposal data, but the implementation
constructor calls HUB.getAssetId(underlying), which reverts until
addAsset runs in the same transaction, so the address cannot be
occupied ahead of time. A hard revert is also the behaviour we want on
a governance path rather than silently adopting a pre-existing spoke.

Since the revert previously doubled as the signal for an accidental
re-run, the adoption branch emits Create2DeploymentAdopted. A fresh
deployment emits nothing, so the event distinguishes a real deployment
from a no-op adoption in the deployment receipt.

Testing

tests/contracts 1548 passed, 1 skipped (pre-existing). tests/deployments,
tests/scripts and tests/config-engine 440 passed. No failures. Gas
snapshots run, unchanged; no runtime path in src/hub or src/spoke is
touched.

Related

If any future call site deploys through the permissionless factory with
a fixed, publicly-derivable salt as a mandatory step, it has the same
property and should route through create2DeployIdempotent rather than
create2Deploy, for the same reason as proxify.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant