Morpho Vaults v2 Fix Review

Description

when converting shares to assets to vice versa using convertToShares and convertToAssets call the corresponding preview functions which call the accrueInterestView() and finally the performance and management fee shares are used when deriving the final value. According to EIP-4626 these conversion endpoints:

MUST NOT be inclusive of any fees that are charged against assets in the Vault.

Recommendation

Either make sure the convertToShares and convertToAssets do not take into consideration the fees to perhaps a @dev NatSpec comment should be added to highlight this.

Cantina

NatSpec comments has been added.

Description

Currently there is only one role identified by isAllocator that can call:

• allocate
• setLiquidityAdapterAndData
• deallocate

The role is not fine-grained enough. It might make sense to define separate role for each of these endpoints as some are more critical that the others. Perhaps even an allocation role and a deallocation role.

Moreover the setIsAllocator endpoint is time locked. That means for both assigning and removing an entity from the allocation role, the curator and the vault might have to wait till the delegated executable timestamp after the intent of submitting to call setIsAllocator is reached. This can introduce potential problems when the curator wants to remove an allocation role for a potentially malicious entity. There are other operations related to change of configuration for the vault that when the operation would potentially expose the vault to some risks, that operation is time locked. But there are also the anti-operations (the reverse operations) where performing those operations would remove some potential risks from the vault. These reverse operations are not time locked currently and can be called/performed either by the curator or one of the sentinels. In that light, it would make sense that removing an entity from the allocation role should in theory remove some unwanted risk from the vault and thus, perhaps should not be behind a time lock.

Recommendation

1. Assign more segregated roles for the allocation endpoints.
2. Allow the curator or one of the sentinels to remove an allocator without going through a time-lock. Assigning an allocation role to an entity should still be time locked.
3. The curator should also be able to call deallocate.

If either of these recommendations are not applied, it might make sense to add documentation to expose these potential scenarios to the users and the devs.

Description

In allocate and deallocate if the newAllocation is 0:

1. Allocation flow

If the oldAllocation is also 0 and the marketParams is not in the marketParamsList, in the allocation flow it would not get added to the list. Assume the amount of assets provided is $a$ where the total assets and total supply for this market are $A$ and $S$ respectively. Then the shares minted would be $s = \left\lfloor \frac{a(S + 10^6)}{A+1} \right\rfloor$ and assume that $1 \leq s$, ie a non-zero share is minted for this allocation but the returned new allocation is $0$. That means:

Let:

We get:

from $(1)$ we can deduce that:

or:

Thus:

But $a$ cannot be $0$ since then $s$ would also end up being $0$. So $a$ should be $1$ and we should have:

and

Thus if $\frac{S + 10^6}{A + 1}\in (1,\infty) \setminus \mathbb{Z}$ and the asset to be allocated $a = 1$, a non-zero share would be minted, but the market parameter would not get added to the list, since right after supplying the value of those shares would be $0$ although in the future they might accumulate more value.

Also in some cases, one might have a non-zero old allocation for a market parameter, but the new allocation would turn $0$ and thus the market parameter would get removed from the list, even though there might have been some non-zero shares left.

2. Deallocation flow

Same as the allocation flow, it might be the case that the current new allocation might be $0$ but there are still non-zero shares left for the market parameter in the context.

Note that in this route market parameter could not get added to the list since for that to happen the old allocation must have been $0$ and the new allocation should end up non-zero. But after the deallocation flow in the adapter, in the main deallocation internal flow of the vault v2 we have:

for (uint256 i; i < ids.length; i++) {    Caps storage _caps = caps[ids[i]];    require(_caps.allocation > 0, ErrorsLib.ZeroAllocation());    _caps.allocation = (int256(_caps.allocation) + change).toUint256();}

which makes sure the must fine-grained unique allocation cap for this market paramter and adapter (ie the old allocation) must have been non-zero before getting updated to the new one.

So in the allocation flow, the only possible outcome is that the market parameter could only potentially get removed from the list. Addition is not possible in this flow (which also makes sense).

Recommendation

For both flows, one can see that a market parameter could potentially get removed from the marketParamsList even though for that paramter a non-zero share amount might have been left in the Morpho market contract. Therefore in the interest accural routes when the realAssets() of this adapter is called, the value of those left-over shares would not get accounted for.

Possible solutions would be to only remove the market params from the list if the share for the market parameter would end up being $0$ (this applies to both allocation and deallocation flows). Although having lots of market paramters that would have small values might add up a lot of gas when querying the realAssets() of this adapter.

