Security Audit
January 22, 2024
Version 1.0.0
Presented by 0xMacro
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.
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.
Our understanding of the specification was based on the following sources:
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.
The following source code was reviewed during the audit:
59e723a2b5f39bc314e4597887cb68a03640224b
731165df55092a655ff892dfb221f0a34ef660c2
Specifically, we audited the following contracts within this repository.
Contract | SHA256 |
---|---|
src/base/Cellar.sol |
|
src/base/permutations/CellarWithMultiAssetDeposit.sol |
|
src/base/permutations/CellarWithNativeSupport.sol |
|
src/modules/adaptors/Morpho/MorphoBlue/MorphoBlueCollateralAdaptor.sol |
|
src/modules/adaptors/Morpho/MorphoBlue/MorphoBlueDebtAdaptor.sol |
|
src/modules/adaptors/Morpho/MorphoBlue/MorphoBlueHelperLogic.sol |
|
src/modules/adaptors/Morpho/MorphoBlue/MorphoBlueSupplyAdaptor.sol |
|
src/base/permutations/advanced/CellarWithOracleWithBalancerFlashLoansWithMultiAssetDeposit.sol |
|
src/base/permutations/advanced/CellarWithOracleWithBalancerFlashLoansWithMultiAssetDepositWithNativeSupport.sol |
|
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.
Click on an issue to jump to it, or scroll down to see them all.
Deposit
event breaks ERC4626
standard
_validateMBMarket
code is duplicated
withdrawableFrom
calculation can be optimized
We quantify issues in three parts:
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. |
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.
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.
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:
- We only rely on the Morpho Blue code itself vs relying on library contracts.
- Gas efficiency.
- 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.
Deposit
event breaks ERC4626
standard
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()
.
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.
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.
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()
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
_validateMBMarket
code is duplicated
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.
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.
withdrawableFrom
calculation can be optimized
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()
.
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.
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.
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.