Sommelier A-4

This issue is similar to M-2, in that they both involve dividing, and then using the result from the division to multiply later.

uint256 totalWithdrawableBalanceInAssets multiplies withdrawableBalance with exchangeRate, but exchangeRate may lose precision due to Solidity division. This may cause a user to not receive all of the assets from the cellar that they are entitled to.

Remediations to Consider

1. Consider using a high internal scaling factor for fixed point numbers so precision loss becomes acceptable

2. Consider making the changes from M-2, to fix the division before multiplication problem in getValues() and getValue(). Then change totalWithdrawableBalanceInAssets in _withdrawInOrder to something like:

(uint256 exchangeRateNumerator, uint256 exchangeRateDenominator) = 
  priceRouter.getExchangeRateFOO(positionAsset, asset); 
  // modified version of getExchangeRate that returns numerator and denominator 
     instead of dividing, so values can be plugged into equation below, without 
     loss of precision.

uint256 totalWithdrawableBalanceInAssets = exchangeRateNumerator * 
withdrawableBalance / exchangeRateDenominator / onePositionAsset;

With the remediations from M-2 and the totalWithdrawableBalanceInAssets change, assets will no longer be erroneously left inside the Cellar.

While allowedRebalanceDeviation puts a limit on the damage, a strategist can submit a call to callOnAdaptor that swaps into an asset that is not tracked by the Cellar. If an LP withdraws their shares with redeem before the untracked asset issue is found and rectified, the LP would unknowingly leave some of their assets in the Cellar.

This can can manifest in a number of ways:

1. Accidentally. The strategist swapped into an untracked positions with no malicious intent.
2. On purpose. A malicious strategist can do the following:

• Create an ERC20, let’s call it AttackCoin. Only the attacker’s EOA holds AttackCoin.
• Create a UniswapV3 pool between the Cellar base asset and AttackCoin and provide liquidity. There is no risk to the attacker at this stage because no one else holds AttackCoin, meaning that no one can drain the base asset liquidity that they provided to the pool. The Attacker doesn’t need to provide much liquidity, because they also control the amountOutMin argument.
• Call callOnAdaptor to swap the base asset for AttackCoin. The value will be the max amount allowed through allowedRebalanceDeviation.
• Use the EOA to swap AttackCoin for the base asset and remove all remaining liquidity from pool.
• Attacker profits, draining funds from the Cellar/LPs. Cellar/LPs are stuck with worthless AttackCoin.

As the value in the Cellar rises, it becomes more profitable and attractive to attack. For example, a malicious strategist could take ~ 300_000e18 DAI out of a 100_000_000e18 DAI Cellar, given the default allowedRebalanceDeviation. This attack can also be repeated by calling callOnAdaptor many times, but the rate limiting that will be put in place on the Sommelier chain side should help mitigate its repeatability.

Remediations to Consider

When calling callOnAdaptor, consider adding a check that all final out assets are tracked assets in the Cellar. Intermediate swaps of unchecked out assets should still be allowed to allow for greater strategist flexibility.

A strategist can add or remove a position, trusted by the Registry, using Cellar:addPosition or Cellar:removePosition.

However, this can change what the holdingIndex and the holding position (position to deposit to) refer to. holdingIndex is a fixed index in the creditPositions array.

Downstream impact of confusing the holding position for another arbitrary position:

1. Withdrawal: When adjusting the withdrawal preferences for positions using swapPositions(someIndex, holdingIndex ...), a scenario can arise where the intention is to promote the holding position for withdrawal. However, Strategists can inadvertently promote an appreciating position for withdrawal. This can cause a reduction in the appreciating position. In other words, assets are withdrawn in the wrong order, inadvertently lowering the overall value of the Cellar.
2. Deposit: When a LP deposits, the Cellar can inadvertently deposit into a depreciating position where the intention was depositing into the holdingPosition.

Remediations to Consider

To simplify the implementation, consider defining new state holdingPosition (the uint32 ID) instead of holdingIndex; this reduces the developer’s mental burden by removing the concern of array accesses:

• When mutating the underlying array with removePosition consider checking if creditPositions[toRemoveindex] refers to holdingPosition instead and revert if needed
• Consider removing the code dealing with the holding position in swapPositions because the notion of the holding position is represented as a name instead of an index

(The important difference between this and H-5 is: H-5 is about the liability side of an Aave account, and this issue about the equity side.)

Strategists can call addPosition to add position configuration. A type of position configuration is the minHealthFactor. If there is little collateral to liability (low healthFactor), the cellar’s collateral becomes liquidatable. minHealthFactor helps restrict LPs from withdrawing too much collateral from Cellar’s Aave account by stopping LPs from withdrawing the aToken positions excessively.

However, the position configuration does not need to be approved by governance.

// Cellar.sol; addPosition called by Strategist
function addPosition(
  uint32 index,
  uint32 positionId,
  bytes memory configurationData,
  bool inDebtArray
) external onlyOwner whenNotShutdown;

// Registry:trustPosition; called by Governance
getPositionIdToPositionData[positionId] = PositionData({
  adaptor: adaptor,
  isDebt: isDebt,
  adaptorData: adaptorData,
  // Note it is initalized to 0 and
  // to be set in Cellar:addPosition by Strategist
  configurationData: abi.encode(0) 
});

An issue can arise when:

1. Strategist calls addPosition for an aToken position with 0.1 as the minHealthFactor [1] [4]
2. Strategist deposits collateral into Aave, then borrows assets
3. A LP withdraws from the aToken position. The withdrawal causes healthFactor to be close to below 1 e.g. 1.0000001 [2][3]
4. The Chainlink oracle price drops for the collateral and the Cellar Aave account becomes liquidatable

Remediations to Consider

Consider making position configuration data configurable by Governance instead of by Strategists to ensure minHealthFactor is sufficiently conservative.

Notes

1. Position cannot be withdrawn when value is 0
2. Withdraws cannot directly cause a liquidation because Aave blocks it [error code 6])
3. The difference here is this issue brings down healthFactor by withdrawing collateral and H-5 does it by borrowing more
4. This issue can happen regardless of strategist’s intention. Strategists can set a dangerous (< 1) minHealthFactor, so an excessive withdrawal of Aave collateral can happen

The strategist of a cellar can borrow from Aave.

However, this power is unchecked. Strategists can borrow on cellar’s behalf at arbitrarily low health factor, provided borrowing does not cause the position to be liquidatable (an Aave restriction, error 11 - "There is not enough collateral to cover a new borrow").

Consequently, strategists are incentivized to sandwich attack around an oracle price drop (e.g. USDC-USD is dropping 1%) because they profit from Aave liquidation fees.

For example, borrow WETH using USDC as collateral

1. Wait for a TX for Chainlink USDC price drop to enter into the mempool
2. Front run TX and borrow WETH to bring health factor lower e.g. 1.02 (below 1 is liquidatable) [1]
3. Chainlink USDC-USD price drop executes
4. Backrun a TX to liquidate collateral

• Health factor was 1.02 before Chainlink’s TX
• Suppose USDC price update is -3%, new health factor becomes < 1 after price update tx executes

Remediations to Consider

The root cause is that strategists can borrow on a cellar’s behalf at an arbitrarily low health-factor.

Consider fetching the health factor from Aave to compare to a configured minimum in AaveDebtTokenAdaptor:borrowFromAave.

However, consider using extra caution when designing a solution.

1. Presumably using the same health factor for both the credit and debt adaptor is desirable, for restricting withdrawing too much collateral or borrowing too much. Note that the health factor is configuration for AaveATokenAdaptor. Therefore, setting the health factor in 2 different places can lead to inconsistent configuration.
2. In general, a credit and debt adaptor pair for the same protocol are special because they manipulate the same protocol state. Therefore, there can be the same pattern of 2 adaptors depending on the same configuration in Cellar. Consider to be careful with having inconsistent configuration when it comes to a credit and debt adaptor pair.
3. The call from cellar:callOnAdaptor to AaveDebtTokenAdaptor:borrowFromAave does not allow for passing configuration data. Consider adding a configurable limit for borrowing.