Also note that if the values not considered for the removed market parameters in question become significant on the next call to allocate they would get added back to the list even if one calls allocate with $0$ assets.

In the allocation flow, it might also make sense to revert if newAllocation is 0. This suggestion does not apply for the deallocation flow.

Morpho

Acknowledged. Related comment has been added in PR #733.

Footnote

The above logic also applies to MorphoVaultV1Adapter where the new allocation might be 0 but the adapter might still have non-zero shares in the yield source. And at a future timestamp the call to IERC4626(morphoVaultV1).previewRedeem(IERC4626(morphoVaultV1).balanceOf(address(this))) might return a non-zero value. But currently the decision to make this call or not in realAssets() is based on the last updated allocation which would have a stale value.

Description

In some cases when the IMorpho(morpho).supply(...) is called $0$ shares could be minted for the MorphoMarketV1Adapter. Thus in these cases the assets provided would get donated to Morpho.

Recommendation

Use the returned values from IMorpho(morpho).supply(...) to make sure there are non-zero shares minted.

Description

In the mint and deposit flow of the VaultV2 there might be cases where the share to be minted for the user might end up being 0 even when the provided asset is non-zero.

Recommendation

It would be best to revert in enter if shares is 0.

Description

In the NatSpec comments we have:

/// FIRST TOTAL ASSETS/// @dev The variable firstTotalAssets tracks the total assets after the first interest accrual of the transaction./// @dev Used to implement a mechanism that prevents bypassing relative caps with flashloans./// @dev This mechanism can generate false positives on relative cap breach when such a cap is nearly reached,/// for big deposits that go through the liquidity adapter.

The comment is not quite accurate. We have:

