Sommelier A-3

A Cellar rebalance() operation performs swaps on Uniswap liquidity pools, which incur slippage in relation to the size of the swap and liquidity of the pool. High slippage may result in Cellar totalAssets being substantially reduced after swap. A malicious strategist may be able to leverage these conditions to mint more shares per asset value than otherwise possible by orchestrating calls to rebalance() intermingled with deposit() and withdraw().

Example - Sandwich Attack

Strategist selects a liquidity pool from a pair of Cellar asset positions which has the lowest pool liquidity at present moment. Consider it's an X-Y Pool.

1) Rebalance
  - Swap all Cellar assets into one particular asset X
  - Swap all X into Y
  => totalAssets goes down

2) Deposit
  - mint()
  => shares are minted as per reduced totalAssets()

3) Rebalance
  - Swap all Y into X
  => totalAssets are brought back to the initial level.

4) Withdraw
  - withdraw()
  => shares corresponds to more assets than when minted

This issue is more severe in the case of UniswapV2 pools since it deploys all liquidity across the whole xyk curve. With UniswapV3, the liquidity is concentrated in the normal range, making it less attractive.

An MEV attack vector is possible in cases where a rebalance will increase totalAssets post-rebalance: Anyone can see a rebalance transaction in mempool and sandwich it between deposit and withdrawal to extract value from the cellar.

Remediations to Consider

Consider delaying reward distribution to resolve this.

A delay in Chainlink price updates can undervalue cellar positions, which creates arbitrage opportunities that can be exploited via sandwich attack for guaranteed profit. This can occur because Cellar.sol uses Chainlink to value positions in terms of the Cellar holding asset when pricing share value during deposit and withdraw:

Valuation of positions = sum(holdingPosition-to-position-exchange-rate * position), over all Cellar positions

When an arbitrager observes a price increase transaction queued in the mempool, they can profit by sandwiching the price update with a Cellar deposit and withdraw. e.g. to get cheap WETH and convert at a higher price post-oracle price update.

Example Sandwich Attack

Initial Conditions:

• Chainlink: WETH-USDC = 100
• Cellar positions: [1MM WETH, 0 USDC (holding position)]
• Cellar share supply: 100MM (@ 1 share/USDC)
• Cellar withdrawType: ORDERLY

Attack Block Transactions:

• [mempool: Chainlink: update WETH-USDC = 101]
• Arbitrager: Deposits 100MM USDC
  • Receives 100MM shares (50% of new total supply)
  • Cellar position: [1MM WETH, 100MM USDC]
  • Cellar total shares: 200MM
• Chainlink: update WETH-USDC = 101
• Arbitrager: Redeems 50% shares: receives 100.5MM USDC worth of WETH, making 0.5MM USDC worth of WETH risk-free-profit
  • Total assets: (1MM WETH @ WETH-USDC 101) + 100MM USDC = 201MM USDC
  • Redemption: 201MM USDC * 50% = 100.5MM USDC

In the scenario above the cellar acts a market maker on Cellar holdingPositon-position pairs: buying holdingPosition (USDC) and selling position (WETH).

The size is limited by the balance of the position and the buy offer price is set by chainlink oracles. And how much to withdraw from each position depends on withdrawType:

• ORDERLY: the buy offer is buying holdingPosition and selling first (nonholding) position, when first position is depleted then second position and so on.
• PROPORTIONAL is buying holdingPosition and selling a fixed percentage of all positions.

Any deviation between a Chainlink rate and an exchange attracts arbitrager to trade against the Cellar (arbitrager sells holdingPosition, buys position).

Remediations to Consider

Consider requiring the shares to have cliff-vesting. It means the depositor only gets the shares e.g. 10 blocks after depositing.

• An arbitrager will be exposed to the volatility for the positions for 10 blocks. This increases the amount of uncertainty what oracle holdingPosition-position rate will be, thereby increasing uncertainty in arbitrager's expected profit.
• The cost is making deposit and withdraw not executable in series in the same block for users. Consider finding breaking usecases. It's unlikely there is any as cellars are for retail users that would typically prefer to hold cellar shares for more than 1 block.

