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.

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
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
6 changes: 3 additions & 3 deletions src/MetaMorpho.sol
MathisGD marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -268,10 +268,10 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph
/// @inheritdoc IMetaMorphoBase
function submitCap(MarketParams memory marketParams, uint256 newSupplyCap) external onlyCuratorRole {
Id id = marketParams.id();
uint256 supplyCap = config[id].cap;
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.

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

if (newSupplyCap < supplyCap) {
Expand All @@ -286,10 +286,10 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph
/// @inheritdoc IMetaMorphoBase
function submitMarketRemoval(Id id) external onlyCuratorRole {
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();

_setCap(id, 0);

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

Expand Down
5 changes: 2 additions & 3 deletions src/interfaces/IMetaMorpho.sol
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ 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.
Expand All @@ -85,8 +85,8 @@ interface IMetaMorphoBase {
function revokePendingTimelock() 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 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`.
Expand All @@ -96,7 +96,6 @@ interface IMetaMorphoBase {
function revokePendingCap(Id id) external;

/// @notice Submits a forced market removal from the vault, potentially losing all funds supplied to the market.
/// @dev Warning: Submitting a forced removal will overwrite the timestamp at which the market will be removable.
function submitMarketRemoval(Id id) external;

/// @notice Revokes the pending removal of the market defined by `id`.
Expand Down
6 changes: 6 additions & 0 deletions src/libraries/ErrorsLib.sol
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@ library ErrorsLib {
/// @notice Thrown when a value is already pending.
error AlreadyPending();

/// @notice Thrown when submitting a market removal for a market for which a cap is already pending.
MerlinEgalite marked this conversation as resolved.
Show resolved Hide resolved
error PendingCap();

/// @notice Thrown when submitting a market removal for a market with a non zero cap.abi
MerlinEgalite marked this conversation as resolved.
Show resolved Hide resolved
error NonZeroCap();

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

Expand Down
22 changes: 20 additions & 2 deletions test/forge/MarketTest.sol
Original file line number Diff line number Diff line change
Expand Up @@ -182,17 +182,35 @@ 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.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());
Expand Down
Loading