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

Sommelier A-8

Security Audit

June 28, 2023

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 June 5, 2023 to June 23, 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
Medium 4 - - 4
Low 1 - - 1
Code Quality 6 - - 6
Informational 3 1 - 2
Gas Optimization 3 - - 3

Sommelier was quick to respond to these issues.

Specification

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

Source Code

The following source code was reviewed during the audit:

Specifically, we audited the following contracts within this repository for the initial part of the audit:

Contract SHA256
src/AxelarProxy.sol

5b08e2710e4bddc89395b419211816996f6c68c1925e15179da1ea53c128a4bd

src/modules/adaptors/Aave/AaveATokenAdaptor.sol

5bba14f3d4d46656a98160dc9e481170172f2cd8269e2f69cc4daf7291898972

src/modules/adaptors/Compound/CTokenAdaptor.sol

7e94a1d5901343bffba944c2f1576a07211d4bb39d7669e3c358392bcf887198

src/modules/adaptors/Frax/FTokenAdaptor.sol

a0035d5f8d636ca32b05d8fcc42659d351b34f2b0dd46ef2c755df2be0f3ec49

src/modules/adaptors/Frax/FTokenAdaptorV1.sol

ee8ccb8874001059295fd95a5d5943f0a0bfab2518335fbe5154b4cf39280c49

src/modules/adaptors/Morpho/MorphoAaveV2ATokenAdaptor.sol

ed6ff6f79d6bf3fe014a01f60c62e6c8eaa7d6cbb527cfe4c6e1dbfc886ce516

src/modules/adaptors/Morpho/MorphoAaveV2DebtTokenAdaptor.sol

76896a9f0352edf8b25a6287a15f7039caddb519d4eb9640aaa53e47640944ab

src/modules/adaptors/Morpho/MorphoAaveV3ATokenCollateralAdaptor.sol

29f437c5fecafa4e24ff37b029dfff639496bb31d199ffde20489f4e2e89b2e4

src/modules/adaptors/Morpho/MorphoAaveV3ATokenP2PAdaptor.sol

2314003f887e0c70456b7240858de90dc403b03d688f950c63741c5ce6981e27

src/modules/adaptors/Morpho/MorphoAaveV3DebtTokenAdaptor.sol

1e20d24666d6ae80d3c9d55e7cd2b4b6e9ab677f75eda540ae402612253a96d5

src/modules/adaptors/Morpho/MorphoRewardHandler.sol

2d727f2aab4b42aff42020a79da48dc483b0156eb4831b2808372fc50bb34603

src/modules/price-router/Extensions/Balancer/BalancerPoolExtension.sol

bc338f5a88de65c91e211fb214b35b7fb25cdcd6918e0eaeea07e8ef65a6e6a9

src/modules/price-router/Extensions/Balancer/BalancerStablePoolExtension.sol

780fdb18c9b0166fd933338655492e9ea23283f8a0ffd7bc0619945b49c6b423

src/modules/price-router/Extensions/Lido/WstEthExtension.sol

6d88140a08f55b9d94a4dc86083565450a1ca06441c0cd284adf6a7b55fae140

src/modules/price-router/PriceRouter.sol

4170fe390f8a9bee1640331fd1ad32d8912cc0cd4a1c7834f5fd7081c8153658

We audited the following contracts as part of the second part of the audit:

Contract SHA256
src/modules/adaptors/Balancer/BalancerPoolAdaptor.sol

c17ce6d5cb3832a7e9fdc75166e30fae7e0e902691f1530b1b2917bec7086193

src/modules/adaptors/Sommelier/CellarAdaptor.sol

97bb6b75a90c297f134c0fd33f065d3c2f441fcf71ca1de90eb69ddbb499ee64

src/modules/price-router/Extensions/Redstone/RedstonePriceFeedExtension.sol

d3233c38925e9a414bc3377a992663669974d124d09efe6824835175c14b6406

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

M-1

Strategist can lower Morpho health factors to the liquidation threshold

Topic
Trust Assumption
Status
Impact
Medium
Likelihood
Medium

In MorphoAaveV2ATokenAdaptor and MorphoAaveV3ATokenCollateralAdaptor, each allows the strategist to withdraw assets from Morpho via withdrawFromAaveV2Morpho(), and withdrawFromAaveV3Morpho() respectively.

