Reach out for an audit or to learn more about Macro
or Message on Telegram

Sommelier A-16a

Security Audit

January 22, 2024

Version 1.0.0

Presented by 0xMacro

Table of Contents

Introduction

This document includes the results of the security audit for Sommelier's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from January 15, 2024 to January 22, 2023.

The purpose of this audit is to review the source code of certain Sommelier Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.

Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.

Overall Assessment

The following is an aggregation of issues found by the Macro Audit team:

Severity Count Acknowledged Won't Do Addressed
High 1 - - 1
Medium 1 1 - -
Low 2 - - 2
Code Quality 4 - - 4
Informational 2 2 - -
Gas Optimization 2 - - 2

Sommelier was quick to respond to these issues.

Specification

Our understanding of the specification was based on the following sources:

Trust Model, Assumptions, and Accepted Risks (TMAAR)

Trusted entities:

The goal of the system is to to have checks and balances for each permissioned action, where if any one permissioned entity acts malicious, the others can remedy the situation, requiring multiple points failure before it can negatively impact users.

Assumptions:

Accepted Risks:

Source Code

The following source code was reviewed during the audit:

Specifically, we audited the following contracts within this repository.

Contract SHA256
src/base/Cellar.sol

6967e427be04533ee4456ac796a91b408ede61fc63cfddf2c1f2226693036fc9

src/base/permutations/CellarWithMultiAssetDeposit.sol

e482ad2938ad4e283470f9f83d2b9f9ccb0a4dd474d4c8024dae34a86aef3d78

src/base/permutations/CellarWithNativeSupport.sol

e88a29520db83cd1b820ee334bac8ab17e6c0ccead850563a02bbe05b370717b

src/modules/adaptors/Morpho/MorphoBlue/MorphoBlueCollateralAdaptor.sol

bc48a55e176a0e51832e0ba7805c95b3e2b3380c9bfa950897b7fd7c3d2f077f

src/modules/adaptors/Morpho/MorphoBlue/MorphoBlueDebtAdaptor.sol

cb2373e057d9b8b356f4d23baab1c381a96dd1ca968f91ace4da967547804501

src/modules/adaptors/Morpho/MorphoBlue/MorphoBlueHelperLogic.sol

c7a5fb7654e969ff054bba7cdfe49e69d1905cdfb222395c2bbb822ad23ba0e6

src/modules/adaptors/Morpho/MorphoBlue/MorphoBlueSupplyAdaptor.sol

cb67f9da166123357c54709477599460532b8a228de8a55b3add6135e5e2c290

src/base/permutations/advanced/CellarWithOracleWithBalancerFlashLoansWithMultiAssetDeposit.sol

0957274d973e48c9046f817b05c4405ae72884c1c8f8834b43a3b1764282a6f4

src/base/permutations/advanced/CellarWithOracleWithBalancerFlashLoansWithMultiAssetDepositWithNativeSupport.sol

60465233b95b4be8fb0af375de79ed42d1e126b57a67fe3d92aceb829919abaa

Note: This document contains an audit solely of the Solidity contracts listed above. Specifically, the audit pertains only to the contracts themselves, and does not pertain to any other programs or scripts, including deployment scripts.

Issue Descriptions and Recommendations

Click on an issue to jump to it, or scroll down to see them all.

Security Level Reference

We quantify issues in three parts:

  1. The high/medium/low/spec-breaking impact of the issue:
    • How bad things can get (for a vulnerability)
    • The significance of an improvement (for a code quality issue)
    • The amount of gas saved (for a gas optimization)
  2. The high/medium/low likelihood of the issue:
    • How likely is the issue to occur (for a vulnerability)
  3. The overall critical/high/medium/low severity of the issue.

This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:

Severity Description
(C-x)
Critical

We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost.

(H-x)
High

We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec.

(M-x)
Medium

We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner.

(L-x)
Low

The risk is small, unlikely, or may not relevant to the project in a meaningful way.

Whether or not the project wants to develop a fix is up to the goals and needs of the project.

(Q-x)
Code Quality

The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design.

