From 60bab4d02300bc4682b24c4308eccb141c798f51 Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Wed, 18 Oct 2023 16:22:58 +0200 Subject: [PATCH 01/30] feat(pending): revoke pending values --- README.md | 38 +++++---- src/MetaMorpho.sol | 30 +++++--- src/interfaces/IMetaMorpho.sol | 1 + src/libraries/ErrorsLib.sol | 6 ++ src/libraries/EventsLib.sol | 7 +- test/forge/GuardianTest.sol | 136 ++++++++++++++++++++++++++++++++- 6 files changed, 189 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index e8bc00eb..5720d387 100644 --- a/README.md +++ b/README.md @@ -43,13 +43,16 @@ After the timelock, the action can be executed by anyone until 3 days have passe Only one single address can have this role. It can: + - Do whatever the curator and allocators can do. - Transfer or renounce the ownership. - Set the curator. - Set allocators. - Set the rewards recipient. - [Timelocked] Set the timelock. +- Revoke the pending timelock. - [Timelocked with no possible veto] Set the performance fee (capped to 50%). +- Revoke the pending fee. - [Timelocked] Set the guardian. - Set the fee recipient. @@ -58,21 +61,24 @@ It can: Only one single address can have this role. It can: + - Do whatever the allocators can do. - [Timelocked] Enable or disable a market by setting a cap to a specific market. - - The cap must be set to 0 to disable the market. - - Disabling a market can then only be done if the vault has no liquidity supplied on the market. + - The cap must be set to 0 to disable the market. + - Disabling a market can then only be done if the vault has no liquidity supplied on the market. +- Revoke the pending cap of any market. #### Allocator Multiple addresses can have this role. It can: + - Set the `supplyQueue` and `withdrawQueue`, ie decides on the order of the markets to supply/withdraw from. - - Upon a deposit, the vault will supply up to the cap of each Morpho Blue market in the supply queue in the order set. The remaining funds are left as idle supply on the vault (uncapped). - - Upon a withdrawal, the vault will first withdraw from the idle supply, then withdraw up to the liquidity of each Morpho Blue market in the withdrawal queue in the order set. - - The `supplyQueue` can only contain enabled markets. - - The `withdrawQueue` MUST contain all enabled markets on which the vault has still liquidity (enabled market are markets with non-zero cap or with non-zero vault's supply). + - Upon a deposit, the vault will supply up to the cap of each Morpho Blue market in the supply queue in the order set. The remaining funds are left as idle supply on the vault (uncapped). + - Upon a withdrawal, the vault will first withdraw from the idle supply, then withdraw up to the liquidity of each Morpho Blue market in the withdrawal queue in the order set. + - The `supplyQueue` can only contain enabled markets. + - The `withdrawQueue` MUST contain all enabled markets on which the vault has still liquidity (enabled market are markets with non-zero cap or with non-zero vault's supply). - Reallocate funds among the enabled market at any moment. #### Guardian @@ -80,7 +86,12 @@ It can: Only one single address can have this role. It can: -- Revoke any timelocked action except it can't revoke a pending fee. + +- Revoke the pending timelock. +- Revoke the pending guardian. +- Revoke the pending cap of any market. + +In particular, it cannot revoke a pending fee. ### Rewards @@ -89,18 +100,19 @@ To redistribute rewards to vault depositors, it is advised to use the [Universal Below is a typical example of how this use case would take place: - If not already done: - - Create a URD using the [UrdFactory](https://github.com/morpho-org/universal-rewards-distributor/blob/main/src/UrdFactory.sol) (can be done with an EOA). - - Set the vault’s rewards recipient address to the created URD using `setRewardsRecipient`. + + - Create a URD using the [UrdFactory](https://github.com/morpho-org/universal-rewards-distributor/blob/main/src/UrdFactory.sol) (can be done with an EOA). + - Set the vault’s rewards recipient address to the created URD using `setRewardsRecipient`. - Claim tokens from the Morpho Blue distribution to the vault. - NB: Anyone can claim tokens on behalf of the vault and automatically transfer them to the vault. - Thus, this step might be already performed by some third-party. + NB: Anyone can claim tokens on behalf of the vault and automatically transfer them to the vault. + Thus, this step might be already performed by some third-party. - Transfer rewards from the vault to the rewards distributor using the `transferRewards` function. - NB: Anyone can transfer rewards from the vault to the rewards distributor unless it is unset. Thus, this step might be already performed by some third-party. - Note: the amount of rewards transferred is calculated based on the balance in the reward asset of the vault. In case the reward asset is the vault’s asset, the vault’s idle liquidity is automatically subtracted to prevent stealing idle liquidity. + NB: Anyone can transfer rewards from the vault to the rewards distributor unless it is unset. Thus, this step might be already performed by some third-party. + Note: the amount of rewards transferred is calculated based on the balance in the reward asset of the vault. In case the reward asset is the vault’s asset, the vault’s idle liquidity is automatically subtracted to prevent stealing idle liquidity. - Compute the new root for the vault’s rewards distributor, submit it, wait for the timelock (if any), accept the root, and let vault depositors claim their rewards according to the vault manager’s rewards re-distribution strategy. diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index b64f6656..dfffa4d6 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -137,13 +137,6 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph _; } - /// @dev Reverts if the caller is not the `guardian`. - modifier onlyGuardian() { - if (_msgSender() != guardian) revert ErrorsLib.NotGuardian(); - - _; - } - /// @dev Makes sure conditions are met to accept a pending value. /// @dev Reverts if: /// - there's no pending value; @@ -381,24 +374,39 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph } } - /* ONLY GUARDIAN FUNCTIONS */ + /* REVOKE FUNCTIONS */ + + /// @notice Revokes the pending cap of the market defined by `id`. + function revokeFee() external onlyOwner { + emit EventsLib.RevokeFee(msg.sender, pendingFee); + + delete pendingFee; + } /// @notice Revokes the `pendingTimelock`. - function revokeTimelock() external onlyGuardian { + function revokeTimelock() external { + if (_msgSender() != owner() && _msgSender() != guardian) revert ErrorsLib.NotOwnerOrGuardian(); + emit EventsLib.RevokeTimelock(msg.sender, pendingTimelock); delete pendingTimelock; } /// @notice Revokes the `pendingGuardian`. - function revokeGuardian() external onlyGuardian { + function revokeGuardian() external { + if (_msgSender() != guardian) revert ErrorsLib.NotGuardian(); + emit EventsLib.RevokeGuardian(msg.sender, pendingGuardian); delete pendingGuardian; } /// @notice Revokes the pending cap of the market defined by `id`. - function revokeCap(Id id) external onlyGuardian { + function revokeCap(Id id) external { + if (_msgSender() != guardian && _msgSender() != curator && _msgSender() != owner()) { + revert ErrorsLib.NotCuratorOrGuardian(); + } + emit EventsLib.RevokeCap(msg.sender, id, pendingCap[id]); delete pendingCap[id]; diff --git a/src/interfaces/IMetaMorpho.sol b/src/interfaces/IMetaMorpho.sol index 572598c4..8ea05d8c 100644 --- a/src/interfaces/IMetaMorpho.sol +++ b/src/interfaces/IMetaMorpho.sol @@ -82,6 +82,7 @@ interface IMetaMorpho is IERC4626 { } interface IPending { + function pendingFee() external view returns (PendingUint192 memory); function pendingTimelock() external view returns (PendingUint192 memory); function pendingCap(Id) external view returns (PendingUint192 memory); function pendingGuardian() external view returns (PendingAddress memory); diff --git a/src/libraries/ErrorsLib.sol b/src/libraries/ErrorsLib.sol index 1370ebe3..e4f47dcd 100644 --- a/src/libraries/ErrorsLib.sol +++ b/src/libraries/ErrorsLib.sol @@ -20,6 +20,12 @@ library ErrorsLib { /// @notice Thrown when the caller is not the guardian. error NotGuardian(); + /// @notice Thrown when the caller is not the owner nor the guardian. + error NotOwnerOrGuardian(); + + /// @notice Thrown when the caller doesn't have the curator's privilege and is not the guardian. + error NotCuratorOrGuardian(); + /// @notice Thrown when the market `id` cannot be set in the supply queue. error UnauthorizedMarket(Id id); diff --git a/src/libraries/EventsLib.sol b/src/libraries/EventsLib.sol index 729b8e12..162b0857 100644 --- a/src/libraries/EventsLib.sol +++ b/src/libraries/EventsLib.sol @@ -48,15 +48,18 @@ library EventsLib { /// @notice Emitted when an `allocator` is set to `isAllocator`. event SetIsAllocator(address indexed allocator, bool isAllocator); - /// @notice Emitted when a `pendingTimelock` is revoked by `guardian`. + /// @notice Emitted when a `pendingTimelock` is revoked by `guardian` or `owner`. event RevokeTimelock(address indexed guardian, PendingUint192 pendingTimelock); - /// @notice Emitted when a `pendingCap` for the market identified by `id` is revoked by `guardian`. + /// @notice Emitted when a `pendingCap` for the market identified by `id` is revoked by `guardian` or `owner`. event RevokeCap(address indexed guardian, Id indexed id, PendingUint192 pendingCap); /// @notice Emitted when a `pendingGuardian` is revoked by `guardian`. event RevokeGuardian(address indexed guardian, PendingAddress pendingGuardian); + /// @notice Emitted when a `pendingFee` is revoked by `guardian` or `owner`. + event RevokeFee(address indexed guardian, PendingUint192 pendingFee); + /// @notice Emitted when the `supplyQgueue` is set to `newSupplyQueue`. event SetSupplyQueue(address indexed allocator, Id[] newSupplyQueue); diff --git a/test/forge/GuardianTest.sol b/test/forge/GuardianTest.sol index dc54c3c4..8a117af3 100644 --- a/test/forge/GuardianTest.sol +++ b/test/forge/GuardianTest.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.0; import "./helpers/IntegrationTest.sol"; +uint256 constant FEE = 0.1 ether; // 10% uint256 constant TIMELOCK = 1 weeks; contract GuardianTest is IntegrationTest { @@ -13,6 +14,10 @@ contract GuardianTest is IntegrationTest { function setUp() public override { super.setUp(); + vm.prank(OWNER); + vault.setFeeRecipient(FEE_RECIPIENT); + + _setFee(FEE); _setTimelock(TIMELOCK); _setGuardian(GUARDIAN); } @@ -28,7 +33,7 @@ contract GuardianTest is IntegrationTest { vault.submitGuardian(GUARDIAN); } - function testRevokeTimelockDecreased(uint256 timelock, uint256 elapsed) public { + function testGuardianRevokeTimelockDecreased(uint256 timelock, uint256 elapsed) public { timelock = bound(timelock, MIN_TIMELOCK, TIMELOCK - 1); elapsed = bound(elapsed, 0, TIMELOCK - 1); @@ -50,7 +55,29 @@ contract GuardianTest is IntegrationTest { assertEq(submittedAt, 0, "submittedAt"); } - function testRevokeCapIncreased(uint256 seed, uint256 cap, uint256 elapsed) public { + function testOwnerRevokeTimelockDecreased(uint256 timelock, uint256 elapsed) public { + timelock = bound(timelock, MIN_TIMELOCK, TIMELOCK - 1); + elapsed = bound(elapsed, 0, TIMELOCK - 1); + + vm.prank(OWNER); + vault.submitTimelock(timelock); + + vm.warp(block.timestamp + elapsed); + + vm.expectEmit(); + emit EventsLib.RevokeTimelock(OWNER, IPending(address(vault)).pendingTimelock()); + vm.prank(OWNER); + vault.revokeTimelock(); + + uint256 newTimelock = vault.timelock(); + (uint256 pendingTimelock, uint64 submittedAt) = vault.pendingTimelock(); + + assertEq(newTimelock, TIMELOCK, "newTimelock"); + assertEq(pendingTimelock, 0, "pendingTimelock"); + assertEq(submittedAt, 0, "submittedAt"); + } + + function testGuardianRevokeCapIncreased(uint256 seed, uint256 cap, uint256 elapsed) public { MarketParams memory marketParams = _randomMarketParams(seed); elapsed = bound(elapsed, 0, TIMELOCK - 1); cap = bound(cap, 1, type(uint192).max); @@ -76,7 +103,59 @@ contract GuardianTest is IntegrationTest { assertEq(submittedAt, 0, "submittedAt"); } - function testRevokeGuardian(uint256 elapsed) public { + function testCuratorRevokeCapIncreased(uint256 seed, uint256 cap, uint256 elapsed) public { + MarketParams memory marketParams = _randomMarketParams(seed); + elapsed = bound(elapsed, 0, TIMELOCK - 1); + cap = bound(cap, 1, type(uint192).max); + + vm.prank(OWNER); + vault.submitCap(marketParams, cap); + + vm.warp(block.timestamp + elapsed); + + Id id = marketParams.id(); + + vm.expectEmit(); + emit EventsLib.RevokeCap(CURATOR, id, IPending(address(vault)).pendingCap(id)); + vm.prank(CURATOR); + vault.revokeCap(id); + + (uint192 newCap, uint64 withdrawRank) = vault.config(id); + (uint256 pendingCap, uint64 submittedAt) = vault.pendingCap(id); + + assertEq(newCap, 0, "newCap"); + assertEq(withdrawRank, 0, "withdrawRank"); + assertEq(pendingCap, 0, "pendingCap"); + assertEq(submittedAt, 0, "submittedAt"); + } + + function testOwnerRevokeCapIncreased(uint256 seed, uint256 cap, uint256 elapsed) public { + MarketParams memory marketParams = _randomMarketParams(seed); + elapsed = bound(elapsed, 0, TIMELOCK - 1); + cap = bound(cap, 1, type(uint192).max); + + vm.prank(OWNER); + vault.submitCap(marketParams, cap); + + vm.warp(block.timestamp + elapsed); + + Id id = marketParams.id(); + + vm.expectEmit(); + emit EventsLib.RevokeCap(OWNER, id, IPending(address(vault)).pendingCap(id)); + vm.prank(OWNER); + vault.revokeCap(id); + + (uint192 newCap, uint64 withdrawRank) = vault.config(id); + (uint256 pendingCap, uint64 submittedAt) = vault.pendingCap(id); + + assertEq(newCap, 0, "newCap"); + assertEq(withdrawRank, 0, "withdrawRank"); + assertEq(pendingCap, 0, "pendingCap"); + assertEq(submittedAt, 0, "submittedAt"); + } + + function testGuardianRevokeGuardian(uint256 elapsed) public { elapsed = bound(elapsed, 0, TIMELOCK - 1); address guardian = makeAddr("Guardian2"); @@ -98,4 +177,55 @@ contract GuardianTest is IntegrationTest { assertEq(pendingGuardian, address(0), "pendingGuardian"); assertEq(submittedAt, 0, "submittedAt"); } + + function testOwnerRevokeGuardian(uint256 elapsed) public { + elapsed = bound(elapsed, 0, TIMELOCK - 1); + + address guardian = makeAddr("Guardian2"); + + vm.prank(OWNER); + vault.submitGuardian(guardian); + + vm.warp(block.timestamp + elapsed); + + vm.expectRevert(ErrorsLib.NotGuardian.selector); + vm.prank(OWNER); + vault.revokeGuardian(); + } + + function testOwnerRevokeFeeIncreased(uint256 fee, uint256 elapsed) public { + fee = bound(fee, FEE + 1, MAX_FEE); + elapsed = bound(elapsed, 0, TIMELOCK - 1); + + vm.prank(OWNER); + vault.submitFee(fee); + + vm.warp(block.timestamp + elapsed); + + vm.expectEmit(); + emit EventsLib.RevokeFee(OWNER, IPending(address(vault)).pendingFee()); + vm.prank(OWNER); + vault.revokeFee(); + + uint256 newFee = vault.fee(); + (uint256 pendingFee, uint64 submittedAt) = vault.pendingFee(); + + assertEq(newFee, FEE, "newFee"); + assertEq(pendingFee, 0, "pendingFee"); + assertEq(submittedAt, 0, "submittedAt"); + } + + function testGuardianRevokeFeeIncreased(uint256 fee, uint256 elapsed) public { + fee = bound(fee, FEE + 1, MAX_FEE); + elapsed = bound(elapsed, 0, TIMELOCK - 1); + + vm.prank(OWNER); + vault.submitFee(fee); + + vm.warp(block.timestamp + elapsed); + + vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, GUARDIAN)); + vm.prank(GUARDIAN); + vault.revokeFee(); + } } From 83c642f2ae945281a3fe5f2dc03f8ebe3720fbbb Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Thu, 19 Oct 2023 09:34:50 +0200 Subject: [PATCH 02/30] fix(events): remove role description --- src/libraries/EventsLib.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/EventsLib.sol b/src/libraries/EventsLib.sol index 162b0857..83d47b74 100644 --- a/src/libraries/EventsLib.sol +++ b/src/libraries/EventsLib.sol @@ -48,16 +48,16 @@ library EventsLib { /// @notice Emitted when an `allocator` is set to `isAllocator`. event SetIsAllocator(address indexed allocator, bool isAllocator); - /// @notice Emitted when a `pendingTimelock` is revoked by `guardian` or `owner`. + /// @notice Emitted when a `pendingTimelock` is revoked. event RevokeTimelock(address indexed guardian, PendingUint192 pendingTimelock); - /// @notice Emitted when a `pendingCap` for the market identified by `id` is revoked by `guardian` or `owner`. + /// @notice Emitted when a `pendingCap` for the market identified by `id` is revoked. event RevokeCap(address indexed guardian, Id indexed id, PendingUint192 pendingCap); - /// @notice Emitted when a `pendingGuardian` is revoked by `guardian`. + /// @notice Emitted when a `pendingGuardian` is revoked. event RevokeGuardian(address indexed guardian, PendingAddress pendingGuardian); - /// @notice Emitted when a `pendingFee` is revoked by `guardian` or `owner`. + /// @notice Emitted when a `pendingFee` is revoked. event RevokeFee(address indexed guardian, PendingUint192 pendingFee); /// @notice Emitted when the `supplyQgueue` is set to `newSupplyQueue`. From f63c2a834aec7cba4c1220edf5bb9c59ea712cc2 Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Fri, 20 Oct 2023 16:51:50 +0200 Subject: [PATCH 03/30] fix(events): rename guardian to revoker --- src/MetaMorpho.sol | 12 ++++++------ src/libraries/ErrorsLib.sol | 4 ++-- src/libraries/EventsLib.sol | 8 ++++---- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index 358482d4..d69b7c6e 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -145,15 +145,15 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph } /// @dev Reverts if the caller is not the guardian. - modifier onlyOwnerOrGuardian() { - if (_msgSender() != owner() && _msgSender() != guardian) revert ErrorsLib.NotOwnerOrGuardian(); + modifier onlyOwnerNorGuardian() { + if (_msgSender() != owner() && _msgSender() != guardian) revert ErrorsLib.NotOwnerNorGuardian(); _; } - modifier onlyCuratorOrGuardian() { + modifier onlyCuratorNorGuardian() { if (_msgSender() != guardian && _msgSender() != curator && _msgSender() != owner()) { - revert ErrorsLib.NotCuratorOrGuardian(); + revert ErrorsLib.NotCuratorNorGuardian(); } _; @@ -404,7 +404,7 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph } /// @notice Revokes the `pendingTimelock`. - function revokeTimelock() external onlyOwnerOrGuardian { + function revokeTimelock() external onlyOwnerNorGuardian { emit EventsLib.RevokeTimelock(_msgSender(), pendingTimelock); delete pendingTimelock; @@ -418,7 +418,7 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph } /// @notice Revokes the pending cap of the market defined by `id`. - function revokeCap(Id id) external onlyCuratorOrGuardian { + function revokeCap(Id id) external onlyCuratorNorGuardian { emit EventsLib.RevokeCap(_msgSender(), id, pendingCap[id]); delete pendingCap[id]; diff --git a/src/libraries/ErrorsLib.sol b/src/libraries/ErrorsLib.sol index e4f47dcd..a97562ee 100644 --- a/src/libraries/ErrorsLib.sol +++ b/src/libraries/ErrorsLib.sol @@ -21,10 +21,10 @@ library ErrorsLib { error NotGuardian(); /// @notice Thrown when the caller is not the owner nor the guardian. - error NotOwnerOrGuardian(); + error NotOwnerNorGuardian(); /// @notice Thrown when the caller doesn't have the curator's privilege and is not the guardian. - error NotCuratorOrGuardian(); + error NotCuratorNorGuardian(); /// @notice Thrown when the market `id` cannot be set in the supply queue. error UnauthorizedMarket(Id id); diff --git a/src/libraries/EventsLib.sol b/src/libraries/EventsLib.sol index 83d47b74..78023ec0 100644 --- a/src/libraries/EventsLib.sol +++ b/src/libraries/EventsLib.sol @@ -49,16 +49,16 @@ library EventsLib { event SetIsAllocator(address indexed allocator, bool isAllocator); /// @notice Emitted when a `pendingTimelock` is revoked. - event RevokeTimelock(address indexed guardian, PendingUint192 pendingTimelock); + event RevokeTimelock(address indexed revoker, PendingUint192 pendingTimelock); /// @notice Emitted when a `pendingCap` for the market identified by `id` is revoked. - event RevokeCap(address indexed guardian, Id indexed id, PendingUint192 pendingCap); + event RevokeCap(address indexed revoker, Id indexed id, PendingUint192 pendingCap); /// @notice Emitted when a `pendingGuardian` is revoked. - event RevokeGuardian(address indexed guardian, PendingAddress pendingGuardian); + event RevokeGuardian(address indexed revoker, PendingAddress pendingGuardian); /// @notice Emitted when a `pendingFee` is revoked. - event RevokeFee(address indexed guardian, PendingUint192 pendingFee); + event RevokeFee(address indexed revoker, PendingUint192 pendingFee); /// @notice Emitted when the `supplyQgueue` is set to `newSupplyQueue`. event SetSupplyQueue(address indexed allocator, Id[] newSupplyQueue); From 3de7993a1b795d856857ee1cce887c8b89292b58 Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Fri, 20 Oct 2023 17:11:32 +0200 Subject: [PATCH 04/30] fix(roles): revert renaming --- README.md | 2 +- src/MetaMorpho.sol | 13 +++---------- src/libraries/ErrorsLib.sol | 3 --- 3 files changed, 4 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 5720d387..ee602ddc 100644 --- a/README.md +++ b/README.md @@ -45,12 +45,12 @@ Only one single address can have this role. It can: - Do whatever the curator and allocators can do. +- Do whatever the guardian can do. - Transfer or renounce the ownership. - Set the curator. - Set allocators. - Set the rewards recipient. - [Timelocked] Set the timelock. -- Revoke the pending timelock. - [Timelocked with no possible veto] Set the performance fee (capped to 50%). - Revoke the pending fee. - [Timelocked] Set the guardian. diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index d69b7c6e..07456463 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -139,19 +139,12 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph /// @dev Reverts if the caller is not the guardian. modifier onlyGuardian() { - if (_msgSender() != guardian) revert ErrorsLib.NotGuardian(); - - _; - } - - /// @dev Reverts if the caller is not the guardian. - modifier onlyOwnerNorGuardian() { if (_msgSender() != owner() && _msgSender() != guardian) revert ErrorsLib.NotOwnerNorGuardian(); _; } - modifier onlyCuratorNorGuardian() { + modifier onlyCuratorOrGuardian() { if (_msgSender() != guardian && _msgSender() != curator && _msgSender() != owner()) { revert ErrorsLib.NotCuratorNorGuardian(); } @@ -404,7 +397,7 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph } /// @notice Revokes the `pendingTimelock`. - function revokeTimelock() external onlyOwnerNorGuardian { + function revokeTimelock() external onlyGuardian { emit EventsLib.RevokeTimelock(_msgSender(), pendingTimelock); delete pendingTimelock; @@ -418,7 +411,7 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph } /// @notice Revokes the pending cap of the market defined by `id`. - function revokeCap(Id id) external onlyCuratorNorGuardian { + function revokeCap(Id id) external onlyCuratorOrGuardian { emit EventsLib.RevokeCap(_msgSender(), id, pendingCap[id]); delete pendingCap[id]; diff --git a/src/libraries/ErrorsLib.sol b/src/libraries/ErrorsLib.sol index a97562ee..5b45a7c2 100644 --- a/src/libraries/ErrorsLib.sol +++ b/src/libraries/ErrorsLib.sol @@ -17,9 +17,6 @@ library ErrorsLib { /// @notice Thrown when the caller doesn't have the allocator's privilege. error NotAllocator(); - /// @notice Thrown when the caller is not the guardian. - error NotGuardian(); - /// @notice Thrown when the caller is not the owner nor the guardian. error NotOwnerNorGuardian(); From acfaead39f8b79b1ceb139dbb2438c563b76c9a7 Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Fri, 20 Oct 2023 17:24:06 +0200 Subject: [PATCH 05/30] fix(roles): adapt tests --- src/MetaMorpho.sol | 2 +- src/libraries/ErrorsLib.sol | 6 +++--- test/forge/GuardianTest.sol | 12 ++++++++++-- test/metamorpho_tests.tree | 20 ++++++++++++++------ 4 files changed, 28 insertions(+), 12 deletions(-) diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index 07456463..f92bf000 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -139,7 +139,7 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph /// @dev Reverts if the caller is not the guardian. modifier onlyGuardian() { - if (_msgSender() != owner() && _msgSender() != guardian) revert ErrorsLib.NotOwnerNorGuardian(); + if (_msgSender() != owner() && _msgSender() != guardian) revert ErrorsLib.NotGuardian(); _; } diff --git a/src/libraries/ErrorsLib.sol b/src/libraries/ErrorsLib.sol index 5b45a7c2..95874452 100644 --- a/src/libraries/ErrorsLib.sol +++ b/src/libraries/ErrorsLib.sol @@ -17,10 +17,10 @@ library ErrorsLib { /// @notice Thrown when the caller doesn't have the allocator's privilege. error NotAllocator(); - /// @notice Thrown when the caller is not the owner nor the guardian. - error NotOwnerNorGuardian(); + /// @notice Thrown when the caller doesn't have the guardian's privilege. + error NotGuardian(); - /// @notice Thrown when the caller doesn't have the curator's privilege and is not the guardian. + /// @notice Thrown when the caller doesn't have the curator's privilege nor the guardian's privilege. error NotCuratorNorGuardian(); /// @notice Thrown when the market `id` cannot be set in the supply queue. diff --git a/test/forge/GuardianTest.sol b/test/forge/GuardianTest.sol index f7734af5..d2d5f04f 100644 --- a/test/forge/GuardianTest.sol +++ b/test/forge/GuardianTest.sol @@ -188,9 +188,17 @@ contract GuardianTest is IntegrationTest { vm.warp(block.timestamp + elapsed); - vm.expectRevert(ErrorsLib.NotGuardian.selector); - vm.prank(OWNER); + vm.expectEmit(); + emit EventsLib.RevokeGuardian(GUARDIAN, IPending(address(vault)).pendingGuardian()); + vm.prank(GUARDIAN); vault.revokeGuardian(); + + address newGuardian = vault.guardian(); + (address pendingGuardian, uint96 submittedAt) = vault.pendingGuardian(); + + assertEq(newGuardian, GUARDIAN, "newGuardian"); + assertEq(pendingGuardian, address(0), "pendingGuardian"); + assertEq(submittedAt, 0, "submittedAt"); } function testOwnerRevokeFeeIncreased(uint256 fee, uint256 elapsed) public { diff --git a/test/metamorpho_tests.tree b/test/metamorpho_tests.tree index 6d2e8f79..b2e69b8b 100644 --- a/test/metamorpho_tests.tree +++ b/test/metamorpho_tests.tree @@ -280,12 +280,20 @@ └── it should emit TransferRewards(msg.sender, rewardsRecipient, token, amount) -/* ONLY GUARDIAN FUNCTIONS */ +/* REVOKE FUNCTIONS */ +. +└── revokeFee() external + ├── when msg.sender not owner + │ └── revert with OwnableUnauthorizedAccount() + └── when msg.sender is owner + ├── it should emit RevokeFee(msg.sender, pendingFee) + └── it should delete pendingFee + . └── revokeTimelock() external - ├── when msg.sender not guardian + ├── when msg.sender not guardian nor owner │ └── revert with NotGuardian() └── when msg.sender is guardian ├── it should emit RevokeTimelock(msg.sender, pendingTimelock) @@ -293,15 +301,15 @@ . └── revokeCap(Id id) external - ├── when msg.sender not guardian - │ └── revert with NotGuardian() - └── when msg.sender is guardian + ├── when msg.sender not curator nor guardian nor owner + │ └── revert with NotCuratorNorGuardian() + └── when msg.sender is curator or guardian or owner ├── it should emit RevokeCap(msg.sender, id, pendingCap[id]) └── it should delete pendingCap[id] . └── revokeGuardian() external - ├── when msg.sender not guardian + ├── when msg.sender not guardian nor owner │ └── revert with NotGuardian() └── when msg.sender is guardian ├── it should emit RevokeGuardian(msg.sender, pendingGuardian); From 7ad08fce666f166397b7cc121687405c2ec681b5 Mon Sep 17 00:00:00 2001 From: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com> Date: Fri, 20 Oct 2023 19:38:45 +0200 Subject: [PATCH 06/30] test: rename test Signed-off-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com> --- test/forge/GuardianTest.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/forge/GuardianTest.sol b/test/forge/GuardianTest.sol index d2d5f04f..c4924643 100644 --- a/test/forge/GuardianTest.sol +++ b/test/forge/GuardianTest.sol @@ -223,7 +223,7 @@ contract GuardianTest is IntegrationTest { assertEq(submittedAt, 0, "submittedAt"); } - function testGuardianRevokeFeeIncreased(uint256 fee, uint256 elapsed) public { + function testGuardianShouldNotRevokeFeeIncreased(uint256 fee, uint256 elapsed) public { fee = bound(fee, FEE + 1, ConstantsLib.MAX_FEE); elapsed = bound(elapsed, 0, TIMELOCK - 1); From 723dfb3ed54fed73f333a9cfa60b819b6a5c0704 Mon Sep 17 00:00:00 2001 From: MerlinEgalite Date: Mon, 23 Oct 2023 15:13:20 +0300 Subject: [PATCH 07/30] refactor: remove timelock for fee --- src/MetaMorpho.sol | 43 +++---------- src/interfaces/IMetaMorpho.sol | 5 +- src/libraries/EventsLib.sol | 3 - test/forge/FeeTest.sol | 23 +++++-- test/forge/RoleTest.sol | 2 +- test/forge/TimelockTest.sol | 85 -------------------------- test/forge/helpers/IntegrationTest.sol | 9 +-- test/hardhat/MetaMorpho.spec.ts | 2 +- test/metamorpho_tests.tree | 4 +- 9 files changed, 32 insertions(+), 144 deletions(-) diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index 3219f9ee..0fa9b7d5 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -77,9 +77,6 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph /// @notice The pending timelock. PendingUint192 public pendingTimelock; - /// @notice The pending fee. - PendingUint192 public pendingFee; - /// @dev Stores the order of markets on which liquidity is supplied upon deposit. /// @dev Can contain any market. A market is skipped as soon as its supply cap is reached. Id[] public supplyQueue; @@ -209,21 +206,19 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph } } - /// @notice Submits a `newFee`. - /// @dev In case the new fee is lower than the current one, the fee is set immediately. - /// @dev Warning: Submitting a fee will overwrite the current pending fee. - function submitFee(uint256 newFee) external onlyOwner { + /// @notice Sets the `fee` to `newFee`. + function setFee(uint256 newFee) external onlyOwner { if (newFee == fee) revert ErrorsLib.AlreadySet(); if (newFee > ConstantsLib.MAX_FEE) revert ErrorsLib.MaxFeeExceeded(); + if (newFee != 0 && feeRecipient == address(0)) revert ErrorsLib.ZeroFeeRecipient(); - if (newFee < fee) { - _setFee(newFee); - } else { - // Safe "unchecked" cast because newFee <= MAX_FEE. - pendingFee = PendingUint192(uint192(newFee), uint64(block.timestamp)); + // Accrue interest using the previous fee set before changing it. + _updateLastTotalAssets(_accrueFee()); - emit EventsLib.SubmitFee(newFee); - } + // Safe "unchecked" cast because newFee <= MAX_FEE. + fee = uint96(newFee); + + emit EventsLib.SetFee(fee); } /// @notice Sets `feeRecipient` to `newFeeRecipient`. @@ -436,11 +431,6 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph _setTimelock(pendingTimelock.value); } - /// @notice Accepts the `pendingFee`. - function acceptFee() external withinTimelockWindow(pendingFee.submittedAt) { - _setFee(pendingFee.value); - } - /// @notice Accepts the `pendingGuardian`. function acceptGuardian() external withinTimelockWindow(pendingGuardian.submittedAt) { _setGuardian(pendingGuardian.value); @@ -686,21 +676,6 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph delete pendingCap[id]; } - /// @dev Sets `fee` to `newFee`. - function _setFee(uint256 newFee) internal { - if (newFee != 0 && feeRecipient == address(0)) revert ErrorsLib.ZeroFeeRecipient(); - - // Accrue interest using the previous fee set before changing it. - _updateLastTotalAssets(_accrueFee()); - - // Safe "unchecked" cast because newFee <= MAX_FEE. - fee = uint96(newFee); - - emit EventsLib.SetFee(newFee); - - delete pendingFee; - } - /* LIQUIDITY ALLOCATION */ /// @dev Supplies `assets` to Morpho and increase the idle liquidity if necessary. diff --git a/src/interfaces/IMetaMorpho.sol b/src/interfaces/IMetaMorpho.sol index 99ce58b6..e36037f7 100644 --- a/src/interfaces/IMetaMorpho.sol +++ b/src/interfaces/IMetaMorpho.sol @@ -65,10 +65,6 @@ interface IMetaMorpho is IERC4626 { function revokeCap(Id id) external; function pendingCap(Id) external view returns (uint192 value, uint64 submittedAt); - function submitFee(uint256 newFee) external; - function acceptFee() external; - function pendingFee() external view returns (uint192 value, uint64 submittedAt); - function submitGuardian(address newGuardian) external; function acceptGuardian() external; function revokeGuardian() external; @@ -78,6 +74,7 @@ interface IMetaMorpho is IERC4626 { function setIsAllocator(address newAllocator, bool newIsAllocator) external; function setCurator(address newCurator) external; + function setFee(uint256 newFee) external; function setFeeRecipient(address newFeeRecipient) external; function setRewardsRecipient(address) external; diff --git a/src/libraries/EventsLib.sol b/src/libraries/EventsLib.sol index fceea864..22fdb41d 100644 --- a/src/libraries/EventsLib.sol +++ b/src/libraries/EventsLib.sol @@ -18,9 +18,6 @@ library EventsLib { /// @notice Emitted `rewardsDistibutor` is set to `newRewardsRecipient`. event SetRewardsRecipient(address indexed newRewardsRecipient); - /// @notice Emitted when a pending `newFee` is submitted. - event SubmitFee(uint256 newFee); - /// @notice Emitted `fee` is set to `newFee`. event SetFee(uint256 newFee); diff --git a/test/forge/FeeTest.sol b/test/forge/FeeTest.sol index 85eb1c40..74877f90 100644 --- a/test/forge/FeeTest.sol +++ b/test/forge/FeeTest.sol @@ -40,6 +40,17 @@ contract FeeTest is IntegrationTest { _setCap(allMarkets[0], CAP); } + function testSetFee(uint256 fee) public { + fee = bound(fee, 0, ConstantsLib.MAX_FEE); + + vm.prank(OWNER); + vm.expectEmit(); + emit EventsLib.SetFee(fee); + vault.setFee(fee); + + assertEq(vault.fee(), fee, "fee"); + } + function _feeShares(uint256 totalAssetsBefore) internal view returns (uint256) { uint256 totalAssetsAfter = vault.totalAssets(); uint256 interest = totalAssetsAfter - totalAssetsBefore; @@ -248,23 +259,23 @@ contract FeeTest is IntegrationTest { assertEq(vault.balanceOf(address(1)), 0, "vault.balanceOf(address(1))"); } - function testSubmitFeeNotOwner(uint256 fee) public { + function testSetFeeNotOwner(uint256 fee) public { vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, address(this))); - vault.submitFee(fee); + vault.setFee(fee); } - function testSubmitFeeMaxFeeExceeded(uint256 fee) public { + function testSetFeeMaxFeeExceeded(uint256 fee) public { fee = bound(fee, ConstantsLib.MAX_FEE + 1, type(uint256).max); vm.prank(OWNER); vm.expectRevert(ErrorsLib.MaxFeeExceeded.selector); - vault.submitFee(fee); + vault.setFee(fee); } - function testSubmitFeeAlreadySet() public { + function testSetFeeAlreadySet() public { vm.prank(OWNER); vm.expectRevert(ErrorsLib.AlreadySet.selector); - vault.submitFee(FEE); + vault.setFee(FEE); } function testSetFeeRecipientAlreadySet() public { diff --git a/test/forge/RoleTest.sol b/test/forge/RoleTest.sol index 033c2a24..16a34dee 100644 --- a/test/forge/RoleTest.sol +++ b/test/forge/RoleTest.sol @@ -64,7 +64,7 @@ contract RoleTest is IntegrationTest { vault.submitTimelock(1); vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, address(caller))); - vault.submitFee(1); + vault.setFee(1); vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, address(caller))); vault.submitGuardian(address(1)); diff --git a/test/forge/TimelockTest.sol b/test/forge/TimelockTest.sol index 04230e9d..1df37aac 100644 --- a/test/forge/TimelockTest.sol +++ b/test/forge/TimelockTest.sol @@ -148,91 +148,6 @@ contract TimelockTest is IntegrationTest { vault.acceptTimelock(); } - function testSubmitFeeDecreased(uint256 fee) public { - fee = bound(fee, 0, FEE - 1); - - vm.expectEmit(); - emit EventsLib.UpdateLastTotalAssets(vault.totalAssets()); - emit EventsLib.SetFee(fee); - vm.prank(OWNER); - vault.submitFee(fee); - - uint256 newFee = vault.fee(); - (uint256 pendingFee, uint64 submittedAt) = vault.pendingFee(); - - assertEq(newFee, fee, "newFee"); - assertEq(pendingFee, 0, "pendingFee"); - assertEq(submittedAt, 0, "submittedAt"); - } - - function testSubmitFeeIncreased(uint256 fee) public { - fee = bound(fee, FEE + 1, ConstantsLib.MAX_FEE); - - vm.expectEmit(); - emit EventsLib.SubmitFee(fee); - vm.prank(OWNER); - vault.submitFee(fee); - - uint256 newFee = vault.fee(); - (uint256 pendingFee, uint64 submittedAt) = vault.pendingFee(); - - assertEq(newFee, FEE, "newFee"); - assertEq(pendingFee, fee, "pendingFee"); - assertEq(submittedAt, block.timestamp, "submittedAt"); - } - - function testAcceptFee(uint256 fee) public { - fee = bound(fee, FEE + 1, ConstantsLib.MAX_FEE); - - vm.prank(OWNER); - vault.submitFee(fee); - - vm.warp(block.timestamp + TIMELOCK); - - vm.expectEmit(); - emit EventsLib.UpdateLastTotalAssets(vault.totalAssets()); - emit EventsLib.SetFee(fee); - vault.acceptFee(); - - uint256 newFee = vault.fee(); - (uint256 pendingFee, uint64 submittedAt) = vault.pendingFee(); - - assertEq(newFee, fee, "newFee"); - assertEq(pendingFee, 0, "pendingFee"); - assertEq(submittedAt, 0, "submittedAt"); - } - - function testAcceptFeeNoPendingValue() public { - vm.expectRevert(ErrorsLib.NoPendingValue.selector); - vault.acceptFee(); - } - - function testAcceptFeeTimelockNotElapsed(uint256 fee, uint256 elapsed) public { - fee = bound(fee, FEE + 1, ConstantsLib.MAX_FEE); - elapsed = bound(elapsed, 1, TIMELOCK - 1); - - vm.prank(OWNER); - vault.submitFee(fee); - - vm.warp(block.timestamp + elapsed); - - vm.expectRevert(ErrorsLib.TimelockNotElapsed.selector); - vault.acceptFee(); - } - - function testAcceptFeeTimelockExpirationExceeded(uint256 fee, uint256 elapsed) public { - fee = bound(fee, FEE + 1, ConstantsLib.MAX_FEE); - elapsed = bound(elapsed, TIMELOCK + ConstantsLib.TIMELOCK_EXPIRATION + 1, type(uint64).max); - - vm.prank(OWNER); - vault.submitFee(fee); - - vm.warp(block.timestamp + elapsed); - - vm.expectRevert(ErrorsLib.TimelockExpirationExceeded.selector); - vault.acceptFee(); - } - function testSubmitGuardian() public { address guardian = makeAddr("Guardian2"); diff --git a/test/forge/helpers/IntegrationTest.sol b/test/forge/helpers/IntegrationTest.sol index e9f453ef..1dd9f536 100644 --- a/test/forge/helpers/IntegrationTest.sol +++ b/test/forge/helpers/IntegrationTest.sol @@ -75,14 +75,7 @@ contract IntegrationTest is BaseTest { if (newFee == fee) return; vm.prank(OWNER); - vault.submitFee(newFee); - - uint256 timelock = vault.timelock(); - if (newFee < fee || timelock == 0) return; - - vm.warp(block.timestamp + timelock); - - vault.acceptFee(); + vault.setFee(newFee); assertEq(vault.fee(), newFee, "_setFee"); } diff --git a/test/hardhat/MetaMorpho.spec.ts b/test/hardhat/MetaMorpho.spec.ts index 872d982e..e93bd065 100644 --- a/test/hardhat/MetaMorpho.spec.ts +++ b/test/hardhat/MetaMorpho.spec.ts @@ -204,7 +204,7 @@ describe("MetaMorpho", () => { await metaMorpho.setIsAllocator(allocator.address, true); await metaMorpho.setFeeRecipient(admin.address); - await metaMorpho.submitFee(BigInt.WAD / 10n); + await metaMorpho.setFee(BigInt.WAD / 10n); await forwardTimestamp(timelock); await metaMorpho.connect(admin).acceptFee(); diff --git a/test/metamorpho_tests.tree b/test/metamorpho_tests.tree index c5f1ff0f..959f37a2 100644 --- a/test/metamorpho_tests.tree +++ b/test/metamorpho_tests.tree @@ -72,7 +72,7 @@ └── it should delete pendingTimelock . -└── submitFee(uint256 newFee) external +└── setFee(uint256 newFee) external ├── when msg.sender not owner │ └── revert with NOT_OWNER └── when msg.sender is owner @@ -89,7 +89,7 @@ │ └── it should delete pendingFee └── when newFee > fee ├── it should set pendingFee to PendingUint192(uint192(newFee), uint64(block.timestamp)) - └── it should emit SubmitFee(newFee) + └── it should emit SetFee(newFee) . └── acceptFee() external From 83e00be2862e7d0cc785267ef9507dbbade2bf61 Mon Sep 17 00:00:00 2001 From: MerlinEgalite Date: Mon, 23 Oct 2023 15:27:33 +0300 Subject: [PATCH 08/30] test: fix hardhat and tree --- test/hardhat/MetaMorpho.spec.ts | 3 --- test/metamorpho_tests.tree | 24 ++---------------------- 2 files changed, 2 insertions(+), 25 deletions(-) diff --git a/test/hardhat/MetaMorpho.spec.ts b/test/hardhat/MetaMorpho.spec.ts index e93bd065..8fefb93e 100644 --- a/test/hardhat/MetaMorpho.spec.ts +++ b/test/hardhat/MetaMorpho.spec.ts @@ -206,9 +206,6 @@ describe("MetaMorpho", () => { await metaMorpho.setFeeRecipient(admin.address); await metaMorpho.setFee(BigInt.WAD / 10n); - await forwardTimestamp(timelock); - await metaMorpho.connect(admin).acceptFee(); - supplyCap = (BigInt.WAD * 20n * toBigInt(suppliers.length)) / toBigInt(nbMarkets); for (const marketParams of allMarketParams) { await metaMorpho.connect(curator).submitCap(marketParams, supplyCap); diff --git a/test/metamorpho_tests.tree b/test/metamorpho_tests.tree index 959f37a2..74c107e6 100644 --- a/test/metamorpho_tests.tree +++ b/test/metamorpho_tests.tree @@ -82,29 +82,9 @@ ├── when newFee == fee │ └── revert with AlreadySet() └── when newFee != fee - ├── when newFee < fee - │ ├── it should accrue fees - │ ├── it should set fee to newFee - │ ├── it should emit SetFee(newFee) - │ └── it should delete pendingFee - └── when newFee > fee - ├── it should set pendingFee to PendingUint192(uint192(newFee), uint64(block.timestamp)) - └── it should emit SetFee(newFee) - -. -└── acceptFee() external - ├── when pendingFee.submittedAt == 0 - │ └── revert with NoPendingValue() - └── when pendingFee.submittedAt != 0 - ├── when block.timestamp < pendingFee.submittedAt + timelock - │ └── revert with TimelockNotElapsed() - └── when block.timestamp >= pendingFee.submittedAt + timelock - ├── when block.timestamp > pendingFee.submittedAt + timelock + TIMELOCK_EXPIRATION - │ └── revert with TIMELOCK_EXPIRATION_EXCEEDED - └── whenblock.timestamp <= pendingFee.submittedAt + timelock + TIMELOCK_EXPIRATION ├── it should accrue fees - ├── it should set fee to pendingFee.value - ├── it should emit SetFee(pendingFee.value) + ├── it should set fee to newFee + ├── it should emit SetFee(newFee) └── it should delete pendingFee . From bc670b78db069cb85c0e73d64e5a7f9b808f4949 Mon Sep 17 00:00:00 2001 From: MerlinEgalite Date: Mon, 23 Oct 2023 15:50:08 +0300 Subject: [PATCH 09/30] test: prank after emit --- test/forge/FeeTest.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/forge/FeeTest.sol b/test/forge/FeeTest.sol index 74877f90..dfeb4e95 100644 --- a/test/forge/FeeTest.sol +++ b/test/forge/FeeTest.sol @@ -43,9 +43,9 @@ contract FeeTest is IntegrationTest { function testSetFee(uint256 fee) public { fee = bound(fee, 0, ConstantsLib.MAX_FEE); - vm.prank(OWNER); vm.expectEmit(); emit EventsLib.SetFee(fee); + vm.prank(OWNER); vault.setFee(fee); assertEq(vault.fee(), fee, "fee"); From 302785555896e9993c2318b365308ef1f39d8008 Mon Sep 17 00:00:00 2001 From: MerlinEgalite Date: Wed, 25 Oct 2023 13:46:56 +0300 Subject: [PATCH 10/30] refactor: pending values close to current ones --- src/MetaMorpho.sol | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index 0fa9b7d5..0d30d5de 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -59,15 +59,6 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph /// @notice The current timelock. uint256 public timelock; - /// @notice The current fee. - uint96 public fee; - - /// @notice The fee recipient. - address public feeRecipient; - - /// @notice The rewards recipient. - address public rewardsRecipient; - /// @notice The pending guardian. PendingAddress public pendingGuardian; @@ -77,6 +68,15 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph /// @notice The pending timelock. PendingUint192 public pendingTimelock; + /// @notice The current fee. + uint96 public fee; + + /// @notice The fee recipient. + address public feeRecipient; + + /// @notice The rewards recipient. + address public rewardsRecipient; + /// @dev Stores the order of markets on which liquidity is supplied upon deposit. /// @dev Can contain any market. A market is skipped as soon as its supply cap is reached. Id[] public supplyQueue; From 109321e07f16803be1ccfa764116aef9c21dc61d Mon Sep 17 00:00:00 2001 From: MerlinEgalite Date: Wed, 25 Oct 2023 13:57:39 +0300 Subject: [PATCH 11/30] test: add missing test --- test/forge/FeeTest.sol | 14 ++++++++++++++ test/metamorpho_tests.tree | 11 +++++++---- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/test/forge/FeeTest.sol b/test/forge/FeeTest.sol index dfeb4e95..bdfacef8 100644 --- a/test/forge/FeeTest.sol +++ b/test/forge/FeeTest.sol @@ -278,6 +278,20 @@ contract FeeTest is IntegrationTest { vault.setFee(FEE); } + function testSetFeeZeroFeeRecipient(uint256 fee) public { + fee = bound(fee, 1, ConstantsLib.MAX_FEE); + + vm.startPrank(OWNER); + + vault.setFee(0); + vault.setFeeRecipient(address(0)); + + vm.expectRevert(ErrorsLib.ZeroFeeRecipient.selector); + vault.setFee(fee); + + vm.stopPrank(); + } + function testSetFeeRecipientAlreadySet() public { vm.prank(OWNER); vm.expectRevert(ErrorsLib.AlreadySet.selector); diff --git a/test/metamorpho_tests.tree b/test/metamorpho_tests.tree index 74c107e6..674b9d0a 100644 --- a/test/metamorpho_tests.tree +++ b/test/metamorpho_tests.tree @@ -82,10 +82,13 @@ ├── when newFee == fee │ └── revert with AlreadySet() └── when newFee != fee - ├── it should accrue fees - ├── it should set fee to newFee - ├── it should emit SetFee(newFee) - └── it should delete pendingFee + ├── when newFee != 0 and feeRecipient == address(0) + │ └── revert with ZeroFeeRecipient() + └── when newFee == 0 or feeRecipient != address(0) + ├── it should accrue fees + ├── it should set fee to newFee + ├── it should emit SetFee(newFee) + └── it should delete pendingFee . └── setFeeRecipient(address newFeeRecipient) external From dc8b76b586b0bb0e8ad46d857810f849624a7959 Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Fri, 27 Oct 2023 14:28:33 +0200 Subject: [PATCH 12/30] fix(revoke): revert if no pending value --- src/MetaMorpho.sol | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index 722db6e5..bd0ad730 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -408,6 +408,8 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph /// @notice Revokes the pending cap of the market defined by `id`. function revokeFee() external onlyOwner { + if (pendingFee.value == 0) revert ErrorsLib.NoPendingValue(); + emit EventsLib.RevokeFee(_msgSender(), pendingFee); delete pendingFee; @@ -415,6 +417,8 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph /// @notice Revokes the `pendingTimelock`. function revokeTimelock() external onlyGuardian { + if (pendingTimelock.value == 0) revert ErrorsLib.NoPendingValue(); + emit EventsLib.RevokeTimelock(_msgSender(), pendingTimelock); delete pendingTimelock; @@ -429,6 +433,8 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph /// @notice Revokes the pending cap of the market defined by `id`. function revokeCap(Id id) external onlyCuratorOrGuardian { + if (pendingCap[id].value == 0) revert ErrorsLib.NoPendingValue(); + emit EventsLib.RevokeCap(_msgSender(), id, pendingCap[id]); delete pendingCap[id]; From 6756888c9ba6af7440d3e3d7b13386c8022dbccf Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Fri, 27 Oct 2023 14:35:14 +0200 Subject: [PATCH 13/30] fix(revoke): revert with no pending value --- test/forge/GuardianTest.sol | 97 --------------------- test/forge/RevokeTest.sol | 163 ++++++++++++++++++++++++++++++++++++ test/metamorpho_tests.tree | 6 ++ 3 files changed, 169 insertions(+), 97 deletions(-) create mode 100644 test/forge/RevokeTest.sol diff --git a/test/forge/GuardianTest.sol b/test/forge/GuardianTest.sol index d2d5f04f..4cae4872 100644 --- a/test/forge/GuardianTest.sol +++ b/test/forge/GuardianTest.sol @@ -103,58 +103,6 @@ contract GuardianTest is IntegrationTest { assertEq(submittedAt, 0, "submittedAt"); } - function testCuratorRevokeCapIncreased(uint256 seed, uint256 cap, uint256 elapsed) public { - MarketParams memory marketParams = _randomMarketParams(seed); - elapsed = bound(elapsed, 0, TIMELOCK - 1); - cap = bound(cap, 1, type(uint192).max); - - vm.prank(OWNER); - vault.submitCap(marketParams, cap); - - vm.warp(block.timestamp + elapsed); - - Id id = marketParams.id(); - - vm.expectEmit(); - emit EventsLib.RevokeCap(CURATOR, id, IPending(address(vault)).pendingCap(id)); - vm.prank(CURATOR); - vault.revokeCap(id); - - (uint192 newCap, uint64 withdrawRank) = vault.config(id); - (uint256 pendingCap, uint64 submittedAt) = vault.pendingCap(id); - - assertEq(newCap, 0, "newCap"); - assertEq(withdrawRank, 0, "withdrawRank"); - assertEq(pendingCap, 0, "pendingCap"); - assertEq(submittedAt, 0, "submittedAt"); - } - - function testOwnerRevokeCapIncreased(uint256 seed, uint256 cap, uint256 elapsed) public { - MarketParams memory marketParams = _randomMarketParams(seed); - elapsed = bound(elapsed, 0, TIMELOCK - 1); - cap = bound(cap, 1, type(uint192).max); - - vm.prank(OWNER); - vault.submitCap(marketParams, cap); - - vm.warp(block.timestamp + elapsed); - - Id id = marketParams.id(); - - vm.expectEmit(); - emit EventsLib.RevokeCap(OWNER, id, IPending(address(vault)).pendingCap(id)); - vm.prank(OWNER); - vault.revokeCap(id); - - (uint192 newCap, uint64 withdrawRank) = vault.config(id); - (uint256 pendingCap, uint64 submittedAt) = vault.pendingCap(id); - - assertEq(newCap, 0, "newCap"); - assertEq(withdrawRank, 0, "withdrawRank"); - assertEq(pendingCap, 0, "pendingCap"); - assertEq(submittedAt, 0, "submittedAt"); - } - function testGuardianRevokeGuardian(uint256 elapsed) public { elapsed = bound(elapsed, 0, TIMELOCK - 1); @@ -178,51 +126,6 @@ contract GuardianTest is IntegrationTest { assertEq(submittedAt, 0, "submittedAt"); } - function testOwnerRevokeGuardian(uint256 elapsed) public { - elapsed = bound(elapsed, 0, TIMELOCK - 1); - - address guardian = makeAddr("Guardian2"); - - vm.prank(OWNER); - vault.submitGuardian(guardian); - - vm.warp(block.timestamp + elapsed); - - vm.expectEmit(); - emit EventsLib.RevokeGuardian(GUARDIAN, IPending(address(vault)).pendingGuardian()); - vm.prank(GUARDIAN); - vault.revokeGuardian(); - - address newGuardian = vault.guardian(); - (address pendingGuardian, uint96 submittedAt) = vault.pendingGuardian(); - - assertEq(newGuardian, GUARDIAN, "newGuardian"); - assertEq(pendingGuardian, address(0), "pendingGuardian"); - assertEq(submittedAt, 0, "submittedAt"); - } - - function testOwnerRevokeFeeIncreased(uint256 fee, uint256 elapsed) public { - fee = bound(fee, FEE + 1, ConstantsLib.MAX_FEE); - elapsed = bound(elapsed, 0, TIMELOCK - 1); - - vm.prank(OWNER); - vault.submitFee(fee); - - vm.warp(block.timestamp + elapsed); - - vm.expectEmit(); - emit EventsLib.RevokeFee(OWNER, IPending(address(vault)).pendingFee()); - vm.prank(OWNER); - vault.revokeFee(); - - uint256 newFee = vault.fee(); - (uint256 pendingFee, uint64 submittedAt) = vault.pendingFee(); - - assertEq(newFee, FEE, "newFee"); - assertEq(pendingFee, 0, "pendingFee"); - assertEq(submittedAt, 0, "submittedAt"); - } - function testGuardianRevokeFeeIncreased(uint256 fee, uint256 elapsed) public { fee = bound(fee, FEE + 1, ConstantsLib.MAX_FEE); elapsed = bound(elapsed, 0, TIMELOCK - 1); diff --git a/test/forge/RevokeTest.sol b/test/forge/RevokeTest.sol new file mode 100644 index 00000000..3ae1469b --- /dev/null +++ b/test/forge/RevokeTest.sol @@ -0,0 +1,163 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +pragma solidity ^0.8.0; + +import "./helpers/IntegrationTest.sol"; + +uint256 constant FEE = 0.1 ether; // 10% +uint256 constant TIMELOCK = 1 weeks; + +contract RevokeTest is IntegrationTest { + using Math for uint256; + using MathLib for uint256; + using MarketParamsLib for MarketParams; + + function setUp() public override { + super.setUp(); + + vm.prank(OWNER); + vault.setFeeRecipient(FEE_RECIPIENT); + + _setFee(FEE); + _setTimelock(TIMELOCK); + _setGuardian(GUARDIAN); + } + + function testOwnerRevokeTimelockDecreased(uint256 timelock, uint256 elapsed) public { + timelock = bound(timelock, ConstantsLib.MIN_TIMELOCK, TIMELOCK - 1); + elapsed = bound(elapsed, 0, TIMELOCK - 1); + + vm.prank(OWNER); + vault.submitTimelock(timelock); + + vm.warp(block.timestamp + elapsed); + + vm.expectEmit(); + emit EventsLib.RevokeTimelock(OWNER, IPending(address(vault)).pendingTimelock()); + vm.prank(OWNER); + vault.revokeTimelock(); + + uint256 newTimelock = vault.timelock(); + (uint256 pendingTimelock, uint64 submittedAt) = vault.pendingTimelock(); + + assertEq(newTimelock, TIMELOCK, "newTimelock"); + assertEq(pendingTimelock, 0, "pendingTimelock"); + assertEq(submittedAt, 0, "submittedAt"); + } + + function testCuratorRevokeCapIncreased(uint256 seed, uint256 cap, uint256 elapsed) public { + MarketParams memory marketParams = _randomMarketParams(seed); + elapsed = bound(elapsed, 0, TIMELOCK - 1); + cap = bound(cap, 1, type(uint192).max); + + vm.prank(OWNER); + vault.submitCap(marketParams, cap); + + vm.warp(block.timestamp + elapsed); + + Id id = marketParams.id(); + + vm.expectEmit(); + emit EventsLib.RevokeCap(CURATOR, id, IPending(address(vault)).pendingCap(id)); + vm.prank(CURATOR); + vault.revokeCap(id); + + (uint192 newCap, uint64 withdrawRank) = vault.config(id); + (uint256 pendingCap, uint64 submittedAt) = vault.pendingCap(id); + + assertEq(newCap, 0, "newCap"); + assertEq(withdrawRank, 0, "withdrawRank"); + assertEq(pendingCap, 0, "pendingCap"); + assertEq(submittedAt, 0, "submittedAt"); + } + + function testOwnerRevokeCapIncreased(uint256 seed, uint256 cap, uint256 elapsed) public { + MarketParams memory marketParams = _randomMarketParams(seed); + elapsed = bound(elapsed, 0, TIMELOCK - 1); + cap = bound(cap, 1, type(uint192).max); + + vm.prank(OWNER); + vault.submitCap(marketParams, cap); + + vm.warp(block.timestamp + elapsed); + + Id id = marketParams.id(); + + vm.expectEmit(); + emit EventsLib.RevokeCap(OWNER, id, IPending(address(vault)).pendingCap(id)); + vm.prank(OWNER); + vault.revokeCap(id); + + (uint192 newCap, uint64 withdrawRank) = vault.config(id); + (uint256 pendingCap, uint64 submittedAt) = vault.pendingCap(id); + + assertEq(newCap, 0, "newCap"); + assertEq(withdrawRank, 0, "withdrawRank"); + assertEq(pendingCap, 0, "pendingCap"); + assertEq(submittedAt, 0, "submittedAt"); + } + + function testOwnerRevokeGuardian(uint256 elapsed) public { + elapsed = bound(elapsed, 0, TIMELOCK - 1); + + address guardian = makeAddr("Guardian2"); + + vm.prank(OWNER); + vault.submitGuardian(guardian); + + vm.warp(block.timestamp + elapsed); + + vm.expectEmit(); + emit EventsLib.RevokeGuardian(GUARDIAN, IPending(address(vault)).pendingGuardian()); + vm.prank(GUARDIAN); + vault.revokeGuardian(); + + address newGuardian = vault.guardian(); + (address pendingGuardian, uint96 submittedAt) = vault.pendingGuardian(); + + assertEq(newGuardian, GUARDIAN, "newGuardian"); + assertEq(pendingGuardian, address(0), "pendingGuardian"); + assertEq(submittedAt, 0, "submittedAt"); + } + + function testOwnerRevokeFeeIncreased(uint256 fee, uint256 elapsed) public { + fee = bound(fee, FEE + 1, ConstantsLib.MAX_FEE); + elapsed = bound(elapsed, 0, TIMELOCK - 1); + + vm.prank(OWNER); + vault.submitFee(fee); + + vm.warp(block.timestamp + elapsed); + + vm.expectEmit(); + emit EventsLib.RevokeFee(OWNER, IPending(address(vault)).pendingFee()); + vm.prank(OWNER); + vault.revokeFee(); + + uint256 newFee = vault.fee(); + (uint256 pendingFee, uint64 submittedAt) = vault.pendingFee(); + + assertEq(newFee, FEE, "newFee"); + assertEq(pendingFee, 0, "pendingFee"); + assertEq(submittedAt, 0, "submittedAt"); + } + + function testOwnerRevokeFeeNoPendingValue() public { + vm.prank(OWNER); + vm.expectRevert(ErrorsLib.NoPendingValue.selector); + vault.revokeFee(); + } + + function testOwnerRevokeCapNoPendingValue(uint256 seed) public { + MarketParams memory marketParams = _randomMarketParams(seed); + + vm.prank(OWNER); + vm.expectRevert(ErrorsLib.NoPendingValue.selector); + vault.revokeCap(marketParams.id()); + } + + function testOwnerRevokeTimelockNoPendingValue() public { + vm.prank(OWNER); + vm.expectRevert(ErrorsLib.NoPendingValue.selector); + vault.revokeTimelock(); + } +} diff --git a/test/metamorpho_tests.tree b/test/metamorpho_tests.tree index 0d1069c3..a6758da9 100644 --- a/test/metamorpho_tests.tree +++ b/test/metamorpho_tests.tree @@ -292,6 +292,8 @@ └── revokeFee() external ├── when msg.sender not owner │ └── revert with OwnableUnauthorizedAccount() + ├── when pending fee is zero + │ └── revert with NoPendingValue() └── when msg.sender is owner ├── it should emit RevokeFee(msg.sender, pendingFee) └── it should delete pendingFee @@ -300,6 +302,8 @@ └── revokeTimelock() external ├── when msg.sender not guardian nor owner │ └── revert with NotGuardian() + ├── when pending timelock is zero + │ └── revert with NoPendingValue() └── when msg.sender is guardian ├── it should emit RevokeTimelock(msg.sender, pendingTimelock) └── it should delete pendingTimelock @@ -308,6 +312,8 @@ └── revokeCap(Id id) external ├── when msg.sender not curator nor guardian nor owner │ └── revert with NotCuratorNorGuardian() + ├── when pending cap is zero + │ └── revert with NoPendingValue() └── when msg.sender is curator or guardian or owner ├── it should emit RevokeCap(msg.sender, id, pendingCap[id]) └── it should delete pendingCap[id] From 2a74f23e1cde09bb2588d70220d510390fe14d09 Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Mon, 30 Oct 2023 09:36:57 +0100 Subject: [PATCH 14/30] refactor(metamorpho): rename modifiers --- src/MetaMorpho.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index fe7e28b1..5d0f2a90 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -139,13 +139,13 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph } /// @dev Reverts if the caller is not the guardian. - modifier onlyGuardian() { + modifier onlyGuardianRole() { if (_msgSender() != owner() && _msgSender() != guardian) revert ErrorsLib.NotGuardian(); _; } - modifier onlyCuratorOrGuardian() { + modifier onlyCuratorOrGuardianRole() { if (_msgSender() != guardian && _msgSender() != curator && _msgSender() != owner()) { revert ErrorsLib.NotCuratorNorGuardian(); } @@ -402,7 +402,7 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph /* REVOKE FUNCTIONS */ /// @notice Revokes the pending timelock. - function revokeTimelock() external onlyGuardian { + function revokeTimelock() external onlyGuardianRole { if (pendingTimelock.value == 0) revert ErrorsLib.NoPendingValue(); emit EventsLib.RevokeTimelock(_msgSender(), pendingTimelock); @@ -411,14 +411,14 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph } /// @notice Revokes the pending guardian. - function revokeGuardian() external onlyGuardian { + function revokeGuardian() external onlyGuardianRole { emit EventsLib.RevokeGuardian(_msgSender(), pendingGuardian); delete pendingGuardian; } /// @notice Revokes the pending cap of the market defined by `id`. - function revokeCap(Id id) external onlyCuratorOrGuardian { + function revokeCap(Id id) external onlyCuratorOrGuardianRole { if (pendingCap[id].value == 0) revert ErrorsLib.NoPendingValue(); emit EventsLib.RevokeCap(_msgSender(), id, pendingCap[id]); From 2b7fbb32ebdbf5cf3037e074a168a99238355dc2 Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Mon, 30 Oct 2023 09:42:57 +0100 Subject: [PATCH 15/30] docs(README): update roles --- README.md | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/README.md b/README.md index 3e10a11c..b194bcb4 100644 --- a/README.md +++ b/README.md @@ -48,7 +48,6 @@ It can: - Set the rewards recipient. - [Timelocked] Set the timelock. - [Timelocked with no possible veto] Set the performance fee (capped to 50%). -- Revoke the pending fee. - [Timelocked] Set the guardian. - Set the fee recipient. @@ -87,10 +86,7 @@ It can: - Revoke the pending guardian. - Revoke the pending cap of any market. -In particular, the guardian: - -- Cannot revoke a pending fee. -- Can revoke any guardian change. +In particular, the guardian **can revoke any guardian change**. ### Rewards From 5344d01f9011e645105c8bc5e7bcee78b810646d Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Mon, 30 Oct 2023 10:36:40 +0100 Subject: [PATCH 16/30] fix(revoke-guardian): add submittedAt check --- src/MetaMorpho.sol | 6 ++++-- test/forge/RevokeTest.sol | 6 ++++++ test/metamorpho_tests.tree | 6 ++++-- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index 5d0f2a90..3019bbc0 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -403,7 +403,7 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph /// @notice Revokes the pending timelock. function revokeTimelock() external onlyGuardianRole { - if (pendingTimelock.value == 0) revert ErrorsLib.NoPendingValue(); + if (pendingTimelock.submittedAt == 0) revert ErrorsLib.NoPendingValue(); emit EventsLib.RevokeTimelock(_msgSender(), pendingTimelock); @@ -412,6 +412,8 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph /// @notice Revokes the pending guardian. function revokeGuardian() external onlyGuardianRole { + if (pendingGuardian.submittedAt == 0) revert ErrorsLib.NoPendingValue(); + emit EventsLib.RevokeGuardian(_msgSender(), pendingGuardian); delete pendingGuardian; @@ -419,7 +421,7 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph /// @notice Revokes the pending cap of the market defined by `id`. function revokeCap(Id id) external onlyCuratorOrGuardianRole { - if (pendingCap[id].value == 0) revert ErrorsLib.NoPendingValue(); + if (pendingCap[id].submittedAt == 0) revert ErrorsLib.NoPendingValue(); emit EventsLib.RevokeCap(_msgSender(), id, pendingCap[id]); diff --git a/test/forge/RevokeTest.sol b/test/forge/RevokeTest.sol index 5356932b..e3c878d5 100644 --- a/test/forge/RevokeTest.sol +++ b/test/forge/RevokeTest.sol @@ -132,4 +132,10 @@ contract RevokeTest is IntegrationTest { vm.expectRevert(ErrorsLib.NoPendingValue.selector); vault.revokeTimelock(); } + + function testOwnerRevokeGuardianNoPendingValue() public { + vm.prank(OWNER); + vm.expectRevert(ErrorsLib.NoPendingValue.selector); + vault.revokeGuardian(); + } } diff --git a/test/metamorpho_tests.tree b/test/metamorpho_tests.tree index e083f88d..4b335552 100644 --- a/test/metamorpho_tests.tree +++ b/test/metamorpho_tests.tree @@ -275,7 +275,7 @@ └── revokeTimelock() external ├── when msg.sender not guardian nor owner │ └── revert with NotGuardian() - ├── when pending timelock is zero + ├── when pending timelock's submittedAt is zero │ └── revert with NoPendingValue() └── when msg.sender is guardian ├── it should emit RevokeTimelock(msg.sender, pendingTimelock) @@ -285,7 +285,7 @@ └── revokeCap(Id id) external ├── when msg.sender not curator nor guardian nor owner │ └── revert with NotCuratorNorGuardian() - ├── when pending cap is zero + ├── when pending cap's submittedAt is zero │ └── revert with NoPendingValue() └── when msg.sender is curator or guardian or owner ├── it should emit RevokeCap(msg.sender, id, pendingCap[id]) @@ -295,6 +295,8 @@ └── revokeGuardian() external ├── when msg.sender not guardian nor owner │ └── revert with NotGuardian() + ├── when pending guardian's submittedAt is zero + │ └── revert with NoPendingValue() └── when msg.sender is guardian ├── it should emit RevokeGuardian(msg.sender, pendingGuardian); └── it should delete pendingGuardian From 15c51d4c886a0e9757b3e9fb4c63987948fea959 Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Thu, 2 Nov 2023 15:00:55 +0100 Subject: [PATCH 17/30] refactor(timelock): change submittedAt to validAt --- src/MetaMorpho.sol | 39 ++++++++++++++----------- src/interfaces/IMetaMorpho.sol | 24 ++++------------ src/libraries/EventsLib.sol | 3 +- src/libraries/PendingLib.sol | 38 +++++++++++++++++++++++++ test/forge/GuardianTest.sol | 12 ++++---- test/forge/TimelockTest.sol | 52 +++++++++++++++++----------------- test/metamorpho_tests.tree | 32 ++++++++++----------- 7 files changed, 117 insertions(+), 83 deletions(-) create mode 100644 src/libraries/PendingLib.sol diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index f0f4ac16..5702acd1 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -2,11 +2,10 @@ pragma solidity 0.8.21; import {IMorphoMarketParams} from "./interfaces/IMorphoMarketParams.sol"; -import { - IMetaMorpho, MarketConfig, PendingUint192, PendingAddress, MarketAllocation -} from "./interfaces/IMetaMorpho.sol"; +import {IMetaMorpho, MarketConfig, MarketAllocation} from "./interfaces/IMetaMorpho.sol"; import {Id, MarketParams, Market, IMorpho} from "@morpho-blue/interfaces/IMorpho.sol"; +import {PendingUint192, PendingAddress, PendingLib} from "./libraries/PendingLib.sol"; import {ConstantsLib} from "./libraries/ConstantsLib.sol"; import {ErrorsLib} from "./libraries/ErrorsLib.sol"; import {EventsLib} from "./libraries/EventsLib.sol"; @@ -36,6 +35,8 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph using SharesMathLib for uint256; using MorphoBalancesLib for IMorpho; using MarketParamsLib for MarketParams; + using PendingLib for PendingUint192; + using PendingLib for PendingAddress; /* IMMUTABLES */ @@ -152,9 +153,9 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph /// @dev Reverts if: /// - there's no pending value; /// - the timelock has not elapsed since the pending value has been submitted. - modifier afterTimelock(uint256 submittedAt) { - if (submittedAt == 0) revert ErrorsLib.NoPendingValue(); - if (block.timestamp < submittedAt + timelock) revert ErrorsLib.TimelockNotElapsed(); + modifier afterTimelock(uint256 validAt) { + if (validAt == 0) revert ErrorsLib.NoPendingValue(); + if (block.timestamp < validAt) revert ErrorsLib.TimelockNotElapsed(); _; } @@ -198,8 +199,11 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph if (newTimelock > timelock) { _setTimelock(newTimelock); } else { - // Safe "unchecked" cast because newTimelock <= MAX_TIMELOCK. - pendingTimelock = PendingUint192(uint192(newTimelock), uint64(block.timestamp)); + pendingTimelock.update( + // Safe "unchecked" cast because newTimelock <= MAX_TIMELOCK. + uint192(newTimelock), + timelock + ); emit EventsLib.SubmitTimelock(newTimelock); } @@ -215,8 +219,11 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph if (newFee < fee) { _setFee(newFee); } else { - // Safe "unchecked" cast because newFee <= MAX_FEE. - pendingFee = PendingUint192(uint192(newFee), uint64(block.timestamp)); + pendingFee.update( + // Safe "unchecked" cast because newFee <= MAX_FEE. + uint192(newFee), + timelock + ); emit EventsLib.SubmitFee(newFee); } @@ -245,7 +252,7 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph if (guardian == address(0)) { _setGuardian(newGuardian); } else { - pendingGuardian = PendingAddress(newGuardian, uint64(block.timestamp)); + pendingGuardian.update(newGuardian, timelock); emit EventsLib.SubmitGuardian(newGuardian); } @@ -267,7 +274,7 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph if (newSupplyCap < supplyCap) { _setCap(id, newSupplyCap.toUint192()); } else { - pendingCap[id] = PendingUint192(newSupplyCap.toUint192(), uint64(block.timestamp)); + pendingCap[id].update(newSupplyCap.toUint192(), timelock); emit EventsLib.SubmitCap(id, newSupplyCap); } @@ -428,22 +435,22 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph } /// @notice Accepts the `pendingTimelock`. - function acceptTimelock() external afterTimelock(pendingTimelock.submittedAt) { + function acceptTimelock() external afterTimelock(pendingTimelock.validAt) { _setTimelock(pendingTimelock.value); } /// @notice Accepts the `pendingFee`. - function acceptFee() external afterTimelock(pendingFee.submittedAt) { + function acceptFee() external afterTimelock(pendingFee.validAt) { _setFee(pendingFee.value); } /// @notice Accepts the `pendingGuardian`. - function acceptGuardian() external afterTimelock(pendingGuardian.submittedAt) { + function acceptGuardian() external afterTimelock(pendingGuardian.validAt) { _setGuardian(pendingGuardian.value); } /// @notice Accepts the pending cap of the market defined by `id`. - function acceptCap(Id id) external afterTimelock(pendingCap[id].submittedAt) { + function acceptCap(Id id) external afterTimelock(pendingCap[id].validAt) { _setCap(id, pendingCap[id].value); } diff --git a/src/interfaces/IMetaMorpho.sol b/src/interfaces/IMetaMorpho.sol index 99ce58b6..47b4cc43 100644 --- a/src/interfaces/IMetaMorpho.sol +++ b/src/interfaces/IMetaMorpho.sol @@ -4,6 +4,8 @@ pragma solidity >=0.5.0; import {IMorpho, Id, MarketParams} from "@morpho-blue/interfaces/IMorpho.sol"; import {IERC4626} from "@openzeppelin/interfaces/IERC4626.sol"; +import {PendingUint192, PendingAddress} from "../libraries/PendingLib.sol"; + struct MarketConfig { /// @notice The maximum amount of assets that can be allocated to the market. uint192 cap; @@ -11,20 +13,6 @@ struct MarketConfig { uint64 withdrawRank; } -struct PendingUint192 { - /// @notice The pending value to set. - uint192 value; - /// @notice The timestamp at which the value was submitted. - uint64 submittedAt; -} - -struct PendingAddress { - /// @notice The pending value to set. - address value; - /// @notice The timestamp at which the value was submitted. - uint96 submittedAt; -} - /// @dev Either `assets` or `shares` should be zero. struct MarketAllocation { /// @notice The market to allocate. @@ -58,21 +46,21 @@ interface IMetaMorpho is IERC4626 { function submitTimelock(uint256 newTimelock) external; function acceptTimelock() external; function revokeTimelock() external; - function pendingTimelock() external view returns (uint192 value, uint64 submittedAt); + function pendingTimelock() external view returns (uint192 value, uint64 validAt); function submitCap(MarketParams memory marketParams, uint256 supplyCap) external; function acceptCap(Id id) external; function revokeCap(Id id) external; - function pendingCap(Id) external view returns (uint192 value, uint64 submittedAt); + function pendingCap(Id) external view returns (uint192 value, uint64 validAt); function submitFee(uint256 newFee) external; function acceptFee() external; - function pendingFee() external view returns (uint192 value, uint64 submittedAt); + function pendingFee() external view returns (uint192 value, uint64 validAt); function submitGuardian(address newGuardian) external; function acceptGuardian() external; function revokeGuardian() external; - function pendingGuardian() external view returns (address guardian, uint96 submittedAt); + function pendingGuardian() external view returns (address guardian, uint96 validAt); function transferRewards(address) external; diff --git a/src/libraries/EventsLib.sol b/src/libraries/EventsLib.sol index fceea864..7fc4eb1a 100644 --- a/src/libraries/EventsLib.sol +++ b/src/libraries/EventsLib.sol @@ -2,7 +2,8 @@ pragma solidity ^0.8.0; import {Id} from "@morpho-blue/interfaces/IMorpho.sol"; -import {PendingUint192, PendingAddress} from "../interfaces/IMetaMorpho.sol"; + +import {PendingUint192, PendingAddress} from "./PendingLib.sol"; /// @title EventsLib /// @author Morpho Labs diff --git a/src/libraries/PendingLib.sol b/src/libraries/PendingLib.sol new file mode 100644 index 00000000..507f3847 --- /dev/null +++ b/src/libraries/PendingLib.sol @@ -0,0 +1,38 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +pragma solidity ^0.8.0; + +struct PendingUint192 { + /// @notice The pending value to set. + uint192 value; + /// @notice The timestamp at which the value was submitted. + uint64 validAt; +} + +struct PendingAddress { + /// @notice The pending value to set. + address value; + /// @notice The timestamp at which the value was submitted. + uint96 validAt; +} + +/// @title ConstantsLib +/// @author Morpho Labs +/// @custom:contact security@morpho.org +/// @notice Library to manage pending values and their validity timestamp. +library PendingLib { + /// @dev Updates `pending`'s value to `newValue` and its corresponding `validAt` timestamp. + /// @dev Assumes `timelock` <= `MAX_TIMELOCK`. + function update(PendingUint192 storage pending, uint192 newValue, uint256 timelock) internal { + pending.value = newValue; + // Safe "unchecked" cast because timelock <= MAX_TIMELOCK. + pending.validAt = uint64(block.timestamp) + uint64(timelock); + } + + /// @dev Updates `pending`'s value to `newValue` and its corresponding `validAt` timestamp. + /// @dev Assumes `timelock` <= `MAX_TIMELOCK`. + function update(PendingAddress storage pending, address newValue, uint256 timelock) internal { + pending.value = newValue; + // Safe "unchecked" cast because timelock <= MAX_TIMELOCK. + pending.validAt = uint64(block.timestamp) + uint64(timelock); + } +} diff --git a/test/forge/GuardianTest.sol b/test/forge/GuardianTest.sol index 4e6a8a2d..3a53f0dd 100644 --- a/test/forge/GuardianTest.sol +++ b/test/forge/GuardianTest.sol @@ -43,11 +43,11 @@ contract GuardianTest is IntegrationTest { vault.revokeTimelock(); uint256 newTimelock = vault.timelock(); - (uint256 pendingTimelock, uint64 submittedAt) = vault.pendingTimelock(); + (uint256 pendingTimelock, uint64 validAt) = vault.pendingTimelock(); assertEq(newTimelock, TIMELOCK, "newTimelock"); assertEq(pendingTimelock, 0, "pendingTimelock"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(validAt, 0, "validAt"); } function testRevokeCapIncreased(uint256 seed, uint256 cap, uint256 elapsed) public { @@ -68,12 +68,12 @@ contract GuardianTest is IntegrationTest { vault.revokeCap(id); (uint192 newCap, uint64 withdrawRank) = vault.config(id); - (uint256 pendingCap, uint64 submittedAt) = vault.pendingCap(id); + (uint256 pendingCap, uint64 validAt) = vault.pendingCap(id); assertEq(newCap, 0, "newCap"); assertEq(withdrawRank, 0, "withdrawRank"); assertEq(pendingCap, 0, "pendingCap"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(validAt, 0, "validAt"); } function testRevokeGuardian(uint256 elapsed) public { @@ -92,10 +92,10 @@ contract GuardianTest is IntegrationTest { vault.revokeGuardian(); address newGuardian = vault.guardian(); - (address pendingGuardian, uint96 submittedAt) = vault.pendingGuardian(); + (address pendingGuardian, uint96 validAt) = vault.pendingGuardian(); assertEq(newGuardian, GUARDIAN, "newGuardian"); assertEq(pendingGuardian, address(0), "pendingGuardian"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(validAt, 0, "validAt"); } } diff --git a/test/forge/TimelockTest.sol b/test/forge/TimelockTest.sol index 338fb2fc..0508ecf9 100644 --- a/test/forge/TimelockTest.sol +++ b/test/forge/TimelockTest.sol @@ -31,11 +31,11 @@ contract TimelockTest is IntegrationTest { vault.submitTimelock(timelock); uint256 newTimelock = vault.timelock(); - (uint256 pendingTimelock, uint64 submittedAt) = vault.pendingTimelock(); + (uint256 pendingTimelock, uint64 validAt) = vault.pendingTimelock(); assertEq(newTimelock, timelock, "newTimelock"); assertEq(pendingTimelock, 0, "pendingTimelock"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(validAt, 0, "validAt"); } function testSubmitTimelockDecreased(uint256 timelock) public { @@ -47,11 +47,11 @@ contract TimelockTest is IntegrationTest { vault.submitTimelock(timelock); uint256 newTimelock = vault.timelock(); - (uint256 pendingTimelock, uint64 submittedAt) = vault.pendingTimelock(); + (uint256 pendingTimelock, uint64 validAt) = vault.pendingTimelock(); assertEq(newTimelock, TIMELOCK, "newTimelock"); assertEq(pendingTimelock, timelock, "pendingTimelock"); - assertEq(submittedAt, block.timestamp, "submittedAt"); + assertEq(validAt, block.timestamp + TIMELOCK, "validAt"); } function testSubmitTimelockNotOwner(uint256 timelock) public { @@ -110,11 +110,11 @@ contract TimelockTest is IntegrationTest { vault.acceptTimelock(); uint256 newTimelock = vault.timelock(); - (uint256 pendingTimelock, uint64 submittedAt) = vault.pendingTimelock(); + (uint256 pendingTimelock, uint64 validAt) = vault.pendingTimelock(); assertEq(newTimelock, timelock, "newTimelock"); assertEq(pendingTimelock, 0, "pendingTimelock"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(validAt, 0, "validAt"); } function testAcceptTimelockNoPendingValue() public { @@ -145,11 +145,11 @@ contract TimelockTest is IntegrationTest { vault.submitFee(fee); uint256 newFee = vault.fee(); - (uint256 pendingFee, uint64 submittedAt) = vault.pendingFee(); + (uint256 pendingFee, uint64 validAt) = vault.pendingFee(); assertEq(newFee, fee, "newFee"); assertEq(pendingFee, 0, "pendingFee"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(validAt, 0, "validAt"); } function testSubmitFeeIncreased(uint256 fee) public { @@ -161,11 +161,11 @@ contract TimelockTest is IntegrationTest { vault.submitFee(fee); uint256 newFee = vault.fee(); - (uint256 pendingFee, uint64 submittedAt) = vault.pendingFee(); + (uint256 pendingFee, uint64 validAt) = vault.pendingFee(); assertEq(newFee, FEE, "newFee"); assertEq(pendingFee, fee, "pendingFee"); - assertEq(submittedAt, block.timestamp, "submittedAt"); + assertEq(validAt, block.timestamp + TIMELOCK, "validAt"); } function testAcceptFee(uint256 fee) public { @@ -182,11 +182,11 @@ contract TimelockTest is IntegrationTest { vault.acceptFee(); uint256 newFee = vault.fee(); - (uint256 pendingFee, uint64 submittedAt) = vault.pendingFee(); + (uint256 pendingFee, uint64 validAt) = vault.pendingFee(); assertEq(newFee, fee, "newFee"); assertEq(pendingFee, 0, "pendingFee"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(validAt, 0, "validAt"); } function testAcceptFeeNoPendingValue() public { @@ -216,11 +216,11 @@ contract TimelockTest is IntegrationTest { vault.submitGuardian(guardian); address newGuardian = vault.guardian(); - (address pendingGuardian, uint96 submittedAt) = vault.pendingGuardian(); + (address pendingGuardian, uint96 validAt) = vault.pendingGuardian(); assertEq(newGuardian, GUARDIAN, "newGuardian"); assertEq(pendingGuardian, guardian, "pendingGuardian"); - assertEq(submittedAt, block.timestamp, "submittedAt"); + assertEq(validAt, block.timestamp + TIMELOCK, "validAt"); } function testSubmitGuardianFromZero() public { @@ -232,11 +232,11 @@ contract TimelockTest is IntegrationTest { vault.submitGuardian(GUARDIAN); address newGuardian = vault.guardian(); - (address pendingGuardian, uint96 submittedAt) = vault.pendingGuardian(); + (address pendingGuardian, uint96 validAt) = vault.pendingGuardian(); assertEq(newGuardian, GUARDIAN, "newGuardian"); assertEq(pendingGuardian, address(0), "pendingGuardian"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(validAt, 0, "validAt"); } function testSubmitGuardianZero() public { @@ -244,11 +244,11 @@ contract TimelockTest is IntegrationTest { vault.submitGuardian(address(0)); address newGuardian = vault.guardian(); - (address pendingGuardian, uint96 submittedAt) = vault.pendingGuardian(); + (address pendingGuardian, uint96 validAt) = vault.pendingGuardian(); assertEq(newGuardian, GUARDIAN, "newGuardian"); assertEq(pendingGuardian, address(0), "pendingGuardian"); - assertEq(submittedAt, block.timestamp, "submittedAt"); + assertEq(validAt, block.timestamp + TIMELOCK, "validAt"); } function testAcceptGuardian() public { @@ -264,11 +264,11 @@ contract TimelockTest is IntegrationTest { vault.acceptGuardian(); address newGuardian = vault.guardian(); - (address pendingGuardian, uint96 submittedAt) = vault.pendingGuardian(); + (address pendingGuardian, uint96 validAt) = vault.pendingGuardian(); assertEq(newGuardian, guardian, "newGuardian"); assertEq(pendingGuardian, address(0), "pendingGuardian"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(validAt, 0, "validAt"); } function testAcceptGuardianNoPendingValue() public { @@ -302,12 +302,12 @@ contract TimelockTest is IntegrationTest { vault.submitCap(marketParams, cap); (uint192 newCap, uint64 withdrawRank) = vault.config(id); - (uint192 pendingCap, uint64 submittedAt) = vault.pendingCap(id); + (uint192 pendingCap, uint64 validAt) = vault.pendingCap(id); assertEq(newCap, cap, "newCap"); assertEq(withdrawRank, 1, "withdrawRank"); assertEq(pendingCap, 0, "pendingCap"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(validAt, 0, "validAt"); } function testSubmitCapIncreased(uint256 cap) public { @@ -322,12 +322,12 @@ contract TimelockTest is IntegrationTest { vault.submitCap(marketParams, cap); (uint192 newCap, uint64 withdrawRank) = vault.config(id); - (uint192 pendingCap, uint64 submittedAt) = vault.pendingCap(id); + (uint192 pendingCap, uint64 validAt) = vault.pendingCap(id); assertEq(newCap, 0, "newCap"); assertEq(withdrawRank, 0, "withdrawRank"); assertEq(pendingCap, cap, "pendingCap"); - assertEq(submittedAt, block.timestamp, "submittedAt"); + assertEq(validAt, block.timestamp + TIMELOCK, "validAt"); assertEq(vault.supplyQueueSize(), 1, "supplyQueueSize"); assertEq(vault.withdrawQueueSize(), 1, "withdrawQueueSize"); } @@ -348,12 +348,12 @@ contract TimelockTest is IntegrationTest { vault.acceptCap(id); (uint192 newCap, uint64 withdrawRank) = vault.config(id); - (uint192 pendingCapAfter, uint64 submittedAtAfter) = vault.pendingCap(id); + (uint192 pendingCapAfter, uint64 validAtAfter) = vault.pendingCap(id); assertEq(newCap, cap, "newCap"); assertEq(withdrawRank, 1, "withdrawRank"); assertEq(pendingCapAfter, 0, "pendingCapAfter"); - assertEq(submittedAtAfter, 0, "submittedAtAfter"); + assertEq(validAtAfter, 0, "validAtAfter"); assertEq(Id.unwrap(vault.supplyQueue(0)), Id.unwrap(id), "supplyQueue"); assertEq(Id.unwrap(vault.withdrawQueue(0)), Id.unwrap(id), "withdrawQueue"); } diff --git a/test/metamorpho_tests.tree b/test/metamorpho_tests.tree index d9d772ab..caa0c055 100644 --- a/test/metamorpho_tests.tree +++ b/test/metamorpho_tests.tree @@ -58,12 +58,12 @@ . └── acceptTimelock() external - ├── when pendingTimelock.submittedAt == 0 + ├── when pendingTimelock.validAt == 0 │ └── revert with NoPendingValue() - └── when pendingTimelock.submittedAt != 0 - ├── when block.timestamp < pendingTimelock.submittedAt + timelock + └── when pendingTimelock.validAt != 0 + ├── when block.timestamp < pendingTimelock.validAt │ └── revert with TimelockNotElapsed() - └── when block.timestamp >= pendingTimelock.submittedAt + timelock + └── when block.timestamp >= pendingTimelock.validAt ├── it should set timelock to pendingTimelock.value ├── it should emit SetTimelock(pendingTimelock.value) └── it should delete pendingTimelock @@ -90,12 +90,12 @@ . └── acceptFee() external - ├── when pendingFee.submittedAt == 0 + ├── when pendingFee.validAt == 0 │ └── revert with NoPendingValue() - └── when pendingFee.submittedAt != 0 - ├── when block.timestamp < pendingFee.submittedAt + timelock + └── when pendingFee.validAt != 0 + ├── when block.timestamp < pendingFee.validAt │ └── revert with TimelockNotElapsed() - └── when block.timestamp >= pendingFee.submittedAt + timelock + └── when block.timestamp >= pendingFee.validAt ├── it should accrue fees ├── it should set fee to pendingFee.value ├── it should emit SetFee(pendingFee.value) @@ -134,12 +134,12 @@ . └── acceptGuardian() external - ├── when pendingGuardian.submittedAt == 0 + ├── when pendingGuardian.validAt == 0 │ └── revert with NoPendingValue() - └── when pendingGuardian.submittedAt != 0 - ├── when block.timestamp < pendingGuardian.submittedAt + timelock + └── when pendingGuardian.validAt != 0 + ├── when block.timestamp < pendingGuardian.validAt │ └── revert with TimelockNotElapsed() - └── when block.timestamp >= pendingGuardian.submittedAt + timelock + └── when block.timestamp >= pendingGuardian.validAt ├── it should set guardian to pendingGuardian ├── it should emit SetGuardian(pendingGuardian) └── it should delete pendingGuardian @@ -178,12 +178,12 @@ . └── acceptCap(Id id) external - ├── when pendingCap[id].submittedAt == 0 + ├── when pendingCap[id].validAt == 0 │ └── revert with NoPendingValue() - └── when pendingCap[id].submittedAt != 0 - ├── when block.timestamp < pendingCap[id].submittedAt + timelock + └── when pendingCap[id].validAt != 0 + ├── when block.timestamp < pendingCap[id].validAt │ └── revert with TimelockNotElapsed() - └── when block.timestamp >= pendingCap[id].submittedAt + timelock + └── when block.timestamp >= pendingCap[id].validAt ├── when supplyCap > 0 and marketConfig.withdrawRank == 0 │ ├── it should push id to supplyQueue │ ├── it should push id to withdrawQueue From c80091db43c3e51ed211c8b3de824a218fa6b1c3 Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Thu, 2 Nov 2023 15:55:02 +0100 Subject: [PATCH 18/30] docs(pending): update natspecs --- src/libraries/PendingLib.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/PendingLib.sol b/src/libraries/PendingLib.sol index 507f3847..8fe04b32 100644 --- a/src/libraries/PendingLib.sol +++ b/src/libraries/PendingLib.sol @@ -4,18 +4,18 @@ pragma solidity ^0.8.0; struct PendingUint192 { /// @notice The pending value to set. uint192 value; - /// @notice The timestamp at which the value was submitted. + /// @notice The timestamp at which the pending value becomes valid. uint64 validAt; } struct PendingAddress { /// @notice The pending value to set. address value; - /// @notice The timestamp at which the value was submitted. + /// @notice The timestamp at which the pending value becomes valid. uint96 validAt; } -/// @title ConstantsLib +/// @title PendingLib /// @author Morpho Labs /// @custom:contact security@morpho.org /// @notice Library to manage pending values and their validity timestamp. From 508682a7a4f38ad9e6b474e0b957c80f5552eda7 Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Thu, 2 Nov 2023 16:33:23 +0100 Subject: [PATCH 19/30] feat(pending): add already pending error --- src/MetaMorpho.sol | 14 ++++++++ src/libraries/ErrorsLib.sol | 3 ++ test/forge/TimelockTest.sol | 46 ++++++++++++++++++++++++++ test/forge/helpers/IntegrationTest.sol | 43 ++++++++++++++---------- 4 files changed, 89 insertions(+), 17 deletions(-) diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index 0987b492..e4f65547 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -202,6 +202,10 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph if (newTimelock > timelock) { _setTimelock(newTimelock); } else { + if (pendingTimelock.submittedAt != 0 && newTimelock == pendingTimelock.value) { + revert ErrorsLib.AlreadyPending(); + } + // Safe "unchecked" cast because newTimelock <= MAX_TIMELOCK. pendingTimelock = PendingUint192(uint192(newTimelock), uint64(block.timestamp)); @@ -219,6 +223,8 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph if (newFee < fee) { _setFee(newFee); } else { + if (pendingFee.submittedAt != 0 && newFee == pendingFee.value) revert ErrorsLib.AlreadyPending(); + // Safe "unchecked" cast because newFee <= MAX_FEE. pendingFee = PendingUint192(uint192(newFee), uint64(block.timestamp)); @@ -249,6 +255,10 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph if (guardian == address(0)) { _setGuardian(newGuardian); } else { + if (pendingGuardian.submittedAt != 0 && newGuardian == pendingGuardian.value) { + revert ErrorsLib.AlreadyPending(); + } + pendingGuardian = PendingAddress(newGuardian, uint64(block.timestamp)); emit EventsLib.SubmitGuardian(newGuardian); @@ -271,6 +281,10 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph if (newSupplyCap < supplyCap) { _setCap(id, newSupplyCap.toUint192()); } else { + if (pendingCap[id].submittedAt != 0 && newSupplyCap == pendingCap[id].value) { + revert ErrorsLib.AlreadyPending(); + } + pendingCap[id] = PendingUint192(newSupplyCap.toUint192(), uint64(block.timestamp)); emit EventsLib.SubmitCap(id, newSupplyCap); diff --git a/src/libraries/ErrorsLib.sol b/src/libraries/ErrorsLib.sol index fe16ec44..1bc1c1f4 100644 --- a/src/libraries/ErrorsLib.sol +++ b/src/libraries/ErrorsLib.sol @@ -36,6 +36,9 @@ library ErrorsLib { /// @notice Thrown when the value is already set. error AlreadySet(); + /// @notice Thrown when the value is already pending. + error AlreadyPending(); + /// @notice Thrown when market `id` is a duplicate in the new withdraw queue to set. error DuplicateMarket(Id id); diff --git a/test/forge/TimelockTest.sol b/test/forge/TimelockTest.sol index 04230e9d..f7ec3404 100644 --- a/test/forge/TimelockTest.sol +++ b/test/forge/TimelockTest.sol @@ -54,6 +54,17 @@ contract TimelockTest is IntegrationTest { assertEq(submittedAt, block.timestamp, "submittedAt"); } + function testSubmitTimelockAlreadyPending(uint256 timelock) public { + timelock = bound(timelock, ConstantsLib.MIN_TIMELOCK, TIMELOCK - 1); + + vm.prank(OWNER); + vault.submitTimelock(timelock); + + vm.expectRevert(ErrorsLib.AlreadyPending.selector); + vm.prank(OWNER); + vault.submitTimelock(timelock); + } + function testSubmitTimelockNotOwner(uint256 timelock) public { vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, address(this))); vault.submitTimelock(timelock); @@ -181,6 +192,17 @@ contract TimelockTest is IntegrationTest { assertEq(submittedAt, block.timestamp, "submittedAt"); } + function testSubmitFeeAlreadyPending(uint256 fee) public { + fee = bound(fee, FEE + 1, ConstantsLib.MAX_FEE); + + vm.prank(OWNER); + vault.submitFee(fee); + + vm.expectRevert(ErrorsLib.AlreadyPending.selector); + vm.prank(OWNER); + vault.submitFee(fee); + } + function testAcceptFee(uint256 fee) public { fee = bound(fee, FEE + 1, ConstantsLib.MAX_FEE); @@ -277,6 +299,17 @@ contract TimelockTest is IntegrationTest { assertEq(submittedAt, block.timestamp, "submittedAt"); } + function testSubmitGuardianAlreadyPending() public { + address guardian = makeAddr("Guardian2"); + + vm.prank(OWNER); + vault.submitGuardian(guardian); + + vm.expectRevert(ErrorsLib.AlreadyPending.selector); + vm.prank(OWNER); + vault.submitGuardian(guardian); + } + function testAcceptGuardian() public { address guardian = makeAddr("Guardian2"); @@ -372,6 +405,19 @@ contract TimelockTest is IntegrationTest { assertEq(vault.withdrawQueueSize(), 1, "withdrawQueueSize"); } + function testSubmitCapAlreadyPending(uint256 cap) public { + cap = bound(cap, 1, type(uint192).max); + + MarketParams memory marketParams = allMarkets[1]; + + vm.prank(CURATOR); + vault.submitCap(marketParams, cap); + + vm.expectRevert(ErrorsLib.AlreadyPending.selector); + vm.prank(CURATOR); + vault.submitCap(marketParams, cap); + } + function testAcceptCapIncreased(uint256 cap) public { cap = bound(cap, CAP + 1, type(uint192).max); diff --git a/test/forge/helpers/IntegrationTest.sol b/test/forge/helpers/IntegrationTest.sol index e9f453ef..caed6c90 100644 --- a/test/forge/helpers/IntegrationTest.sol +++ b/test/forge/helpers/IntegrationTest.sol @@ -41,10 +41,13 @@ contract IntegrationTest is BaseTest { // block.timestamp defaults to 1 which may lead to an unrealistic state: block.timestamp < timelock. if (block.timestamp < timelock) vm.warp(block.timestamp + timelock); - vm.prank(OWNER); - vault.submitTimelock(newTimelock); + (uint192 pendingTimelock, uint64 submittedAt) = vault.pendingTimelock(); + if (submittedAt == 0 || newTimelock != pendingTimelock) { + vm.prank(OWNER); + vault.submitTimelock(newTimelock); + } - if (newTimelock > timelock || timelock == 0) return; + if (newTimelock > timelock) return; vm.warp(block.timestamp + timelock); @@ -57,13 +60,15 @@ contract IntegrationTest is BaseTest { address guardian = vault.guardian(); if (newGuardian == guardian) return; - vm.prank(OWNER); - vault.submitGuardian(newGuardian); + (address pendingGuardian, uint96 submittedAt) = vault.pendingGuardian(); + if (submittedAt == 0 || newGuardian != pendingGuardian) { + vm.prank(OWNER); + vault.submitGuardian(newGuardian); + } - uint256 timelock = vault.timelock(); - if (guardian == address(0) || timelock == 0) return; + if (guardian == address(0)) return; - vm.warp(block.timestamp + timelock); + vm.warp(block.timestamp + vault.timelock()); vault.acceptGuardian(); @@ -74,13 +79,15 @@ contract IntegrationTest is BaseTest { uint256 fee = vault.fee(); if (newFee == fee) return; - vm.prank(OWNER); - vault.submitFee(newFee); + (uint192 pendingFee, uint64 submittedAt) = vault.pendingFee(); + if (submittedAt == 0 || newFee != pendingFee) { + vm.prank(OWNER); + vault.submitFee(newFee); + } - uint256 timelock = vault.timelock(); - if (newFee < fee || timelock == 0) return; + if (newFee < fee) return; - vm.warp(block.timestamp + timelock); + vm.warp(block.timestamp + vault.timelock()); vault.acceptFee(); @@ -92,13 +99,15 @@ contract IntegrationTest is BaseTest { (uint256 cap,) = vault.config(id); if (newCap == cap) return; - vm.prank(CURATOR); - vault.submitCap(marketParams, newCap); + (uint192 pendingCap, uint64 submittedAt) = vault.pendingCap(id); + if (submittedAt == 0 || newCap != pendingCap) { + vm.prank(CURATOR); + vault.submitCap(marketParams, newCap); + } - uint256 timelock = vault.timelock(); if (newCap < cap) return; - vm.warp(block.timestamp + timelock); + vm.warp(block.timestamp + vault.timelock()); vault.acceptCap(id); From b25b305b1fecf5bc9aa7f91db0e77241628d4d1b Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Thu, 2 Nov 2023 18:04:08 +0100 Subject: [PATCH 20/30] test(timelock): add validAt tests --- test/forge/TimelockTest.sol | 147 ++++++++++++++++++++++++++++++++++++ 1 file changed, 147 insertions(+) diff --git a/test/forge/TimelockTest.sol b/test/forge/TimelockTest.sol index 0508ecf9..fd532f99 100644 --- a/test/forge/TimelockTest.sol +++ b/test/forge/TimelockTest.sol @@ -189,6 +189,52 @@ contract TimelockTest is IntegrationTest { assertEq(validAt, 0, "validAt"); } + function testAcceptFeeTimelockIncreased(uint256 fee, uint256 timelock, uint256 elapsed) public { + fee = bound(fee, FEE + 1, ConstantsLib.MAX_FEE); + timelock = bound(timelock, TIMELOCK + 1, ConstantsLib.MAX_TIMELOCK); + elapsed = bound(elapsed, TIMELOCK + 1, timelock); + + vm.prank(OWNER); + vault.submitFee(fee); + + _setTimelock(timelock); + + vm.warp(block.timestamp + elapsed); + + vm.expectEmit(address(vault)); + emit EventsLib.UpdateLastTotalAssets(vault.totalAssets()); + emit EventsLib.SetFee(fee); + vault.acceptFee(); + + uint256 newFee = vault.fee(); + (uint256 pendingFee, uint64 validAt) = vault.pendingFee(); + + assertEq(newFee, fee, "newFee"); + assertEq(pendingFee, 0, "pendingFee"); + assertEq(validAt, 0, "validAt"); + } + + function testAcceptFeeTimelockDecreased(uint256 fee, uint256 timelock, uint256 elapsed) public { + fee = bound(fee, FEE + 1, ConstantsLib.MAX_FEE); + timelock = bound(timelock, ConstantsLib.MIN_TIMELOCK, TIMELOCK - 1); + elapsed = bound(elapsed, 1, TIMELOCK - 1); + + vm.prank(OWNER); + vault.submitTimelock(timelock); + + vm.warp(block.timestamp + elapsed); + + vm.prank(OWNER); + vault.submitFee(fee); + + vm.warp(block.timestamp + TIMELOCK - elapsed); + + vault.acceptTimelock(); + + vm.expectRevert(ErrorsLib.TimelockNotElapsed.selector); + vault.acceptFee(); + } + function testAcceptFeeNoPendingValue() public { vm.expectRevert(ErrorsLib.NoPendingValue.selector); vault.acceptFee(); @@ -271,6 +317,53 @@ contract TimelockTest is IntegrationTest { assertEq(validAt, 0, "validAt"); } + function testAcceptGuardianTimelockIncreased(uint256 timelock, uint256 elapsed) public { + timelock = bound(timelock, TIMELOCK + 1, ConstantsLib.MAX_TIMELOCK); + elapsed = bound(elapsed, TIMELOCK + 1, timelock); + + address guardian = makeAddr("Guardian2"); + + vm.prank(OWNER); + vault.submitGuardian(guardian); + + _setTimelock(timelock); + + vm.warp(block.timestamp + elapsed); + + vm.expectEmit(address(vault)); + emit EventsLib.SetGuardian(guardian); + vault.acceptGuardian(); + + address newGuardian = vault.guardian(); + (address pendingGuardian, uint96 validAt) = vault.pendingGuardian(); + + assertEq(newGuardian, guardian, "newGuardian"); + assertEq(pendingGuardian, address(0), "pendingGuardian"); + assertEq(validAt, 0, "validAt"); + } + + function testAcceptGuardianTimelockDecreased(uint256 timelock, uint256 elapsed) public { + timelock = bound(timelock, ConstantsLib.MIN_TIMELOCK, TIMELOCK - 1); + elapsed = bound(elapsed, 1, TIMELOCK - 1); + + vm.prank(OWNER); + vault.submitTimelock(timelock); + + vm.warp(block.timestamp + elapsed); + + address guardian = makeAddr("Guardian2"); + + vm.prank(OWNER); + vault.submitGuardian(guardian); + + vm.warp(block.timestamp + TIMELOCK - elapsed); + + vault.acceptTimelock(); + + vm.expectRevert(ErrorsLib.TimelockNotElapsed.selector); + vault.acceptGuardian(); + } + function testAcceptGuardianNoPendingValue() public { vm.expectRevert(ErrorsLib.NoPendingValue.selector); vault.acceptGuardian(); @@ -358,6 +451,60 @@ contract TimelockTest is IntegrationTest { assertEq(Id.unwrap(vault.withdrawQueue(0)), Id.unwrap(id), "withdrawQueue"); } + function testAcceptCapIncreasedTimelockIncreased(uint256 cap, uint256 timelock, uint256 elapsed) public { + cap = bound(cap, CAP + 1, type(uint192).max); + timelock = bound(timelock, TIMELOCK + 1, ConstantsLib.MAX_TIMELOCK); + elapsed = bound(elapsed, TIMELOCK + 1, timelock); + + MarketParams memory marketParams = allMarkets[0]; + Id id = marketParams.id(); + + vm.prank(CURATOR); + vault.submitCap(marketParams, cap); + + _setTimelock(timelock); + + vm.warp(block.timestamp + elapsed); + + vm.expectEmit(); + emit EventsLib.SetCap(id, cap); + vault.acceptCap(id); + + (uint192 newCap, uint64 withdrawRank) = vault.config(id); + (uint192 pendingCapAfter, uint64 validAtAfter) = vault.pendingCap(id); + + assertEq(newCap, cap, "newCap"); + assertEq(withdrawRank, 1, "withdrawRank"); + assertEq(pendingCapAfter, 0, "pendingCapAfter"); + assertEq(validAtAfter, 0, "validAtAfter"); + assertEq(Id.unwrap(vault.supplyQueue(0)), Id.unwrap(id), "supplyQueue"); + assertEq(Id.unwrap(vault.withdrawQueue(0)), Id.unwrap(id), "withdrawQueue"); + } + + function testAcceptCapIncreasedTimelockDecreased(uint256 cap, uint256 timelock, uint256 elapsed) public { + cap = bound(cap, CAP + 1, type(uint192).max); + timelock = bound(timelock, ConstantsLib.MIN_TIMELOCK, TIMELOCK - 1); + elapsed = bound(elapsed, 1, TIMELOCK - 1); + + vm.prank(OWNER); + vault.submitTimelock(timelock); + + vm.warp(block.timestamp + elapsed); + + MarketParams memory marketParams = allMarkets[0]; + Id id = marketParams.id(); + + vm.prank(CURATOR); + vault.submitCap(marketParams, cap); + + vm.warp(block.timestamp + TIMELOCK - elapsed); + + vault.acceptTimelock(); + + vm.expectRevert(ErrorsLib.TimelockNotElapsed.selector); + vault.acceptCap(id); + } + function testAcceptCapNoPendingValue() public { vm.expectRevert(ErrorsLib.NoPendingValue.selector); vault.acceptCap(allMarkets[0].id()); From 78a0e16610a3a6e414f5028718bee9a79aac4d1a Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Fri, 3 Nov 2023 09:37:48 +0100 Subject: [PATCH 21/30] perf(submit-timelock): add small opti --- src/MetaMorpho.sol | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index e4f65547..969d1a74 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -202,9 +202,8 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph if (newTimelock > timelock) { _setTimelock(newTimelock); } else { - if (pendingTimelock.submittedAt != 0 && newTimelock == pendingTimelock.value) { - revert ErrorsLib.AlreadyPending(); - } + // newTimelock >= MIN_TIMELOCK > 0 so there's no need to check `pendingTimelock.submittedAt != 0`. + if (newTimelock == pendingTimelock.value) revert ErrorsLib.AlreadyPending(); // Safe "unchecked" cast because newTimelock <= MAX_TIMELOCK. pendingTimelock = PendingUint192(uint192(newTimelock), uint64(block.timestamp)); @@ -223,7 +222,8 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph if (newFee < fee) { _setFee(newFee); } else { - if (pendingFee.submittedAt != 0 && newFee == pendingFee.value) revert ErrorsLib.AlreadyPending(); + // newFee > fee >= 0 so there's no need to check `pendingFee.submittedAt != 0`. + if (newFee == pendingFee.value) revert ErrorsLib.AlreadyPending(); // Safe "unchecked" cast because newFee <= MAX_FEE. pendingFee = PendingUint192(uint192(newFee), uint64(block.timestamp)); @@ -281,7 +281,8 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph if (newSupplyCap < supplyCap) { _setCap(id, newSupplyCap.toUint192()); } else { - if (pendingCap[id].submittedAt != 0 && newSupplyCap == pendingCap[id].value) { + // newSupplyCap > supplyCap >= 0 so there's no need to check `pendingCap[id].submittedAt != 0`. + if (newSupplyCap == pendingCap[id].value) { revert ErrorsLib.AlreadyPending(); } From 63cd2e751adcdb42af3704ca44e3b8235d6c60a2 Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Mon, 6 Nov 2023 11:51:05 +0100 Subject: [PATCH 22/30] refactor(role): apply naming suggestions --- README.md | 2 +- src/MetaMorpho.sol | 7 ++++--- src/libraries/ErrorsLib.sol | 8 ++++---- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index e240c51a..d5d28705 100644 --- a/README.md +++ b/README.md @@ -74,7 +74,7 @@ It can: - Upon a withdrawal, the vault will first withdraw from the idle supply and then withdraw up to the liquidity of each Morpho Blue market in the `withdrawalQueue` in the order set. - The `supplyQueue` only contains markets which cap has previously been non-zero. - The `withdrawQueue` contains all markets that have a non-zero cap or a non-zero vault allocation. -- Instantaneously reallocate funds across enabled markets at any moment. +- Instantaneously reallocate funds across markets of the `withdrawQueue` at any moment. #### Guardian diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index 03ab393b..e21b27a7 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -141,16 +141,17 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph _; } - /// @dev Reverts if the caller is not the guardian. + /// @dev Reverts if the caller doesn't have the guardian role. modifier onlyGuardianRole() { - if (_msgSender() != owner() && _msgSender() != guardian) revert ErrorsLib.NotGuardian(); + if (_msgSender() != owner() && _msgSender() != guardian) revert ErrorsLib.NotGuardianRole(); _; } + /// @dev Reverts if the caller doesn't have the curator nor the guardian role. modifier onlyCuratorOrGuardianRole() { if (_msgSender() != guardian && _msgSender() != curator && _msgSender() != owner()) { - revert ErrorsLib.NotCuratorNorGuardian(); + revert ErrorsLib.NotCuratorNorGuardianRole(); } _; diff --git a/src/libraries/ErrorsLib.sol b/src/libraries/ErrorsLib.sol index 7f5f3559..9eeafb57 100644 --- a/src/libraries/ErrorsLib.sol +++ b/src/libraries/ErrorsLib.sol @@ -17,11 +17,11 @@ library ErrorsLib { /// @notice Thrown when the caller doesn't have the allocator role. error NotAllocatorRole(); - /// @notice Thrown when the caller doesn't have the guardian's privilege. - error NotGuardian(); + /// @notice Thrown when the caller doesn't have the guardian role. + error NotGuardianRole(); - /// @notice Thrown when the caller doesn't have the curator's privilege nor the guardian's privilege. - error NotCuratorNorGuardian(); + /// @notice Thrown when the caller doesn't have the curator nor the guardian role. + error NotCuratorNorGuardianRole(); /// @notice Thrown when the market `id` cannot be set in the supply queue. error UnauthorizedMarket(Id id); From 34684da3fd28ecfdad10ca2f478331c7609a397c Mon Sep 17 00:00:00 2001 From: MerlinEgalite Date: Mon, 6 Nov 2023 15:12:25 +0300 Subject: [PATCH 23/30] test: add missing caller in event --- test/forge/FeeTest.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/forge/FeeTest.sol b/test/forge/FeeTest.sol index bdfacef8..c48653dc 100644 --- a/test/forge/FeeTest.sol +++ b/test/forge/FeeTest.sol @@ -44,7 +44,7 @@ contract FeeTest is IntegrationTest { fee = bound(fee, 0, ConstantsLib.MAX_FEE); vm.expectEmit(); - emit EventsLib.SetFee(fee); + emit EventsLib.SetFee(OWNER, fee); vm.prank(OWNER); vault.setFee(fee); From d972b3722c255ff41e0c858d4385ba18f811e178 Mon Sep 17 00:00:00 2001 From: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com> Date: Mon, 6 Nov 2023 14:38:53 +0100 Subject: [PATCH 24/30] test: apply suggestion Co-authored-by: Romain Milon Signed-off-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com> --- test/forge/FeeTest.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/forge/FeeTest.sol b/test/forge/FeeTest.sol index c48653dc..cca4c4e4 100644 --- a/test/forge/FeeTest.sol +++ b/test/forge/FeeTest.sol @@ -43,7 +43,7 @@ contract FeeTest is IntegrationTest { function testSetFee(uint256 fee) public { fee = bound(fee, 0, ConstantsLib.MAX_FEE); - vm.expectEmit(); + vm.expectEmit(address(vault)); emit EventsLib.SetFee(OWNER, fee); vm.prank(OWNER); vault.setFee(fee); From 3ce8d87db0169c037544b30941473c3ec5448dc3 Mon Sep 17 00:00:00 2001 From: Quentin Garchery Date: Tue, 7 Nov 2023 17:00:12 +0100 Subject: [PATCH 25/30] refactor: split interfaces --- src/MetaMorpho.sol | 32 ++++----- src/interfaces/IMetaMorpho.sol | 44 +++++++++--- test/forge/GuardianTest.sol | 24 +++---- test/forge/TimelockTest.sol | 96 +++++++++++++------------- test/forge/helpers/BaseTest.sol | 2 +- test/forge/helpers/IntegrationTest.sol | 15 ++-- 6 files changed, 118 insertions(+), 95 deletions(-) diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index 7f76cfd1..f01fcdfe 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -3,7 +3,11 @@ pragma solidity 0.8.21; import {IMorphoMarketParams} from "./interfaces/IMorphoMarketParams.sol"; import { - IMetaMorpho, MarketConfig, PendingUint192, PendingAddress, MarketAllocation + MarketConfig, + PendingUint192, + PendingAddress, + MarketAllocation, + IMetaMorphoStaticTyping } from "./interfaces/IMetaMorpho.sol"; import {Id, MarketParams, Market, IMorpho} from "@morpho-blue/interfaces/IMorpho.sol"; @@ -28,7 +32,7 @@ import {IERC20, IERC4626, ERC20, ERC4626, Math, SafeERC20} from "@openzeppelin/t /// @author Morpho Labs /// @custom:contact security@morpho.org /// @notice ERC4626 compliant vault allowing users to deposit assets to Morpho. -contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorpho { +contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorphoStaticTyping { using Math for uint256; using UtilsLib for uint256; using SafeCast for uint256; @@ -474,28 +478,28 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph /* ERC4626 (PUBLIC) */ /// @inheritdoc IERC20Metadata - function decimals() public view override(IERC20Metadata, ERC20, ERC4626) returns (uint8) { + function decimals() public view override(ERC20, ERC4626) returns (uint8) { return ERC4626.decimals(); } /// @inheritdoc IERC4626 /// @dev Warning: May be lower than the actual amount of assets that can be withdrawn by `owner` due to conversion /// roundings between shares and assets. - function maxWithdraw(address owner) public view override(IERC4626, ERC4626) returns (uint256 assets) { + function maxWithdraw(address owner) public view override returns (uint256 assets) { (assets,,) = _maxWithdraw(owner); } /// @inheritdoc IERC4626 /// @dev Warning: May be lower than the actual amount of shares that can be redeemed by `owner` due to conversion /// roundings between shares and assets. - function maxRedeem(address owner) public view override(IERC4626, ERC4626) returns (uint256) { + function maxRedeem(address owner) public view override returns (uint256) { (uint256 assets, uint256 newTotalSupply, uint256 newTotalAssets) = _maxWithdraw(owner); return _convertToSharesWithTotals(assets, newTotalSupply, newTotalAssets, Math.Rounding.Floor); } /// @inheritdoc IERC4626 - function deposit(uint256 assets, address receiver) public override(IERC4626, ERC4626) returns (uint256 shares) { + function deposit(uint256 assets, address receiver) public override returns (uint256 shares) { uint256 newTotalAssets = _accrueFee(); shares = _convertToSharesWithTotals(assets, totalSupply(), newTotalAssets, Math.Rounding.Floor); @@ -503,7 +507,7 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph } /// @inheritdoc IERC4626 - function mint(uint256 shares, address receiver) public override(IERC4626, ERC4626) returns (uint256 assets) { + function mint(uint256 shares, address receiver) public override returns (uint256 assets) { uint256 newTotalAssets = _accrueFee(); assets = _convertToAssetsWithTotals(shares, totalSupply(), newTotalAssets, Math.Rounding.Ceil); @@ -511,11 +515,7 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph } /// @inheritdoc IERC4626 - function withdraw(uint256 assets, address receiver, address owner) - public - override(IERC4626, ERC4626) - returns (uint256 shares) - { + function withdraw(uint256 assets, address receiver, address owner) public override returns (uint256 shares) { uint256 newTotalAssets = _accrueFee(); // Do not call expensive `maxWithdraw` and optimistically withdraw assets. @@ -525,11 +525,7 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph } /// @inheritdoc IERC4626 - function redeem(uint256 shares, address receiver, address owner) - public - override(IERC4626, ERC4626) - returns (uint256 assets) - { + function redeem(uint256 shares, address receiver, address owner) public override returns (uint256 assets) { uint256 newTotalAssets = _accrueFee(); // Do not call expensive `maxRedeem` and optimistically redeem shares. @@ -539,7 +535,7 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph } /// @inheritdoc IERC4626 - function totalAssets() public view override(IERC4626, ERC4626) returns (uint256 assets) { + function totalAssets() public view override returns (uint256 assets) { for (uint256 i; i < withdrawQueue.length; ++i) { assets += _supplyBalance(_marketParams(withdrawQueue[i])); } diff --git a/src/interfaces/IMetaMorpho.sol b/src/interfaces/IMetaMorpho.sol index 67e6327b..93a4e91e 100644 --- a/src/interfaces/IMetaMorpho.sol +++ b/src/interfaces/IMetaMorpho.sol @@ -3,6 +3,7 @@ pragma solidity >=0.5.0; import {IMorpho, Id, MarketParams} from "@morpho-blue/interfaces/IMorpho.sol"; import {IERC4626} from "@openzeppelin/interfaces/IERC4626.sol"; +import {IERC20Permit} from "@openzeppelin/token/ERC20/extensions/IERC20Permit.sol"; struct MarketConfig { /// @notice The maximum amount of assets that can be allocated to the market. @@ -35,7 +36,21 @@ struct MarketAllocation { uint256 shares; } -interface IMetaMorpho is IERC4626 { +interface IMulticall { + function multicall(bytes[] calldata) external returns (bytes[] memory); +} + +interface IOwnable { + function owner() external returns (address); + function transferOwnership(address) external; + function renounceOwnership() external; + function acceptOwnership() external; + function pendingOwner() external view returns (address); +} + +/// @dev This interface is used for factorizing IMetaMorphoStaticTyping and IMetaMorpho. +/// @dev Consider using the IMetaMorpho interface instead of this one. +interface IMetaMorphoBase { function MORPHO() external view returns (IMorpho); function curator() external view returns (address); @@ -50,7 +65,6 @@ interface IMetaMorpho is IERC4626 { function supplyQueueLength() external view returns (uint256); function withdrawQueue(uint256) external view returns (Id); function withdrawQueueLength() external view returns (uint256); - function config(Id) external view returns (uint192 cap, uint64 withdrawRank); function idle() external view returns (uint256); function lastTotalAssets() external view returns (uint256); @@ -58,21 +72,17 @@ interface IMetaMorpho is IERC4626 { function submitTimelock(uint256 newTimelock) external; function acceptTimelock() external; function revokePendingTimelock() external; - function pendingTimelock() external view returns (uint192 value, uint64 submittedAt); function submitCap(MarketParams memory marketParams, uint256 supplyCap) external; function acceptCap(Id id) external; function revokePendingCap(Id id) external; - function pendingCap(Id) external view returns (uint192 value, uint64 submittedAt); function submitFee(uint256 newFee) external; function acceptFee() external; - function pendingFee() external view returns (uint192 value, uint64 submittedAt); function submitGuardian(address newGuardian) external; function acceptGuardian() external; function revokePendingGuardian() external; - function pendingGuardian() external view returns (address guardian, uint64 submittedAt); function transferRewards(address) external; @@ -86,8 +96,24 @@ interface IMetaMorpho is IERC4626 { function reallocate(MarketAllocation[] calldata withdrawn, MarketAllocation[] calldata supplied) external; } -interface IPending { - function pendingTimelock() external view returns (PendingUint192 memory); - function pendingCap(Id) external view returns (PendingUint192 memory); +/// @dev This interface is inherited by MetaMorpho so that function signatures are checked by the compiler. +/// @dev Consider using the IMetaMorpho interface instead of this one. +interface IMetaMorphoStaticTyping is IMetaMorphoBase { + function config(Id) external view returns (uint192 cap, uint64 withdrawRank); + function pendingGuardian() external view returns (address guardian, uint64 submittedAt); + function pendingCap(Id) external view returns (uint192 value, uint64 submittedAt); + function pendingTimelock() external view returns (uint192 value, uint64 submittedAt); + function pendingFee() external view returns (uint192 value, uint64 submittedAt); +} + +/// @title IMetaMorpho +/// @author Morpho Labs +/// @custom:contact security@morpho.org +/// @dev Use this interface for MetaMorpho to have access to all the functions with the appropriate function signatures. +interface IMetaMorpho is IMetaMorphoBase, IERC4626, IERC20Permit, IOwnable, IMulticall { + function config(Id) external view returns (MarketConfig memory); function pendingGuardian() external view returns (PendingAddress memory); + function pendingCap(Id) external view returns (PendingUint192 memory); + function pendingTimelock() external view returns (PendingUint192 memory); + function pendingFee() external view returns (PendingUint192 memory); } diff --git a/test/forge/GuardianTest.sol b/test/forge/GuardianTest.sol index ea917401..b82bf46c 100644 --- a/test/forge/GuardianTest.sol +++ b/test/forge/GuardianTest.sol @@ -43,11 +43,11 @@ contract GuardianTest is IntegrationTest { vault.revokePendingTimelock(); uint256 newTimelock = vault.timelock(); - (uint256 pendingTimelock, uint64 submittedAt) = vault.pendingTimelock(); + PendingUint192 memory pendingTimelock = vault.pendingTimelock(); assertEq(newTimelock, TIMELOCK, "newTimelock"); - assertEq(pendingTimelock, 0, "pendingTimelock"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(pendingTimelock.value, 0, "pendingTimelock.value"); + assertEq(pendingTimelock.submittedAt, 0, "pendingTimelock.submittedAt"); } function testRevokePendingCapIncreased(uint256 seed, uint256 cap, uint256 elapsed) public { @@ -67,13 +67,13 @@ contract GuardianTest is IntegrationTest { vm.prank(GUARDIAN); vault.revokePendingCap(id); - (uint192 newCap, uint64 withdrawRank) = vault.config(id); - (uint256 pendingCap, uint64 submittedAt) = vault.pendingCap(id); + MarketConfig memory marketConfig = vault.config(id); + PendingUint192 memory pendingCap = vault.pendingCap(id); - assertEq(newCap, 0, "newCap"); - assertEq(withdrawRank, 0, "withdrawRank"); - assertEq(pendingCap, 0, "pendingCap"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(marketConfig.cap, 0, "marketConfig.cap"); + assertEq(marketConfig.withdrawRank, 0, "marketConfig.withdrawRank"); + assertEq(pendingCap.value, 0, "pendingCap.value"); + assertEq(pendingCap.submittedAt, 0, "pendingCap.submittedAt"); } function testRevokePendingGuardian(uint256 elapsed) public { @@ -92,10 +92,10 @@ contract GuardianTest is IntegrationTest { vault.revokePendingGuardian(); address newGuardian = vault.guardian(); - (address pendingGuardian, uint96 submittedAt) = vault.pendingGuardian(); + PendingAddress memory pendingGuardian = vault.pendingGuardian(); assertEq(newGuardian, GUARDIAN, "newGuardian"); - assertEq(pendingGuardian, address(0), "pendingGuardian"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(pendingGuardian.value, address(0), "pendingGuardian.value"); + assertEq(pendingGuardian.submittedAt, 0, "pendingGuardian.submittedAt"); } } diff --git a/test/forge/TimelockTest.sol b/test/forge/TimelockTest.sol index 5d35635a..f2176438 100644 --- a/test/forge/TimelockTest.sol +++ b/test/forge/TimelockTest.sol @@ -31,11 +31,11 @@ contract TimelockTest is IntegrationTest { vault.submitTimelock(timelock); uint256 newTimelock = vault.timelock(); - (uint256 pendingTimelock, uint64 submittedAt) = vault.pendingTimelock(); + PendingUint192 memory pendingTimelock = vault.pendingTimelock(); assertEq(newTimelock, timelock, "newTimelock"); - assertEq(pendingTimelock, 0, "pendingTimelock"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(pendingTimelock.value, 0, "pendingTimelock.value"); + assertEq(pendingTimelock.submittedAt, 0, "pendingTimelock.submittedAt"); } function testSubmitTimelockDecreased(uint256 timelock) public { @@ -47,11 +47,11 @@ contract TimelockTest is IntegrationTest { vault.submitTimelock(timelock); uint256 newTimelock = vault.timelock(); - (uint256 pendingTimelock, uint64 submittedAt) = vault.pendingTimelock(); + PendingUint192 memory pendingTimelock = vault.pendingTimelock(); assertEq(newTimelock, TIMELOCK, "newTimelock"); - assertEq(pendingTimelock, timelock, "pendingTimelock"); - assertEq(submittedAt, block.timestamp, "submittedAt"); + assertEq(pendingTimelock.value, timelock, "pendingTimelock.value"); + assertEq(pendingTimelock.submittedAt, block.timestamp, "pendingTimelock.submittedAt"); } function testSubmitTimelockNotOwner(uint256 timelock) public { @@ -110,11 +110,11 @@ contract TimelockTest is IntegrationTest { vault.acceptTimelock(); uint256 newTimelock = vault.timelock(); - (uint256 pendingTimelock, uint64 submittedAt) = vault.pendingTimelock(); + PendingUint192 memory pendingTimelock = vault.pendingTimelock(); assertEq(newTimelock, timelock, "newTimelock"); - assertEq(pendingTimelock, 0, "pendingTimelock"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(pendingTimelock.value, 0, "pendingTimelock.value"); + assertEq(pendingTimelock.submittedAt, 0, "pendingTimelock.submittedAt"); } function testAcceptTimelockNoPendingValue() public { @@ -145,11 +145,11 @@ contract TimelockTest is IntegrationTest { vault.submitFee(fee); uint256 newFee = vault.fee(); - (uint256 pendingFee, uint64 submittedAt) = vault.pendingFee(); + PendingUint192 memory pendingFee = vault.pendingFee(); assertEq(newFee, fee, "newFee"); - assertEq(pendingFee, 0, "pendingFee"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(pendingFee.value, 0, "pendingFee.value"); + assertEq(pendingFee.submittedAt, 0, "pendingFee.submittedAt"); } function testSubmitFeeIncreased(uint256 fee) public { @@ -161,11 +161,11 @@ contract TimelockTest is IntegrationTest { vault.submitFee(fee); uint256 newFee = vault.fee(); - (uint256 pendingFee, uint64 submittedAt) = vault.pendingFee(); + PendingUint192 memory pendingFee = vault.pendingFee(); assertEq(newFee, FEE, "newFee"); - assertEq(pendingFee, fee, "pendingFee"); - assertEq(submittedAt, block.timestamp, "submittedAt"); + assertEq(pendingFee.value, fee, "pendingFee.value"); + assertEq(pendingFee.submittedAt, block.timestamp, "pendingFee.submittedAt"); } function testAcceptFee(uint256 fee) public { @@ -182,11 +182,11 @@ contract TimelockTest is IntegrationTest { vault.acceptFee(); uint256 newFee = vault.fee(); - (uint256 pendingFee, uint64 submittedAt) = vault.pendingFee(); + PendingUint192 memory pendingFee = vault.pendingFee(); assertEq(newFee, fee, "newFee"); - assertEq(pendingFee, 0, "pendingFee"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(pendingFee.value, 0, "pendingFee.value"); + assertEq(pendingFee.submittedAt, 0, "pendingFee.submittedAt"); } function testAcceptFeeNoPendingValue() public { @@ -216,11 +216,11 @@ contract TimelockTest is IntegrationTest { vault.submitGuardian(guardian); address newGuardian = vault.guardian(); - (address pendingGuardian, uint96 submittedAt) = vault.pendingGuardian(); + PendingAddress memory pendingGuardian = vault.pendingGuardian(); assertEq(newGuardian, GUARDIAN, "newGuardian"); - assertEq(pendingGuardian, guardian, "pendingGuardian"); - assertEq(submittedAt, block.timestamp, "submittedAt"); + assertEq(pendingGuardian.value, guardian, "pendingGuardian.value"); + assertEq(pendingGuardian.submittedAt, block.timestamp, "pendingGuardian.submittedAt"); } function testSubmitGuardianFromZero() public { @@ -232,11 +232,11 @@ contract TimelockTest is IntegrationTest { vault.submitGuardian(GUARDIAN); address newGuardian = vault.guardian(); - (address pendingGuardian, uint96 submittedAt) = vault.pendingGuardian(); + PendingAddress memory pendingGuardian = vault.pendingGuardian(); assertEq(newGuardian, GUARDIAN, "newGuardian"); - assertEq(pendingGuardian, address(0), "pendingGuardian"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(pendingGuardian.value, address(0), "pendingGuardian.value"); + assertEq(pendingGuardian.submittedAt, 0, "pendingGuardian.submittedAt"); } function testSubmitGuardianZero() public { @@ -244,11 +244,11 @@ contract TimelockTest is IntegrationTest { vault.submitGuardian(address(0)); address newGuardian = vault.guardian(); - (address pendingGuardian, uint96 submittedAt) = vault.pendingGuardian(); + PendingAddress memory pendingGuardian = vault.pendingGuardian(); assertEq(newGuardian, GUARDIAN, "newGuardian"); - assertEq(pendingGuardian, address(0), "pendingGuardian"); - assertEq(submittedAt, block.timestamp, "submittedAt"); + assertEq(pendingGuardian.value, address(0), "pendingGuardian.value"); + assertEq(pendingGuardian.submittedAt, block.timestamp, "pendingGuardian.submittedAt"); } function testAcceptGuardian() public { @@ -264,11 +264,11 @@ contract TimelockTest is IntegrationTest { vault.acceptGuardian(); address newGuardian = vault.guardian(); - (address pendingGuardian, uint96 submittedAt) = vault.pendingGuardian(); + PendingAddress memory pendingGuardian = vault.pendingGuardian(); assertEq(newGuardian, guardian, "newGuardian"); - assertEq(pendingGuardian, address(0), "pendingGuardian"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(pendingGuardian.value, address(0), "pendingGuardian.value"); + assertEq(pendingGuardian.submittedAt, 0, "pendingGuardian.submittedAt"); } function testAcceptGuardianNoPendingValue() public { @@ -301,13 +301,13 @@ contract TimelockTest is IntegrationTest { vm.prank(CURATOR); vault.submitCap(marketParams, cap); - (uint192 newCap, uint64 withdrawRank) = vault.config(id); - (uint192 pendingCap, uint64 submittedAt) = vault.pendingCap(id); + MarketConfig memory marketConfig = vault.config(id); + PendingUint192 memory pendingCap = vault.pendingCap(id); - assertEq(newCap, cap, "newCap"); - assertEq(withdrawRank, 1, "withdrawRank"); - assertEq(pendingCap, 0, "pendingCap"); - assertEq(submittedAt, 0, "submittedAt"); + assertEq(marketConfig.cap, cap, "marketConfig.cap"); + assertEq(marketConfig.withdrawRank, 1, "marketConfig.withdrawRank"); + assertEq(pendingCap.value, 0, "pendingCap.value"); + assertEq(pendingCap.submittedAt, 0, "pendingCap.submittedAt"); } function testSubmitCapIncreased(uint256 cap) public { @@ -321,13 +321,13 @@ contract TimelockTest is IntegrationTest { vm.prank(CURATOR); vault.submitCap(marketParams, cap); - (uint192 newCap, uint64 withdrawRank) = vault.config(id); - (uint192 pendingCap, uint64 submittedAt) = vault.pendingCap(id); + MarketConfig memory marketConfig = vault.config(id); + PendingUint192 memory pendingCap = vault.pendingCap(id); - assertEq(newCap, 0, "newCap"); - assertEq(withdrawRank, 0, "withdrawRank"); - assertEq(pendingCap, cap, "pendingCap"); - assertEq(submittedAt, block.timestamp, "submittedAt"); + assertEq(marketConfig.cap, 0, "marketConfig.cap"); + assertEq(marketConfig.withdrawRank, 0, "marketConfig.withdrawRank"); + assertEq(pendingCap.value, cap, "pendingCap.value"); + assertEq(pendingCap.submittedAt, block.timestamp, "pendingCap.submittedAt"); assertEq(vault.supplyQueueLength(), 1, "supplyQueueLength"); assertEq(vault.withdrawQueueLength(), 1, "withdrawQueueLength"); } @@ -347,13 +347,13 @@ contract TimelockTest is IntegrationTest { emit EventsLib.SetCap(address(this), id, cap); vault.acceptCap(id); - (uint192 newCap, uint64 withdrawRank) = vault.config(id); - (uint192 pendingCapAfter, uint64 submittedAtAfter) = vault.pendingCap(id); + MarketConfig memory marketConfig = vault.config(id); + PendingUint192 memory pendingCap = vault.pendingCap(id); - assertEq(newCap, cap, "newCap"); - assertEq(withdrawRank, 1, "withdrawRank"); - assertEq(pendingCapAfter, 0, "pendingCapAfter"); - assertEq(submittedAtAfter, 0, "submittedAtAfter"); + assertEq(marketConfig.cap, cap, "marketConfig.cap"); + assertEq(marketConfig.withdrawRank, 1, "marketConfig.withdrawRank"); + assertEq(pendingCap.value, 0, "pendingCap.value"); + assertEq(pendingCap.submittedAt, 0, "pendingCap.submittedAt"); assertEq(Id.unwrap(vault.supplyQueue(0)), Id.unwrap(id), "supplyQueue"); assertEq(Id.unwrap(vault.withdrawQueue(0)), Id.unwrap(id), "withdrawQueue"); } diff --git a/test/forge/helpers/BaseTest.sol b/test/forge/helpers/BaseTest.sol index 3a3cc469..5f87d323 100644 --- a/test/forge/helpers/BaseTest.sol +++ b/test/forge/helpers/BaseTest.sol @@ -9,7 +9,7 @@ import {MarketParamsLib} from "@morpho-blue/libraries/MarketParamsLib.sol"; import {MorphoLib} from "@morpho-blue/libraries/periphery/MorphoLib.sol"; import {MorphoBalancesLib} from "@morpho-blue/libraries/periphery/MorphoBalancesLib.sol"; -import {IPending} from "src/interfaces/IMetaMorpho.sol"; +import "src/interfaces/IMetaMorpho.sol"; import "src/libraries/ConstantsLib.sol"; import {ErrorsLib} from "src/libraries/ErrorsLib.sol"; diff --git a/test/forge/helpers/IntegrationTest.sol b/test/forge/helpers/IntegrationTest.sol index e9f453ef..3171df8f 100644 --- a/test/forge/helpers/IntegrationTest.sol +++ b/test/forge/helpers/IntegrationTest.sol @@ -7,13 +7,16 @@ contract IntegrationTest is BaseTest { using MathLib for uint256; using MarketParamsLib for MarketParams; - MetaMorpho internal vault; + IMetaMorpho internal vault; function setUp() public virtual override { super.setUp(); - vault = - new MetaMorpho(OWNER, address(morpho), ConstantsLib.MIN_TIMELOCK, address(loanToken), "MetaMorpho Vault", "MMV"); + vault = IMetaMorpho( + address( + new MetaMorpho(OWNER, address(morpho), ConstantsLib.MIN_TIMELOCK, address(loanToken), "MetaMorpho Vault", "MMV") + ) + ); vm.startPrank(OWNER); vault.setCurator(CURATOR); @@ -89,7 +92,7 @@ contract IntegrationTest is BaseTest { function _setCap(MarketParams memory marketParams, uint256 newCap) internal { Id id = marketParams.id(); - (uint256 cap,) = vault.config(id); + uint256 cap = vault.config(id).cap; if (newCap == cap) return; vm.prank(CURATOR); @@ -102,8 +105,6 @@ contract IntegrationTest is BaseTest { vault.acceptCap(id); - (cap,) = vault.config(id); - - assertEq(cap, newCap, "_setCap"); + assertEq(vault.config(id).cap, newCap, "_setCap"); } } From 0ad301d3fda636924998f38e6e794580825a2fe9 Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Tue, 7 Nov 2023 17:47:17 +0100 Subject: [PATCH 26/30] docs(metamorpho): re-align updatres --- src/MetaMorpho.sol | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index c7b8eea1..f6f52f00 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -208,11 +208,8 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph if (newTimelock > timelock) { _setTimelock(newTimelock); } else { - pendingTimelock.update( - // Safe "unchecked" cast because newTimelock <= MAX_TIMELOCK. - uint192(newTimelock), - timelock - ); + // Safe "unchecked" cast because newTimelock <= MAX_TIMELOCK. + pendingTimelock.update(uint192(newTimelock), timelock); emit EventsLib.SubmitTimelock(newTimelock); } @@ -228,11 +225,8 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph if (newFee < fee) { _setFee(newFee); } else { - pendingFee.update( - // Safe "unchecked" cast because newFee <= MAX_FEE. - uint192(newFee), - timelock - ); + // Safe "unchecked" cast because newFee <= MAX_FEE. + pendingFee.update(uint192(newFee), timelock); emit EventsLib.SubmitFee(newFee); } From 3506219655acba1a47f23b04c091030956aa3e7b Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Wed, 8 Nov 2023 08:41:54 +0100 Subject: [PATCH 27/30] refactor(pending-update): inline uint64 cast --- src/libraries/PendingLib.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/PendingLib.sol b/src/libraries/PendingLib.sol index 6c37cca8..88022107 100644 --- a/src/libraries/PendingLib.sol +++ b/src/libraries/PendingLib.sol @@ -25,7 +25,7 @@ library PendingLib { function update(PendingUint192 storage pending, uint192 newValue, uint256 timelock) internal { pending.value = newValue; // Safe "unchecked" cast because timelock <= MAX_TIMELOCK. - pending.validAt = uint64(block.timestamp) + uint64(timelock); + pending.validAt = uint64(block.timestamp + timelock); } /// @dev Updates `pending`'s value to `newValue` and its corresponding `validAt` timestamp. @@ -33,6 +33,6 @@ library PendingLib { function update(PendingAddress storage pending, address newValue, uint256 timelock) internal { pending.value = newValue; // Safe "unchecked" cast because timelock <= MAX_TIMELOCK. - pending.validAt = uint64(block.timestamp) + uint64(timelock); + pending.validAt = uint64(block.timestamp + timelock); } } From 7b2d9aa7f7d28f46e3b33adc9c5650ee2270fa27 Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Wed, 8 Nov 2023 15:26:36 +0100 Subject: [PATCH 28/30] docs(readme): fix incorrect sentences --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index d5d28705..d6f7a178 100644 --- a/README.md +++ b/README.md @@ -74,7 +74,7 @@ It can: - Upon a withdrawal, the vault will first withdraw from the idle supply and then withdraw up to the liquidity of each Morpho Blue market in the `withdrawalQueue` in the order set. - The `supplyQueue` only contains markets which cap has previously been non-zero. - The `withdrawQueue` contains all markets that have a non-zero cap or a non-zero vault allocation. -- Instantaneously reallocate funds across markets of the `withdrawQueue` at any moment. +- Instantaneously reallocate funds by supplying on markets of the `withdrawQueue` and withdrawing from markets that have the same loan asset as the vault's asset. #### Guardian @@ -83,7 +83,7 @@ Only one address can have this role. It can: - Revoke the pending timelock. -- Revoke the pending guardian (in particular, it can revoke any submission of another guardian). +- Revoke the pending guardian (which means it can revoke any attempt to change the guardian). - Revoke the pending cap of any market. ### Rewards From 4d9ae92be90a220dc5f8b6b71642acb462721687 Mon Sep 17 00:00:00 2001 From: MerlinEgalite Date: Thu, 9 Nov 2023 16:02:42 +0300 Subject: [PATCH 29/30] test: fix fee test --- test/forge/FeeTest.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/test/forge/FeeTest.sol b/test/forge/FeeTest.sol index cca4c4e4..7714222b 100644 --- a/test/forge/FeeTest.sol +++ b/test/forge/FeeTest.sol @@ -41,6 +41,7 @@ contract FeeTest is IntegrationTest { } function testSetFee(uint256 fee) public { + vm.assume(fee != vault.fee()); fee = bound(fee, 0, ConstantsLib.MAX_FEE); vm.expectEmit(address(vault)); From b334c32f15593e8e5f2688a974f4deaa88879047 Mon Sep 17 00:00:00 2001 From: MerlinEgalite Date: Thu, 9 Nov 2023 16:23:19 +0300 Subject: [PATCH 30/30] fix: merge conflict required changes --- test/forge/GuardianTest.sol | 2 +- test/forge/RevokeTest.sol | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/test/forge/GuardianTest.sol b/test/forge/GuardianTest.sol index 522046a4..0c338024 100644 --- a/test/forge/GuardianTest.sol +++ b/test/forge/GuardianTest.sol @@ -74,7 +74,7 @@ contract GuardianTest is IntegrationTest { assertEq(newTimelock, TIMELOCK, "newTimelock"); assertEq(pendingTimelock.value, 0, "value"); - assertEq(pendingTimelock.submittedAt, 0, "submittedAt"); + assertEq(pendingTimelock.validAt, 0, "validAt"); } function testGuardianRevokePendingCapIncreased(uint256 seed, uint256 cap, uint256 elapsed) public { diff --git a/test/forge/RevokeTest.sol b/test/forge/RevokeTest.sol index ccfc5cd7..26cd2ce4 100644 --- a/test/forge/RevokeTest.sol +++ b/test/forge/RevokeTest.sol @@ -41,7 +41,7 @@ contract RevokeTest is IntegrationTest { assertEq(newTimelock, TIMELOCK, "newTimelock"); assertEq(pendingTimelock.value, 0, "value"); - assertEq(pendingTimelock.submittedAt, 0, "submittedAt"); + assertEq(pendingTimelock.validAt, 0, "validAt"); } function testCuratorRevokeCapIncreased(uint256 seed, uint256 cap, uint256 elapsed) public { @@ -67,7 +67,7 @@ contract RevokeTest is IntegrationTest { assertEq(marketConfig.cap, 0, "cap"); assertEq(marketConfig.withdrawRank, 0, "withdrawRank"); assertEq(pendingCap.value, 0, "value"); - assertEq(pendingCap.submittedAt, 0, "submittedAt"); + assertEq(pendingCap.validAt, 0, "validAt"); } function testOwnerRevokeCapIncreased(uint256 seed, uint256 cap, uint256 elapsed) public { @@ -93,7 +93,7 @@ contract RevokeTest is IntegrationTest { assertEq(marketConfig.cap, 0, "cap"); assertEq(marketConfig.withdrawRank, 0, "withdrawRank"); assertEq(pendingCap.value, 0, "value"); - assertEq(pendingCap.submittedAt, 0, "submittedAt"); + assertEq(pendingCap.validAt, 0, "validAt"); } function testOwnerRevokeGuardian(uint256 elapsed) public { @@ -116,7 +116,7 @@ contract RevokeTest is IntegrationTest { assertEq(newGuardian, GUARDIAN, "newGuardian"); assertEq(pendingGuardian.value, address(0), "value"); - assertEq(pendingGuardian.submittedAt, 0, "submittedAt"); + assertEq(pendingGuardian.validAt, 0, "validAt"); } function testOwnerRevokePendingCapNoPendingValue(uint256 seed) public {