A malicious third party ERC4626 vault owner can drain all assets of Cellars which hold their vault as a position. This attack can be realized if the vault maxWithdraw() logic returns 0 when paused: Cellar totalAssets() will exclude the position balance, causing mint() to award an increased portion of assets during this time.

See _balanceOf() on Cellar.sol line 1475:

    function _balanceOf(address position) internal view returns (uint256) {
        PositionType positionType = getPositionType[position];

        if (positionType == PositionType.ERC4626 || positionType == PositionType.Cellar) {
1475:     return ERC4626(position).maxWithdraw(address(this));
        } else {
            return ERC20(position).balanceOf(address(this));
        }
    }

The malicious owner can - via Flash loan - execute a series of mint() and redeem()/withdraw() operations made very favorable by strategically pausing/unpausing their vault, ultimately draining the Cellar of all assets. All of the attack functions are non-privileged. The complexity of executing this vulnerability is variable based Cellar positions, their order, balance ratios, and withdraw type.

Note that a legitimate, ERC4626-conformant vault can be used maliciously by its owner for this purpose: temporarily returning 0 from ERC4626 maxWithdraw() is an allowed use-case specifically called out within EIP-4626:

MUST factor in both global and user-specific limits, like if withdrawals are entirely disabled (even temporarily) it MUST return 0.

Note that this attack vector is not immediately available to malicious Cellar position owners: shutdown is intended to be executable only by governance which makes the Flash Loan attack non-viable.

Related Issues

The root of this issue is maxWithdraw() returning a value that may differ from the full balance held. Generalizing on this, there are a number of other potential issues that can occur, which are not necessarily maliciously motivated:

1) Any Cellar may mint too many shares when it holds an ERC4626/Cellar position which under-reports maxWithdraw().

This is a more benign view of the above attack vector. Such under-reporting could be due to non-malicious pausing, staking & locking, or any arbitrary logic which the Vault position owner chooses to implement. Any of these are legitimate and allowed, and arguably the very reason that the maxWithdraw() method exists. This scenario grants immediate value to the new minters, which can be extracted when maxWithdraw() aligns more closely with balance held (e.g. becomes unpaused; locking period ends, etc).

Consider updating Cellar.sol such that maxWithdraw() cannot be overridden in inheriting Cellar logic. This eliminates Cellar positions from this risk category.

2) redeem() and withdraw() do not revert when all assets cannot be withdrawn, locking assets

Calls to redeem() or withdraw() can cause a user to receive less assets than expected, while still burning shares corresponding to the full request. This is most likely to occur when all of the following conditions are true:

• An ERC4626 / Cellar position with a non-zero asset balance returns 0 for maxWithdraw(). This may occur if the ERC4626 has logic to return 0 when it is paused/shutdown.
• withdrawType == PROPORTIONAL OR the amount of assets to be withdrawn exceeds the amount available from all other positions

The EIP4626 spec which states for both withdraw() and redeem():

MUST revert if all of assets / shares cannot be withdrawn (due to withdrawal limit being reached, slippage, the owner not having enough shares, etc).

This shortchanging of value stems from the _balanceOf() logic noted above. Subsequently, neither _withdrawInOrder() nor _withdrawInProportion() validate that the entire assets value has been fully withdrawn after cycling through all the positions.

3) Cellars with affected positions may allow minting beyond deposit and liquidity limits

This occurs because totalAssets() and _convertToAssets() will both return smaller than expected values within maxDeposit(), which impacts subsequent calculations to yield a higher than expected values for leftUntilDepositLimit and leftUntilLiquidityLimit See Cellar.sol lines 1258-1265 of maxDeposit():

uint256 _totalAssets = totalAssets();
uint256 ownedAssets = _convertToAssets(balanceOf[receiver], _totalAssets);