function withdrawFromAaveV2Morpho(IAaveToken aToken, uint256 amountToWithdraw) public {
    morpho().withdraw(address(aToken), amountToWithdraw, address(this));
}

Reference: MorphoAaveV2ATokenAdaptor.sol#L157-L158

function withdrawFromAaveV3Morpho(ERC20 tokenToWithdraw, uint256 amountToWithdraw) public {
    morpho().withdrawCollateral(address(tokenToWithdraw), amountToWithdraw, address(this), address(this));
}

Reference: MorphoAaveV3ATokenCollateralAdaptor.sol#L138-L140

The MorphoAaveV2DebtTokenAdaptor also allows the strategist to borrow assets from Morpho with borrowFromAaveV2Morpho().

function borrowFromAaveV2Morpho(address aToken, uint256 amountToBorrow) public {
// Check that debt position is properly set up to be tracked in the Cellar.
bytes32 positionHash = keccak256(abi.encode(identifier(), true, abi.encode(aToken)));
uint32 positionId = Cellar(address(this)).registry().getPositionHashToPositionId(positionHash);
if (!Cellar(address(this)).isPositionUsed(positionId))
    revert MorphoAaveV2DebtTokenAdaptor__DebtPositionsMustBeTracked(aToken);

// Borrow from morpho.
morpho().borrow(aToken, amountToBorrow);
}

Reference: MorphoAaveV2DebtTokenAdaptor#L104-L114

However, each of these functions can decrease the health factor of the cellars Morpho position and there are no checks to ensure the health factor is maintained above a minimum safe value. This can allow a strategist to bring the health factor of the cellars position down to the liquidation threshold of Aave, causing the cellar’s Morpho positions to be at risk of liquidation.

Remediations to Consider

Check the health factor of the cellar after a strategist withdraws funds, to ensure the positions do not become too close to being liquidated.

M-2

MorphoV2 adaptor can be liquidated by users if set up improperly

Topic
Trust Assumption
Status
Impact
Medium
Likelihood
Medium

In MorphoAaveV2ATokenAdaptor.sol a isLiquid is set in the positions configurationData value that will determine if the position is allowed to be withdrawn by users using withdraw(), this is to prevent users from withdrawing assets from a position that is used as collateral to support a debt position:

function withdraw(
    uint256 assets,
    address receiver,
    bytes memory adaptorData,
    bytes memory configData
) public override {
    // Run external receiver check.
    _externalReceiverCheck(receiver);

    // Make sure position is setup to be liquid.
    bool isLiquid = abi.decode(configData, (bool));
    if (!isLiquid) revert BaseAdaptor__UserWithdrawsNotAllowed();

    IAaveToken aToken = abi.decode(adaptorData, (IAaveToken));

    // Withdraw assets from Morpho.
    morpho().withdraw(address(aToken), assets, receiver);
}

Reference: MorphoAaveV2TokenAdaptor#L78-L95

However there is the possibility for a MorphoAaveV2ATokenAdaptor position to be set as isLiquid but then have the cellar later take on debt from Morpho via a MorphoAaveV2DebtTokenAdaptor position, or for a position to be setup incorrectly by the strategist. In this case, any withdraws from the liquid Morpho lending position would effect the health factor of the debt position, potentially allowing the health factor to be manipulated by cellar share holders to allow the cellars Morpho positions to be liquidated maliciously.

Remediations to Consider

Either

  • Check if the cellar has any borrows on Morpho before allowing withdraws.
  • Check if withdrawing lowers the cellars health factor below a minimum.
  • Keep track of debt with an external contract to easily check if there is debt in the cellar before withdrawing.

Doing either of these will prevent the possibility of users maliciously effecting the health of a misconfigured cellars debt position.

M-3

Arbitrary calls made for relayerJoinPool() and relayerExitPool()

Topic
Spec
Status
Impact
Medium
Likelihood
Medium

In BalancerPoolAdaptor.sol's relayerJoinPool and relayerExitPool, it takes a bytes array of calldata that is directly used to make a call on the relayer.

