Update last total assets in _setCap

[Jean-Grimal]

Fixes https://cantina.xyz/ai/8409a0ce-6c21-4cc9-8ef2-bd77ce7425af/findings/8b4fe500-afc7-43a2-9c14-d2ca13c5ae0a

[adhusson]

I find the lastTotalAssets = lastTotalAssets + supply(vault)update suspicious. It looks like the start of a loop that inflates lastTotalAssets. There is also no update of lastTotalAssets when supplyCap == 0.

Suggestion: unconditionally call _updateLastTotalAssets(totalAssets()) at the end of every _setCap. Unless _setCap is called very often I think it's worth the gas cost.

In that case the assert in testEnableMarketWithLiquidity should become:

uint depositedWithInterest = morpho.expectedSupplyAssets(allMarkets[0],address(vault));
assertEq(vault.lastTotalAssets(), depositedWithInterest + additionalSupply);

[Jean-Grimal]

Suggestion: unconditionally call _updateLastTotalAssets(totalAssets()) at the end of every _setCap. Unless _setCap is called very often I think it's worth the gas cost.

I first used _updateLastTotalAssets(totalAssets()) as you can see here, but the gas savings are worth it imo.

In that case the assert in testEnableMarketWithLiquidity should become:

uint depositedWithInterest = morpho.expectedSupplyAssets(allMarkets[0],address(vault));
assertEq(vault.lastTotalAssets(), depositedWithInterest + additionalSupply);

I think assertEq(vault.lastTotalAssets(), depositedWithInterest + additionalSupply); and assertEq(vault.lastTotalAssets(), deposited + additionalSupply); are equivalent

[MerlinEgalite]

There is also no update of lastTotalAssets when supplyCap == 0.

Was there a reason not to do that @morpho-org/onchain-reviewers? I can't remember

[Rubilmax]

I find the lastTotalAssets = lastTotalAssets + supply(vault) update suspicious. It looks like the start of a loop that inflates lastTotalAssets. There is also no update of lastTotalAssets when supplyCap == 0.

Suggestion: unconditionally call _updateLastTotalAssets(totalAssets()) at the end of every _setCap. Unless _setCap is called very often I think it's worth the gas cost.

Do you mean it could open up a vulnerability?
Because at the same time, it looks totally correct to me: increasing the cap of a market adds enables this market (if it wasn't enabled before), thus adds it to the withdraw queue and the assets supplied in this market will be counted in the total assets, but should not be taxed (hence why we increase lastTotalAssets)

There is also no update of lastTotalAssets when supplyCap == 0.

Was there a reason not to do that @morpho-org/onchain-reviewers? I can't remember

Because even if a cap is zero, the vault has access to this supply hence why the total assets are not decreased (and why lastTotalAssets shouldn't be decreased either)

[adhusson]

The idea was that lastTotalAssets could be artificially inflated by alternating calls to 1) setCap on a market, with a nonzero cap, and 2)updateWithdrawQueue such that the vault removes the market but maintains nonzero assets in the market. Then calling lastTotalAssets = lastTotalAssets + morpho.supply(market,vault) would have double-counted the assets owned by the vault in the market.

I couldn't see a way to make the loop work without timelocks during which lastTotalAssets could be reset, and anyway #337 will prevent the "vault is still counting its assets in a removed market" situation. So I think it's fine.

[MathisGD]

I'm not super convinced about this issue. Why shouldn't a donation be taxed by the fee? And for me a removed market is lost funds, thus it's weird to modify the logic for this edge case.

[QGarchery]

I'm not super convinced about this issue. Why shouldn't a donation be taxed by the fee? And for me a removed market is lost funds, thus it's weird to modify the logic for this edge case.

I believe this comment also answers this question

[Rubilmax]

In the end I'm fine taxing twice temporarily lost funds, and I'm also fine to prevent taxing it because the cost is not much

[MathisGD]

I still find it weird to merge this PR without having #337

[StErMi]

Consider avoiding executing this function if MORPHO.expectedSupplyAssets(marketParams, address(this)) is equal to zero.

I would say that the normal scenario should be that you are enabling a market where no one has supplied on behalf of MM.

[MerlinEgalite]

True, but it would save only ~2k gas since we still need to call expectedSupplyAssets. In the current configuration we're adding one cold SLOAD and one warm SSTORE that is not changing the value. So no sure it's worth considering this case.

[MerlinEgalite]

I'm not sure it's worth it considering this case. The current implementation only adds one cold SLOAD and one warm SSTORE so around 2k gas.

[StErMi]

If not documented yet, consider documenting the side effects of this operation. The share value (of MM) is increased.

[StErMi]

Should there be a check that reverts if the market is broken?

MORPHO.expectedSupplyAssets won't revert if elapsed == 0 (and the IRM is broken) and it does not use the oracle (called once you need to withdraw)

[MerlinEgalite]

I think it's ok not to revert in this case. Vaults curators should not list broken markets in the first place.

[StErMi]

Just to recap: this feature allows you to add the existing supply of a new (supplyCap > 0 && market.enabled == false) market to the lastTotalAssets state variable without taxing it (the increase is not seen as interest accrual that needs to be taxed). The result is that share value is increased without taxing it.

The scenarios where this is needed are:

• donated funds
• re-adding a previously removed market

This does not handle the case that suppliers that were present at removal time could end up with less value in their shares if new suppliers have minted new shares in the meanwhile (before the market is re-added)