uint256 leftUntilDepositLimit = asssetDepositLimit.subMinZero(ownedAssets);
uint256 leftUntilLiquidityLimit = asssetLiquidityLimit.subMinZero(_totalAssets);

// Only return the more relevant of the two.
assets = Math.min(leftUntilDepositLimit, leftUntilLiquidityLimit);

Remediations to Consider

Presently _balanceOf() returns “withdrawable” data but is used for both “withdrawable” and “holdings” concerns. Consider distinguishing “withdrawable assets” from “held assets”, and using the former within withdraw-related logic. To this end note that Cellar.sol line 1475 could be updated to:

return ERC4626(position).convertToAssets(ERC4626(position).balanceOf(address(this)));

OR

return ERC4626(position).previewRedeem(ERC4626(position).balanceOf(address(this)));

If selecting one of these options, consider that previewRedeem() would be in conformance with EIP-4626 ( totalAssets() must be inclusive of fees) but introduce potential slippage to a wider set of Cellar activity, whereas convertToAssets() would break EIP-4626 conformance but reduce the scope of slippage concerns to Cellar withdraw activity. Consider documenting your choice and its related considerations.

Also consider extra handling within redeem() and withdraw() in cases which withdraw-able values do not match balance values. For example:

• There is a shortfall of withdraw-able assets in one or more positions. Consider either reverting with an insightful error, or completing the withdraw from other positions.
• There are not enough withdraw-able assets in all available positions. Consider always reverting with an insightful error.

If a Cellar has a trusted position for an asset that is unsupported by the PriceRouter, the Cellar will become in-operable and all funds locked. Additionally, a new Cellar can be created with an unsupported asset as a trusted position, rendering the Cellar in-operable from the start.

This can occur because the position trust logic within Cellar.sol does not validate the asset is supported in PriceRouter.sol.

Over time — as the number of Cellars increase and PriceRouter integrations are added — it will become increasingly difficult to manage via off-chain mechanisms that trusted position proposals correspond to supported assets.

Remediations to Consider

Consider adding logic to inspect PriceRouter.sol asset support prior to trusting positions in Cellar.sol:

• constructor() line 715
• trustPosition() lines 283-291

If an on-chain asset deprecation notification feature similar to L-2 is implemented, consider reverting when attempting to trust a position whose asset is marked for deprecation.

If the ETH-USD Chainlink quote were to exceed allowed Chainlink range (presently 1 - 10,000) the ChainlinkPriceFeedAdapter.sol may return stale exchange rate data, allowing attackers to buy-in to Cellar positions are reduced cost.

This can occurs because _getValueInUSDAndTimestamp() and _getPriceRangeInUSD() disregard the ETH-USD quote timestamp; only returning the asset-ETH timestamp.

Consider the following scenario, in which 1 Cellar share = 1 asset:

Example baseline conditions (ETH-USD ⇒ 1,000, within Chainlink range):

• asset-ETH quote: 0.01
• ETH-USD quote: 1,000
• The cost in USD of 1,000 assets is 1,000 * 0.01 * 1,000 = 10,000 USD

ETH price sees steep increase (ETH-USD ⇒ 15,000: outside of Chainlink range)

• ETH-USD quote: 10,000 (Chainlink stops recording quotes > 10,000)
• The cost of 1,000 assets:
  • Within Cellars: 1,000 * 0.01 * 10,000 = 100,000 USD
  • In actuality: 1,000 * 0.01 * 15,000 = 150,000 USD
• 1,000 assets mints 1,000 Cellar share, costing 100,000 USD

Users will be able to buy into asset positions at a discount. If the new ETH price is indeed valid Chainlink will deploy Aggregators with an updated, wider range. At that point the assets deposited at a discount within the Cellar can be redeemed with arbitrage profit:

ETH maintains high value, Chainlink updates ranges (ETH-USD ⇒ 15,000: within Chainlink range):