function relayerJoinPool(
    ERC20[] memory tokensIn,
    uint256[] memory amountsIn,
    ERC20 bptOut,
    bytes[] memory callData
) public {
    for (uint256 i; i < tokensIn.length; ++i) {
        tokensIn[i].approve(address(vault), amountsIn[i]);
    }
    uint256 startingBpt = bptOut.balanceOf(address(this));
    relayer.multicall(callData);

    uint256 endingBpt = bptOut.balanceOf(address(this));
    uint256 amountBptOut = endingBpt - startingBpt;
    PriceRouter priceRouter = Cellar(address(this)).priceRouter();
    uint256 amountBptIn = priceRouter.getValues(tokensIn, amountsIn, bptOut);

    if (amountBptOut < amountBptIn.mulDivDown(slippage(), 1e4)) revert BalancerPoolAdaptor___Slippage();

    // revoke token in approval
    for (uint256 i; i < tokensIn.length; ++i) {
        _revokeExternalApproval(tokensIn[i], address(vault));
    }
}

Reference: BalancerPoolAdaptor#L208-L231

function relayerExitPool(ERC20 bptIn, uint256 amountIn, ERC20[] memory tokensOut, bytes[] memory callData) public {
PriceRouter priceRouter = Cellar(address(this)).priceRouter();
uint256[] memory tokenAmount = new uint256[](tokensOut.length);

for (uint256 i; i < tokensOut.length; ++i) {
    tokenAmount[i] = tokensOut[i].balanceOf(address(this));
}

relayer.multicall(callData);

for (uint256 i; i < tokensOut.length; ++i) {
    tokenAmount[i] = tokensOut[i].balanceOf(address(this)) - tokenAmount[i];
}
uint256 bptEquivalent = priceRouter.getValues(tokensOut, tokenAmount, bptIn);
if (bptEquivalent < amountIn.mulDivDown(slippage(), 1e4)) revert BalancerPoolAdaptor___Slippage();
}

Reference: BalancerPoolAdaptor.sol#L243-L256

This can allow a strategist to make multiple arbitrary calls to balancer relayer and is not limited to exiting or joining a pool as intended. This opens the strategist to potentially make swaps or interact with balancer in unintended ways, like setting internal balancer balances that are not tracked by the cellar.

Remediations to Consider

Explicitly call joinPool() or exitPool() on the vault contract, or with the relayer to ensure the function executes as expected.

M-4

Slippage checks can be skipped in relayerExitPool()

Topic
Input Validation
Status
Impact
Medium
Likelihood
Medium

In BalancerPoolAdaptor.sol’s relayerExitPool(), there are input parameters passed in to determine if tokens received from exiting the pool is within an accepted slippage range.

function relayerExitPool(ERC20 bptIn, uint256 amountIn, ERC20[] memory tokensOut, bytes[] memory callData) public {
PriceRouter priceRouter = Cellar(address(this)).priceRouter();
uint256[] memory tokenAmount = new uint256[](tokensOut.length);

for (uint256 i; i < tokensOut.length; ++i) {
    tokenAmount[i] = tokensOut[i].balanceOf(address(this));
}

relayer.multicall(callData);

for (uint256 i; i < tokensOut.length; ++i) {
    tokenAmount[i] = tokensOut[i].balanceOf(address(this)) - tokenAmount[i];
}
uint256 bptEquivalent = priceRouter.getValues(tokensOut, tokenAmount, bptIn);
if (bptEquivalent < amountIn.mulDivDown(slippage(), 1e4)) revert BalancerPoolAdaptor___Slippage();
}

Reference: BalancerPoolAdaptor.sol#L243-L256

However, since only the calldata is used to make the call with the relayer, these other input parameters can be unrelated to the call itself. This allows a strategist to add empty values to amountIn and tokensOut, which allows the strategist to ignore the slippage check.

Remediations to Consider

Explicitly call exitPool with set input parameters on the vault contract or ensure the calldata sent to the relayer matches the values entered in order to properly make slippage checks.

L-1

Chainlink prices can be incorrect

Topic
Trust Assumption
Status
Addressed
Impact
Medium
Likelihood
Low

The PriceRouter.sol’s _getPriceInUSD() takes the price of assets and converts them to their price in USD with 8 decimals.

/**
* @notice Helper function to get an assets price in USD.
* @dev Returns price in USD with 8 decimals.
*/
function _getPriceInUSD(ERC20 asset, AssetSettings memory settings) internal view returns (uint256) {
    // Call get price function using appropriate derivative.
    uint256 price;
    if (settings.derivative == 1) {
        price = _getPriceForChainlinkDerivative(asset, settings.source);
    } else if (settings.derivative == 2) {
        price = _getPriceForTwapDerivative(asset, settings.source);
    } else if (settings.derivative == 3) {
        price = Extension(settings.source).getPriceInUSD(asset);
    } else revert PriceRouter__UnknownDerivative(settings.derivative);

    return price;
}