function accrueInterest() public {    (uint256 newTotalAssets, ...) = accrueInterestView();    // ...    _totalAssets = newTotalAssets.toUint128();    if (firstTotalAssets == 0) firstTotalAssets = newTotalAssets;    // ...    lastUpdate = uint64(block.timestamp);}
function `accrueInterestView()` ... {    if (firstTotalAssets != 0) return (_totalAssets, 0, 0);    //...
    return (newTotalAssets, ...);}

And so the transient storage parameter firstTotalAssets at any specific point in the call frame tree of a root transaction is either 0 or the first non-zero newTotalAssets in the ordered call-tree frame. Thus it could be 0 for a few frames and then as soon as it becomes non-zero it would stay the same value and the calls to accrueInterestView() would return early from this point on.

• Example 1: assume $A_0$ is a non-zero value. Initially in the beginning of the root tx $T$, one starts with $A_0$ as the _totalAssets, then the maybe due to some losses, newTotalAssets becomes 0 and thus for all the following call frames if we enter back into accrueInterestView() and accrueInterest() both _totalAssets and newTotalAssets stay 0, except somehow we increase _totalAssets to a non-zero value. This would only be possible through mint or deposit at this point. After this firstTotalAssets would be set to a non-zero value:

• Example 2: This is like example 1. but at the a state where there are no total assets in the vault due to perhaps using a vault that is freshly deployed or perhaps due to the fact that all previous share holders have exited the vault.

As one can see in both examples the value of the firstTotalAssets is not the value new total assets calculated in the first visit of accrueInterestView() in the whole call-frame tree of a root transaction. Moreover until this value is set to a non-zero value the flow in accrueInterestView() performs all the operations without exiting early.

function allocateInternal(...) internal {    // ...
    accrueInterest();
    // ...
    for (...) {        // ...
        require(... ||            _caps.allocation <= firstTotalAssets.mulDivDown(_caps.relativeCap, WAD),        ...);    }}

The value of firstTotalAssets is used to limit the relative cap during an allocation process. Thus due to above the correct value is not used in the above examples. The value that should have been used, must have been $0$. But currently the last non-zero value is used.

Recommendation

One can store a flag in firstTotalAssets to indicate that the call-tree flow has gone through the accrueInterest() process even though the new total assets could be $0$. In all cases, we can set firstTotalAssets to store the returned value from accrueInterestView() in its [1:255] bits and the first bit would be flag bit indicating that accrueInterest() was called.

Here is a rough patch:

diff --git a/src/VaultV2.sol b/src/VaultV2.solindex c86ec7f..90e68a4 100644--- a/src/VaultV2.sol+++ b/src/VaultV2.sol@@ -518,8 +518,10 @@ contract VaultV2 is IVaultV2 {              require(_caps.absoluteCap > 0, ErrorsLib.ZeroAbsoluteCap());             require(_caps.allocation <= _caps.absoluteCap, ErrorsLib.AbsoluteCapExceeded());++            uint256 cleanedFirstTotalAssets = firstTotalAssets >> 1;             require(-                _caps.relativeCap == WAD || _caps.allocation <= firstTotalAssets.mulDivDown(_caps.relativeCap, WAD),+                _caps.relativeCap == WAD || _caps.allocation <= cleanedFirstTotalAssets.mulDivDown(_caps.relativeCap, WAD),                 ErrorsLib.RelativeCapExceeded()             );         }@@ -561,10 +563,11 @@ contract VaultV2 is IVaultV2 {     /* EXCHANGE RATE FUNCTIONS */      function accrueInterest() public {+        if (firstTotalAssets != 0) return;         (uint256 newTotalAssets, uint256 performanceFeeShares, uint256 managementFeeShares) = accrueInterestView();         emit EventsLib.AccrueInterest(_totalAssets, newTotalAssets, performanceFeeShares, managementFeeShares);         _totalAssets = newTotalAssets.toUint128();-        if (firstTotalAssets == 0) firstTotalAssets = newTotalAssets;+        if (firstTotalAssets == 0) firstTotalAssets = (newTotalAssets << 1) + 1;         if (performanceFeeShares != 0) createShares(performanceFeeRecipient, performanceFeeShares);         if (managementFeeShares != 0) createShares(managementFeeRecipient, managementFeeShares);         lastUpdate = uint64(block.timestamp);

Moreover, one can modify the accrue interest flow even further by strategically returning early if the last timestamp has not changed:

diff --git a/src/VaultV2.sol b/src/VaultV2.solindex c86ec7f..12aefde 100644--- a/src/VaultV2.sol+++ b/src/VaultV2.sol@@ -518,8 +518,10 @@ contract VaultV2 is IVaultV2 {              require(_caps.absoluteCap > 0, ErrorsLib.ZeroAbsoluteCap());             require(_caps.allocation <= _caps.absoluteCap, ErrorsLib.AbsoluteCapExceeded());++            uint256 cleanedFirstTotalAssets = firstTotalAssets >> 1;             require(-                _caps.relativeCap == WAD || _caps.allocation <= firstTotalAssets.mulDivDown(_caps.relativeCap, WAD),+                _caps.relativeCap == WAD || _caps.allocation <= cleanedFirstTotalAssets.mulDivDown(_caps.relativeCap, WAD),                 ErrorsLib.RelativeCapExceeded()             );         }@@ -561,10 +563,15 @@ contract VaultV2 is IVaultV2 {     /* EXCHANGE RATE FUNCTIONS */      function accrueInterest() public {+        if (firstTotalAssets != 0) return;+        if (lastUpdate == block.timestamp) {+            firstTotalAssets = (_totalAssets << 1) + 1;+            return;+        }         (uint256 newTotalAssets, uint256 performanceFeeShares, uint256 managementFeeShares) = accrueInterestView();         emit EventsLib.AccrueInterest(_totalAssets, newTotalAssets, performanceFeeShares, managementFeeShares);         _totalAssets = newTotalAssets.toUint128();-        if (firstTotalAssets == 0) firstTotalAssets = newTotalAssets;+        if (firstTotalAssets == 0) firstTotalAssets = (newTotalAssets << 1) + 1;         if (performanceFeeShares != 0) createShares(performanceFeeRecipient, performanceFeeShares);         if (managementFeeShares != 0) createShares(managementFeeRecipient, managementFeeShares);         lastUpdate = uint64(block.timestamp);@@ -575,8 +582,12 @@ contract VaultV2 is IVaultV2 {     /// @dev The performance and management fees are taken even if the vault incurs some losses.     /// @dev Both fees are rounded down, so fee recipients could receive less than expected.     function accrueInterestView() public view returns (uint256, uint256, uint256) {-        if (firstTotalAssets != 0) return (_totalAssets, 0, 0);         uint256 elapsed = block.timestamp - lastUpdate;++        if (elapsed == 0 || firstTotalAssets != 0) {+            return (_totalAssets, 0, 0);+        }+         uint256 realAssets = IERC20(asset).balanceOf(address(this));         for (uint256 i = 0; i < adapters.length; i++) {             realAssets += IAdapter(adapters[i]).realAssets();

Cantina

Client has added the following comment in 2ed9336b71dcac440a98a47348475b3ff027a940:

@dev The behavior of firstTotalAssets is different when the vault has totalAssets=0, but it does not matter internally because in this case there are no investments to cap.

Footnote

call flow diagram

flowchart LR

    AI(accrueInterest) --> AIV(accrueInterestView);
    totalAssets --> AIV;
    previewDeposit --> AIV;
    previewMint --> AIV
    previewWithdraw --> AIV;
    previewRedeem --> AIV;

    convertToShares --> previewDeposit;

    deposit --2--> previewDeposit;
    mint --2--> previewMint;
    withdraw --2--> previewWithdraw;
    redeem --2--> previewRedeem;
    
    allocate --> allocateInternal;
    enter --> allocateInternal;

    convertToAssets --> previewRedeem;

    setMaxRate --> AI;
    setPerformanceFee --> AI;
    setManagementFee --> AI;
    setPerformanceFeeRecipient --> AI;
    setManagementFeeRecipient --> AI;
    allocateInternal --> AI;

    deposit --1--> AI;
    mint --1--> AI;
    withdraw --1--> AI;
    redeem --1--> AI;

    forceDeallocate --2--> withdraw;

    deposit --3--> enter;
    mint --3--> enter;
    withdraw --3--> exit;
    redeem --3--> exit;

    exit --> deallocateInternal;
    forceDeallocate --1--> deallocateInternal;
    deallocate --> deallocateInternal



    style AI stroke: orange, stroke-width: 4px;
    style AIV stroke: orange, stroke-width: 4px;

    style enter stroke: green, stroke-width: 4px;
    style exit stroke: red, stroke-width: 4px;

    linkStyle 20 stroke: red, stroke-width: 1px;
    linkStyle 21 stroke: red, stroke-width: 1px;
    linkStyle 22 stroke: red, stroke-width: 1px;
    linkStyle 23 stroke: red, stroke-width: 1px;

    linkStyle 24 stroke: orange, stroke-width: 1px;

    linkStyle 7 stroke: orange, stroke-width: 1px;
    linkStyle 8 stroke: orange, stroke-width: 1px;
    linkStyle 9 stroke: orange, stroke-width: 1px;
    linkStyle 10 stroke: orange, stroke-width: 1px;

    linkStyle 25 stroke: blue, stroke-width: 1px;
    linkStyle 26 stroke: blue, stroke-width: 1px;
    linkStyle 27 stroke: blue, stroke-width: 1px;
    linkStyle 28 stroke: blue, stroke-width: 1px;

Related Competition Findings:

And a few other ones that are not comepletely related. Search is based on looking for firstTotalAssets when on the finding page and pressing CMD + K.

related test: testRelativeCapManipulationProtection

Description

Some of the allocation ids returned are coarse allocation ids corresponding to multiple allocation routes. For example the allocation id for a specific collateral or the allocation id for the morpho market adapter.

Now assume a malicious or buggy adapter returns a very high allocation change (even something like type(uint128).max ), if that happens one would not be able to allocate any funds to any other allocation id $j$ such that $R_j$ has an id in $R_i$ where $i$ is the allocation used in the buggy adapter since the new allocation including the change would be bigger than in value that could fit into a uint128 and thus bigger than $c_{i_f}^{abs}$.

An example would be for the specific collateral $C$ id, if the above scenario happens. The vault would not be able to allocate to any market using any morpho adapter if the market parameters would use that specific collateral.

Recommendation

Perhaps bound-checks could be performed on the allocation changes. The checks mixed with some time and estimate analysis should be performed to make sure the above issue could not happen within a certain time range. But even this might not cover all the potential cases.

But also note that the above could only happen in some extreme cases when the values $c_i^{abs}$ were already at or close to maximum. If they were low enough, one could remove the buggy adapter and add to the caps for the any incorrect amount of allocation change was introduced by the buggy adapter. This change would need to be applied to all $j$ such that $R_j \cap R_i \neq \emptyset$.

Description/Recommendation

Events

1. MorphoMarketV1AdapterFactory.sol#L20: Also emit morpho.
2. VaultV2.sol#L552: The returned ids parameter from deallocateInternal is only used in forceDeallocate for the event ForceDeallocate(..., ids, ...) but this parameter is also emitted in deallocateInternal's Deallocate(..., ids, ...) event and thus it can be deduced from the earlier Deallocate event. If it is not necessary to also re-emit this parameter in ForceDeallocate. The ids parameter can be removed from ForceDeallocate and also deallocateInternal would not need to return this value.

Cantina

1. has been fixed in PR #723.
2. has been acknowledged. The client has added the ids in the event of forceDeallocate because it helps indexing (recovering the right associated deallocate isn't that easy) on purpose.

Description/Recommendation

In the current two implementation of the adapters:

• MorphoMarketV1Adapter
• MorphoVaultV1Adapter

If at some point tokens are donated to these adapters the donated tokens would would not get used during the allocation and dellocation flows the assets for this flows are transiently moved to and from the adapter in atomic transactions from the parent vault.

Any donated assets can be skimmed out of these adapters using the skimRecipient set by the parent vault owner.

The exceptions are:

• For MorphoVaultV1Adapter if morphoVaultV1 tokens/shares are donated they cannot be skimmed out and their values will be accounted for in the next allocation, deallocation or realAssets() query.
• Any donated tokens to MorphoMarketV1Adapter would not affect any of the accountings and can be skimmed out. Note that asset/funds can be directly supplied (donated) for loan or collateral tokens on behalf of the adapter in Morpho. That would affect only the morpho market accountings that have been used by the privileged allocators in the VaultV2. Donations to unallocated market parameters would also not change any of the accountings in the current context in VaultV2.

Description

With the new implementation of VaultV2, this vault is not a fixed interest type vault where those fixed interests are dripped into that can be shared. The current type could be called a bounded variable interest vault where the upper bound in on the variable interest is determined by maxRate.

Also there are no safeguards regarding the max loss rate.

Description

Currently both adapter implementations give unlimited approvals to the:

• parent vault
• and their yield source

1. In the allocation flow the approval for the asset token to the yield source is needed, since the yield source supply/deposit endpoints would need to transfer assets/tokens on behalf of the adapter.
2. In the deallocation flow, the deallocated assets are not transferred back to the parent vault. Instead the parent vault afterwards pull/transfer those deallocated tokens/assets from the adapter to itself afterwards.

Recommendation

One needs to evaluate these unlimited approval decisions. Alternatives would be:

1. The adapter would give atomic approvals equal to the amount of assets it would want to supply/deposit into the yield source and afterwards to make sure to reset the approval to 0. This would eliminate potential cases where unnecessary approvals can be used in the future to pull more assets in some unexpected ways from the adapter. Currently such cases are not possible. Note this change would introduce extra gas cost for the allocation flow.
2. The adapter can transfer the deallocated assets in its deallocate endpoint and thus giving unlimited approval to the parent vault would not be necessary. Applying this change would put more trust into the adapter, but also currently the parent vault has no way of pulling unused extra tokens from the adapters (they can only be skimmed). This change would also introduce a change into the order of external call flows and storage checks updates for allocation amounts in the parent vault, thus extra caution and analysis would need to be applied.

Description

In both adapter implementations and also in general, one should try to if possible overestimate the new allocation value in the allocation flows. That way the allocation upper bounds used with the absolute and relative caps would be more strict.

Currently, in the implemented adapters endpoints are used that might slightly underestimate the values of the allocated yield sources. Thus the endorsed upper bounds might not be as accurate as one might hope.

Recommendation

To combat the above issue, the curator could enforce a slightly stricter upper bound. Although this might not be a solution that would work in all cases. Moreover, it would be great to document the above so the curators would be aware when adding new adapters to be used during the allocation and deallocation flows.

Description

In this finding we use the notations from the following finding from a previous review:

• Finding #14 Incorrect allocation calculation for ids corresponding to coarser set of routes.

For a coarse id $i$ we have:

an example of a coarse id would be:

• allocation id for a specific collateral
• allocation id for the morpho market adapter (without identifying the specific market parameters)

In each call to the current implementations of the IAdapter, IAdapter(adapter).allocate(...) only the change in allocation of one specific fine-grained allocation id is returned back, let's call this id $i_{\star}$. Then the allocations for the ids returns by the call get updated one by one and for each id $I$ only the term related to the specific $i_{\star}$ gets updated. The other terms for the coarse id allocation sum keep their stale value. ie:

All the red terms above are stale. Thus the value $a_i^{new}$ might be lower that its actual value if all the stale terms would have been updated to their live value. Thus the upper bound checks performed are not accurate.

The only accurate upper bound checks would be only for the only fine-grained id $i_{\star}$, ie $a_{i_{\star}}$

For example the upper bound checks performed for an id to corresponds to a collateral $C$ which is used in two different market parameters would not be accurate as only of the terms would be stale. Whereas the upper bound check performed for the specific market parameter of the morpho adapter which was used for the allocation call would be accurate.

Recommendation

The above can only be better documented. Applying a fix could be costly and complicated.