• ETH-USD quote: 15,000
• The cost of 1,000 assets is: 1,000 * 0.01 * 15,000 = 150,000 USD
• 1,000 shares redeems 1,000 assets, valued at 150,000 USD

In this example scenario, an attacker made 150,000 - 100,000 = 50,000 USD in arbitrage.

Remediations to Consider

Consider updating ChainlinkPriceFeedAdaptor.sol to also retrieve the ETH-USD timestamp, and return the older of the two timestamp values to PriceRouter.sol. This allows circuit-breaker logic to halt Cellar operations under these conditions.

Also consider a shorter default heartbeat value. Presently DEFAULT_HEART_BEAT = 1 days creates a window of 1 day during which this arbitrage attack is still possible.

changeDecimals() in the Math library is used to make conversion between assets with different decimals. This function divides the amount by the difference between decimal numbers if a conversion from an asset with higher decimal to an asset with a lower decimal is requested. However, it is rounding numbers down to 0 if the value is less than 1e^(fromDecimals - toDecimals).

mint() calls _previewMint() to calculate the assets that will be payed which uses changeDecimals(). Because of the issue mentioned above, changeDecimals() returns zero for small amounts which causes the mint to be executed without any assets transferred. The requested amount of shares will be minted to the receiver without any payments for the first mint. For the subsequent mints, _totalAssets in _previewMint() will be zero, causing asset payment amount to be zero as well. This bug will also block deposits and will make the cellar unusable.

The assumption on the L:840 is not correct for the first mint:

// No need to check for rounding error, previewMint rounds up.

Remediations to Consider

Consider reverting in case it returns zero similar to how it is handled in deposit().

Since changeDecimals() is a library function, be sure it works as expected for all use cases.

Consider there is a Cellar with a position in flash-loan featured ERC4626 vault.

Here attacker can take a flash loan on a given vault and then in a flashloan callback, deposit assets in a cellar. They would get more shares than actual since totalAssets for Cellar is temporarily reduced.

Remediations to Consider

Consider doing a thorough analysis of a position before allowing it in isTrusted. There shouldn't be a way for anyone to temporarily reduce the totalAssets of the underlying vault and then bring it back.

A Cellar can invest in other Cellars, which can themselves do the same in turn. Increased complexity of this "investment graph" can cause out-of-gas issues for Cellars higher in the hierarchy, yielding denial of service and locking assets in those Cellars.

Consider a Cellar X that holds a position in Cellar Y, among other other ERC20/ERC4626 positions. Calling X.totalAssets() will cycle through all of it's positions to request their balance held, which includes a call Y. This call to Y will also invoke Y.totalAssets(). In this way the entire investment graph will be traversed. Investment additions which extend the graph at lower levels can cause the higher-level Cellars to experience out-of-gas for any calls to totalAssets().

In some conditions the locked Cellar strategist may be able to rebalance out of positions to unlock their Cellar. However, extreme conditions may make this impossible. In those cases, the only means to unlock the Cellar would be to have the other Cellar lower in the hierarchy rebalance out of positions.

A Cellar depends upon Chainlink to retrieve asset values in USD, and is therefore susceptible to potential attacks from within the Chainlink network.

A chainlink price feed is:

• a proxy that relays price requests to an aggregator
• an aggregator caches the medianized rate aggregated from offchain oracles

Chainlink docs on price feed overview:

"Using proxies enables the underlying aggregator to be upgraded without any service interruption to consuming contracts."

However, there are multiple attack vectors:

1. Proxies are owned: The 4-9 multisig owner controlled by Chainlink can potentially upgrade the aggregator the proxy points to to a malicious one.
   1. A malicious aggregator can then report a rate close to the typical rate so the attack doesn't trip dapps' circuit breakers and still over/undervalue an asset.
   2. The protection from min and max price checks will have room for malicious price manipulation within volitile assets such as ETH-USD (+/- 20% in one day).
2. Offchain oracles can collude and report a malicious rate, without needing to change aggregator.