Reference: PriceRouter.sol#L515-L531

However, there are some assets supported by chainlink that their price in USD is not 8 decimals. In the case of AMPL/USD the price returned is 18 decimals. This can disrupt with the valuation of assets by either inflating or deflating the price from the actual value of the asset.

Remediations to Consider

Either

  • Ensure any asset using chainlink as a data feed uses 8 decimals before adding.
  • Consider the decimals of the price returned by the feed into the price calculations.
Response by Sommelier

Will not support assets via chainlink if they do not have 8 decimals

G-1

Costly Balancer read-only reentrancy check

Topic
Gas
Status
Gas Savings
High

In BalancerPoolExtension.sol, there is a check to ensure the vault has not in the middle of execution, which could lead to a known read-only reentrancy issue.

function _ensureNotInVaultContext(IVault vault) internal view {
// Perform the following operation to trigger the Vault's reentrancy guard.
// Use a static call so that it can be a view function (even though the
// function is non-view).
//
// IVault.UserBalanceOp[] memory noop = new IVault.UserBalanceOp[](0);
// _vault.manageUserBalance(noop);

// solhint-disable-next-line var-name-mixedcase
bytes32 REENTRANCY_ERROR_HASH = keccak256(abi.encodeWithSignature("Error(string)", "BAL#400"));

// read-only re-entrancy protection - this call is always unsuccessful but we need to make sure
// it didn't fail due to a re-entrancy attack
// This might just look like an issue in foundry. Running a testnet test does not use an insane amount of gas.
(, bytes memory revertData) = address(vault).staticcall(
    abi.encodeWithSelector(vault.manageUserBalance.selector, new address[](0))
);

if (keccak256(revertData) == REENTRANCY_ERROR_HASH) revert BalancerPoolExtension__Reentrancy();
}

Reference: BalancerPoolExtension.sol#L43-L62

However, as suggested by Balancer, static calls will consume all gas forwarded to them if they revert due to a storage modification, which is the goal of this check. Forwarding most of the available gas towards this static call, as is done here, would cause a lot additional gas to be consumed.

Additionally, it is more gas efficient to check if the reverting data’s length is greater than zero, than it is to check the returned error, as any error when static calling this function would be the reentrancy error.

Remediations to Consider

Use the code suggested by balancer to prevent excess gas consumption when making this check.

G-2

Loop index unnecessarily cast as uint8

Topic
Gas
Status
Gas Savings
Low

In PriceRouter.sol’s _getValues(), it loops though all an array of assets and amounts to determine the value of all assets.

for (uint8 i = 0; i < baseAssets.length; i++) {
    // Skip zero amount values.
    if (amounts[i] == 0) continue;
    ERC20 baseAsset = baseAssets[i];
    if (baseAsset == quoteAsset) valueInQuote += amounts[i];
    else {
        uint256 basePrice;
        {
            AssetSettings memory baseSettings = getAssetSettings[baseAsset];
            if (baseSettings.derivative == 0) revert PriceRouter__UnsupportedAsset(address(baseAsset));
            basePrice = _getPriceInUSD(baseAsset, baseSettings);
        }
        valueInQuote += _getValueInQuote(
            basePrice,
            quotePrice,
            baseAsset.decimals(),
            quoteDecimals,
            amounts[i]
        );
    }
}

Reference: PriceRouter.sol#L585-L605

However, the index i used in the loop is a uint8 which gets cast as a uint256 each time it is used to index from the amounts or baseAssets array. Each casting consumes additional gas.

Remediations to Consider

Set i as a uint256 instead of a uint8 to prevent unnecessary casting and gas consumption.

G-3

Use ++i instead of i++

Topic
Gas
Status
Gas Savings
Low

In PriceRouter.sol, the functions _getValues() and getExchangeRates() use for loops when executing.