(I-x)
Informational

Warnings and things to keep in mind when operating the protocol. No immediate action required.

(G-x)
Gas Optimizations

The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it.

Issue Details

H-1

Strategist can drain cellar using malicious Morpho Blue market

Topic
Input Validation
Status
Impact
Critical
Likelihood
Medium

In MorphoBlueSupplyAdaptor.sol’s deposit() function, the market provided is not validated to be a position in the cellar, as is done by other functions by calling _validateMBMarket(). Since the market passed into deposit() can be any market, this allows a strategist to deposit assets into a market that is untracked, which can allow assets to be lost up to the rebalance deviation on each call.

/**
 * @notice Allows user to deposit into MB markets, only if Cellar has a MBSupplyAdaptorPosition as its holding position.
 * @dev Cellar must approve Morpho Blue to spend its assets, then call deposit to lend its assets.
 * @param assets the amount of assets to lend on Morpho Blue.
 * @param adaptorData adaptor data containing the abi encoded Morpho Blue market.
 * @dev configurationData is NOT used.
 */
function deposit(uint256 assets, bytes memory adaptorData, bytes memory) public override {
    MarketParams memory market = abi.decode(adaptorData, (MarketParams));
    ERC20 loanToken = ERC20(market.loanToken);
    loanToken.safeApprove(address(morphoBlue), assets);
    _deposit(market, assets, address(this));

    // Zero out approvals if necessary.
    _revokeExternalApproval(loanToken, address(morphoBlue));
}

Reference: MorphoBlueSupplyAdaptor.sol#L69-L84

Since Morpho Blue markets can be created by anyone and they are oracle agnostic, a market can be created with any oracle used to price the collateral asset relative to the loan asset, as well as set any token as collateral.

//@audit notice there is no check on marketParams.oracle or the collateral token
function createMarket(MarketParams memory marketParams) external {
    Id id = marketParams.id();
    require(isIrmEnabled[marketParams.irm], ErrorsLib.IRM_NOT_ENABLED);
    require(isLltvEnabled[marketParams.lltv], ErrorsLib.LLTV_NOT_ENABLED);
    require(market[id].lastUpdate == 0, ErrorsLib.MARKET_ALREADY_CREATED);

    // Safe "unchecked" cast.
    market[id].lastUpdate = uint128(block.timestamp);
    idToMarketParams[id] = marketParams;

    emit EventsLib.CreateMarket(id, marketParams);

    // Call to initialize the IRM in case it is stateful.
    if (marketParams.irm != address(0)) IIrm(marketParams.irm).borrowRate(marketParams, market[id]);
}

Reference: Morpho.sol#L150-L164

If a strategist were to create a token where they own the entire supply and make a market using it as collateral with a malicious oracle contract that inaccurately prices this collateral as valuable relative to the loan asset, it could allow the strategist to borrow all the assets in the market that have been provided by the cellar using the worthless token as collateral, effectively stealing all assets sent into the market from the cellar.

Remediations to Consider

In MorphoBlueSupplyAdaptor:deposit() call _validateMBMarket() to verify the cellar has the provided market as a position, which ensures the market is trusted.

M-1

Balances don’t account for interest or debt accrued can lead to inaccurate share price

Topic
Data consistency
Status
Acknowledged
Impact
Medium
Likelihood
Low

In both MorphoBlueSupplyAdaptor.sol and MorphoBlueDebtAdaptor.sol’s balanceOf() functions the balance returned only reads from the current state, and does not consider pending interest or debt incurred since the last time the market was interacted with. Since balanceOf() is used to determine the assets held when determining the cellar’s share price, this can lead to an inaccuracies in the share price. Although the true balances after interest/debt has been accounted for may not typically deviate from the actual balance, it could make a noticable difference if there are positions in markets that are not interacted with frequently, and/or if cellars have multiple MorphoBlue market positions. These differences could open up arbitrage opportunities that could be exploited.

Remediations to Consider

Use the MorphoBalancesLib.sol and pull the expected values required that take interest/debt into consideration.

Response by Sommelier