Notes

1. This TX is not the easiest to front run, but it is still possible. There will likely be a very short time window between the Sommelier chain validating Strategist instructions and them getting executed.
2. Because there can be MEV bots watching the Sommelier chain for the signatures for borrowing and frontend the attack on Ethereum.
3. Also because of the fees present in submitLogicCall, which will eventually result in bots trying to claim them. In the worst case for Strategists, the Cellar’s Aave account will be liquidatable before they can backrun with a liquidation call, leaving the liquidation opportunity to others.

Cellar calls mutative and trusted adaptor functions via delegatecall to change the Cellar’s state according to the adaptor’s logic. And Cellar uses staticcall for the non-mutative functions.

withdraw is a mutative adaptor function for the Aave aToken adaptor. withdraw fetches the Aave health factor to:

• Compare against a configured minimum
• Decide whether the withdrawal will cause the Cellar’s Aave positions to be at risk for liquidation

However, withdraw uses msg.sender for fetching the health factor meaning msg.sender will be the LP because Cellar delegatecalls into withdraw. In other words, the health factor of the LP withdrawing will be used instead of the Cellar’s.

Confusing the LP’s health factor for the Cellar’s health factor can lead to the LP not being able to redeem shares when:

• The LP’s health factor is poor (health factor < minHealthFactor) but the Cellar’s health factor is good.

Remediations to Consider

1. Consider changing msg.sender to address(this) to correctly represent the cellar’s address in AaveATokenAdaptor:withdraw
2. Consider defining 2 different helper functions to help with readability for reading Cellar address in delegatecall and staticcall context to reduce the amount of mental translation needed for the readers

• Note one can leverage Solidity’s modifiers and declare _cellarDelegateCaller as mutative even it is not, so that using _cellarDelegateCaller in a non-mutative context will be a compile-time error

3. function _cellarDelegateCaller() internal() returns (address); i.e. address(this)
4. function _cellarStaticCaller() internal view() returns (address); i.e. msg(sender)

References

• https://docs.aave.com/risk/v/aave-v2/asset-risk/risk-parameters#liquidation-threshold

The cause for this discrepancy is getValue() and getValues() swap the ordering of steps 2 and 3 as outlined below. Multiplication and division are mathematically commutative, but division in Solidity takes the floor, which makes the operation not commutative.

getValue() order:

1. Get price in USD of base asset from _getPriceInUSD()
2. getExchangeRate executes basePrice.mulDivDown(10**quoteAssetDecimals, quotePrice);
3. Final amount is amount.mulDivDown(getExchangeRate(baseAsset, quoteAsset), 10baseAsset.decimals());

getValues() order:

1. Get price in USD of base asset from _getPriceInUSD()
2. _getValues() executes (amounts[i].mulDivDown(price, 10**baseAsset.decimals()));
3. Final amount is valueInUSD.mulDivDown(10**quoteDecimals, quotePrice)

The issue is exacerbated as the size of amount argument increases.

In BaseAdaptor::oracleSwap, amountIn relies on priceRouter.getValue() , which may artificially push the value down causing otherwise valid swaps to revert on BaseAdaptor__BadSlippage(). It can also push amountIn up, allowing swaps that should have reverted to succeed.

In AaveATokenAdaptor::withdrawableFrom , the withdrawable amount depends on priceRouter.getValue(), which may result in it being higher or lower than expected.

Remediations to Consider

Consider grouping all multiplication together first, and then doing all division at the end, so no precision is lost in the intermediate steps. For example in getValues():

valueInQuote += (amounts[i] * basePrice * 10**quoteDecimals) / 10**baseAsset.decimals() / quotePrice;

Also, consider having getValue() and getValues() follow more similar code paths so they return the same value for the same pricing call. One option is to have getValue() construct the proper arguments and call _getValues() instead of calling getExchangeRate().

This issue is similar to H-1 and M-2. The underlying cause is dividing and using that result to multiply. The divisions in AaveATokenAdaptor and CTokenAdaptor that are multiplied later in Cellar::_withdrawInOrder are the following:

// AaveATokenAdaptor::withdrawableFrom
maxBorrowableWithMin =
  totalCollateralETH - (((minHealthFactor) * totalDebtETH) / 
    (currentLiquidationThreshold * 1e14));

// ...

uint256 withdrawable = priceRouter.getValue(WETH(), 
  maxBorrowableWithMin, underlying);

// CTokenAdaptor::withdrawableFrom
return cTokenBalance.mulDivDown(cToken.exchangeRateStored(), 1e18);

These divisions can cause totalWithdrawableBalanceInAssets in Cellar.sol to be lower than expected, resulting in the same scenario as H-1.

Remediations to Consider

1. Consider using a scaling factor outlined in H-1.
2. Consider returning the numerator and denominator from withdrawableFrom similarly outlined in H-1.

Some ERC20 tokens return false when transferFrom fails instead of reverting, which could cause VestingSimple::deposit to successfully run even though it didn’t receive the tokens it expected.

Remediations to Consider

Consider using safeTransferFrom instead of transferFrom in VestingSimple::deposit.

When a position is trusted in the Registry, the Registry checks to make sure the position asset is supported by the PriceRouter.

However, if a UniswapV3 position is added to the Cellar and the second token in the UniV3 pair is not supported by the PriceRouter, no one will be able to call deposit(), withdraw(), mint(), or redeem(). This is because these functions need to calculate totalAssets, and totalAssets is found by finding the balance of every position in the Cellar. When finding the balance of a UniswapV3 position, the price of both the first token and the second token in the UniV3 pair are needed. Since the Registry never checks to make sure the second token in the pair is supported by the PriceRouter, if the PriceRouter does not support the token, these functions cannot be called and will revert.

Remediations to Consider

When trusting a UniswapV3 Position in registry.trustPosition(), check to make sure the second token in the UniV3 pair is also supported by the PriceRouter.

Also consider defining a standardized hook to allow Adaptor authors to define custom validation logic.

// Registry.sol:trustPosition

// RECOMMENDATION:
// 1. Move logic for "Check that asset of position is supported for pricing operations" from Registry.sol to BaseAdaptor.sol
//    It's more coherent this way.
// 2. Optionally, run validation logic custom to the adaptor.
if (!BaseAdaptor(adaptor).isPositionTrustworthy(positionData)) 
  revert ...

This issue is similar to H-1 and M-2 in that they both involve dividing, and then using the result from the division to multiply later.

When totalAssets of the cellar is calculated and the cellar has UniswapV3 positions, UniwswapV3Adaptor.balanceOf() is called to calculate the UniV3 position’s underlying worth in terms of the first token in the pair.

However, the amount of token1 is converted into the amount of token0 like this:

