Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always revert when there is a value already pending #353

Merged
merged 16 commits into from
Dec 28, 2023
Merged

Always revert when there is a value already pending #353

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
420095e
refactor: revert already pending
MathisGD Dec 15, 2023
36ddce7
fix: minor improvements
MathisGD Dec 18, 2023
950d9f0
fix: minor improvement
MathisGD Dec 18, 2023
0878090
docs: minor improvement
MathisGD Dec 26, 2023
6989d7d
feat: pending removal
MathisGD Dec 26, 2023
1b0ae2b
Merge branch 'post-cantina' into refactor/revert-already-pending
MathisGD Dec 26, 2023
205f06b
docs: apply suggestions
MerlinEgalite Dec 27, 2023
09ae7dc
style: fmt
MerlinEgalite Dec 27, 2023
179d1cc
refactor: no revert on revoke
MerlinEgalite Dec 27, 2023
3a47e4e
Merge pull request #366 from morpho-org/refactor/no-revert-on-revoke
MathisGD Dec 27, 2023
3a4ae54
Merge remote-tracking branch 'origin/post-cantina' into refactor/reve…
MathisGD Dec 27, 2023
ed35fd5
Merge remote-tracking branch 'origin/refactor/revert-already-pending'…
MathisGD Dec 27, 2023
93d9275
style: revoke naming
MathisGD Dec 27, 2023
b7bb3eb
test: add revoke no revert test
MathisGD Dec 27, 2023
b3602c1
Merge remote-tracking branch 'origin/post-cantina' into refactor/reve…
MathisGD Dec 28, 2023
d32cb14
style: rollback name change and acknowledge behavior in natspec
MathisGD Dec 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 10 additions & 23 deletions src/MetaMorpho.sol
View file
Open in desktop
MathisGD marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -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();
MathisGD marked this conversation as resolved.
Show resolved Hide resolved
Copy link

@StErMi StErMi Dec 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_checkTimelockBounds(newTimelock);

