diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index 81148f6e..5035b99c 100644 --- a/src/MetaMorpho.sol +++ b/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(); _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); @@ -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); @@ -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(); 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(); - pendingCap[id].update(newSupplyCap.toUint184(), timelock); emit EventsLib.SubmitCap(_msgSender(), id, newSupplyCap); @@ -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(); 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); @@ -425,8 +418,6 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph /// @inheritdoc IMetaMorphoBase function revokePendingTimelock() external onlyGuardianRole { - if (pendingTimelock.validAt == 0) revert ErrorsLib.NoPendingValue(); - delete pendingTimelock; emit EventsLib.RevokePendingTimelock(_msgSender()); @@ -434,8 +425,6 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph /// @inheritdoc IMetaMorphoBase function revokePendingGuardian() external onlyGuardianRole { - if (pendingGuardian.validAt == 0) revert ErrorsLib.NoPendingValue(); - delete pendingGuardian; emit EventsLib.RevokePendingGuardian(_msgSender()); @@ -443,8 +432,6 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph /// @inheritdoc IMetaMorphoBase function revokePendingCap(Id id) external onlyCuratorOrGuardianRole { - if (pendingCap[id].validAt == 0) revert ErrorsLib.NoPendingValue(); - delete pendingCap[id]; emit EventsLib.RevokePendingCap(_msgSender(), id); diff --git a/src/interfaces/IMetaMorpho.sol b/src/interfaces/IMetaMorpho.sol index 2320aef3..294bb943 100644 --- a/src/interfaces/IMetaMorpho.sol +++ b/src/interfaces/IMetaMorpho.sol @@ -77,25 +77,28 @@ 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. + /// @dev Does not revert if there is no pending timelock. 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 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`. + /// @dev Does not revert if there is no pending cap. function revokePendingCap(Id id) external; /// @notice Submits a forced market removal from the vault, eventually losing all funds supplied to the market. @@ -105,9 +108,12 @@ interface IMetaMorphoBase { /// To softly remove a sane market, the curator role is expected to bundle a reallocation that empties the market /// first (using `reallocate`), followed by the removal of the market (using `updateWithdrawQueue`). /// @dev Warning: Removing a market with non-zero supply will instantly impact the vault's price per share. + /// @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`. + /// @dev Does not revert if there is no pending market removal. function revokePendingMarketRemoval(Id id) external; /// @notice Submits a `newGuardian`. diff --git a/src/libraries/ErrorsLib.sol b/src/libraries/ErrorsLib.sol index 1e54c83a..402b590f 100644 --- a/src/libraries/ErrorsLib.sol +++ b/src/libraries/ErrorsLib.sol @@ -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); diff --git a/test/forge/MarketTest.sol b/test/forge/MarketTest.sol index ef769d43..e2147f3e 100644 --- a/test/forge/MarketTest.sol +++ b/test/forge/MarketTest.sol @@ -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(); @@ -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(); } @@ -279,4 +315,13 @@ contract MarketTest is IntegrationTest { ); vault.updateWithdrawQueue(indexes); } + + function testRevokeNoRevert() public { + vm.startPrank(OWNER); + vault.revokePendingTimelock(); + vault.revokePendingGuardian(); + vault.revokePendingCap(Id.wrap(bytes32(0))); + vault.revokePendingMarketRemoval(Id.wrap(bytes32(0))); + vm.stopPrank(); + } } diff --git a/test/forge/RevokeTest.sol b/test/forge/RevokeTest.sol index f0b12681..31619f5b 100644 --- a/test/forge/RevokeTest.sol +++ b/test/forge/RevokeTest.sol @@ -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(); - } }