fix(reallocate): prevent withdrawing from market not enabled

[Rubilmax]

Fixes https://cantina.xyz/code/8409a0ce-6c21-4cc9-8ef2-bd77ce7425af/findings/131b79f3-83fc-4674-a222-c5c4dc7d0539

This PR makes reverting when withdrawing from a market that is not enabled. This prevents users to benefit from a donation but it's considered quite unlikely. It also prevents the vault managers to tax the principal of funds deposited on a removed market.

We also acknowledge that the fee recipient will lose entitled fees in case of bad debt events. This is considered fairer since the loss result from the bad management from vault curators.

[Rubilmax]

2 is what worries me the most: at worst, a vault manager cannot reallocate liquidity during the duration of their timelock
But it may be fine

[MerlinEgalite]

I don't think taxing donations is a big deal. It's bonus for everyone anyway.

But it may be fine

I don't think it's fine. Like if one market is frozen it could be for any reason including reason that could affect similar assets. The allocator would not be able to rebalance the portfolio to reduce risk exposure.

The increase of the gas cost is worrying. IMO reallocation should be as cheap as possible.

[Rubilmax]

It's bonus for everyone anyway.

Not in all cases, see point 1 + in case of a forced market removal that no longer reverts for some reason

[MerlinEgalite]

Not in all cases, see point 1 + in case of a forced market removal that no longer reverts for some reason

But this is fixed by #338 right?

[Jean-Grimal]

fixes the fact that a withdrawn donation gets taxed from the vault manager

Romain is right. #338 allows to recover (without being "taxed" by the feeRecipient) previously lost supply on reverting markets only when these markets are enabled again. But without enabling these markets it's possible (if they stop reverting) to withdraw from these market (to recover the supply) with reallocate. And in this case, if we don't accrue interest at the start of the function, fee will accrue from this supply.

[MerlinEgalite]

Ok right I get it now, sorry

[MerlinEgalite]

what's the cost of reallocate @Rubilmax ?

[Rubilmax]

what's the cost of reallocate @Rubilmax ?

according to hardhat tests, 360k gas before this change, 560k gas now (on avg)

[MerlinEgalite]

This must be tested I think

[MerlinEgalite]

a (simpler?) solution would be to prevent withdrawing from a market that is not listed? It removes the possibility to withdraw donations but maybe the spec is clearer?

[MathisGD]

I'm not sure to agree with this fix.

• if the manager enables the market, the donation/previously loosed funds will be slashed right ?
• why shouldn't a donation profit to the fee too ?

[QGarchery]

if the manager enables the market, the donation/previously loosed funds will be slashed right ?

(in english loosed -> lost, and also loose -> lose)

If by "slashed" you mean that it will be subject to the MM fee, then yes.

why shouldn't a donation profit to the fee too ?

I don't think it's a problem that a donation can be subject to the MM fee. In effect, donations to enabled markets are subject to the fee, even with the changes of this PR. We can imagine vaults making profit in other ways, and this can be cleanly accounted for with a donation.
What's more problematic is if funds of forced-removed markets are subject to the fee. This is because in this case the fee is taken on the principal, and not on the interest.
Moreover, this change is a safety measure that was already requested in #219, and marked as not planned because not doing it allowed to recover funds more cleanly from previously forced-removed markets. Indeed, enabling such markets again previously added them in the supply queue, so there would be some weird time where users would be able to supply on broken markets. Markets are not automatically added to the supply queue anymore (since #351), so it enables doing this change without problems

[Rubilmax]

LGTM!

[StErMi]

The entrypoint reallocate now reverts as soon as at least 1 market of the withdraw queue is broken. The curator no longer can reallocate liquidity across working markets as soon as 1 market is broken: they are forced to submitMarketRemoval and cannot reallocate for the duration of the timelock

@Rubilmax would you mind explaining this? I don't see how the PR has changed this behavior, compared to before

[MerlinEgalite]

The entrypoint reallocate now reverts as soon as at least 1 market of the withdraw queue is broken. The curator no longer can reallocate liquidity across working markets as soon as 1 market is broken: they are forced to submitMarketRemoval and cannot reallocate for the duration of the timelock

@Rubilmax would you mind explaining this? I don't see how the PR has changed this behavior, compared to before

This is indeed no longer true (it has been removed in the second commit of this PR).

The point 3 is no longer true as well I think.

[StErMi]

If you can update the description of the PR with the logic that you want to achieve with the changes, it would allow me to verify that the code is correct and has no side effects.

In general:

1. what it should do
2. what it should not change

[MerlinEgalite]

If you can update the description of the PR with the logic that you want to achieve with the changes, it would allow me to verify that the code is correct and has no side effects.

In general:

1. what it should do
2. what it should not change

I updated the description