Skip to content

Commit

Permalink
Fix H-3
Browse files Browse the repository at this point in the history 
by converting holding index into a holding position id
  • Loading branch information
@crispymangoes
crispymangoes committed Dec 19, 2022
1 parent d4e40c7 commit 601040e
Show file tree
Hide file tree
Showing 10 changed files with 85 additions and 93 deletions.
64 changes: 31 additions & 33 deletions src/base/Cellar.sol
View file
Expand Up @@ -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.
Expand All @@ -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
Expand All @@ -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.
*/
Expand Down Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
Expand All @@ -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);
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
Expand Down
20 changes: 6 additions & 14 deletions src/base/CellarInitializable.sol
View file
Expand Up @@ -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)
)
{}

Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand Down
57 changes: 29 additions & 28 deletions test/Cellar.t.sol
View file
Expand Up @@ -134,7 +134,7 @@ contract CellarTest is Test {
debtPositions,
positionConfigs,
debtConfigs,
0,
usdcPosition,
strategist,
type(uint128).max,
type(uint128).max
Expand Down Expand Up @@ -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);
Expand All @@ -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);

Expand Down Expand Up @@ -814,7 +814,7 @@ contract CellarTest is Test {
debtPositions,
positionConfigs,
debtConfigs,
0,
usdcPosition,
strategist,
type(uint128).max,
type(uint128).max
Expand Down Expand Up @@ -1081,7 +1081,7 @@ contract CellarTest is Test {
debtPositions,
positionConfigs,
debtConfigs,
0,
usdcPosition,
strategist,
type(uint128).max,
type(uint128).max
Expand Down Expand Up @@ -1150,7 +1150,7 @@ contract CellarTest is Test {
debtPositions,
positionConfigs,
debtConfigs,
0,
usdcPosition,
strategist,
type(uint128).max,
type(uint128).max
Expand All @@ -1172,7 +1172,7 @@ contract CellarTest is Test {
debtPositions,
positionConfigs,
debtConfigs,
0,
cellarBPosition,
strategist,
type(uint128).max,
type(uint128).max
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions test/CellarRouter.t.sol
View file
Expand Up @@ -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");

Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 601040e

Please sign in to comment.