The high-level design where these smart contracts read state versus relying on the MorphoBalancesLib.sol for expected values is a battle-tested setup in principle within production cellars and other adaptor integrations. There are a number of reasons that reading state, and thus having a bit of discrepancy (due to possible lack of accrueInterest being called to update markets) is favorable. These include:

  1. We only rely on the Morpho Blue code itself vs relying on library contracts.
  2. Gas efficiency.
  3. Since share price is not always changing, strategists are free to collect fees owed without making share price goe down.

Under normal market conditions, Morpho Blue markets will be interacted with regularly, making the reported values sufficiently close to the ideal values(that include pending interest). Also lending/supply interest rates are usually very low, so pending interest is usually not that large.

Under extreme market conditions, interest rates can be much higher which would cause pending interest values to be larger than normal, however, given that interest rates are much higher, market activity will also be much higher as lenders want to capture large interest rates, and borrowers want to repay expensive debt. All of these factors will help to keep the reported values close to the ideal values.

Finally the vast majority of vaults using Morpho Blue markets will use share price oracles. So even if we have some black swan event that causes pending interest to be much larger than anticipated, it is still a difficult arbitrage to capture, as the share price takes days to reflect a large change.

L-1

Deposit event breaks ERC4626 standard

Topic
Interoperability
Status
Impact
Low
Likelihood
Medium

Cellar.sol adjusted the Deposit event to add the specific deposit asset used for the deposit.

/**
* @notice Emitted during deposits.
*/
event Deposit(address indexed caller, address indexed owner, address depositAsset, uint256 assets, uint256 shares);

Reference: Cellar.sol#L710-L713

However, this breaks the ERC4626 standard, as it requires a specific event signature of: Deposit(address indexed sender, address indexed owner, uint256 assets, uint256 shares). Deviation from the expected event signature could disrupt indexers or front/backend applications that expect ERC4626 compliant contracts to emit.

Remediations to Consider

Add a separate event that specifies when an asset has been deposited that is not the expected cellar’s primary asset().

L-2

Adapters won't work with MorphoBlue markets that don’t have an interest rate

Topic
Use cases
Status
Impact
Medium
Likelihood
Low

The documentation on "Building adapters" states:

withdrawableFrom should never revert.

However, by using MorphoBlue (MB) adapters, withdrawableFrom reverts when MB markets don’t have an IRM address specified. In MB, markets can be created without specifying an interest rate. This seems to be a valid configuration for MB markets and is explicitly handled in the MB codebase, specifically in the _accrueInterest function.

Sommeliers MB adapters use an outdated MorphoBalancesLib.sol, which doesn't correctly handle this case and reverts on MorphoBalancesLib.sol#L46 inside the expectedMarketBalances function. As a consequence, all functions in the MorphoBalancesLib's will revert when called for MB markets with zero interest rates (IRM = 0x0).

This in particular breaks the MorphoBlueSupplyAdaptor:withdrawableFrom function, since it internally calls expectedMarketBalances.

The severity of this issue is considered low, as it seems very unlikely that strategists would want to integrate Morpho Blue markets that have zero interest rate configured.

Remediations to Consider

Replace MorphoBalancesLib.sol with the updated version, including the fix to support markets with IRM = 0x0.

Q-1

A Multi asset deposit should be a separate function

Topic
Interoperability
Status
Quality Impact
Medium

In CellarWithMultiAssetDeposit.sol and its variants, an alternative asset is deposited by calling deposit() with additional data encoded into the call that it parsed by _getDepositAssetAndAdjustedAssetsAndPosition(), if no data is present a normal deposit of the cellars asset() occurs.

