From 4d6e1491942fc6f78d4ec6d98bdbb3fd9e489831 Mon Sep 17 00:00:00 2001 From: BhariGowda Date: Fri, 14 Aug 2026 22:16:24 +0530 Subject: [PATCH] fix: make deterministic CREATE2 deployment idempotent 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. --- scripts/utils/SpokeDeployUtils.sol | 7 +- ...AccessManagerEnumerableDeployProcedure.sol | 2 +- .../AaveV4HubConfiguratorDeployProcedure.sol | 2 +- .../deploy/hub/AaveV4HubDeployProcedure.sol | 2 +- ...eV4InterestRateStrategyDeployProcedure.sol | 2 +- ...V4ConfigPositionManagerDeployProcedure.sol | 2 +- ...eV4GiverPositionManagerDeployProcedure.sol | 2 +- ...aveV4NativeTokenGatewayDeployProcedure.sol | 2 +- .../AaveV4SignatureGatewayDeployProcedure.sol | 2 +- ...eV4TakerPositionManagerDeployProcedure.sol | 2 +- ...AaveV4SpokeConfiguratorDeployProcedure.sol | 2 +- .../spoke/AaveV4SpokeDeployProcedure.sol | 2 +- ...AaveV4TokenizationSpokeDeployProcedure.sol | 2 +- .../AaveV4TreasurySpokeDeployProcedure.sol | 2 +- .../utils/libraries/Create2Utils.sol | 35 ++++- ...aveV4DeployOrchestration.Idempotency.t.sol | 141 ++++++++++++++++++ tests/scripts/SpokeDeployUtils.t.sol | 112 ++++++++++++++ 17 files changed, 306 insertions(+), 15 deletions(-) create mode 100644 tests/deployments/AaveV4DeployOrchestration.Idempotency.t.sol create mode 100644 tests/scripts/SpokeDeployUtils.t.sol diff --git a/scripts/utils/SpokeDeployUtils.sol b/scripts/utils/SpokeDeployUtils.sol index 00ba67e81..270077796 100644 --- a/scripts/utils/SpokeDeployUtils.sol +++ b/scripts/utils/SpokeDeployUtils.sol @@ -15,11 +15,16 @@ library SpokeDeployUtils { /// @notice Deploys LiquidationLogic via CREATE2. /// @dev The CREATE2 factory must already be deployed on the target chain. + /// @dev Idempotent: the salt is a fixed constant and the Safe Singleton Factory is + /// permissionless, so the deterministic address can already be occupied, either by a + /// previous run of this step or by a third party. Any contract at that address must have + /// been created from this exact creation code, so reusing it is safe and keeps this + /// mandatory first deployment step from being blockable or non-repeatable. /// @param salt The CREATE2 salt for deterministic deployment. /// @return The deployed library address. function deployLiquidationLogic(bytes32 salt) internal returns (address) { bytes memory bytecode = vm.getCode('src/spoke/libraries/LiquidationLogic.sol:LiquidationLogic'); - return Create2Utils.create2Deploy(salt, bytecode); + return Create2Utils.create2DeployIdempotent(salt, bytecode); } /// @notice Returns the FOUNDRY_LIBRARIES-compatible string for library linking. diff --git a/src/deployments/procedures/deploy/AaveV4AccessManagerEnumerableDeployProcedure.sol b/src/deployments/procedures/deploy/AaveV4AccessManagerEnumerableDeployProcedure.sol index e008f8e23..4ea8d66d0 100644 --- a/src/deployments/procedures/deploy/AaveV4AccessManagerEnumerableDeployProcedure.sol +++ b/src/deployments/procedures/deploy/AaveV4AccessManagerEnumerableDeployProcedure.sol @@ -16,7 +16,7 @@ contract AaveV4AccessManagerEnumerableDeployProcedure is AaveV4DeployProcedureBa function _deployAccessManagerEnumerable(address admin, bytes32 salt) internal returns (address) { require(admin != address(0), 'invalid admin'); return - Create2Utils.create2Deploy( + Create2Utils.create2DeployIdempotent( salt, abi.encodePacked(type(AccessManagerEnumerable).creationCode, abi.encode(admin)) ); diff --git a/src/deployments/procedures/deploy/hub/AaveV4HubConfiguratorDeployProcedure.sol b/src/deployments/procedures/deploy/hub/AaveV4HubConfiguratorDeployProcedure.sol index 86e5ece8f..335152e0c 100644 --- a/src/deployments/procedures/deploy/hub/AaveV4HubConfiguratorDeployProcedure.sol +++ b/src/deployments/procedures/deploy/hub/AaveV4HubConfiguratorDeployProcedure.sol @@ -16,7 +16,7 @@ contract AaveV4HubConfiguratorDeployProcedure is AaveV4DeployProcedureBase { function _deployHubConfigurator(address authority, bytes32 salt) internal returns (address) { require(authority != address(0), 'invalid authority'); return - Create2Utils.create2Deploy( + Create2Utils.create2DeployIdempotent( salt, abi.encodePacked(type(HubConfigurator).creationCode, abi.encode(authority)) ); diff --git a/src/deployments/procedures/deploy/hub/AaveV4HubDeployProcedure.sol b/src/deployments/procedures/deploy/hub/AaveV4HubDeployProcedure.sol index 5bd14be99..02c521292 100644 --- a/src/deployments/procedures/deploy/hub/AaveV4HubDeployProcedure.sol +++ b/src/deployments/procedures/deploy/hub/AaveV4HubDeployProcedure.sol @@ -24,7 +24,7 @@ contract AaveV4HubDeployProcedure is AaveV4DeployProcedureBase { ) internal returns (address hubProxy, address hubImplementation) { require(proxyAdminOwner != address(0), 'invalid proxy admin owner'); require(authority != address(0), 'invalid authority'); - hubImplementation = Create2Utils.create2Deploy({salt: salt, bytecode: hubBytecode}); + hubImplementation = Create2Utils.create2DeployIdempotent({salt: salt, bytecode: hubBytecode}); hubProxy = Create2Utils.proxify({ salt: salt, logic: hubImplementation, diff --git a/src/deployments/procedures/deploy/hub/AaveV4InterestRateStrategyDeployProcedure.sol b/src/deployments/procedures/deploy/hub/AaveV4InterestRateStrategyDeployProcedure.sol index c3dba7c9b..a372ff027 100644 --- a/src/deployments/procedures/deploy/hub/AaveV4InterestRateStrategyDeployProcedure.sol +++ b/src/deployments/procedures/deploy/hub/AaveV4InterestRateStrategyDeployProcedure.sol @@ -16,7 +16,7 @@ contract AaveV4InterestRateStrategyDeployProcedure is AaveV4DeployProcedureBase function _deployInterestRateStrategy(address hub, bytes32 salt) internal returns (address) { require(hub != address(0), 'invalid hub'); return - Create2Utils.create2Deploy( + Create2Utils.create2DeployIdempotent( salt, abi.encodePacked(type(AssetInterestRateStrategy).creationCode, abi.encode(hub)) ); diff --git a/src/deployments/procedures/deploy/position-manager/AaveV4ConfigPositionManagerDeployProcedure.sol b/src/deployments/procedures/deploy/position-manager/AaveV4ConfigPositionManagerDeployProcedure.sol index 019132ab5..cf7606c2a 100644 --- a/src/deployments/procedures/deploy/position-manager/AaveV4ConfigPositionManagerDeployProcedure.sol +++ b/src/deployments/procedures/deploy/position-manager/AaveV4ConfigPositionManagerDeployProcedure.sol @@ -16,7 +16,7 @@ contract AaveV4ConfigPositionManagerDeployProcedure is AaveV4DeployProcedureBase function _deployConfigPositionManager(address owner, bytes32 salt) internal returns (address) { require(owner != address(0), 'invalid owner'); return - Create2Utils.create2Deploy({ + Create2Utils.create2DeployIdempotent({ salt: salt, bytecode: abi.encodePacked(type(ConfigPositionManager).creationCode, abi.encode(owner)) }); diff --git a/src/deployments/procedures/deploy/position-manager/AaveV4GiverPositionManagerDeployProcedure.sol b/src/deployments/procedures/deploy/position-manager/AaveV4GiverPositionManagerDeployProcedure.sol index 839fe96db..a76922ead 100644 --- a/src/deployments/procedures/deploy/position-manager/AaveV4GiverPositionManagerDeployProcedure.sol +++ b/src/deployments/procedures/deploy/position-manager/AaveV4GiverPositionManagerDeployProcedure.sol @@ -16,7 +16,7 @@ contract AaveV4GiverPositionManagerDeployProcedure is AaveV4DeployProcedureBase function _deployGiverPositionManager(address owner, bytes32 salt) internal returns (address) { require(owner != address(0), 'invalid owner'); return - Create2Utils.create2Deploy({ + Create2Utils.create2DeployIdempotent({ salt: salt, bytecode: abi.encodePacked(type(GiverPositionManager).creationCode, abi.encode(owner)) }); diff --git a/src/deployments/procedures/deploy/position-manager/AaveV4NativeTokenGatewayDeployProcedure.sol b/src/deployments/procedures/deploy/position-manager/AaveV4NativeTokenGatewayDeployProcedure.sol index 3347b75cd..30e859752 100644 --- a/src/deployments/procedures/deploy/position-manager/AaveV4NativeTokenGatewayDeployProcedure.sol +++ b/src/deployments/procedures/deploy/position-manager/AaveV4NativeTokenGatewayDeployProcedure.sol @@ -22,7 +22,7 @@ contract AaveV4NativeTokenGatewayDeployProcedure is AaveV4DeployProcedureBase { require(nativeWrapper != address(0), 'invalid native wrapper'); require(owner != address(0), 'invalid owner'); return - Create2Utils.create2Deploy({ + Create2Utils.create2DeployIdempotent({ salt: salt, bytecode: abi.encodePacked( type(NativeTokenGateway).creationCode, diff --git a/src/deployments/procedures/deploy/position-manager/AaveV4SignatureGatewayDeployProcedure.sol b/src/deployments/procedures/deploy/position-manager/AaveV4SignatureGatewayDeployProcedure.sol index e4e4a5073..9ddd14c6a 100644 --- a/src/deployments/procedures/deploy/position-manager/AaveV4SignatureGatewayDeployProcedure.sol +++ b/src/deployments/procedures/deploy/position-manager/AaveV4SignatureGatewayDeployProcedure.sol @@ -16,7 +16,7 @@ contract AaveV4SignatureGatewayDeployProcedure is AaveV4DeployProcedureBase { function _deploySignatureGateway(address owner, bytes32 salt) internal returns (address) { require(owner != address(0), 'invalid owner'); return - Create2Utils.create2Deploy({ + Create2Utils.create2DeployIdempotent({ salt: salt, bytecode: abi.encodePacked(type(SignatureGateway).creationCode, abi.encode(owner)) }); diff --git a/src/deployments/procedures/deploy/position-manager/AaveV4TakerPositionManagerDeployProcedure.sol b/src/deployments/procedures/deploy/position-manager/AaveV4TakerPositionManagerDeployProcedure.sol index b7c3e4c72..043a7be03 100644 --- a/src/deployments/procedures/deploy/position-manager/AaveV4TakerPositionManagerDeployProcedure.sol +++ b/src/deployments/procedures/deploy/position-manager/AaveV4TakerPositionManagerDeployProcedure.sol @@ -16,7 +16,7 @@ contract AaveV4TakerPositionManagerDeployProcedure is AaveV4DeployProcedureBase function _deployTakerPositionManager(address owner, bytes32 salt) internal returns (address) { require(owner != address(0), 'invalid owner'); return - Create2Utils.create2Deploy({ + Create2Utils.create2DeployIdempotent({ salt: salt, bytecode: abi.encodePacked(type(TakerPositionManager).creationCode, abi.encode(owner)) }); diff --git a/src/deployments/procedures/deploy/spoke/AaveV4SpokeConfiguratorDeployProcedure.sol b/src/deployments/procedures/deploy/spoke/AaveV4SpokeConfiguratorDeployProcedure.sol index 7839c0872..63b91c3e6 100644 --- a/src/deployments/procedures/deploy/spoke/AaveV4SpokeConfiguratorDeployProcedure.sol +++ b/src/deployments/procedures/deploy/spoke/AaveV4SpokeConfiguratorDeployProcedure.sol @@ -15,7 +15,7 @@ contract AaveV4SpokeConfiguratorDeployProcedure is AaveV4DeployProcedureBase { function _deploySpokeConfigurator(address authority, bytes32 salt) internal returns (address) { require(authority != address(0), 'invalid authority'); return - Create2Utils.create2Deploy( + Create2Utils.create2DeployIdempotent( salt, abi.encodePacked(type(SpokeConfigurator).creationCode, abi.encode(authority)) ); diff --git a/src/deployments/procedures/deploy/spoke/AaveV4SpokeDeployProcedure.sol b/src/deployments/procedures/deploy/spoke/AaveV4SpokeDeployProcedure.sol index 22221ce67..dab7e5e3e 100644 --- a/src/deployments/procedures/deploy/spoke/AaveV4SpokeDeployProcedure.sol +++ b/src/deployments/procedures/deploy/spoke/AaveV4SpokeDeployProcedure.sol @@ -30,7 +30,7 @@ contract AaveV4SpokeDeployProcedure is AaveV4DeployProcedureBase { require(authority != address(0), 'invalid authority'); require(oracle != address(0), 'invalid oracle'); require(maxUserReservesLimit > 0, 'invalid max user reserves limit'); - spokeImplementation = Create2Utils.create2Deploy({ + spokeImplementation = Create2Utils.create2DeployIdempotent({ salt: salt, bytecode: _getSpokeInstanceInitCode(spokeBytecode, oracle, maxUserReservesLimit) }); diff --git a/src/deployments/procedures/deploy/spoke/AaveV4TokenizationSpokeDeployProcedure.sol b/src/deployments/procedures/deploy/spoke/AaveV4TokenizationSpokeDeployProcedure.sol index 531ccbc89..8000e5ed6 100644 --- a/src/deployments/procedures/deploy/spoke/AaveV4TokenizationSpokeDeployProcedure.sol +++ b/src/deployments/procedures/deploy/spoke/AaveV4TokenizationSpokeDeployProcedure.sol @@ -33,7 +33,7 @@ contract AaveV4TokenizationSpokeDeployProcedure is AaveV4DeployProcedureBase { require(bytes(shareName).length > 0, 'invalid share name'); require(bytes(shareSymbol).length > 0, 'invalid share symbol'); - tokenizationSpokeImplementation = Create2Utils.create2Deploy({ + tokenizationSpokeImplementation = Create2Utils.create2DeployIdempotent({ salt: salt, bytecode: _getTokenizationSpokeInstanceInitCode(hub, underlying) }); diff --git a/src/deployments/procedures/deploy/spoke/AaveV4TreasurySpokeDeployProcedure.sol b/src/deployments/procedures/deploy/spoke/AaveV4TreasurySpokeDeployProcedure.sol index aca87b2d0..57d7eb142 100644 --- a/src/deployments/procedures/deploy/spoke/AaveV4TreasurySpokeDeployProcedure.sol +++ b/src/deployments/procedures/deploy/spoke/AaveV4TreasurySpokeDeployProcedure.sol @@ -15,7 +15,7 @@ contract AaveV4TreasurySpokeDeployProcedure is AaveV4DeployProcedureBase { /// @return The address of the deployed transparent proxy contract. function _deployTreasurySpoke(address owner, bytes32 salt) internal returns (address) { require(owner != address(0), 'invalid owner'); - address implementation = Create2Utils.create2Deploy( + address implementation = Create2Utils.create2DeployIdempotent( salt, type(TreasurySpokeInstance).creationCode ); diff --git a/src/deployments/utils/libraries/Create2Utils.sol b/src/deployments/utils/libraries/Create2Utils.sol index f380ffb8f..e142064b6 100644 --- a/src/deployments/utils/libraries/Create2Utils.sol +++ b/src/deployments/utils/libraries/Create2Utils.sol @@ -10,6 +10,15 @@ library Create2Utils { // https://github.com/safe-global/safe-singleton-factory address public constant CREATE2_FACTORY = 0x914d7Fec6aaC8cd542e72Bca78B30650d45643d7; + /// @notice Emitted when a contract already present at the computed CREATE2 address is adopted + /// instead of being deployed again. + /// @dev Absence of this event on a deployment step means the contract was freshly deployed. + /// Its presence means the step was a no-op adoption, which is the signal an operator needs + /// to notice an accidental re-run or a third party having occupied the address first. + /// @param deployed The address that was adopted. + /// @param salt The CREATE2 salt the address was derived from. + event Create2DeploymentAdopted(address indexed deployed, bytes32 salt); + error MissingCreate2Factory(); error Create2AddressDerivationFailure(); error FailedCreate2FactoryCall(); @@ -32,7 +41,31 @@ library Create2Utils { return deployedAt; } + /// @notice Deploys a contract via CREATE2, returning the existing address if it is already + /// deployed instead of reverting. + /// @dev Safe because a CREATE2 address commits to `keccak256(bytecode)`: any contract living at + /// the computed address must have been created by this factory running this exact creation + /// code, so its runtime code is necessarily identical to what this call would produce. + /// Use this for steps that must be idempotent and must not be blockable by a third party + /// occupying the deterministic address first. + /// @param salt The CREATE2 salt. + /// @param bytecode The contract creation bytecode. + /// @return The deployed contract address. + function create2DeployIdempotent(bytes32 salt, bytes memory bytecode) internal returns (address) { + address computed = computeCreate2Address({salt: salt, bytecode: bytecode}); + if (isContractDeployed(computed)) { + emit Create2DeploymentAdopted(computed, salt); + return computed; + } + return create2Deploy({salt: salt, bytecode: bytecode}); + } + /// @notice Deploys a TransparentUpgradeableProxy via CREATE2. + /// @dev Idempotent, matching the deployment procedures. The proxy admin owner and the + /// initializer calldata are both constructor arguments, so they are covered by the init + /// code hash the CREATE2 address commits to. No protocol initializer reads `msg.sender`, + /// block data or any other environment value, so a proxy already present at the computed + /// address is state-equivalent to the one this call would create. /// @param salt The CREATE2 salt. /// @param logic The implementation contract address. /// @param initialOwner The initial proxy admin owner. @@ -45,7 +78,7 @@ library Create2Utils { bytes memory data ) internal returns (address) { return - create2Deploy( + create2DeployIdempotent( salt, abi.encodePacked( type(TransparentUpgradeableProxy).creationCode, diff --git a/tests/deployments/AaveV4DeployOrchestration.Idempotency.t.sol b/tests/deployments/AaveV4DeployOrchestration.Idempotency.t.sol new file mode 100644 index 000000000..1a1327921 --- /dev/null +++ b/tests/deployments/AaveV4DeployOrchestration.Idempotency.t.sol @@ -0,0 +1,141 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +import 'tests/utils/BatchTestProcedures.sol'; + +import {AccessManagerEnumerable} from 'src/access/AccessManagerEnumerable.sol'; + +/// @dev Deterministic CREATE2 deployment must be idempotent. +/// +/// The orchestration derives its root salt as +/// `bytes32(bytes20(deployer)) | (keccak256(abi.encode(SALT, deployInputs.salt)) >> 160)` +/// which looks deployer-namespaced. But the Safe Singleton Factory does not enforce the salt +/// prefix, so namespacing only helps if the salt is UNPREDICTABLE. When `deployInputs.salt` is +/// left at `bytes32(0)` — which the deploy script permits with a warning only, and which the +/// repo's own deployment tests use — every input is public and the whole root salt is computable +/// in advance from the (public) deployer address alone. +contract AaveV4DeployOrchestrationIdempotencyTest is BatchTestProcedures { + address internal ATTACKER = makeAddr('ATTACKER'); + + function setUp() public override { + super.setUp(); + + _inputs = InputUtils.FullDeployInputs({ + accessManagerAdmin: makeAddr('accessManagerAdmin'), + proxyAdminOwner: makeAddr('proxyAdminOwner'), + hubAdmin: makeAddr('hubAdmin'), + hubConfiguratorAdmin: makeAddr('hubConfiguratorAdmin'), + treasurySpokeOwner: makeAddr('treasurySpokeOwner'), + spokeAdmin: makeAddr('spokeAdmin'), + spokeConfiguratorAdmin: makeAddr('spokeConfiguratorAdmin'), + gatewayOwner: makeAddr('gatewayOwner'), + positionManagerOwner: makeAddr('positionManagerOwner'), + nativeWrapper: _weth9, + deployNativeTokenGateway: false, + deploySignatureGateway: false, + deployPositionManagers: false, + grantRoles: true, + hubLabels: _hubLabels, + spokeLabels: _spokeLabels, + spokeMaxReservesLimits: _defaultSpokeMaxReservesLimits(_spokeLabels.length), + salt: bytes32(0) + }); + } + + /// @dev Reproduces AaveV4DeployOrchestration._deriveSalt using only public information. + function _rootSalt(address deployer, bytes32 userSalt) internal pure returns (bytes32) { + return + bytes32(bytes20(deployer)) | + (keccak256(abi.encode(keccak256('AAVE_V4'), userSalt)) >> 160); + } + + /// @dev External so `vm.expectRevert` binds to the deployment frame; `deployAaveV4` is an + /// internal library and would otherwise be inlined at test depth. + function runDeploymentExternal() external { + vm.startPrank(_deployer); + AaveV4DeployOrchestration.deployAaveV4( + _logger, + _deployer, + _inputs, + BytecodeHelper.getHubBytecode(), + BytecodeHelper.getSpokeBytecode() + ); + vm.stopPrank(); + } + + function _runDeployment() internal { + this.runDeploymentExternal(); + } + + /// @dev Control: the deployment works when nobody interferes. + function test_baseline_deploymentSucceeds() public { + _runDeployment(); + } + + /// @dev PROOF: with the zero user salt, the AccessManagerEnumerable address — the very first + /// contract of the deployment — is computable by anyone, and its init code has no precondition + /// (constructor only stores an admin), so a third party can occupy it and brick the whole run. + function test_attackerOccupiesAccessManager_deploymentStillCompletes() public { + bytes32 salt = _rootSalt(_deployer, bytes32(0)); + + // Root admin is the deployer itself (AaveV4DeployOrchestration sets initialAdmin = deployer). + bytes memory initCode = abi.encodePacked( + type(AccessManagerEnumerable).creationCode, + abi.encode(_deployer) + ); + address predicted = Create2Utils.computeCreate2Address(salt, initCode); + + assertEq(predicted.code.length, 0, 'address free before the attack'); + + vm.prank(ATTACKER); + (bool ok, ) = Create2Utils.CREATE2_FACTORY.call(abi.encodePacked(salt, initCode)); + assertTrue(ok, 'attacker can deploy it: no unmet constructor precondition'); + assertGt(predicted.code.length, 0, 'attacker occupies the AccessManager address'); + + // BEFORE THE FIX: reverted with Create2Utils.ContractAlreadyDeployed, bricking the run. + // AFTER THE FIX: the deployment reuses the identical contract and completes. + this.runDeploymentExternal(); + assertGt(predicted.code.length, 0, 'deployment completed reusing the occupied address'); + } + + /// @dev The Hub implementation is the softest target of all: `create2Deploy` is called with the + /// raw compiled bytecode and NO constructor arguments, so its init code is a public constant. + /// Only the child salt is needed, and that is derived from the root salt plus the hub label. + function test_attackerOccupiesHubImplementation_deploymentStillCompletes() public { + bytes32 salt = _rootSalt(_deployer, bytes32(0)); + bytes32 childSalt = keccak256(abi.encode(salt, 'hub', _hubLabels[0])); + + bytes memory hubBytecode = BytecodeHelper.getHubBytecode(); + address predicted = Create2Utils.computeCreate2Address(childSalt, hubBytecode); + + vm.prank(ATTACKER); + (bool ok, ) = Create2Utils.CREATE2_FACTORY.call(abi.encodePacked(childSalt, hubBytecode)); + assertTrue(ok, 'HubInstance has no constructor args and no preconditions'); + assertGt(predicted.code.length, 0, 'attacker occupies the Hub implementation address'); + + // BEFORE THE FIX: reverted with Create2Utils.ContractAlreadyDeployed. + this.runDeploymentExternal(); + assertGt(predicted.code.length, 0, 'deployment completed reusing the occupied address'); + } + + /// @dev A non-zero, unguessable user salt removes the pre-positioning capability: the attacker + /// cannot compute the address ahead of the deployment transaction. + function test_nonZeroSecretSalt_addressIsNotPrecomputable() public { + bytes32 secret = keccak256('operator chosen secret salt'); + bytes32 guessed = _rootSalt(_deployer, bytes32(0)); + bytes32 actual = _rootSalt(_deployer, secret); + + assertTrue(guessed != actual, 'a secret user salt changes the derived root salt'); + + _inputs.salt = secret; + // Occupying the address the attacker CAN compute (zero-salt derivation) is now harmless. + bytes memory initCode = abi.encodePacked( + type(AccessManagerEnumerable).creationCode, + abi.encode(_deployer) + ); + vm.prank(ATTACKER); + Create2Utils.CREATE2_FACTORY.call(abi.encodePacked(guessed, initCode)); + + _runDeployment(); + } +} diff --git a/tests/scripts/SpokeDeployUtils.t.sol b/tests/scripts/SpokeDeployUtils.t.sol new file mode 100644 index 000000000..f7542a8b2 --- /dev/null +++ b/tests/scripts/SpokeDeployUtils.t.sol @@ -0,0 +1,112 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +import {Test} from 'forge-std/Test.sol'; +import {Vm} from 'forge-std/Vm.sol'; +import {Create2Utils} from 'src/deployments/utils/libraries/Create2Utils.sol'; +import {SpokeDeployUtils} from 'scripts/utils/SpokeDeployUtils.sol'; +import {Create2TestHelper} from 'tests/utils/Create2TestHelper.sol'; + +/// @dev `LibraryPreCompile` (the mandatory first deployment step) deploys LiquidationLogic +/// through the permissionless Safe Singleton Factory with a HARDCODED `bytes32(0)` salt and no +/// deployer namespacing. `Create2Utils.create2Deploy` reverts when the computed address is already +/// occupied, and LiquidationLogic is a plain library: its init code has no constructor arguments +/// and no dependency on any chain state, so anyone can deploy it at that exact address first. +/// @dev Thin external wrapper so `vm.expectRevert` sees a sub-call frame. +contract PreCompileHarness { + function deploy(bytes32 salt) external returns (address) { + return SpokeDeployUtils.deployLiquidationLogic(salt); + } +} + +contract SpokeDeployUtilsTest is Test, Create2TestHelper { + PreCompileHarness internal harness; + address internal ATTACKER = makeAddr('ATTACKER'); + + function setUp() public { + _etchCreate2Factory(); + harness = new PreCompileHarness(); + } + + function _liquidationLogicInitCode() internal view returns (bytes memory) { + return vm.getCode('src/spoke/libraries/LiquidationLogic.sol:LiquidationLogic'); + } + + /// @dev Control: on a clean chain the pre-compile step works. + function test_baseline_deploysLiquidationLogic() public { + address deployed = harness.deploy(bytes32(0)); + assertGt(deployed.code.length, 0, 'library should be deployed'); + } + + /// @dev PROOF: the salt is a public constant and the init code is state-independent, so a third + /// party can occupy the address and the mandatory deployment step then reverts. + function test_attackerOccupiesZeroSaltAddress_blocksDeploymentStep() public { + bytes memory initCode = _liquidationLogicInitCode(); + address predicted = Create2Utils.computeCreate2Address(bytes32(0), initCode); + + assertEq(predicted.code.length, 0, 'address must be free before the attack'); + + // Anyone can do this: no arguments to guess, no protocol state required. + vm.prank(ATTACKER); + (bool ok, ) = Create2Utils.CREATE2_FACTORY.call(abi.encodePacked(bytes32(0), initCode)); + assertTrue(ok, 'attacker deployment succeeds'); + assertGt(predicted.code.length, 0, 'attacker occupies the deterministic address'); + + // BEFORE THE FIX this reverted with Create2Utils.ContractAlreadyDeployed, blocking + // `make deploy-precompile` with no configurable salt to route around it. + // AFTER THE FIX the step reuses the identical contract already at that address. + address deployed = harness.deploy(bytes32(0)); + assertEq(deployed, predicted, 'step reuses the deterministic address'); + assertEq( + predicted.codehash, + keccak256(deployed.code), + 'reused contract is the one the step would have deployed' + ); + } + + /// @dev Re-running the step after a legitimate deployment (e.g. when the .env guard in + /// LibraryPreCompile is absent) must be a no-op rather than a revert. + function test_stepIsIdempotent() public { + address first = harness.deploy(bytes32(0)); + // BEFORE THE FIX this second call reverted with ContractAlreadyDeployed. + address second = harness.deploy(bytes32(0)); + + assertEq(second, first, 're-running the step is a no-op'); + assertGt(first.code.length, 0); + } + + /// @dev The adoption branch must stay visible in the deployment receipt: a fresh deploy emits + /// nothing, an adoption emits Create2DeploymentAdopted. This preserves the diagnostic signal + /// that the removed revert used to provide, without reintroducing the DoS. + function test_adoptionIsObservableInLogs() public { + bytes memory initCode = _liquidationLogicInitCode(); + address predicted = Create2Utils.computeCreate2Address(bytes32(0), initCode); + + // Fresh deployment: no adoption event. + vm.recordLogs(); + harness.deploy(bytes32(0)); + Vm.Log[] memory freshLogs = vm.getRecordedLogs(); + for (uint256 i; i < freshLogs.length; ++i) { + assertTrue( + freshLogs[i].topics[0] != Create2Utils.Create2DeploymentAdopted.selector, + 'fresh deployment must not emit an adoption event' + ); + } + + // Re-run: adoption event, emitted by the caller of the inlined library. + vm.expectEmit(true, false, false, true, address(harness)); + emit Create2Utils.Create2DeploymentAdopted(predicted, bytes32(0)); + harness.deploy(bytes32(0)); + } + + /// @dev Contrast with the main orchestration, which namespaces its root salt with the deployer. + /// The pre-compile step does not, and hardcodes zero. + function test_contrast_orchestrationSaltIsDeployerNamespaced() public pure { + address deployer = address(0x1111111111111111111111111111111111111111); + bytes32 derived = bytes32(bytes20(deployer)) | + (keccak256(abi.encode(keccak256('AAVE_V4'), bytes32(0))) >> 160); + + assertEq(address(bytes20(derived)), deployer, 'orchestration salt embeds the deployer'); + assertTrue(derived != bytes32(0), 'orchestration salt is never the zero salt'); + } +}