for (uint8 i = 0; i < baseAssets.length; i++) {

Reference: PriceRouter.sol#L585

for (uint256 i; i < numOfAssets; i++) {

Reference: Pricerouter.sol#L474

However, it is 5 gas cheaper to increment these indexes using ++i than it is to use i++.

Although 5 gas is not much savings, over many loops it can add up, especially when these functions are called as frequently as they are.

Remediations to Consider

Replace instances of i++ with ++i.

Q-1

Inaccurate natSpec comment

Topic
Code Quality
Status
Quality Impact
Low

In MorphoAaveV3ATokenCollateralAdaptor.sol's withdrawableFrom() the natSpec comment states that it “Checks that cellar has no active borrows, and if so reverts.” However, if there are active borrows, withdrawableFrom() returns 0 rather than reverting.

/**
* @notice Checks that cellar has no active borrows, and if so reverts.
*/
function withdrawableFrom(bytes memory adaptorData, bytes memory) public view override returns (uint256) {
    address[] memory borrows = morpho().userBorrows(msg.sender);
    if (borrows.length > 0) return 0;
    else {
        ERC20 underlying = abi.decode(adaptorData, (ERC20));
        return morpho().collateralBalance(address(underlying), msg.sender);
    }
}

Reference: MorphoAaveV3ATokenCollateralAdaptor.sol#L82-L92

Remediations to Consider

Alter the comment to clarify that it will return 0 rather than revert.

Q-2

Unused import

Topic
Code Quality
Status
Quality Impact
Low

In Lido/WstEthExtension.sol, the IChainlinkAggregator import is unused.

import { IChainlinkAggregator } from "src/interfaces/external/IChainlinkAggregator.sol";

Reference: WstEthExtension.sol#L5

Remediations to Consider

Remove the IChainlinkAggregator import.

Q-3

Unnecessary type casting

Topic
Code Quality
Status
Quality Impact
Low

In MorphoAaveV2ATokenAdaptor.sol, MorphoAaveV3ATokenCollateralAdaptor.sol, and MorphoAaveV3ATokenP2PAdaptor.sol, there are functions where adaptorData is decoded into either an IAaveToken or ERC20, but only the address is used.

IAaveToken aToken = abi.decode(adaptorData, (IAaveToken));

// Withdraw assets from Morpho.
morpho().withdraw(address(aToken), assets, receiver);

Reference: MorphoAaveV2ATokenAdaptor.sol#L91-L94

ERC20 underlying = abi.decode(adaptorData, (ERC20));

// Withdraw assets from Morpho.
morpho().withdrawCollateral(address(underlying), assets, address(this), receiver);

Reference: MorphoAaveV3ATokenCollateralAdaptor.sol#L76-L79

ERC20 underlying = abi.decode(adaptorData, (ERC20));
return morpho().collateralBalance(address(underlying), msg.sender);

Reference: MorphoAaveV3ATokenCollateralAdaptor.sol#L89-L90 and MorphoAaveV3ATokenCollateralAdaptor.sol#L98-L99

ERC20 underlying = abi.decode(adaptorData, (ERC20));
uint256 iterations = abi.decode(configurationData, (uint256));

// Withdraw assets from Morpho.
morpho().withdraw(address(underlying), assets, address(this), receiver, iterations);

Reference: MorphoAaveV3ATokenP2PAdaptor.sol#L85-L89

ERC20 underlying = abi.decode(adaptorData, (ERC20));
return morpho().supplyBalance(address(underlying), msg.sender);

Reference: MorphoAaveV3ATokenP2PAdaptor.sol#L96-L97 and MorphoAaveV3ATokenP2PAdaptor.sol#L104-L105

Remediations to Consider

Cast the decoded adaptorData as an address where specified to prevent additional type casting.

Q-4

Misspelling

Topic
Code Quality
Status
Quality Impact
Low

In FTokenAdaptor.sol there is a misspelling of override.

*      and overrid the interface helper functions.

Reference: FTokenAdaptor.sol#L11

Remediations to Consider

Fix this spelling error.

Q-5

Unnecessary casting of liquidityGauge

Topic
Code Quality
Status
Quality Impact
Low

In BalancerPoolAdaptor.sol’s stakeBPT() and unstakeBPT() , there is an unnecessarily cast of liquidityGauge to address when _liquidityGauge is already an address representing the same thing.

function stakeBPT(ERC20 _bpt, address _liquidityGauge, uint256 _amountIn) external {
_validateBptAndGauge(address(_bpt), _liquidityGauge);
uint256 amountIn = _maxAvailable(_bpt, _amountIn);
ILiquidityGaugev3Custom liquidityGauge = ILiquidityGaugev3Custom(_liquidityGauge);
_bpt.approve(address(liquidityGauge), amountIn);
liquidityGauge.deposit(amountIn, address(this));
_revokeExternalApproval(_bpt, address(liquidityGauge));
}

Reference: BalancerPoolAdaptor.sol#L266-L273

function unstakeBPT(ERC20 _bpt, address _liquidityGauge, uint256 _amountOut) public {
_validateBptAndGauge(address(_bpt), _liquidityGauge);
ILiquidityGaugev3Custom liquidityGauge = ILiquidityGaugev3Custom(_liquidityGauge);
_amountOut = _maxAvailable(ERC20(address(liquidityGauge)), _amountOut);
liquidityGauge.withdraw(_amountOut);
}

Reference: BalancerPoolAdaptor.sol#L281-L286

There is also an unnecessary casting in balanceOf() where liquidityGauge is already and address that is cast again to an address.

function balanceOf(bytes memory _adaptorData) public view override returns (uint256) {
    (ERC20 bpt, address liquidityGauge) = abi.decode(_adaptorData, (ERC20, address));
    if (liquidityGauge == address(0)) return ERC20(bpt).balanceOf(msg.sender);
    ERC20 liquidityGaugeToken = ERC20(address(liquidityGauge));
    uint256 stakedBPT = liquidityGaugeToken.balanceOf(msg.sender);
    return ERC20(bpt).balanceOf(msg.sender) + stakedBPT;
}

Reference: BalancerPoolAdaptor.sol#L152-L158

Remediations to Consider

Use _liquidityGauge instead of liquidityGauge where an address is needed.

Q-6

Additional comments

Topic
Code Quality
Status
Quality Impact
Low

Each adaptor has comments that describe the adaptors specification. However in BalancerPoolAdaptor.sol there are additional unnecessary Adaptor Data and Configuration Data comments.

//==================== Adaptor Data Specification ====================
// See Related Open Issues on this for BalancerPoolAdaptor.sol
//================= Configuration Data Specification =================
// NOT USED

Reference: BalancerPoolAdaptor.sol#L36-L39

Remediations to Consider

Remove these additional comments.

I-1

Asset price oracles can be edited

Topic
Informational
Status
Addressed
Impact
Informational

A assets price oracle is set by the PriceRegistry.sol contract which it’s multisig owner can call startEditAsset() to edit where after a 7 day delay, the new asset’s pricing will take effect.

This can allow the multisig to potentially rug cellars if the asset’s new oracle info is malicious, such as setting it to an Extension contract oracle that could be written to return favourable pricing in specific situations.

Editing the asset can be stopped by sommelier governance, by calling transitionOwner() to transition the owner over the course of 7 days, while preventing any owner functions from being called, preventing an asset from completing it pending change. Doing so requires governance and the community to watch for any calls to startEditAsset() and flag anything that seems malicious. Third party bots should be setup that notify the community of calls to startEditAsset().

Response by Sommelier

Sommelier will create guides to teach the community how they can set up OpenZeppelin Sentinel bots to watch important contracts such as the PriceRouter and Registry, and notify them of certain actions taken.

I-2

Governance can transfer ownership of the price router

Topic
Informational
Status
Addressed
Impact
Informational

As mentioned in I-1, governance can call transitionOwner() of the PriceRouter to change the owner. This occurs over the course of 7 days, during which no owner functions can be called. In the case that governance transitions the price router’s owner to a new malicious owner, the community should be aware and respond accordingly. Third party bots should be setup that notify the community of calls to transitionOwner().

Response by Sommelier

Sommelier will create guides to teach the community how they can set up OpenZeppelin Sentinel bots to watch important contracts such as the PriceRouter and Registry, and notify them of certain actions taken.

I-3

Cellar shares can be set to be illiquid

Topic
Informational
Status
Acknowledged
Impact
Informational

Cellars can hold positions in another cellar’s shares using the CellarAdaptor. This can be beneficial when cellars have correlated assets and withdrawing shares from a cellar would result in receiving correlated assets to what is held in the parent cellar. There is the possibility, however, where a strategist may want to hold shares in a cellar that holds unrelated assets. Doing so could cause user withdrawals to receive multiple assets that are unrelated to the parent cellar’s positions, and users may want to sell these. To prevent this, a strategist can set a cellar position to be illiquid, preventing user withdrawals of those cellar shares, and ensuring the assets received from a user withdrawal are more in line with what is expected.

It is important to note that illiquid cellar positions will remain until a strategist withdraws them, leaving shares in the cellar unable to be withdrawn if the only remaining positions are illiquid cellar shares.

Response by Sommelier

Accepted

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.