Always revert when there is a value already pending #353

MathisGD · 2023-12-15T14:14:36Z

https://morpholabs.slack.com/archives/C02N7CSP91C/p1702634824993509

Fixes #221

Rubilmax

This design relies on the fact the pending value is deleted every time a value is set

src/MetaMorpho.sol

test/forge/MarketTest.sol

src/MetaMorpho.sol

Jean-Grimal

The natspec should be updated

Rubilmax

Pointing out that being consistent with #348 requires to revert in submitMarketRemoval and submitCap

src/MetaMorpho.sol

Jean-Grimal

A test on submitting a guardian while there is already a pending guardian should be added

src/MetaMorpho.sol

src/libraries/ErrorsLib.sol

src/interfaces/IMetaMorpho.sol

src/MetaMorpho.sol

MerlinEgalite

I still think the naming change was not necessary

src/MetaMorpho.sol

MerlinEgalite · 2023-12-28T11:10:32Z

@MathisGD can you revert back the naming and fix the conflicts please?

…rt-already-pending

StErMi · 2023-12-30T10:09:48Z

src/MetaMorpho.sol

@@ -214,14 +214,12 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph
    /// @inheritdoc IMetaMorphoBase
    function submitTimelock(uint256 newTimelock) external onlyOwner {
        if (newTimelock == timelock) revert ErrorsLib.AlreadySet();
+        if (pendingTimelock.validAt != 0) revert ErrorsLib.AlreadyPending();


Before, you could have a pending value (decrease of timelock) that could have been deleted by another submitTimelock that set the new timelock to a new one (higher timelock)

Now you won't be able to do that, and you are forced to wait for the timelock to expire or revoke it and call it again.

The same goes for submitting an even lower timelock when one pending timelock was already submitted.

Is this the intended new behavior?

yes we enforce to revoke every pending values before submitting a new one.

StErMi · 2023-12-30T14:17:36Z

src/MetaMorpho.sol

@@ -281,16 +276,14 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph
        Id id = marketParams.id();
        if (marketParams.loanToken != asset()) revert ErrorsLib.InconsistentAsset(id);
        if (MORPHO.lastUpdate(id) == 0) revert ErrorsLib.MarketNotCreated();
-
+        if (pendingCap[id].validAt != 0) revert ErrorsLib.AlreadyPending();
+        if (config[id].removableAt != 0) revert ErrorsLib.PendingRemoval();


I think that this is a welcome check because the removal of a market is a very strong operation that also has a timelock that is different compared to the cap timelock (and previously they were non correlated within the logic). Now if a market is marked as "to-be-removed" such operation has the priority and will lock any changes to the cap.

Now as soon as a market is marked as to-be-removed, there's no way to re-enable the cap (that is already set to zero) unless you explicitly call revokePendingMarketRemoval

StErMi · 2023-12-30T14:20:16Z

src/MetaMorpho.sol

@@ -281,16 +276,14 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph
        Id id = marketParams.id();
        if (marketParams.loanToken != asset()) revert ErrorsLib.InconsistentAsset(id);
        if (MORPHO.lastUpdate(id) == 0) revert ErrorsLib.MarketNotCreated();
-
+        if (pendingCap[id].validAt != 0) revert ErrorsLib.AlreadyPending();


Like for the submitTimelock, this new check brings some side effects. Unless you revoke the previous pending proposal, or you wait for the timelock + accept it, you cannot

create a new proposal

insta-change the supply cap (when the new cap is lower)

In general, you are giving much more priority and power to the pending proposals. Is this the intended behavior?

yes it is. What we'll do is that in the frontend we can leverage the multicall functionality to bundle a revoke + submit in a single tx to perform the required actions.

StErMi · 2023-12-30T14:28:44Z

Would you mind confirming the logic of the behavior? Because otherwise, I'm trying to reverse engineering it and could arrive at the wrong conclusion.

The new logic allows the submission of a change to the

timelock
guardian
supply
market removal

Only if there's no direct or indirect pending proposal currently in action.

To be more clear

direct: you can't submit a cap proposal if there's already one
indirect: you can't submit a cap proposal if there's a pending market removal proposal

Moreover, there are some "custom" requirements (that still need to be documented), like for example

you can't require a market removal if the cap is not already set to zero. In general, you don't want to execute implicit logic, but the state must be already "ready" to perform the action once the timelock has ended.

Would you mind confirming or detailing more/correct my recap?

StErMi · 2023-12-30T14:31:29Z

Giving the immutable nature of MM (you can't upgrade and change the logic) it could make sense to create utility contracts to manage it.

Like for example an external contract (with the owner or curator role) that perform all the actions needed to be able to execute MM.submitMarketRemoval

if the cap is not 0 and there's a pending proposal, revoke it and set it to zero
call the submitMarketRemoval

MerlinEgalite · 2023-12-30T18:31:06Z

I confirm your first comment. We wanted submit*, revoke* and accept* functions to be single purpose functions with less side effects than previously.

For the second comment, the vault inherits from Multicall which allows to extract that logic offchain, so what you proposed will be done in Morpho Blue's frontend. Nonetheless, some vault curators are likely to write their own logic but we won't write it ourselves.

refactor: revert already pending

420095e

MathisGD changed the base branch from main to post-cantina December 15, 2023 14:14

MathisGD self-assigned this Dec 15, 2023

MathisGD requested review from a team, adhusson, Rubilmax, QGarchery, MerlinEgalite and Jean-Grimal and removed request for a team December 15, 2023 14:16

Rubilmax approved these changes Dec 15, 2023

View reviewed changes

src/MetaMorpho.sol Show resolved Hide resolved

src/MetaMorpho.sol Outdated Show resolved Hide resolved

MerlinEgalite requested changes Dec 15, 2023

View reviewed changes

test/forge/MarketTest.sol Outdated Show resolved Hide resolved

src/MetaMorpho.sol Outdated Show resolved Hide resolved

src/MetaMorpho.sol Show resolved Hide resolved

src/MetaMorpho.sol Show resolved Hide resolved

MerlinEgalite mentioned this pull request Dec 15, 2023

Delete pending cap when removing a market #348

Merged

Jean-Grimal requested changes Dec 15, 2023

View reviewed changes

Rubilmax suggested changes Dec 18, 2023

View reviewed changes

MathisGD added 2 commits December 18, 2023 11:39

fix: minor improvements

36ddce7

fix: minor improvement

950d9f0

MerlinEgalite reviewed Dec 18, 2023

View reviewed changes

src/MetaMorpho.sol Show resolved Hide resolved

Jean-Grimal previously approved these changes Dec 19, 2023

View reviewed changes

QGarchery reviewed Dec 23, 2023

View reviewed changes

src/MetaMorpho.sol Show resolved Hide resolved

src/MetaMorpho.sol Show resolved Hide resolved

src/MetaMorpho.sol Show resolved Hide resolved

docs: minor improvement

0878090

MathisGD dismissed Jean-Grimal’s stale review via 0878090 December 26, 2023 21:52

MathisGD added 2 commits December 26, 2023 22:59

feat: pending removal

6989d7d

Merge branch 'post-cantina' into refactor/revert-already-pending

1b0ae2b

MathisGD requested review from Rubilmax, Jean-Grimal, MerlinEgalite and QGarchery December 26, 2023 22:05

MathisGD mentioned this pull request Dec 26, 2023

[protocol review] Possible accidental removal of pending transactions #221

Closed

QGarchery requested changes Dec 27, 2023

View reviewed changes

src/libraries/ErrorsLib.sol Outdated Show resolved Hide resolved

src/libraries/ErrorsLib.sol Outdated Show resolved Hide resolved

src/interfaces/IMetaMorpho.sol Outdated Show resolved Hide resolved

src/MetaMorpho.sol Show resolved Hide resolved

MathisGD dismissed adhusson’s stale review via ed35fd5 December 27, 2023 21:36

adhusson mentioned this pull request Dec 27, 2023

revoke* inconsistency should be explained #364

Closed

style: revoke naming

93d9275

adhusson previously approved these changes Dec 27, 2023

View reviewed changes

MathisGD requested a review from Jean-Grimal December 27, 2023 22:17

test: add revoke no revert test

b7bb3eb

MathisGD dismissed adhusson’s stale review via b7bb3eb December 27, 2023 22:24

MathisGD requested a review from adhusson December 27, 2023 22:25

adhusson previously approved these changes Dec 27, 2023

View reviewed changes

MerlinEgalite previously approved these changes Dec 28, 2023

View reviewed changes

Rubilmax suggested changes Dec 28, 2023

View reviewed changes

src/MetaMorpho.sol Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/post-cantina' into refactor/reve…

b3602c1

…rt-already-pending

MathisGD dismissed stale reviews from MerlinEgalite and adhusson via b3602c1 December 28, 2023 12:33

style: rollback name change and acknowledge behavior in natspec

d32cb14

MathisGD requested review from MerlinEgalite, Rubilmax and adhusson December 28, 2023 12:44

Jean-Grimal approved these changes Dec 28, 2023

View reviewed changes

MerlinEgalite approved these changes Dec 28, 2023

View reviewed changes

MerlinEgalite merged commit 15f96c1 into post-cantina Dec 28, 2023
6 checks passed

MerlinEgalite deleted the refactor/revert-already-pending branch December 28, 2023 13:56

Jean-Grimal mentioned this pull request Dec 28, 2023

Post cantina #357

Merged

StErMi reviewed Dec 30, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always revert when there is a value already pending #353

Always revert when there is a value already pending #353

MathisGD commented Dec 15, 2023 •

edited by MerlinEgalite

Loading

Rubilmax left a comment

Jean-Grimal left a comment

Rubilmax left a comment

Jean-Grimal left a comment

MerlinEgalite left a comment

MerlinEgalite commented Dec 28, 2023

StErMi Dec 30, 2023 •

edited

Loading

MerlinEgalite Dec 30, 2023

StErMi Dec 30, 2023

StErMi Dec 30, 2023

MerlinEgalite Dec 30, 2023

StErMi commented Dec 30, 2023

StErMi commented Dec 30, 2023

MerlinEgalite commented Dec 30, 2023

Always revert when there is a value already pending #353

Always revert when there is a value already pending #353

Conversation

MathisGD commented Dec 15, 2023 • edited by MerlinEgalite Loading

Rubilmax left a comment

Choose a reason for hiding this comment

Jean-Grimal left a comment

Choose a reason for hiding this comment

Rubilmax left a comment

Choose a reason for hiding this comment

Jean-Grimal left a comment

Choose a reason for hiding this comment

MerlinEgalite left a comment

Choose a reason for hiding this comment

MerlinEgalite commented Dec 28, 2023

StErMi Dec 30, 2023 • edited Loading

Choose a reason for hiding this comment

MerlinEgalite Dec 30, 2023

Choose a reason for hiding this comment

StErMi Dec 30, 2023

Choose a reason for hiding this comment

StErMi Dec 30, 2023

Choose a reason for hiding this comment

MerlinEgalite Dec 30, 2023

Choose a reason for hiding this comment

StErMi commented Dec 30, 2023

StErMi commented Dec 30, 2023

MerlinEgalite commented Dec 30, 2023

MathisGD commented Dec 15, 2023 •

edited by MerlinEgalite

Loading

StErMi Dec 30, 2023 •

edited

Loading