Consider using a secondary oracle such as uniswap or tellor, then require the 2 rates be in sync (e.g. < 2% difference).

While totalShares = 0 an attacker can force feed tokens into one of the Cellar positions to cause withdraw(), redeem(), deposit(), mint() and sendFees() to revert, creating denial-of-service conditions.

This occurs because totalAssets() and hence feesInShares will be non-zero, causing _convertToFees() to revert due to underflow when calculating denominator:

    function _convertToFees(uint256 feesInShares) internal view returns (uint256 fees) {
        uint256 totalShares = totalSupply;
        uint256 denominator = totalShares - feesInShares;
        ...

The DoS can be recovered from by executing a rebalance operation.

Remediations to Consider

Consider updating _convertToFees() logic to account for conditions of underflow.

The solmate implementation of ERC4626 has the asset variable as immutable; whereas in base/ERC4626.sol:33 asset is no longer immutable. A mutable value allows inheriting Cellar implementations to potentially change the asset, which may cause a variety of issues including the inability to deposit into a cellar or lack of update to the totalAssets() valuation.

Remediations to Consider

Consider updating asset to be immutable.

addAsset() will accept minPrice and maxPrice arguments with reversed values (min > max). Should this occur, all asset price inquiries will always revert, causing the majority of public Cellar.sol methods to always revert: deposit(), mint(), withdraw(), redeem(), sendFees(), resetHighWatermak(), totalAssets(), convertTo*(), and preview*().

This can occur because addAsset() will not perform validations on minPrice or maxPrice if they are both nonzero. See PriceRouter.sol, line 91-120:

if (minPrice == 0 || maxPrice == 0) {
    ... validation logic ...
}

getAssetConfig[asset] = AssetConfig({
    ...
    minPrice: minPrice,
    maxPrice: maxPrice,
    ...

Price inquiries funnel through _getValueInUSD() within PriceRouter.sol lines 301-305 which contains checks against the configured min/max price values:

uint256 minPrice = config.minPrice;
if (value < minPrice) revert PriceRouter__AssetBelowMinPrice(address(asset), value, minPrice);

uint256 maxPrice = config.maxPrice;
if (value > maxPrice) revert PriceRouter__AssetAboveMaxPrice(address(asset), value, maxPrice);

If minPrice > maxPrice, any value amount will trigger one of these reverts.

Affected Cellars can be unlocked by calling addAsset() with corrected values for minPrice and maxPrice.

Remediations to Consider

Consider updating addAsset() to also validate minPrice and maxPrice when they are both nonzero.

This is the alternate side of M-2. Whereas M-2 is concerned with a Cellar adding a position for an asset unsupported by the PriceRouter, this issue is concerned with the PriceRouter removing an asset which Cellars already have a position in.

Specifically, when removeAsset() is called for an asset which Cellars have an existing position in, all assets in those Cellars will become locked. This occurs because PriceRouter.sol _getValueInUSD() will now revert on line 294:

if (!config.isSupported) revert PriceRouter__UnsupportedAsset(address(asset));

_getValueInUSD() is at the root of many Cellar.sol methods. Most notable for an invested Cellar user are redeem() and withdraw(). These methods will revert, locking all invested funds. Affected Cellars can be unlocked by re-adding the asset.

Remediations to Consider

Consider an on-chain and/or off-chain notification scheme for PriceRouter assets which will become deprecated, allowing Cellar Strategists appropriate time to rebalance out of the asset.

There is no mechanism enforcing the first element of the positions array to be holdingPosition . This will cause _withdrawInOrder() to empty positions with an index smaller than holdingPosition and may require frequent rebalances to keep the strategy as intended.

Remediations to Consider

Consider requiring index of holdingPosition to be zero in constructor and setHoldingPosition()

If a wrong PositionType is assigned for a position in constructor() or trustPosition(), withdraws will be blocked. This situation can be remedied by calling trustPosition() with correct type but since it is a governance only function it will take time to vote and execute.

Remediations to Consider

• Detecting the types of positions by making a view call to the target address instead of manually passing PositionTypes.
• If manual type registration is preferred, it is possible to check if the passed type is correct using the technique mentioned above.

Cellar.sol implements many important baseline requirements which appear to be critical to normalized good behavior within the Sommelier network. When adding a new Cellar to the Sommelier network, consider enforcing these requirements via appropriate mechanisms:

• Cellars must inherit from Cellar.sol
• Cellars should not be upgradeable
• Cellars must not lock withdrawals (referencing M-1)

To further ensure that only truly trusted Cellars are operated within Sommelier, consider enforcing that Cellar positions must have also been explicitly approved on the network (referencing I-1).

For additional on-chain trust, consider implementing a central repository contract to act as a source of truth for all Ethereum-wide trusted Cellar addresses. Such a repository could be managed via appropriate approval/control mechanisms. Consider integrating this registry into the “trusted position” Cellar.sol logic so un-approved Cellars cannot be trusted/added as positions.

Consider enforcing via appropriate mechanisms:

• ERC4626 positions should not be upgradeable
• ERC4626 positions must not lock withdrawals (referencing M-1)
• ERC4626 positions must not have Flash Loan capabilities (referencing M-5)

Consider enforcing via appropriate mechanisms:

• ERC20 should not have a fee on transfer
• ERC20 should not be a rebasing token.
• ERC20 must not be a double-sided token (Synthetix’s Tokens)

The following functions take performance fee shares when yield has accrued: deposit(), mint(), withdraw(), redeem(), sendFees().

Performance fees are taken on the yield of the entire cellar (highwater mark in relation to totalAssets), instead of only on realized yield (highwatermark in relation to individual user investment).

Consider the following example:

• Block T: Current cellar asset value is 100MM USDC, 100MM USDC is the high watermark.
• Block T+1: Current cellar asset value increased to 300MM USDC, a LP withdraws a small amount: 1 USDC. The result is Cellar mints 7% performance fee shares because 0.1 * 200MM / 300MM ~= 7% (performance fee rate * yield / total asset).
• Block T+2: Current cellar asset value returns to 100MM USDC, each LP owns 7% less asset due to one LP decides to withdraw.

Per the maxWithdraw ERC4626 spec:

MUST return the maximum amount of assets that could be transferred from owner through withdraw and not cause a revert

However, the present maxWithdraw implementation does not account for the diluting effect of performance shares, therefore over-estimate withdrawable asset, and causing withdraw(maxWithdraw()) to revert.

From Sommelier docs:

"In general, when designing cellar contracts, one must always minimize trust."

Current implementation allows Governance to maintain an allowlist of assets Strategist can rebalance to/from. This prevents Strategist from adding low liquidity asset and subsequently manipulate the price to extract other Cellar assets. However, Governance still has the same privilege.

While it may make sense from development perspective for Governance to maintain positions allowlist, there is no way to remove that privilege after Cellar contracts is deployed.

Once Governance is confident in the assets in the allowlist, it can improve trust for LPs by ossifying the allowlist.

Consider to use a role to maintain the list and revoke the role when Governance deems appropriate. And consider using Open Zeppelin's AccessControl.sol.

A successful cellar's strategy, in particular its rebalance operations, can be mimicked perfectly regardless how private the offchain computation is. Because all rebalances and position balances are public. There can be an incentive for LPs to start a clone Cellar to save performance and platform fees. One can set up an off-chain event subscriber and onchain contract to mimic the cellar's rebalance i.e. the trades, as well as copy the cellar's strategy to offer lower fees, different incentives, trust policy to attract LPs etc. These cloned trades can even frontrun the cellar's original trades.

If the priceRouter address is changed in Registry.sol, a Cellar may have a trusted position for an asset that is unsupported by the new PriceRouter. This could cause a Cellar to become in-operable and its funds locked.

_previewWithdraw() is susceptible to similar siphoning of small values as found in M-4, under the following conditions:

• totalShares == 0 and totalAssets() != 0. These conditions require that:
  • an asset was directly transferred to the Cellar (not via deposit()/mint())
  • deposit()/mint() have not been called yet
• The asset has decimals > 18

An asset can be successfully added with minPrice and maxPrice outside the valid range that Chainlink reports. This may result in the PriceRouter's price buffering logic no longer taking effect for the asset. This occurs because the addAsset() method only performs Chainlink validations on minPrice and maxPrice if at least one of them is zero. If they are both non-zero, Chainlink price range is not referenced.

Consider the following:

• Chainlink’s asset range: 100 - 10,000
• PriceRouter correctly buffered asset range: 110 - 9,000
• PriceRouter incorrectly configured asset range: 1 - 100,000

In the "incorrectly configured" scenario, the PriceRouter will no longer revert for Chainlink values which fall within the 10% buffered zones: 100-110 or 9,000-10,000. As a result, Cellar activity can proceed while the asset value is within the % buffered zones, which would have otherwise reverted. Asset values within this buffered zone are still valid Chainlink price quotes so there is no hard impacts per-se, except a misalignment with expected business logic.

Remediations to Consider

Consider updating logic to also validate addAsset() minPrice and maxPrice arguments against Chainlink if both are nonzero.

The @notice value on Cellar.sol line 101 appears to be incorrect: suspect copy-paste from prior error comments.

The ShutdownChanged event can be emitted when no change has occurred, which may cause confusion to users. Consider reverting if liftShutdown() is called when the Cellar is not shutdown.

Variables within Cellar.sol _takePerformanceFees() are dealing strictly with performance fees, but are named as platform fees. See lines 1340-1347, in particular feeInAssets and platformFeesInShares:

uint256 feeInAssets = _previewPerformanceFees(_totalAssets);
if (feeInAssets > 0) {
    uint256 platformFeesInShares = _convertToFees(_convertToShares(feeInAssets, _totalAssets));
    if (platformFeesInShares > 0) {
        feeData.highWatermark = _totalAssets;
        _mint(address(this), platformFeesInShares);
    }
}

Consider renaming these variables performanceFeeInAssets and performanceFeeInShares respectively.

PriceRouter.sol:L130 removeAsset() does not check if that asset actually exists.

SwapRouter.sol:L9 Multicall is imported twice

Two different versions of SafeCast are used in the project:

• Cellar.sol:L8
• ChainlinkPriceFeedAdaptor.sol:L10

Cellar.sol:L889 & L936 withdraw() and redeem() don’t check if the allowed amount is bigger than shares which is causing underflow errors. Consider checking if allowed > shares and returning a meaningful error.

ERC4626 specification states the following functions must not revert: totalAssets(), maxDeposit(), maxWithdraw(), maxMint(), maxReeem(). However, totalAssets() -- which the other named functions depend upon -- can revert due to price oracle logic.

Consider documenting this non-conformance for integrators.

Within add(), len - 1 will underflow when array.length = 0:

function add(
        address[] storage array,
        uint256 index,
        address value
    ) internal {
        uint256 len = array.length;
        array.push(array[len - 1]); 
        ...

This is not causing issues within the present Cellar.sol implementation: the Cellar constructor() initializes AddressArray values without using add(); and subsequent conditions prevent the array from becoming empty again.

Remediations to Consider

Consider correcting this logic to account for a 0-length array.

From FAQs on the landing page

Can strategy providers add new positions to the Cellar?

Although strategy providers can change the strategy logic (algorithm which sends position signals to the Cellar), they must submit position additions to Sommelier Governance. This adds a layer of safety for users who have allocated funds to Cellars. They can rest easy knowing that their assets can only be in the prescribed positions that each Cellar manages.

Can strategy providers add new DeFi protocols?

No, for the same strategy provider to add new DeFi protocols new Cellar should be created.

The 2 Q&As are not compatible. Consider to update the documentation for the latter Q&A, to reflect Strategist can add a new position, provided Governance approves the addition.