if (newTimelock > timelock) {
_setTimelock(newTimelock);
} else {
// newTimelock >= MIN_TIMELOCK > 0 so there's no need to check `pendingTimelock.validAt != 0`.
if (newTimelock == pendingTimelock.value) revert ErrorsLib.AlreadyPending();

// Safe "unchecked" cast because newTimelock <= MAX_TIMELOCK.
pendingTimelock.update(uint184(newTimelock), timelock);

Expand Down Expand Up @@ -260,14 +258,11 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph
/// @inheritdoc IMetaMorphoBase
function submitGuardian(address newGuardian) external onlyOwner {
if (newGuardian == guardian) revert ErrorsLib.AlreadySet();
if (pendingGuardian.validAt != 0) revert ErrorsLib.AlreadyPending();

if (guardian == address(0)) {
_setGuardian(newGuardian);
} else {
if (pendingGuardian.validAt != 0 && newGuardian == pendingGuardian.value) {
revert ErrorsLib.AlreadyPending();
}

pendingGuardian.update(newGuardian, timelock);

emit EventsLib.SubmitGuardian(newGuardian);
Expand All @@ -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();
MathisGD marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (config[id].removableAt != 0) revert ErrorsLib.PendingRemoval();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

uint256 supplyCap = config[id].cap;
if (newSupplyCap == supplyCap) revert ErrorsLib.AlreadySet();

if (newSupplyCap < supplyCap) {
_setCap(id, newSupplyCap.toUint184());
} else {
// newSupplyCap > supplyCap >= 0 so there's no need to check `pendingCap[id].validAt != 0`.
if (newSupplyCap == pendingCap[id].value) revert ErrorsLib.AlreadyPending();
MathisGD marked this conversation as resolved.
Show resolved Hide resolved

pendingCap[id].update(newSupplyCap.toUint184(), timelock);

emit EventsLib.SubmitCap(_msgSender(), id, newSupplyCap);
Expand All @@ -299,11 +292,11 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph

/// @inheritdoc IMetaMorphoBase
function submitMarketRemoval(Id id) external onlyCuratorRole {
if (config[id].removableAt != 0) revert ErrorsLib.AlreadySet();
if (config[id].removableAt != 0) revert ErrorsLib.AlreadyPending();
if (pendingCap[id].validAt != 0) revert ErrorsLib.PendingCap();
if (config[id].cap != 0) revert ErrorsLib.NonZeroCap();
MathisGD marked this conversation as resolved.
Show resolved Hide resolved
if (!config[id].enabled) revert ErrorsLib.MarketNotEnabled(id);

_setCap(id, 0);

// Safe "unchecked" cast because timelock <= MAX_TIMELOCK.
config[id].removableAt = uint64(block.timestamp + timelock);

Expand Down Expand Up @@ -424,27 +417,21 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph
/* REVOKE FUNCTIONS */

/// @inheritdoc IMetaMorphoBase
function revokePendingTimelock() external onlyGuardianRole {
if (pendingTimelock.validAt == 0) revert ErrorsLib.NoPendingValue();

function revokePendingTimelockNoRevert() external onlyGuardianRole {
MathisGD marked this conversation as resolved.
Show resolved Hide resolved
delete pendingTimelock;

emit EventsLib.RevokePendingTimelock(_msgSender());
}

/// @inheritdoc IMetaMorphoBase
function revokePendingGuardian() external onlyGuardianRole {
if (pendingGuardian.validAt == 0) revert ErrorsLib.NoPendingValue();

function revokePendingGuardianNoRevert() external onlyGuardianRole {
delete pendingGuardian;

emit EventsLib.RevokePendingGuardian(_msgSender());
}

/// @inheritdoc IMetaMorphoBase
function revokePendingCap(Id id) external onlyCuratorOrGuardianRole {
if (pendingCap[id].validAt == 0) revert ErrorsLib.NoPendingValue();

function revokePendingCapNoRevert(Id id) external onlyCuratorOrGuardianRole {
delete pendingCap[id];

emit EventsLib.RevokePendingCap(_msgSender(), id);
Expand Down
13 changes: 8 additions & 5 deletions src/interfaces/IMetaMorpho.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,28 +77,31 @@ interface IMetaMorphoBase {
function lastTotalAssets() external view returns (uint256);

/// @notice Submits a `newTimelock`.
/// @dev Warning: Reverts if a timelock is already pending. Revoke the pending timelock to overwrite it.
/// @dev In case the new timelock is higher than the current one, the timelock is set immediately.
/// @dev Warning: Submitting a timelock will overwrite the current pending timelock.
function submitTimelock(uint256 newTimelock) external;

/// @notice Accepts the pending timelock.
function acceptTimelock() external;

/// @notice Revokes the pending timelock.
function revokePendingTimelock() external;
function revokePendingTimelockNoRevert() external;

/// @notice Submits a `newSupplyCap` for the market defined by `marketParams`.
/// @dev Warning: Reverts if a cap is already pending. Revoke the pending cap to overwrite it.
/// @dev Warning: Reverts if a market removal is pending.
/// @dev In case the new cap is lower than the current one, the cap is set immediately.
/// @dev Warning: Submitting a cap will overwrite the current pending cap.
function submitCap(MarketParams memory marketParams, uint256 newSupplyCap) external;

/// @notice Accepts the pending cap of the market defined by `id`.
function acceptCap(Id id) external;

/// @notice Revokes the pending cap of the market defined by `id`.
function revokePendingCap(Id id) external;
function revokePendingCapNoRevert(Id id) external;

/// @notice Submits a forced market removal from the vault, eventually losing all funds supplied to the market.
/// @dev Warning: Reverts for non-zero cap or if there is a pending cap. Successfully submitting a zero cap will
/// prevent such reverts.
function submitMarketRemoval(Id id) external;

/// @notice Revokes the pending removal of the market defined by `id`.
Expand All @@ -115,7 +118,7 @@ interface IMetaMorphoBase {
function acceptGuardian() external;

/// @notice Revokes the pending guardian.
function revokePendingGuardian() external;
function revokePendingGuardianNoRevert() external;

/// @notice Skims the vault `token` balance to `skimRecipient`.
function skim(address) external;
Expand Down
11 changes: 10 additions & 1 deletion src/libraries/ErrorsLib.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,18 @@ library ErrorsLib {
/// @notice Thrown when the value is already set.
error AlreadySet();

/// @notice Thrown when the value is already pending.
/// @notice Thrown when a value is already pending.
error AlreadyPending();

/// @notice Thrown when submitting the removal of a market when there is a cap already pending on that market.
error PendingCap();

/// @notice Thrown when submitting a cap for a market with a pending removal.
error PendingRemoval();

/// @notice Thrown when submitting a market removal for a market with a non zero cap.
error NonZeroCap();

/// @notice Thrown when market `id` is a duplicate in the new withdraw queue to set.
error DuplicateMarket(Id id);

Expand Down
8 changes: 4 additions & 4 deletions test/forge/GuardianTest.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ contract GuardianTest is IntegrationTest {
vm.expectEmit(address(vault));
emit EventsLib.RevokePendingTimelock(GUARDIAN);
vm.prank(GUARDIAN);
vault.revokePendingTimelock();
vault.revokePendingTimelockNoRevert();

uint256 newTimelock = vault.timelock();
PendingUint192 memory pendingTimelock = vault.pendingTimelock();
Expand All @@ -59,7 +59,7 @@ contract GuardianTest is IntegrationTest {
vm.expectEmit();
emit EventsLib.RevokePendingTimelock(OWNER);
vm.prank(OWNER);
vault.revokePendingTimelock();
vault.revokePendingTimelockNoRevert();

uint256 newTimelock = vault.timelock();
PendingUint192 memory pendingTimelock = vault.pendingTimelock();
Expand All @@ -84,7 +84,7 @@ contract GuardianTest is IntegrationTest {
vm.expectEmit(address(vault));
emit EventsLib.RevokePendingCap(GUARDIAN, id);
vm.prank(GUARDIAN);
vault.revokePendingCap(id);
vault.revokePendingCapNoRevert(id);

MarketConfig memory marketConfig = vault.config(id);
PendingUint192 memory pendingCap = vault.pendingCap(id);
Expand All @@ -109,7 +109,7 @@ contract GuardianTest is IntegrationTest {
vm.expectEmit(address(vault));
emit EventsLib.RevokePendingGuardian(GUARDIAN);
vm.prank(GUARDIAN);
vault.revokePendingGuardian();
vault.revokePendingGuardianNoRevert();

address newGuardian = vault.guardian();
PendingAddress memory pendingGuardian = vault.pendingGuardian();
Expand Down
42 changes: 39 additions & 3 deletions test/forge/MarketTest.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,24 @@ contract MarketTest is IntegrationTest {
vault.submitCap(allMarkets[0], CAP);
}

function testSubmitCapAlreadyPending() public {
vm.prank(CURATOR);
vault.submitCap(allMarkets[0], CAP + 1);

vm.prank(CURATOR);
vm.expectRevert(ErrorsLib.AlreadyPending.selector);
vault.submitCap(allMarkets[0], CAP + 1);
}

function testSubmitCapPendingRemoval() public {
vm.startPrank(CURATOR);
vault.submitCap(allMarkets[2], 0);
vault.submitMarketRemoval(allMarkets[2].id());

vm.expectRevert(ErrorsLib.PendingRemoval.selector);
vault.submitCap(allMarkets[2], CAP + 1);
}

function testSetSupplyQueue() public {
Id[] memory supplyQueue = new Id[](2);
supplyQueue[0] = allMarkets[1].id();
Expand Down Expand Up @@ -182,19 +200,37 @@ contract MarketTest is IntegrationTest {
}

function testSubmitMarketRemoval() public {
vm.startPrank(CURATOR);
vault.submitCap(allMarkets[2], 0);
vm.expectEmit();
emit EventsLib.SubmitMarketRemoval(CURATOR, allMarkets[2].id());
vm.prank(CURATOR);
vault.submitMarketRemoval(allMarkets[2].id());
vm.stopPrank();

assertEq(vault.config(allMarkets[2].id()).cap, 0);
assertEq(vault.config(allMarkets[2].id()).removableAt, block.timestamp + TIMELOCK);
}

function testSubmitMarketRemovalAlreadySet() public {
function testSubmitMarketRemovalPendingCap() public {
vm.startPrank(CURATOR);
vault.submitCap(allMarkets[2], vault.config(allMarkets[2].id()).cap + 1);
vm.expectRevert(ErrorsLib.PendingCap.selector);
vault.submitMarketRemoval(allMarkets[2].id());
vm.expectRevert(ErrorsLib.AlreadySet.selector);
vm.stopPrank();
}

function testSubmitMarketRemovalNonZeroCap() public {
vm.startPrank(CURATOR);
vm.expectRevert(ErrorsLib.NonZeroCap.selector);
vault.submitMarketRemoval(allMarkets[2].id());
vm.stopPrank();
}

function testSubmitMarketRemovalAlreadyPending() public {
vm.startPrank(CURATOR);
vault.submitCap(allMarkets[2], 0);
vault.submitMarketRemoval(allMarkets[2].id());
vm.expectRevert(ErrorsLib.AlreadyPending.selector);
vault.submitMarketRemoval(allMarkets[2].id());
vm.stopPrank();
}
Expand Down
28 changes: 4 additions & 24 deletions test/forge/RevokeTest.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ contract RevokeTest is IntegrationTest {
vm.expectEmit();
emit EventsLib.RevokePendingTimelock(OWNER);
vm.prank(OWNER);
vault.revokePendingTimelock();
vault.revokePendingTimelockNoRevert();

uint256 newTimelock = vault.timelock();
PendingUint192 memory pendingTimelock = vault.pendingTimelock();
Expand All @@ -54,7 +54,7 @@ contract RevokeTest is IntegrationTest {
vm.expectEmit();
emit EventsLib.RevokePendingCap(CURATOR, id);
vm.prank(CURATOR);
vault.revokePendingCap(id);
vault.revokePendingCapNoRevert(id);

MarketConfig memory marketConfig = vault.config(id);
PendingUint192 memory pendingCap = vault.pendingCap(id);
Expand All @@ -81,7 +81,7 @@ contract RevokeTest is IntegrationTest {
vm.expectEmit();
emit EventsLib.RevokePendingCap(OWNER, id);
vm.prank(OWNER);
vault.revokePendingCap(id);
vault.revokePendingCapNoRevert(id);

MarketConfig memory marketConfig = vault.config(id);
PendingUint192 memory pendingCap = vault.pendingCap(id);
Expand All @@ -106,7 +106,7 @@ contract RevokeTest is IntegrationTest {
vm.expectEmit();
emit EventsLib.RevokePendingGuardian(GUARDIAN);
vm.prank(GUARDIAN);
vault.revokePendingGuardian();
vault.revokePendingGuardianNoRevert();

address newGuardian = vault.guardian();
PendingAddress memory pendingGuardian = vault.pendingGuardian();
Expand All @@ -115,24 +115,4 @@ contract RevokeTest is IntegrationTest {
assertEq(pendingGuardian.value, address(0), "value");
assertEq(pendingGuardian.validAt, 0, "validAt");
}

function testOwnerRevokePendingCapNoPendingValue(uint256 seed) public {
MarketParams memory marketParams = _randomMarketParams(seed);

vm.prank(OWNER);
vm.expectRevert(ErrorsLib.NoPendingValue.selector);
vault.revokePendingCap(marketParams.id());
}

function testOwnerRevokePendingTimelockNoPendingValue() public {
vm.prank(OWNER);
vm.expectRevert(ErrorsLib.NoPendingValue.selector);
vault.revokePendingTimelock();
}

function testOwnerRevokePendingGuardianNoPendingValue() public {
vm.prank(OWNER);
vm.expectRevert(ErrorsLib.NoPendingValue.selector);
vault.revokePendingGuardian();
}
}
6 changes: 3 additions & 3 deletions test/forge/RoleTest.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ contract RoleTest is IntegrationTest {
vm.startPrank(caller);

vm.expectRevert(ErrorsLib.NotCuratorNorGuardianRole.selector);
vault.revokePendingCap(id);
vault.revokePendingCapNoRevert(id);

vm.expectRevert(ErrorsLib.NotCuratorNorGuardianRole.selector);
vault.revokePendingMarketRemoval(id);
Expand All @@ -106,10 +106,10 @@ contract RoleTest is IntegrationTest {
vm.startPrank(caller);

vm.expectRevert(ErrorsLib.NotGuardianRole.selector);
vault.revokePendingTimelock();
vault.revokePendingTimelockNoRevert();

vm.expectRevert(ErrorsLib.NotGuardianRole.selector);
vault.revokePendingGuardian();
vault.revokePendingGuardianNoRevert();

vm.stopPrank();
}
Expand Down
Loading