diff --git a/src/base/Cellar.sol b/src/base/Cellar.sol index bc35895d..f0af2a13 100644 --- a/src/base/Cellar.sol +++ b/src/base/Cellar.sol @@ -63,16 +63,16 @@ contract Cellar is ERC4626, Owned, ERC721Holder { event PositionSwapped(uint32 newPosition1, uint32 newPosition2, uint256 index1, uint256 index2); /** - * @notice Attempted an operation on an untrusted position. - * @param position address of the position + * @notice Attempted to add a position that is already being used. + * @param position id of the position */ - error Cellar__UntrustedPosition(address position); + error Cellar__PositionAlreadyUsed(uint32 position); /** - * @notice Attempted to add a position that is already being used. - * @param position address of the position + * @notice Attempted to make an unused position the holding position. + * @param position id of the position */ - error Cellar__PositionAlreadyUsed(uint32 position); + error Cellar__PositionNotUsed(uint32 position); /** * @notice Attempted an action on a position that is required to be empty before the action can be performed. @@ -88,13 +88,6 @@ contract Cellar is ERC4626, Owned, ERC721Holder { */ error Cellar__AssetMismatch(address asset, address expectedAsset); - /** - * @notice Attempted an action on a position that is not being used by the cellar but must be for - * the operation to succeed. - * @param position address of the invalid position - */ - error Cellar__InvalidPosition(address position); - /** * @notice Attempted to add a position when the position array is full. * @param maxPositions maximum number of positions that can be used @@ -112,6 +105,12 @@ contract Cellar is ERC4626, Owned, ERC721Holder { */ error Cellar__RemovingHoldingPosition(); + /** + * @notice Attempted to add an invalid holding position. + * @param positionId the id of the invalid position. + */ + error Cellar__InvalidHoldingPosition(uint32 positionId); + /** * @notice Array of uint32s made up of cellars credit positions Ids. */ @@ -154,15 +153,20 @@ contract Cellar is ERC4626, Owned, ERC721Holder { /** * @notice Stores the index of the holding position in the creditPositions array. */ - uint8 public holdingIndex; + uint32 public holdingPosition; /** * @notice Allows owner to change the holding position. */ - function setHoldingIndex(uint8 index) external onlyOwner { - uint32 positionId = creditPositions[index]; + function setHoldingPosition(uint32 positionId) external onlyOwner { + _setHoldingPosition(positionId); + } + + function _setHoldingPosition(uint32 positionId) internal { + if (!isPositionUsed[positionId]) revert Cellar__PositionNotUsed(positionId); if (_assetOf(positionId) != asset) revert Cellar__AssetMismatch(address(asset), address(_assetOf(positionId))); - holdingIndex = index; + if (getPositionData[positionId].isDebt) revert Cellar__InvalidHoldingPosition(positionId); + holdingPosition = positionId; } /** @@ -215,8 +219,6 @@ contract Cellar is ERC4626, Owned, ERC721Holder { debtPositions.add(index, positionId); } else { if (creditPositions.length >= MAX_POSITIONS) revert Cellar__PositionArrayFull(MAX_POSITIONS); - if (index == holdingIndex && _assetOf(positionId) != asset) - revert Cellar__AssetMismatch(address(asset), address(_assetOf(positionId))); // Add new position at a specified index. creditPositions.add(index, positionId); } @@ -234,6 +236,8 @@ contract Cellar is ERC4626, Owned, ERC721Holder { // Get position being removed. uint32 positionId = inDebtArray ? debtPositions[index] : creditPositions[index]; + if (positionId == holdingPosition) revert Cellar__RemovingHoldingPosition(); + // Only remove position if it is empty, and if it is not the holding position. uint256 positionBalance = _balanceOf(positionId); if (positionBalance > 0) revert Cellar__PositionNotEmpty(positionId, positionBalance); @@ -242,8 +246,6 @@ contract Cellar is ERC4626, Owned, ERC721Holder { // Remove position at the given index. debtPositions.remove(index); } else { - // If removing the holding position, make sure new holding position is valid. - if (index == holdingIndex) revert Cellar__RemovingHoldingPosition(); creditPositions.remove(index); } @@ -278,11 +280,6 @@ contract Cellar is ERC4626, Owned, ERC721Holder { newPosition2 = creditPositions[index1]; // Swap positions. (creditPositions[index1], creditPositions[index2]) = (newPosition1, newPosition2); - // If we swapped with the holding index make sure new holding position works. - if (index1 == holdingIndex || index2 == holdingIndex) { - if (_assetOf(creditPositions[holdingIndex]) != asset) - revert Cellar__AssetMismatch(address(_assetOf(creditPositions[holdingIndex])), address(asset)); - } } emit PositionSwapped(newPosition1, newPosition2, index1, index2); @@ -485,6 +482,9 @@ contract Cellar is ERC4626, Owned, ERC721Holder { * - _assetRiskTolerance this cellars risk tolerance for assets it is exposed to * - _protocolRiskTolerance this cellars risk tolerance for protocols it will use */ + // TODO add tests related to setting new holding position + // Fix initializer function + // Fix tests. constructor( Registry _registry, ERC20 _asset, @@ -500,17 +500,19 @@ contract Cellar is ERC4626, Owned, ERC721Holder { uint32[] memory _debtPositions, bytes[] memory _creditConfigurationData, bytes[] memory _debtConfigurationData, - uint8 _holdingIndex + uint32 _holdingPosition ) = abi.decode(params, (uint32[], uint32[], bytes[], bytes[], uint8)); // Initialize positions. - holdingIndex = _holdingIndex; for (uint32 i; i < _creditPositions.length; ++i) { _addPosition(i, _creditPositions[i], _creditConfigurationData[i], false); } for (uint32 i; i < _debtPositions.length; ++i) { _addPosition(i, _debtPositions[i], _debtConfigurationData[i], true); } + // This check allows us to deploy an implementation contract. + /// @dev No cellars will be deployed with a zero length credit positions array. + if (_creditPositions.length > 0) _setHoldingPosition(_holdingPosition); } // Initialize last accrual timestamp to time that cellar was created, otherwise the first @@ -675,7 +677,7 @@ contract Cellar is ERC4626, Owned, ERC721Holder { uint256, address receiver ) internal override { - _depositTo(creditPositions[holdingIndex], assets); + _depositTo(holdingPosition, assets); userShareLockStartTime[receiver] = block.timestamp; } @@ -869,10 +871,6 @@ contract Cellar is ERC4626, Owned, ERC721Holder { ); totalWithdrawableBalanceInAssets = totalWithdrawableBalanceInAssets / PRECISION_MULTIPLIER; } - // exchangeRate = priceRouter.getExchangeRate(positionAsset, asset); - - // Denominate withdrawable position balance in cellar's asset. - // uint256 totalWithdrawableBalanceInAssets = withdrawableBalance.mulDivDown(exchangeRate, onePositionAsset); // We want to pull as much as we can from this position, but no more than needed. uint256 amount; diff --git a/src/base/CellarInitializable.sol b/src/base/CellarInitializable.sol index 5b973875..309d1a4e 100644 --- a/src/base/CellarInitializable.sol +++ b/src/base/CellarInitializable.sol @@ -16,16 +16,7 @@ contract CellarInitializable is Cellar, Initializable { ERC20(address(0)), "", "", - abi.encode( - new uint32[](0), - new uint32[](0), - new bytes[](0), - new bytes[](0), - 100, // Pick an invalid holding index so we skip the asset check. - address(0), - 0, - 0 - ) + abi.encode(new uint32[](0), new uint32[](0), new bytes[](0), new bytes[](0), 0, address(0), 0, 0) ) {} @@ -41,7 +32,7 @@ contract CellarInitializable is Cellar, Initializable { * - uint32[] array of debt positions * - bytes[] array of credit config data * - bytes[] array of debt config data - * - uint8 holding index + * - uint32 holding position id * - address strategist payout address * - uint128 asset risk tolerance * - uint128 protocol risk tolerance @@ -71,16 +62,17 @@ contract CellarInitializable is Cellar, Initializable { uint32[] memory _debtPositions, bytes[] memory _creditConfigurationData, bytes[] memory _debtConfigurationData, - uint8 _holdingIndex, + uint32 _holdingPosition, address _strategistPayout, uint128 _assetRiskTolerance, uint128 _protocolRiskTolerance - ) = abi.decode(_params, (uint32[], uint32[], bytes[], bytes[], uint8, address, uint128, uint128)); - holdingIndex = _holdingIndex; + ) = abi.decode(_params, (uint32[], uint32[], bytes[], bytes[], uint32, address, uint128, uint128)); + for (uint32 i; i < _creditPositions.length; i++) _addPosition(i, _creditPositions[i], _creditConfigurationData[i], false); for (uint32 i; i < _debtPositions.length; i++) _addPosition(i, _debtPositions[i], _debtConfigurationData[i], true); + _setHoldingPosition(_holdingPosition); // Initialize remaining values. assetRiskTolerance = _assetRiskTolerance; diff --git a/test/Cellar.t.sol b/test/Cellar.t.sol index bc901fa3..4cb80c69 100644 --- a/test/Cellar.t.sol +++ b/test/Cellar.t.sol @@ -134,7 +134,7 @@ contract CellarTest is Test { debtPositions, positionConfigs, debtConfigs, - 0, + usdcPosition, strategist, type(uint128).max, type(uint128).max @@ -490,12 +490,6 @@ contract CellarTest is Test { deal(address(WETH), address(cellar), 0); cellar.removePosition(4, false); - // Check that adding a position to the holding index reverts if the position asset does not - // equal the cellar asset. - vm.expectRevert( - bytes(abi.encodeWithSelector(Cellar.Cellar__AssetMismatch.selector, address(USDC), address(WETH))) - ); - cellar.addPosition(0, wethPosition, abi.encode(0), false); // Check that addPosition sets position data. cellar.addPosition(4, wethPosition, abi.encode(0), false); @@ -506,33 +500,39 @@ contract CellarTest is Test { assertEq(adaptorData, abi.encode((WETH)), "Adaptor data should be abi encoded WETH."); assertEq(configurationData, abi.encode(0), "Configuration data should be abi encoded ZERO."); - // Try setting the holding index to a position that does not use the holding asset. - vm.expectRevert( - bytes(abi.encodeWithSelector(Cellar.Cellar__AssetMismatch.selector, address(USDC), address(WETH))) - ); - cellar.setHoldingIndex(4); - // Check that `swapPosition` works as expected. cellar.swapPositions(4, 2, false); assertEq(cellar.creditPositions(4), wethCLRPosition, "`positions[4]` should be wethCLR."); assertEq(cellar.creditPositions(2), wethPosition, "`positions[2]` should be WETH."); - // Make sure we can't remove the holding position index. - vm.expectRevert(bytes(abi.encodeWithSelector(Cellar.Cellar__RemovingHoldingPosition.selector))); - cellar.removePosition(0, false); - - // Make sure we can't swap an invalid position into the holding index. + // Try setting the holding position to an unused position. + uint32 invalidPositionId = 100; + vm.expectRevert(bytes(abi.encodeWithSelector(Cellar.Cellar__PositionNotUsed.selector, invalidPositionId))); + cellar.setHoldingPosition(invalidPositionId); + // Try setting holding position with a position with different asset. vm.expectRevert( - bytes(abi.encodeWithSelector(Cellar.Cellar__AssetMismatch.selector, address(WETH), address(USDC))) + bytes(abi.encodeWithSelector(Cellar.Cellar__AssetMismatch.selector, address(USDC), address(WETH))) ); - cellar.swapPositions(0, 2, false); + cellar.setHoldingPosition(wethPosition); + vm.expectRevert(bytes(abi.encodeWithSelector(Cellar.Cellar__RemovingHoldingPosition.selector))); + // Try removing the holding position. + cellar.removePosition(0, false); // Work with debt positions now. + // Try setting holding position to a debt position. ERC20DebtAdaptor debtAdaptor = new ERC20DebtAdaptor(); registry.trustAdaptor(address(debtAdaptor), 0, 0); uint32 debtWethPosition = registry.trustPosition(address(debtAdaptor), abi.encode(WETH), 0, 0); uint32 debtWbtcPosition = registry.trustPosition(address(debtAdaptor), abi.encode(WBTC), 0, 0); + uint32 debtUsdcPosition = registry.trustPosition(address(debtAdaptor), abi.encode(USDC), 0, 0); + cellar.addPosition(0, debtUsdcPosition, abi.encode(0), true); + vm.expectRevert( + bytes(abi.encodeWithSelector(Cellar.Cellar__InvalidHoldingPosition.selector, debtUsdcPosition)) + ); + cellar.setHoldingPosition(debtUsdcPosition); + cellar.removePosition(0, true); + vm.expectRevert(bytes(abi.encodeWithSelector(Cellar.Cellar__DebtMismatch.selector, debtWethPosition))); cellar.addPosition(0, debtWethPosition, abi.encode(0), false); @@ -814,7 +814,7 @@ contract CellarTest is Test { debtPositions, positionConfigs, debtConfigs, - 0, + usdcPosition, strategist, type(uint128).max, type(uint128).max @@ -1081,7 +1081,7 @@ contract CellarTest is Test { debtPositions, positionConfigs, debtConfigs, - 0, + usdcPosition, strategist, type(uint128).max, type(uint128).max @@ -1150,7 +1150,7 @@ contract CellarTest is Test { debtPositions, positionConfigs, debtConfigs, - 0, + usdcPosition, strategist, type(uint128).max, type(uint128).max @@ -1172,7 +1172,7 @@ contract CellarTest is Test { debtPositions, positionConfigs, debtConfigs, - 0, + cellarBPosition, strategist, type(uint128).max, type(uint128).max @@ -1258,7 +1258,7 @@ contract CellarTest is Test { // holding position. Governance uses multicall to rebalance cellar out // of position, set a new holding position, and distrust it. - cellar.swapPositions(0, 1, false); + cellar.setHoldingPosition(usdcCLRPosition); // Rebalance into USDC. No swap is made because both positions use // USDC. @@ -1289,7 +1289,7 @@ contract CellarTest is Test { data[0] = Cellar.AdaptorCall({ adaptor: address(cellarAdaptor), callData: adaptorCalls }); cellar.callOnAdaptor(data); - cellar.swapPositions(0, 1, false); + cellar.setHoldingPosition(usdcPosition); } function testDepeggedCellarAsset() external { @@ -1414,7 +1414,8 @@ contract CellarTest is Test { uint32 maliciousPosition = registry.trustPosition(address(cellarAdaptor), abi.encode(maliciousCellar), 0, 0); cellar.addPosition(5, maliciousPosition, abi.encode(0), false); - cellar.swapPositions(0, 5, false); + + cellar.setHoldingPosition(maliciousPosition); uint256 assets = 10000e6; deal(address(USDC), address(this), assets); @@ -1491,7 +1492,7 @@ contract CellarTest is Test { USDC, "Asset Management Cellar LP Token", "assetmanagement-CLR", - abi.encode(positions, debtPositions, positionConfigs, debtConfigs, 0, strategist) + abi.encode(positions, debtPositions, positionConfigs, debtConfigs, usdcPosition, strategist) ); // Set up worst case scenario where diff --git a/test/CellarRouter.t.sol b/test/CellarRouter.t.sol index 9cd8ae6b..78656ebc 100644 --- a/test/CellarRouter.t.sol +++ b/test/CellarRouter.t.sol @@ -122,7 +122,7 @@ contract CellarRouterTest is Test { USDC, "Multiposition Cellar LP Token", "multiposition-CLR", - abi.encode(positions, debtPositions, positionConfigs, debtConfigs, 0, address(0)) + abi.encode(positions, debtPositions, positionConfigs, debtConfigs, usdcPosition, address(0)) ); vm.label(address(cellar), "cellar"); @@ -158,7 +158,7 @@ contract CellarRouterTest is Test { WETH, "Multiposition Cellar LP Token", "multiposition-CLR", - abi.encode(positions, debtPositions, positionConfigs, debtConfigs, 0, address(0)) + abi.encode(positions, debtPositions, positionConfigs, debtConfigs, wethPosition, address(0)) ); // Generate permit sig diff --git a/test/UltimateStableCoinCellar.t.sol b/test/UltimateStableCoinCellar.t.sol index 1c462257..91a08bd8 100644 --- a/test/UltimateStableCoinCellar.t.sol +++ b/test/UltimateStableCoinCellar.t.sol @@ -233,7 +233,7 @@ contract UltimateStableCoinCellarTest is Test { debtPositions, positionConfigs, debtConfigs, - 0, + usdcPosition, strategist, type(uint128).max, type(uint128).max @@ -323,7 +323,7 @@ contract UltimateStableCoinCellarTest is Test { function testUltimateStableCoinCellar() external { // Start by managing credit positions to reflect the following. // 0) Vesting USDC - // 1) Compound USDC (holding posiiton) + // 1) Compound USDC (holding position) // 2) Aave USDC // 3) Uniswap V3 DAI/USDC LP // 4) Uniswap V3 USDC/USDT LP @@ -333,7 +333,7 @@ contract UltimateStableCoinCellarTest is Test { // Swap cUSDC and DAI position. cellar.swapPositions(1, 8, false); // Change holding position to index 1 - cellar.setHoldingIndex(1); + cellar.setHoldingPosition(cUSDCPosition); // Swap USDC and vesting USDC positions. cellar.swapPositions(0, 11, false); // Swap USDT position and aUSDC. diff --git a/test/testAdaptors/Aave.t.sol b/test/testAdaptors/Aave.t.sol index ee2d9deb..1ed022f7 100644 --- a/test/testAdaptors/Aave.t.sol +++ b/test/testAdaptors/Aave.t.sol @@ -119,7 +119,7 @@ contract CellarAaveTest is Test { debtPositions, positionConfigs, debtConfigs, - 0, + aUSDCPosition, address(0), type(uint128).max, type(uint128).max @@ -323,6 +323,7 @@ contract CellarAaveTest is Test { // First adjust cellar to work primarily with WETH. // Make vanilla USDC the holding position. cellar.swapPositions(0, 1, false); + cellar.setHoldingPosition(usdcPosition); // Add WETH, aWETH, and dWETH as trusted positions to the registry. uint32 wethPosition = registry.trustPosition(address(erc20Adaptor), abi.encode(WETH), 0, 0); diff --git a/test/testAdaptors/CellarAssetManager.t.sol b/test/testAdaptors/CellarAssetManager.t.sol index a48823a1..527b59cf 100644 --- a/test/testAdaptors/CellarAssetManager.t.sol +++ b/test/testAdaptors/CellarAssetManager.t.sol @@ -142,7 +142,7 @@ contract CellarAssetManagerTest is Test { USDC, "Multiposition Cellar LP Token", "multiposition-CLR", - abi.encode(positions, debtPositions, positionConfigs, debtConfigs, 0, strategist) + abi.encode(positions, debtPositions, positionConfigs, debtConfigs, usdcPosition, strategist) ); vm.label(address(cellar), "cellar"); vm.label(strategist, "strategist"); @@ -424,7 +424,7 @@ contract CellarAssetManagerTest is Test { USDC, "Multiposition Cellar LP Token", "multiposition-CLR", - abi.encode(positions, debtPositions, positionConfigs, debtConfigs, 0, strategist) + abi.encode(positions, debtPositions, positionConfigs, debtConfigs, usdcPosition, strategist) ); stdstore.target(address(badCellar)).sig(badCellar.shareLockPeriod.selector).checked_write(uint256(0)); badCellar.setupAdaptor(address(cellarAdaptor)); @@ -542,7 +542,7 @@ contract CellarAssetManagerTest is Test { USDC, "Multiposition Cellar LP Token", "multiposition-CLR", - abi.encode(positions, debtPositions, positionConfigs, debtConfigs, 0, strategist) + abi.encode(positions, debtPositions, positionConfigs, debtConfigs, usdcPosition, strategist) ); } @@ -955,7 +955,7 @@ contract CellarAssetManagerTest is Test { USDC, "Asset Management Cellar LP Token", "assetmanagement-CLR", - abi.encode(positions, debtPositions, positionConfigs, debtConfigs, 0, strategist) + abi.encode(positions, debtPositions, positionConfigs, debtConfigs, usdcPosition, strategist) ); } @@ -1425,7 +1425,7 @@ contract CellarAssetManagerTest is Test { WETH, "Asset Management Cellar LP Token", "assetmanagement-CLR", - abi.encode(positions, debtPositions, positionConfigs, debtConfigs, 0, strategist) + abi.encode(positions, debtPositions, positionConfigs, debtConfigs, wethPosition, strategist) ); } @@ -1737,7 +1737,7 @@ contract CellarAssetManagerTest is Test { WETH, "Asset Management Cellar LP Token", "assetmanagement-CLR", - abi.encode(positions, debtPositions, positionConfigs, debtConfigs, 0, strategist) + abi.encode(positions, debtPositions, positionConfigs, debtConfigs, wethPosition, strategist) ); stdstore diff --git a/test/testAdaptors/Compound.t.sol b/test/testAdaptors/Compound.t.sol index 26dcdd62..c5768140 100644 --- a/test/testAdaptors/Compound.t.sol +++ b/test/testAdaptors/Compound.t.sol @@ -110,7 +110,7 @@ contract CellarCompoundTest is Test { DAI, "Compound Lending Cellar", "COMP-CLR", - abi.encode(positions, debtPositions, positionConfigs, debtConfigs, 0, address(0)) + abi.encode(positions, debtPositions, positionConfigs, debtConfigs, cDAIPosition, address(0)) ); cellar.setupAdaptor(address(cTokenAdaptor)); cellar.setupAdaptor(address(vestingAdaptor)); @@ -229,7 +229,7 @@ contract CellarCompoundTest is Test { function testMaliciousStrategistMovingFundsIntoUntrackedCompoundPosition() external { // Remove cDAI as a position from Cellar. - cellar.setHoldingIndex(1); + cellar.setHoldingPosition(daiPosition); cellar.removePosition(0, false); // Add DAI to the Cellar. diff --git a/test/testAdaptors/UniswapV3.t.sol b/test/testAdaptors/UniswapV3.t.sol index 4665bb04..54faded1 100644 --- a/test/testAdaptors/UniswapV3.t.sol +++ b/test/testAdaptors/UniswapV3.t.sol @@ -141,7 +141,7 @@ contract UniswapV3AdaptorTest is Test { USDC, "Multiposition Cellar LP Token", "multiposition-CLR", - abi.encode(positions, debtPositions, positionConfigs, debtConfigs, 0, strategist) + abi.encode(positions, debtPositions, positionConfigs, debtConfigs, usdcPosition, strategist) ); vm.label(address(cellar), "cellar"); vm.label(strategist, "strategist"); diff --git a/test/testAdaptors/Vesting.t.sol b/test/testAdaptors/Vesting.t.sol index da0e050e..0011ec77 100644 --- a/test/testAdaptors/Vesting.t.sol +++ b/test/testAdaptors/Vesting.t.sol @@ -83,7 +83,7 @@ contract CellarVestingTest is Test { USDC, "Multiposition Cellar LP Token", "multiposition-CLR", - abi.encode(positions, debtPositions, positionConfigs, debtConfigs, 0, strategist) + abi.encode(positions, debtPositions, positionConfigs, debtConfigs, usdcPosition, strategist) ); vm.label(address(cellar), "cellar"); @@ -108,7 +108,7 @@ contract CellarVestingTest is Test { function testCannotTakeUserDeposits() external { // Make the vesting adaptor the first position - cellar.swapPositions(0, 1, false); + cellar.setHoldingPosition(vestingPosition); // Set up user2 with funds and have them attempt to deposit deal(address(USDC), user2, totalDeposit); @@ -122,7 +122,7 @@ contract CellarVestingTest is Test { vm.stopPrank(); // Fix positions - cellar.swapPositions(0, 1, false); + cellar.setHoldingPosition(usdcPosition); } function testDepositToVesting() external {