function deposit(uint256 assets, address receiver) public override nonReentrant returns (uint256 shares) {
    // Use `_calculateTotalAssetsOrTotalAssetsWithdrawable` instead of totalAssets bc re-entrancy is already checked in this function.
    (uint256 _totalAssets, uint256 _totalSupply) = _getTotalAssetsAndTotalSupply(true);

    (
        ERC20 depositAsset,
        uint256 assetsConvertedToAsset,
        uint256 assetsConvertedToAssetWithFeeRemoved,
        uint32 position
    ) = _getDepositAssetAndAdjustedAssetsAndPosition(assets);

    // Perform share calculation using assetsConvertedToAssetWithFeeRemoved.
    // Check for rounding error since we round down in previewDeposit.
    // NOTE for totalAssets, we add the delta between assetsConvertedToAsset, and assetsConvertedToAssetWithFeeRemoved, so that the fee the caller pays
    // to join with the alternative asset is factored into share price calculation.
    if (
        (shares = _convertToShares(
            assetsConvertedToAssetWithFeeRemoved,
            _totalAssets + (assetsConvertedToAsset - assetsConvertedToAssetWithFeeRemoved),
            _totalSupply
        )) == 0
    ) revert Cellar__ZeroShares();

    if ((_totalSupply + shares) > shareSupplyCap) revert Cellar__ShareSupplyCapExceeded();

    // _enter into holding position but passing in actual assets.
    _enter(depositAsset, position, assets, shares, receiver);
}

Reference: CellarWithMultiAssetDeposit.sol#L138-L165

Consider separating out alternative asset deposits into a separate function to make it more clear to users and integrators how to deposit an alternative asset.

Q-2

Unnecessary external receiver check

Topic
Unnecessary call
Status
Quality Impact
Low

In MorphoBlueSupplyAdaptor’s withdrawFromMorphoBlue(), there is a external receiver check made with address(this) as the parameter:

function withdrawFromMorphoBlue(MarketParams memory _market, uint256 _assets) public {
  // Run external receiver check.
  _externalReceiverCheck(address(this));
  _validateMBMarket(_market);
  Id _id = MarketParamsLib.id(_market);
  if (_assets == type(uint256).max) {
      uint256 _shares = _userSupplyShareBalance(_id, address(this));
      _withdrawShares(_market, _shares, address(this));
  } else {
      // Withdraw assets from Morpho Blue.
      _withdraw(_market, _assets, address(this));
  }
}

Reference: MorphoBlueSupplyAdaptor.sol#L186-L198

However, the external receiver check is only relevant if the receiver is an address other than address(this):

function _externalReceiverCheck(address receiver) internal view {
    if (receiver != address(this) && Cellar(address(this)).blockExternalReceiver())
        revert BaseAdaptor__ExternalReceiverBlocked();
}

Reference: BaseAdaptor.sol#L194-L197

Remediations to Consider

Remove the external receiver check from withdrawFromMorphoBlue()

Q-3

Misleading comment

Topic
Comments
Status
Quality Impact
Low

In Cellar.sol’s addAdaptorToCatalogue() there is a comment that mentions making sure the adaptor is not paused:

function addAdaptorToCatalogue(address adaptor) external {
  _onlyOwner();
  // Make sure adaptor is not paused and is trusted.
  registry.revertIfAdaptorIsNotTrusted(adaptor);
  adaptorCatalogue[adaptor] = true;
  emit AdaptorCatalogueAltered(adaptor, true);
}

Reference: Cellar.sol#L316-L322

However, adaptors cannot be paused, only trusted/distrusted.

Consider adjusting this comment to be more accurate

Q-4

_validateMBMarket code is duplicated

Topic
Redundant code
Status
Quality Impact
Medium

Each MorphoBlue adaptor implements is own version of _validateMBMarket(). However, each is mostly the same aside from the unique adapter identifier received and the boolean defining if it is a debt position.

To reduce duplicate code, and reduce the likelihood of errors if this needs to be updated in the future, consider moving _validateMBMarket() to the MorphoBlueHelperLogic contract each inherits, and allow the identifier and is debt bool to be passed into it.

G-1

Redundant market validation when repaying debt

Topic
Redundant Code
Status
Gas Savings
Low

MorphoBlueDebtAdaptor’s repayMorphoBlueDebt calls _validateMBMarket at the beginning of the function. Subsequently, _accrueInterest is called which again validates the market parameters:

function accrueInterest(MarketParams memory _market) public {
    _validateMBMarket(_market);
    _accrueInterest(_market);
}

