From ec309418da57817374b87979b9a252bb7d5de467 Mon Sep 17 00:00:00 2001 From: Doug Hoyte Date: Mon, 28 Apr 2025 14:56:20 -0400 Subject: [PATCH 1/4] test from MiloTruck --- test/FactoryTest.t.sol | 54 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/test/FactoryTest.t.sol b/test/FactoryTest.t.sol index 628cd8c..9aec9bf 100644 --- a/test/FactoryTest.t.sol +++ b/test/FactoryTest.t.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0-or-later pragma solidity ^0.8.24; +import {stdError} from "forge-std/Test.sol"; import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol"; import {PoolManagerDeployer} from "./utils/PoolManagerDeployer.sol"; import {Hooks} from "@uniswap/v4-core/src/libraries/Hooks.sol"; @@ -247,4 +248,57 @@ contract FactoryTest is EulerSwapTestBase { vm.expectRevert(EulerSwap.Locked.selector); EulerSwap(eulerSwapImpl).getReserves(); } + + + + address alice = makeAddr("alice"); + address bob = makeAddr("bob"); + + function test_multipleUninstalls() public { + // Parameters for deployPool() + IEulerSwap.Params memory params = IEulerSwap.Params({ + vault0: address(eTST), + vault1: address(eTST2), + eulerAccount: address(0), + equilibriumReserve0: 1e18, + equilibriumReserve1: 1e18, + priceX: 1e18, + priceY: 1e18, + concentrationX: 0, + concentrationY: 0, + fee: 0, + protocolFee: 0, + protocolFeeRecipient: address(0) + }); + IEulerSwap.InitialState memory initialState = IEulerSwap.InitialState({ + currReserve0: 1e18, + currReserve1: 1e18 + }); + bytes32 salt = bytes32(0); + + // Deploy pool for Alice + params.eulerAccount = alice; + address alicePool = eulerSwapFactory.computePoolAddress(params, salt); + vm.startPrank(alice); + evc.setAccountOperator(alice, alicePool, true); + eulerSwapFactory.deployPool(params, initialState, salt); + + // Deploy pool for Bob + params.eulerAccount = bob; + address bobPool = eulerSwapFactory.computePoolAddress(params, salt); + vm.startPrank(bob); + evc.setAccountOperator(bob, bobPool, true); + eulerSwapFactory.deployPool(params, initialState, salt); + + // Uninstall pool for Alice + vm.startPrank(alice); + evc.setAccountOperator(alice, alicePool, false); + eulerSwapFactory.uninstallPool(); + + // Uninstalling pool for Bob reverts due to an OOB access of the allPools array + vm.startPrank(bob); + evc.setAccountOperator(bob, bobPool, false); + vm.expectRevert(stdError.indexOOBError); + eulerSwapFactory.uninstallPool(); + } } From d568fa0971b027b26ccef0ecf9e43132ec80f224 Mon Sep 17 00:00:00 2001 From: Doug Hoyte Date: Mon, 28 Apr 2025 15:24:18 -0400 Subject: [PATCH 2/4] integrate and expand test --- test/FactoryTest.t.sol | 78 ++++++++++++++++++++++++++---------------- 1 file changed, 48 insertions(+), 30 deletions(-) diff --git a/test/FactoryTest.t.sol b/test/FactoryTest.t.sol index 9aec9bf..47658bd 100644 --- a/test/FactoryTest.t.sol +++ b/test/FactoryTest.t.sol @@ -1,7 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-or-later pragma solidity ^0.8.24; -import {stdError} from "forge-std/Test.sol"; import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol"; import {PoolManagerDeployer} from "./utils/PoolManagerDeployer.sol"; import {Hooks} from "@uniswap/v4-core/src/libraries/Hooks.sol"; @@ -249,56 +248,75 @@ contract FactoryTest is EulerSwapTestBase { EulerSwap(eulerSwapImpl).getReserves(); } - - address alice = makeAddr("alice"); address bob = makeAddr("bob"); function test_multipleUninstalls() public { - // Parameters for deployPool() - IEulerSwap.Params memory params = IEulerSwap.Params({ - vault0: address(eTST), - vault1: address(eTST2), - eulerAccount: address(0), - equilibriumReserve0: 1e18, - equilibriumReserve1: 1e18, - priceX: 1e18, - priceY: 1e18, - concentrationX: 0, - concentrationY: 0, - fee: 0, - protocolFee: 0, - protocolFeeRecipient: address(0) - }); - IEulerSwap.InitialState memory initialState = IEulerSwap.InitialState({ - currReserve0: 1e18, - currReserve1: 1e18 - }); - bytes32 salt = bytes32(0); + (IEulerSwap.Params memory params, IEulerSwap.InitialState memory initialState) = getBasicParams(); // Deploy pool for Alice - params.eulerAccount = alice; - address alicePool = eulerSwapFactory.computePoolAddress(params, salt); + params.eulerAccount = holder = alice; + (address alicePool, bytes32 aliceSalt) = mineSalt(params); + vm.startPrank(alice); evc.setAccountOperator(alice, alicePool, true); - eulerSwapFactory.deployPool(params, initialState, salt); + eulerSwapFactory.deployPool(params, initialState, aliceSalt); // Deploy pool for Bob - params.eulerAccount = bob; - address bobPool = eulerSwapFactory.computePoolAddress(params, salt); + params.eulerAccount = holder = bob; + (address bobPool, bytes32 bobSalt) = mineSalt(params); + vm.startPrank(bob); evc.setAccountOperator(bob, bobPool, true); - eulerSwapFactory.deployPool(params, initialState, salt); + eulerSwapFactory.deployPool(params, initialState, bobSalt); + + { + address[] memory ps = eulerSwapFactory.pools(); + assertEq(ps.length, 2); + assertEq(ps[0], alicePool); + assertEq(ps[1], bobPool); + } + + { + (address asset0, address asset1) = EulerSwap(alicePool).getAssets(); + address[] memory ps = eulerSwapFactory.poolsByPair(asset0, asset1); + assertEq(ps.length, 2); + assertEq(ps[0], alicePool); + assertEq(ps[1], bobPool); + } // Uninstall pool for Alice vm.startPrank(alice); evc.setAccountOperator(alice, alicePool, false); eulerSwapFactory.uninstallPool(); + { + address[] memory ps = eulerSwapFactory.pools(); + assertEq(ps.length, 1); + assertEq(ps[0], bobPool); + } + + { + (address asset0, address asset1) = EulerSwap(alicePool).getAssets(); + address[] memory ps = eulerSwapFactory.poolsByPair(asset0, asset1); + assertEq(ps.length, 1); + assertEq(ps[0], bobPool); + } + // Uninstalling pool for Bob reverts due to an OOB access of the allPools array vm.startPrank(bob); evc.setAccountOperator(bob, bobPool, false); - vm.expectRevert(stdError.indexOOBError); eulerSwapFactory.uninstallPool(); + + { + address[] memory ps = eulerSwapFactory.pools(); + assertEq(ps.length, 0); + } + + { + (address asset0, address asset1) = EulerSwap(alicePool).getAssets(); + address[] memory ps = eulerSwapFactory.poolsByPair(asset0, asset1); + assertEq(ps.length, 0); + } } } From 36350925bd78432c3123bc94af2e36785a0d80df Mon Sep 17 00:00:00 2001 From: Doug Hoyte Date: Mon, 28 Apr 2025 15:35:10 -0400 Subject: [PATCH 3/4] fix issue with multiple uninstalls by using OZ's EnumerableSet library --- src/EulerSwapFactory.sol | 62 +++++++++++++--------------- src/interfaces/IEulerSwapFactory.sol | 6 --- 2 files changed, 28 insertions(+), 40 deletions(-) diff --git a/src/EulerSwapFactory.sol b/src/EulerSwapFactory.sol index c115ec0..08ed404 100644 --- a/src/EulerSwapFactory.sol +++ b/src/EulerSwapFactory.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.27; +import {EnumerableSet} from "openzeppelin-contracts/utils/structs/EnumerableSet.sol"; + import {IEulerSwapFactory, IEulerSwap} from "./interfaces/IEulerSwapFactory.sol"; import {EVCUtil} from "ethereum-vault-connector/utils/EVCUtil.sol"; import {GenericFactory} from "evk/GenericFactory/GenericFactory.sol"; @@ -13,15 +15,19 @@ import {MetaProxyDeployer} from "./utils/MetaProxyDeployer.sol"; /// @custom:security-contact security@euler.xyz /// @author Euler Labs (https://www.eulerlabs.com/) contract EulerSwapFactory is IEulerSwapFactory, EVCUtil, ProtocolFee { - /// @dev An array to store all pools addresses. - address[] private allPools; + using EnumerableSet for EnumerableSet.AddressSet; + /// @dev Vaults must be deployed by this factory address public immutable evkFactory; /// @dev The EulerSwap code instance that will be proxied to address public immutable eulerSwapImpl; - /// @dev Mapping between euler account and EulerAccountState - mapping(address eulerAccount => EulerAccountState state) private eulerAccountState; - mapping(address asset0 => mapping(address asset1 => address[])) private poolMap; + + /// @dev Mapping from euler account to pool, if installed + mapping(address eulerAccount => address) internal installedPools; + /// @dev Set of all pool addresses + EnumerableSet.AddressSet internal allPools; + /// @dev Mapping from sorted pair of underlyings to set of pools + mapping(address asset0 => mapping(address asset1 => EnumerableSet.AddressSet)) internal poolMap; event PoolDeployed(address indexed asset0, address indexed asset1, address indexed eulerAccount, address pool); event PoolConfig(address indexed pool, IEulerSwap.Params params, IEulerSwap.InitialState initialState); @@ -102,12 +108,12 @@ contract EulerSwapFactory is IEulerSwapFactory, EVCUtil, ProtocolFee { /// @inheritdoc IEulerSwapFactory function poolByEulerAccount(address eulerAccount) external view returns (address) { - return eulerAccountState[eulerAccount].pool; + return installedPools[eulerAccount]; } /// @inheritdoc IEulerSwapFactory function poolsLength() external view returns (uint256) { - return allPools.length; + return allPools.length(); } /// @inheritdoc IEulerSwapFactory @@ -122,7 +128,7 @@ contract EulerSwapFactory is IEulerSwapFactory, EVCUtil, ProtocolFee { /// @inheritdoc IEulerSwapFactory function poolsByPairLength(address asset0, address asset1) external view returns (uint256) { - return poolMap[asset0][asset1].length; + return poolMap[asset0][asset1].length(); } /// @inheritdoc IEulerSwapFactory @@ -147,16 +153,10 @@ contract EulerSwapFactory is IEulerSwapFactory, EVCUtil, ProtocolFee { (address asset0, address asset1) = _getAssets(newOperator); - address[] storage poolMapArray = poolMap[asset0][asset1]; + installedPools[eulerAccount] = newOperator; - eulerAccountState[eulerAccount] = EulerAccountState({ - pool: newOperator, - allPoolsIndex: uint48(allPools.length), - poolMapIndex: uint48(poolMapArray.length) - }); - - allPools.push(newOperator); - poolMapArray.push(newOperator); + allPools.add(newOperator); + poolMap[asset0][asset1].add(newOperator); } /// @notice Uninstalls the pool associated with the given Euler account @@ -165,7 +165,7 @@ contract EulerSwapFactory is IEulerSwapFactory, EVCUtil, ProtocolFee { /// @dev If no pool exists for the account, the function returns without any action /// @param eulerAccount The address of the Euler account whose pool should be uninstalled function uninstall(address eulerAccount) internal { - address pool = eulerAccountState[eulerAccount].pool; + address pool = installedPools[eulerAccount]; if (pool == address(0)) return; @@ -173,24 +173,14 @@ contract EulerSwapFactory is IEulerSwapFactory, EVCUtil, ProtocolFee { (address asset0, address asset1) = _getAssets(pool); - address[] storage poolMapArr = poolMap[asset0][asset1]; - - swapAndPop(allPools, eulerAccountState[eulerAccount].allPoolsIndex); - swapAndPop(poolMapArr, eulerAccountState[eulerAccount].poolMapIndex); + allPools.remove(pool); + poolMap[asset0][asset1].remove(pool); - delete eulerAccountState[eulerAccount]; + delete installedPools[eulerAccount]; emit PoolUninstalled(asset0, asset1, eulerAccount, pool); } - /// @notice Swaps the element at the given index with the last element and removes the last element - /// @param arr The storage array to modify - /// @param index The index of the element to remove - function swapAndPop(address[] storage arr, uint256 index) internal { - arr[index] = arr[arr.length - 1]; - arr.pop(); - } - /// @notice Retrieves the asset addresses for a given pool /// @dev Calls the pool contract to get its asset0 and asset1 addresses /// @param pool The address of the pool to query @@ -206,14 +196,18 @@ contract EulerSwapFactory is IEulerSwapFactory, EVCUtil, ProtocolFee { /// @param start The starting index of the slice (inclusive) /// @param end The ending index of the slice (exclusive) /// @return A new memory array containing the requested slice of addresses - function _getSlice(address[] storage arr, uint256 start, uint256 end) internal view returns (address[] memory) { - uint256 length = arr.length; + function _getSlice(EnumerableSet.AddressSet storage arr, uint256 start, uint256 end) + internal + view + returns (address[] memory) + { + uint256 length = arr.length(); if (end == type(uint256).max) end = length; if (end < start || end > length) revert SliceOutOfBounds(); address[] memory slice = new address[](end - start); for (uint256 i; i < end - start; ++i) { - slice[i] = arr[start + i]; + slice[i] = arr.at(start + i); } return slice; diff --git a/src/interfaces/IEulerSwapFactory.sol b/src/interfaces/IEulerSwapFactory.sol index d20c391..bdd4c34 100644 --- a/src/interfaces/IEulerSwapFactory.sol +++ b/src/interfaces/IEulerSwapFactory.sol @@ -4,12 +4,6 @@ pragma solidity >=0.8.0; import {IEulerSwap} from "./IEulerSwap.sol"; interface IEulerSwapFactory { - struct EulerAccountState { - address pool; - uint48 allPoolsIndex; - uint48 poolMapIndex; - } - /// @notice Deploy a new EulerSwap pool with the given parameters /// @dev The pool address is deterministically generated using CREATE2 with a salt derived from /// the euler account address and provided salt parameter. This allows the pool address to be From c751556a304f8fe6f2ed9d5c7849a2cbcfe28d9e Mon Sep 17 00:00:00 2001 From: Doug Hoyte Date: Tue, 29 Apr 2025 14:36:42 -0400 Subject: [PATCH 4/4] use values() method of EnumerableSet when possible, to make the code simpler --- src/EulerSwapFactory.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/EulerSwapFactory.sol b/src/EulerSwapFactory.sol index 6dd25c7..00cfbb5 100644 --- a/src/EulerSwapFactory.sol +++ b/src/EulerSwapFactory.sol @@ -119,7 +119,7 @@ contract EulerSwapFactory is IEulerSwapFactory, EVCUtil, ProtocolFee { /// @inheritdoc IEulerSwapFactory function pools() external view returns (address[] memory) { - return _getSlice(allPools, 0, type(uint256).max); + return allPools.values(); } /// @inheritdoc IEulerSwapFactory @@ -138,7 +138,7 @@ contract EulerSwapFactory is IEulerSwapFactory, EVCUtil, ProtocolFee { /// @inheritdoc IEulerSwapFactory function poolsByPair(address asset0, address asset1) external view returns (address[] memory) { - return _getSlice(poolMap[asset0][asset1], 0, type(uint256).max); + return poolMap[asset0][asset1].values(); } /// @notice Validates operator authorization for euler account and update the relevant EulerAccountState.