amount1.mulDivDown(price, 10**token1.decimals()

price is equal to (basePrice * 10**quoteAssetDecimals) / quotePrice as seen in PriceRouter._getExchangeRate()

So, amount1 is multiplied by price but that multiplication may lose precision due to Solidity division rounding down.

This will cause totalAssets to be undervalued. This is seen especially when the asset of the cellar has 18 decimals.

Remediations to Consider

Consider changing getExchangeRate in UniswapV3Adaptor.balanceOf() to something like:

(uint256 numerator, uint256 denominator) = PriceRouter(
  Cellar(msg.sender).registry().getAddress(
    PRICE_ROUTER_REGISTRY_SLOT())).getExchangeRate(token1, token0);

And then using the numerator and denominator when returning the balance:

return amount0 + (amount1 * numerator) / denominator / (10**token1.decimals());

Cellar funds should not be transferred into a VestingSimple contract, however that is currently possible. The total amount of damage is limited by allowedRebalanceDeviation .

Remediations to Consider

Consider implementing a check to assert that no value is being lost from the Cellar and transferred to a VestingSimple contract.

A strategist can transfer any token into the Cellar (bypassing callOnAdaptor), and then deposit those assets into the vesting contract, causing totalAssets() to increase as vesting occurs. Then, the strategist can withdraw those assets from the vesting contract back into the Cellar after they are vested. Since the asset is not an ERC20 position in the Cellar, totalAssets() will decrease, within the rebalance deviation.

LPs that deposited previously will now receive fewer assets than they are owed when they redeem/withdraw. The assets will be left in the Cellar.

Remediations to Consider

Consider asserting that a Vesting contract position had addPosition called already on the underlying ERC20. Also consider disallowing removePosition calls while that underlying ERC20 is in a Vesting contract.

The s.vested amount argument in emit Withdraw(msg.sender, depositIds[i], s.vested); is set to 0 a few lines before with s.vested = 0;

Remediations to Consider

Consider changing s.vested to shares when emitting Withdraw in VestingSimple::withdrawAll.

When cellars are initialized in CellarInitializable.initialize() , the holdingIndex is set. However, there is no check to ensure that the holdingIndex is within the range of the credit positions length. Therefore, if holdingIndex >= _creditPositions.length() , no one will be able to deposit into the cellar.

Remediations to Consider

In CellarInitializable.initialize(), consider checking to make sure that holdingIndex is less than _creditPositions.length()

The Withdraw event is declared as:

event Withdraw(address indexed user, uint256 depositId, uint256 amount);

The comment on line 29 states that the user parameter is the user receiving the deposit.

However, in the withdrawAnyFor() Withdraw event, the first argument is msg.sender, which is the owner of the deposit, not necessarily the receiver of the deposit.

Remediations to Consider

Consider modifying the event to have both the owner and the receiver as arguments.

Consider changing:

* From: https://github.com/Uniswap/v3-periphery/contracts/base/Multicall.sol

to

* From: https://github.com/Uniswap/v3-periphery/blob/1d69caf0d6c8cfeae9acd1f34ead30018d6e6400/contracts/base/Multicall.sol

The dependency contracts

1. Gravity Bridge
2. Swap Router
3. Price Router

are assigned fixed integer IDs in Registry.sol . The dependency Contract-ID relation does not change through the life of the registry. The dependency address can be changed. For example, Swap Router always has registry ID 2, but the Swap Router address (for implementation) can be changed.

However, not all dependency-IDs are named and only the Price Router has a constant human-readable ID - PRICE_ROUTER_REGISTRY_SLOT = 2 leaving room for readability improvement.

• Consider giving Swap Router and Price Router human-readable IDs to enhance readability. Consider defining immutable names following _register calls so there is a lower chance of changing _register order and forgetting to change the ID when refactoring:

constructor(
  address gravityBridge,
  address swapRouter,
  address priceRouter
) Ownable() {
  register(gravityBridge);
  GRAVITY_BRIDGE_REGISTRY_SLOT = 0; // added
  
  _register(swapRouter);
  SWAP_ROUTER_REGISTRY_SLOT = 1;    // added
  
  _register(priceRouter);
  PRICE_ROUTER_REGISTRY_SLOT = 2    // added
}

• Consider changing the usage of registry.getAddress to use human readable IDs. For example, registry.getAddress(registry.PRICE_ROUTER_REGISTRY_SLOT()) instead of PriceRouter(registry.getAddress(2)).

In PriceRouter.sol, line 936 declares getCurveV2DerivativeStorage.

However, this mapping is never used because whenever a CurveV2 derivative asset is added/set up, the underlying token addresses of the Curve pool are stored in getCurveDerivativeStorage instead of getCurveV2DerivativeStorage

Consider removing this unused mapping or using getCurveV2DerivativeStorage for CurveV2 assets.

In Uint32Array.sol, the comments mention uint256 integers and uint256[] arrays.

However, uint32 integers and uint32[] arrays are being used.

Consider switching the comments to reflect the appropriate integer/array.

CellarFactory.deploy deterministically deploys cellars using Open Zeppelin Clones and initializes the cellars in CellarInitializable.initialize(). Therefore, the initializations in the constructor of Cellar.sol are unnecessary. Consider removing them.

Since there is ongoing vesting, an LP is leaving guaranteed returns behind if they exit their position before the vesting is complete. This may not be immediately obvious to LPs, and may incentivize them to stay as LPs for longer.