refactor(vault): rewrite _deposit & _withdraw

src/MetaMorpho.sol

@@ -5,6 +5,7 @@ import {IMorphoMarketParams} from "./interfaces/IMorphoMarketParams.sol";
 import {MarketAllocation, Pending, IMetaMorpho} from "./interfaces/IMetaMorpho.sol";
 import {Id, MarketParams, Market, IMorpho} from "@morpho-blue/interfaces/IMorpho.sol";
 
+import "src/libraries/ConstantsLib.sol";
 import {ErrorsLib} from "./libraries/ErrorsLib.sol";
 import {EventsLib} from "./libraries/EventsLib.sol";
 import {WAD} from "@morpho-blue/libraries/MathLib.sol";
@@ -30,12 +31,6 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho {
     using MarketParamsLib for MarketParams;
     using MorphoBalancesLib for IMorpho;
 
-    /* CONSTANTS */
-
-    uint256 public constant TIMELOCK_EXPIRATION = 2 days;
-    uint256 public constant MAX_TIMELOCK = 2 weeks;
-    uint256 public constant DECIMALS_OFFSET = 6;
-
     /* IMMUTABMES */
 
     IMorpho internal immutable MORPHO;
@@ -233,7 +228,7 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho {
 
     /* ERC4626 */
 
-    function maxWithdraw(address owner) public view virtual override returns (uint256) {
+    function maxWithdraw(address owner) public view override returns (uint256) {
         _accruedFeeShares();
 
         return _staticWithdrawOrder(super.maxWithdraw(owner));
@@ -243,7 +238,7 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho {
         return _convertToShares(maxWithdraw(owner), Math.Rounding.Down);
     }
 
-    function deposit(uint256 assets, address receiver) public virtual override returns (uint256 shares) {
+    function deposit(uint256 assets, address receiver) public override returns (uint256 shares) {
         uint256 newTotalAssets = _accrueFee();
 
         shares = _convertToSharesWithFeeAccrued(assets, newTotalAssets, Math.Rounding.Down);
@@ -252,7 +247,7 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho {
         _updateLastTotalAssets(newTotalAssets + assets);
     }
 
-    function mint(uint256 shares, address receiver) public virtual override returns (uint256 assets) {
+    function mint(uint256 shares, address receiver) public override returns (uint256 assets) {
         uint256 newTotalAssets = _accrueFee();
 
         assets = _convertToAssetsWithFeeAccrued(shares, newTotalAssets, Math.Rounding.Up);
@@ -261,12 +256,7 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho {
         _updateLastTotalAssets(newTotalAssets + assets);
     }
 
-    function withdraw(uint256 assets, address receiver, address owner)
-        public
-        virtual
-        override
-        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.
@@ -277,7 +267,7 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho {
         _updateLastTotalAssets(newTotalAssets - assets);
     }
 
-    function redeem(uint256 shares, address receiver, address owner) public virtual override 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.
@@ -302,40 +292,53 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho {
 
     /// @dev Used in mint or deposit to deposit the underlying asset to Blue markets.
     function _deposit(address caller, address owner, uint256 assets, uint256 shares) internal override {
-        super._deposit(caller, owner, assets, shares);
+        // If asset is ERC777, `transferFrom` can trigger a reentrancy BEFORE the transfer happens through the
+        // `tokensToSend` hook. On the other hand, the `tokenReceived` hook, that is triggered after the transfer,
+        // calls the vault, which is assumed not malicious.
+        //
+        // Conclusion: we need to do the transfer before we mint so that any reentrancy would happen before the
+        // assets are transferred and before the shares are minted, which is a valid state.
+        // slither-disable-next-line reentrancy-no-eth
+        SafeERC20.safeTransferFrom(IERC20(asset()), caller, address(this), assets);
 
         require(_depositOrder(assets) == 0, ErrorsLib.DEPOSIT_ORDER_FAILED);
+
+        _mint(owner, shares);
+
+        emit Deposit(caller, owner, assets, shares);
     }
 
     /// @dev Used in redeem or withdraw to withdraw the underlying asset from Blue markets.
     function _withdraw(address caller, address receiver, address owner, uint256 assets, uint256 shares)
         internal
         override
     {
+        if (caller != owner) {
+            _spendAllowance(owner, caller, shares);
+        }
+
+        // If asset is ERC777, `transfer` can trigger a reentrancy AFTER the transfer happens through the
+        // `tokensReceived` hook. On the other hand, the `tokensToSend` hook, that is triggered before the transfer,
+        // calls the vault, which is assumed not malicious.
+        //
+        // Conclusion: we need to do the transfer after the burn so that any reentrancy would happen after the
+        // shares are burned and after the assets are transferred, which is a valid state.
+        _burn(owner, shares);
+
         require(_withdrawOrder(assets) == 0, ErrorsLib.WITHDRAW_ORDER_FAILED);
 
-        super._withdraw(caller, receiver, owner, assets, shares);
+        SafeERC20.safeTransfer(IERC20(asset()), receiver, assets);
+
+        emit Withdraw(caller, receiver, owner, assets, shares);
     }
 
-    function _convertToShares(uint256 assets, Math.Rounding rounding)
-        internal
-        view
-        virtual
-        override
-        returns (uint256)
-    {
+    function _convertToShares(uint256 assets, Math.Rounding rounding) internal view override returns (uint256) {
         (uint256 feeShares, uint256 newTotalAssets) = _accruedFeeShares();
 
         return assets.mulDiv(totalSupply() + feeShares + 10 ** _decimalsOffset(), newTotalAssets + 1, rounding);
     }
 
-    function _convertToAssets(uint256 shares, Math.Rounding rounding)
-        internal
-        view
-        virtual
-        override
-        returns (uint256)
-    {
+    function _convertToAssets(uint256 shares, Math.Rounding rounding) internal view override returns (uint256) {
         (uint256 feeShares, uint256 newTotalAssets) = _accruedFeeShares();
 
         return shares.mulDiv(newTotalAssets + 1, totalSupply() + feeShares + 10 ** _decimalsOffset(), rounding);

src/libraries/ConstantsLib.sol

@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+pragma solidity ^0.8.0;
+
+/// @dev The delay after a timelock ends after which the owner must submit a parameter again.
+/// It guarantees users that the owner only accepts parameters submitted recently.
+uint256 constant TIMELOCK_EXPIRATION = 2 days;
+
+/// @dev The maximum delay of a timelock.
+uint256 constant MAX_TIMELOCK = 2 weeks;
+
+/// @dev OpenZeppelin's decimals offset used in MetaMorpho's ERC4626 implementation.
+uint256 constant DECIMALS_OFFSET = 6;

test/forge/ERC4626Test.sol

@@ -167,17 +167,19 @@ contract ERC4626Test is BaseTest {
         vault.transferFrom(ONBEHALF, RECEIVER, shares);
     }
 
-    function testWithdrawTooMuch(uint256 deposited) public {
+    function testWithdrawTooMuch(uint256 deposited, uint256 assets) public {
         deposited = bound(deposited, MIN_TEST_ASSETS, MAX_TEST_ASSETS);
 
         borrowableToken.setBalance(SUPPLIER, deposited);
 
         vm.prank(SUPPLIER);
         vault.deposit(deposited, ONBEHALF);
 
-        vm.prank(SUPPLIER);
-        vm.expectRevert(bytes(ErrorsLib.WITHDRAW_ORDER_FAILED));
-        vault.withdraw(deposited + 1, RECEIVER, ONBEHALF);
+        assets = bound(assets, deposited + 1, type(uint256).max / (deposited + 10 ** DECIMALS_OFFSET));
+
+        vm.prank(ONBEHALF);
+        vm.expectRevert("ERC20: burn amount exceeds balance");
+        vault.withdraw(assets, RECEIVER, ONBEHALF);
     }
 
     function testTransfer(uint256 deposited, uint256 toTransfer) public {

test/forge/MarketTest.sol

@@ -38,7 +38,7 @@ contract MarketTest is BaseTest {
     }
 
     function testEnableMarketShouldRevertWhenTimelockNotElapsed(uint128 timelock, uint256 timeElapsed) public {
-        timelock = uint128(bound(timelock, 1, vault.MAX_TIMELOCK()));
+        timelock = uint128(bound(timelock, 1, MAX_TIMELOCK));
         _submitAndSetTimelock(timelock);
 
         timeElapsed = bound(timeElapsed, 0, timelock - 1);
@@ -53,10 +53,10 @@ contract MarketTest is BaseTest {
     }
 
     function testEnableMarketShouldRevertWhenTimelockExpirationExceeded(uint128 timelock, uint256 timeElapsed) public {
-        timelock = uint128(bound(timelock, 1, vault.MAX_TIMELOCK()));
+        timelock = uint128(bound(timelock, 1, MAX_TIMELOCK));
         _submitAndSetTimelock(timelock);
 
-        timeElapsed = bound(timeElapsed, timelock + vault.TIMELOCK_EXPIRATION() + 1, type(uint128).max);
+        timeElapsed = bound(timeElapsed, timelock + TIMELOCK_EXPIRATION + 1, type(uint128).max);
 
         vm.startPrank(RISK_MANAGER);
         vault.submitMarket(allMarkets[0], CAP);

test/forge/TimelockTest.sol

@@ -5,7 +5,7 @@ import "./helpers/BaseTest.sol";
 
 contract TimelockTest is BaseTest {
     function testSubmitPendingTimelock(uint256 timelock) public {
-        timelock = bound(timelock, 0, vault.MAX_TIMELOCK());
+        timelock = bound(timelock, 0, MAX_TIMELOCK);
         vm.prank(OWNER);
         vault.submitTimelock(timelock);
 
@@ -20,15 +20,15 @@ contract TimelockTest is BaseTest {
     }
 
     function testSubmitPendingTimelockShouldRevertWhenMaxTimelockExceeded(uint256 timelock) public {
-        timelock = bound(timelock, vault.MAX_TIMELOCK() + 1, type(uint256).max);
+        timelock = bound(timelock, MAX_TIMELOCK + 1, type(uint256).max);
 
         vm.prank(OWNER);
         vm.expectRevert(bytes(ErrorsLib.MAX_TIMELOCK_EXCEEDED));
         vault.submitTimelock(timelock);
     }
 
     function testSetTimelock(uint256 timelock) public {
-        timelock = bound(timelock, 0, vault.MAX_TIMELOCK());
+        timelock = bound(timelock, 0, MAX_TIMELOCK);
         vm.startPrank(OWNER);
         vault.submitTimelock(timelock);
 

test/forge/helpers/BaseTest.sol

@@ -8,7 +8,8 @@ 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 {ORACLE_PRICE_SCALE} from "@morpho-blue/libraries/ConstantsLib.sol";
+import "src/libraries/ConstantsLib.sol";
+import "@morpho-blue/libraries/ConstantsLib.sol";
 
 import {IrmMock} from "src/mocks/IrmMock.sol";
 import {ERC20Mock} from "src/mocks/ERC20Mock.sol";