Security Audit
January 11, 2023
Version 1.0.0
Presented by 0xMacro
This document includes the results of the security audit for Sommelier Finance's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from November 28, 2022 to December 23, 2022.
The purpose of this audit is to review the source code of certain Sommelier Finance 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 | 5 | - | - | 5 |
Medium | 8 | 3 | - | 5 |
Low | 2 | - | - | 2 |
Code Quality | 7 | 2 | - | 5 |
Sommelier Finance was quick to respond to these issues.
Our understanding of the specification was based on the following sources:
The following source code was reviewed during the audit:
760dc9cff074b379f1fd18b3d5c646761050ab34
Specifically, we audited the following contracts within this repository:
Contract | SHA256 |
---|---|
src/CellarFactory.sol |
|
src/CellarRouter.sol |
|
src/Registry.sol |
|
src/base/Cellar.sol |
|
src/base/CellarInitializable.sol |
|
src/base/ERC20.sol |
|
src/base/ERC4626.sol |
|
src/base/Multicall.sol |
|
src/base/SafeTransferLib.sol |
|
src/interfaces/ICellarRouter.sol |
|
src/interfaces/ICellarRouterV1_5.sol |
|
src/interfaces/ICellarStaking.sol |
|
src/interfaces/ICellarV1_5.sol |
|
src/interfaces/IMulticall.sol |
|
src/interfaces/external/DataTypes.sol |
|
src/interfaces/external/IAaveToken.sol |
|
src/interfaces/external/IChainlinkAggregator.sol |
|
src/interfaces/external/ICurveFi.sol |
|
src/interfaces/external/ICurvePool.sol |
|
src/interfaces/external/IGravity.sol |
|
src/interfaces/external/IPool.sol |
|
src/interfaces/external/IPoolAddressesProvider.sol |
|
src/interfaces/external/IUniswapV2Router02.sol |
|
src/interfaces/external/IUniswapV3Router.sol |
|
src/modules/adaptors/Aave/AaveATokenAdaptor.sol |
|
src/modules/adaptors/Aave/AaveDebtTokenAdaptor.sol |
|
src/modules/adaptors/BaseAdaptor.sol |
|
src/modules/adaptors/Compound/CTokenAdaptor.sol |
|
src/modules/adaptors/ERC20Adaptor.sol |
|
src/modules/adaptors/Sommelier/CellarAdaptor.sol |
|
src/modules/adaptors/UniSwap/UniswapV3Adaptor.sol |
|
src/modules/adaptors/VestingSimpleAdaptor.sol |
|
src/modules/price-router/PriceRouter.sol |
|
src/modules/swap-router/SwapRouter.sol |
|
src/modules/vesting/VestingSimple.sol |
|
src/utils/Math.sol |
|
src/utils/SigUtils.sol |
|
src/utils/Uint32Array.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.
Cellar::_withdrawInOrder
calculations round down due to division before multiplication, which may leave LP’s assets in the Cellar
redeem
, following a strategist call to callOnAdaptor
that swaps into an untracked asset
holdingIndex
minHealthFactor
is Strategist controlled, potentially making Cellar Aave accounts liquidatable
AaveATokenAdaptor:withdraw
confuses LP for Cellar
PriceRouter::getValue()
may return a different result from PriceRouter::getValues()
due to order of operations and division before multiplication
AaveATokenAdaptor::withdrawableFrom
and CTokenAdaptor::withdrawableFrom
may lose precision
VestingSimple::deposit
calls ERC20(asset).transferFrom
without checking return value
totalAssets()
to be undervalued as well
Cellar
funds from being used to seed a VestingSimple
contract
VestingSimple::withdrawAll
always emits Withdraw
event with amount
as 0
intialize
, no one can deposit into the Cellar
VestingSimple::withdrawAnyFor
’s Withdraw
event does not conform to comments
Multicall.sol
Registry.sol
getCurveV2DerivativeStorage
is an unused mapping in PriceRouter.sol
Uint32Array.sol
comments are misleading
constructor
in Cellar.sol does not initialize anything
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. |
Cellar::_withdrawInOrder
calculations round down due to division before multiplication, which may leave LP’s assets in the Cellar
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
Consider using a high internal scaling factor for fixed point numbers so precision loss becomes acceptable
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.
_withdrawInOrder
has been updated to use a scaling factor, thePRECISION_MULTIPLIER
introduced on line 840 in Cellar.sol. These multipliers are used in thewithdrawInOrder
accounting logic (lines 866-893). This approach also necessitated the creation of a new struct,WithdrawPricing
(lines 827-835) in order to get around stack-too-deep errors in the fixed implementation.
redeem
, following a strategist call to callOnAdaptor
that swaps into an untracked asset
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:
AttackCoin
. Only the attacker’s EOA holds AttackCoin
.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.callOnAdaptor
to swap the base asset for AttackCoin
. The value will be the max amount allowed through allowedRebalanceDeviation
.AttackCoin
for the base asset and remove all remaining liquidity from pool.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.
The discussed attack vector is the effect of a deliberate acceptance of design trade-offs when designing an architecture for Cellar adaptors that allows arbitrary function execution (within the functionality of a cellar’s already-set-up adaptors).
The design choice here is that the Cellar itself should not have any knowledge about the purpose and/or logic of adaptor calls - the alternative would introduce the need for a complex system of interdependent, end-protocol-aware checks within Cellar logic itself. Therefore, the context-aware suggested remediation is not appropriate for this architecture.
In general, the
totalAssets
check and theallowedRebalanceDeviation
are the core on-chain guardrails against misappropriation of cellar funds via adaptor calls. Off-chain, the Sommelier chain’s Steward architecture has and will continue to build capabilities for detecting, and blocking, malicious-seeming strategist calls. Steward can use a wide range of heuristics not available on-chain, such as “close” approaches to the rebalance deviation, unexpected tokens in decoded parameters, and so on. Given that Steward can detect and block malicious activity as described, the security model basically allows a malicious strategist to get one “free” malicious execution, within the rebalance deviation, before misappropriation is detected and stopped.Sommelier believes this is an acceptable trade-off of risk vs. system complexity, given that default allowed rebalance deviations have also been reduced significantly in response to this report (see line 1179 of Cellar.sol). As allowed rebalance deviations decrease, and the TVL of a given Cellar increases, it’s likely that any single-time exploitation of the rebalance deviation would not be game-theoretically “worth it” compared to continuing to operate a large TVL cellar honestly and earning fees. These off-chain, aligned-incentive considerations could also create room for us increasing allowed rebalance deviations for given trusted cellars, which is a desired feature we may want to employ in the future.
holdingIndex
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:
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.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:
removePosition
consider checking if creditPositions[toRemoveindex]
refers to holdingPosition
instead and revert if neededswapPositions
because the notion of the holding position is represented as a name instead of an indexAs suggested, use of
holdingIndex
has been changed to use ofholdingPosition
, with checks modified to check thepositionId
for removal of the holding position (line 239-240 of Cellar.sol), andswapPositions
code dealing with the holding position removed.
minHealthFactor
is Strategist controlled, potentially making Cellar Aave accounts liquidatable
(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:
addPosition
for an aToken
position with 0.1 as the minHealthFactor
[1] [4]aToken
position. The withdrawal causes healthFactor
to be close to below 1 e.g. 1.0000001 [2][3]Remediations to Consider
Consider making position configuration data configurable by Governance instead of by Strategists to ensure minHealthFactor
is sufficiently conservative.
Notes
healthFactor
by withdrawing collateral and H-5 does it by borrowing moreminHealthFactor
, so an excessive withdrawal of Aave collateral can happenA constant minimum health factor,
HFMIN
, has been added to lines 71-73 of AaveATokenAdaptor.sol and lines 51-53 of AaveDebtTokenAdaptor.sol. This value is checked against the strategist-setminHealthFactor
on lines 121-123 of AaveATokenAdaptor.sol and lines 140-142 of AaveDebtTokenAdaptor.sol, putting a hard lower bound on the minimum AAVE health factor and preventing the strategist from liquidating themselves.
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
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.
AaveATokenAdaptor
. Therefore, setting the health factor in 2 different places can lead to inconsistent configuration.Cellar
. Consider to be careful with having inconsistent configuration when it comes to a credit and debt adaptor pair.cellar:callOnAdaptor
to AaveDebtTokenAdaptor:borrowFromAave
does not allow for passing configuration data. Consider adding a configurable limit for borrowing.Notes
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.While we’ve added a lower bound to the min health factor as described in [H-4], we found no additional action was needed for this scenario, and believe the likelihood to be much lower than the reported “Medium”, due to the following mitigating factors:
The hypothetical oracle price drop described in the issue (1%) is double that of Chainlink’s widest deviation threshold, which is 0.5%, making the opportunity less profitable.
The attack requires the strategist/attacker to submit an adaptor call to borrow with extremely precise timing, such that it falls in the same block as the pending oracle update. In practice, strategists have no guarantees about same-block execution - the Sommelier chain needs to perform its own consensus process before submission, as well as Steward validation.
Since AAVE conducts partial liquidation, the upside of such an attack is limited to the size of the oracle deviation plus the liquidation penalty, and could only be performed once under very specific conditions.
AaveATokenAdaptor:withdraw
confuses LP for Cellar
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:
However, withdraw
uses msg.sender
for fetching the health factor meaning msg.sender
will be the LP because Cellar delegatecall
s 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:
minHealthFactor
) but the Cellar’s health factor is good.Remediations to Consider
msg.sender
to address(this)
to correctly represent the cellar’s address in AaveATokenAdaptor:withdraw
delegatecall
and staticcall
context to reduce the amount of mental translation needed for the readers_cellarDelegateCaller
as mutative even it is not, so that using _cellarDelegateCaller
in a non-mutative context will be a compile-time errorfunction _cellarDelegateCaller() internal() returns (address);
i.e. address(this)
function _cellarStaticCaller() internal view() returns (address);
i.e. msg(sender)
References
The
withdrawFromAave
function in AaveATokenAdaptor.sol was updated to use the correct address on line 250.
PriceRouter::getValue()
may return a different result from PriceRouter::getValues()
due to order of operations and division before multiplication
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:
_getPriceInUSD()
getExchangeRate
executes basePrice.mulDivDown(10**quoteAssetDecimals, quotePrice);
amount.mulDivDown(getExchangeRate(baseAsset, quoteAsset), 10baseAsset.decimals());
getValues()
order:
_getPriceInUSD()
_getValues()
executes (amounts[i].mulDivDown(price, 10**baseAsset.decimals()));
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()
.
The Price Router was updated in multiple areas of PriceRouter.sol to use more consistent math and a common internal logic function,
_getValueInQuote
.
AaveATokenAdaptor::withdrawableFrom
and CTokenAdaptor::withdrawableFrom
may lose precision
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
withdrawableFrom
similarly outlined in H-1.The Sommelier team couldn’t detect a significant issue in either of the mentioned adaptors, and no code changes were made.
In
AaveATokenAdaptor
, any loss of precision inwithdrawableFrom
will lead to an undercounting, meaning that no unintended liquidations could occur from withdrawing too much, and any amount “stuck” would not be significant.In
CTokenAdaptor
, we found no way to eliminate the division operation inwithdrawableFrom
without adding a common scaling factor to all adaptor code, which we felt was undesirable due to the “least-knowledge” cellar/adaptor design also described in [H-2].We found both losses of precision to be extremely insignificant (on the order of needing millions of tokens in order to lose a single base unit to truncation), and not worth fixing given that the suggested remediations result in a degraded architecture.
VestingSimple::deposit
calls ERC20(asset).transferFrom
without checking return value
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
.
This was fixed on line 178 of VestingSimple.sol.
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 was fixed by adding an
assetsUsed
field to the common adaptor interface (see base implementation on lines 113-119 of BaseAdaptor.sol).UniswapV3Adaptor
then overrode theassetUsed
function to report both LP tokens (lines 181-189), which were stored in the previously-definedadaptorData
.Finally,
assetsUsed
was added to the price router support check on lines 274-279 ofRegistry.sol
, ensuring that any asset used based on the adaptor’s own reporting is supported by the price router.
totalAssets()
to be undervalued as well
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());
A scaling factor,
precisionPrice
, was added to thebalanceOf
calculation in UniswapV3Adaptor.sol (lines 93-170). This value is derived from the decimals of the token.
Cellar
funds from being used to seed a VestingSimple
contract
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.
As discussed in [H-2] above, there are myriad ways to misappropriate assets within the
allowedRebalanceDeviation
, and any usage of supported adaptors to perform such an action does not require remediation. The Sommelier team is aware of its chosen security trade-offs and believes the combination of on-chain and off-chain security architecture described in the [H-2] response to be the optimal approach for its use case.
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.
Similar to [H-2] above, the suggested remediation does not adhere to the design principles of cellars vis-a-vis their adaptors, and as discussed in both [H-2] and [M-7], any risk from a malicious strategist where the impact is bounded by the rebalance deviation does not require remediation. Congruent to the mentioned issues, the Sommelier team found no required changes here.
VestingSimple::withdrawAll
always emits Withdraw
event with amount
as 0
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
.
This was resolved by storing a
vested
value before resetting the storage slot to 0, on line 238 of VestingSimple.sol, and using thevested
value for event emission on line 248.
intialize
, no one can deposit into the Cellar
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()
Resolved via use of
holdingPosition
, as described by the response to [H-3].
VestingSimple::withdrawAnyFor
’s Withdraw
event does not conform to comments
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.
Resolved by adding a fourth parameter to the event signature, for both the
user
(the owner of the deposit) andreceiver
(the address receiving the withdrawal).In addition, this parameter separation was added to the deposit event, and both events were renamed to
VestingWithdraw
andVestingDeposit
respectively, in order to eliminate overlap with the ERC4626-compliantDeposit
andWithdraw
events of the cellar.
Multicall.sol
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 specified link was updated on line 9 of Multicall.sol.
Registry.sol
The dependency contracts
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.
_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
}
registry.getAddress
to use human readable IDs. For example, registry.getAddress(registry.PRICE_ROUTER_REGISTRY_SLOT())
instead of PriceRouter(registry.getAddress(2))
.All calls to
registry.getAddress
in Cellar.sol were updated to use readable constants instead of integers - lines 497 and 1402.
getCurveV2DerivativeStorage
is an unused mapping in PriceRouter.sol
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.
The specified mapping was deleted from PriceRouter.sol.
Uint32Array.sol
comments are misleading
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.
All comments were updated to reference
uint32
in Uint32Array.sol.
constructor
in Cellar.sol does not initialize anything
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.
No code changes were made - the Sommelier team desires that Cellar.sol remain a standalone deployable contract, independent from use of the factory. Both factory and direct deployments should be supported.
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.
The Sommelier team will consider how to address this in user-facing documentation, with no immediate changes made to in-protocol documentation.
In general, the Sommelier team doesn’t see exiting LPs as “losing guaranteed returns” - rather there exists a balance between the vesting returns LPs give up when exiting, vs. the “unearned” vesting returns they receive from previous vesting deposits before their own LP deposit. As such, we don’t operate as if there is a conceptual link between the tokens pending in vesting, and any LP funds that may have previously been used to “earn” those tokens.
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 Finance 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.