Remediations to Consider

Remove the _validateMBMarket check from the accrueInterest function in the MorphoBlueDebtAdaptor to reduce gas costs. Additionally, consider removing the _validateMBMarket check in the accrueInterest functions of both MorphoBlueSupplyAdaptor and MorphoBlueCollateralAdoptor. Market validation is not necessary in these cases, as a malicious strategist does not benefit from updating an untracked market.

G-2

withdrawableFrom calculation can be optimized

Topic
Duplicate calls
Status
Gas Savings
Medium

MorphoBlueSupplyAdaptor.sol’s withdrawableFrom() calculates the available assets to withdraw by taking into account pending interest:

function withdrawableFrom(
    bytes memory adaptorData,
    bytes memory configurationData
) public view override returns (uint256 withdrawableSupply) {
    bool isLiquid = abi.decode(configurationData, (bool));

    if (isLiquid) {
        MarketParams memory market = abi.decode(adaptorData, (MarketParams));
        (uint256 totalSupplyAssets, , uint256 totalBorrowAssets, ) = morphoBlue.expectedMarketBalances(market);
        if (totalBorrowAssets >= totalSupplyAssets) return 0;
        uint256 liquidSupply = totalSupplyAssets - totalBorrowAssets;
        uint256 cellarSuppliedBalance = morphoBlue.expectedSupplyAssets(market, msg.sender);
        withdrawableSupply = cellarSuppliedBalance > liquidSupply ? liquidSupply : cellarSuppliedBalance;
    } else return 0;
}

It does this by pulling values from the MorphoBlue market using the MorphoBalancesLib functions expectedMarketBalances() and expectedSupplyAssets().

However, the expectSupplyAssets() call, also makes a call to expectedMarketBalances():

function expectedSupplyAssets(IMorpho morpho, MarketParams memory marketParams, address user)
    internal
    view
    returns (uint256)
{
    Id id = marketParams.id();
    uint256 supplyShares = morpho.supplyShares(id, user);
    (uint256 totalSupplyAssets, uint256 totalSupplyShares,,) = expectedMarketBalances(morpho, marketParams);

    return supplyShares.toAssetsDown(totalSupplyAssets, totalSupplyShares);
}

Reference: MorphoBalancesLib.sol#L95-L105

Since expectedMarketBalances() makes external calls to the Morpho Blue contract as well as the markets interest rate model contract to calculate the market’s expected balances after interest, it can consume a fair bit of gas, if the values from the first call to expectedMarketBalances() were used to calculate the expectedSupplyAssets, it would reduce the gas used to call withdrawableFrom().

Remediations to Consider

Prevent a duplicate call to expectedMarketBalances() by using the values returned in the first call to determine the expected supply assets in withdrawableFrom().

I-1

Use caution when adding alternate deposit assets with non ERC20 adaptor holding positions

Topic
Use cases
Status
Acknowledged
Impact
Informational

Cellars that allow for multi asset deposits should use caution when allowing alternate deposits that don’t use a simple ERC20Adaptor for it’s holding position. Each non-simple holding position opens up more attack surface area for the cellar, as user deposits can interact with the protocol associated with the position’s adaptor in potentially malicious ways, and new attack vectors could open up that require multiple adaptors and/or deposit assets.

I-2

Non-standard ERC20 tokens are not supported

Topic
Coding Standards
Status
Acknowledged
Impact
Informational

The cellars underlying adapters don’t support non-standard ERC20 tokens like BNB. BNB's implementation of approve reverts when approving a zero value - which is the case when revoking approvals via the BaseAdapter._revokeExternalApproval.

function _revokeExternalApproval(ERC20 asset, address spender) internal {   // @audit does not support non-standard token like BNB
    if (asset.allowance(address(this), spender) > 0) asset.safeApprove(spender, 0);
}

It is recommended to handle those weird ERC20 tokens specifically, or if they are officially not supported by the protocol, proper documentation should be provided.

Disclaimer

Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.

Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.

The scope of this report and review is limited to a review of only the code presented by the Sommelier team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.