Merge pull request #84 from morpho-labs/refactor/withdraw-deposit

Roll back withdraw and deposit logic
morpho-org · Sep 22, 2023 · 54123ca · 54123ca
2 parents 0071a04 + 1386856
commit 54123ca
Show file tree

Hide file tree

Showing 2 changed files with 53 additions and 28 deletions.
diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol
@@ -448,44 +448,25 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph
 
     /// @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 {
-        // 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);
+        super._deposit(caller, owner, assets, shares);
 
         _supplyMorpho(assets);
-
-        _mint(owner, shares);
-
-        emit Deposit(caller, owner, assets, shares);
     }
 
     /// @dev Used in redeem or withdraw to withdraw the underlying asset from Blue markets.
+    /// @dev Reverts when withdrawing "too much", depending on 3 cases:
+    /// 1. "ERC20: burn amount exceeds balance" when withdrawing more `owner`'s than balance but less than vault's total
+    /// assets.
+    /// 2. "withdraw failed on Morpho" when withdrawing more than vault's total assets.
+    /// 3. "withdraw failed on Morpho" when withdrawing more than `owner`'s balance but less than the current available
+    /// liquidity.
     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(_withdrawMorpho(assets) == 0, ErrorsLib.WITHDRAW_FAILED_MORPHO);
 
-        SafeERC20.safeTransfer(IERC20(asset()), receiver, assets);
-
-        emit Withdraw(caller, receiver, owner, assets, shares);
+        super._withdraw(caller, receiver, owner, assets, shares);
     }
 
     /* INTERNAL */

diff --git a/test/forge/ERC4626Test.sol b/test/forge/ERC4626Test.sol
@@ -167,7 +167,7 @@ contract ERC4626Test is BaseTest {
         vault.transferFrom(ONBEHALF, RECEIVER, shares);
     }
 
-    function testWithdrawTooMuch(uint256 deposited, uint256 assets) public {
+    function testWithdrawMoreThanBalanceButLessThanTotalAssets(uint256 deposited, uint256 assets) public {
         deposited = bound(deposited, MIN_TEST_ASSETS, MAX_TEST_ASSETS);
 
         borrowableToken.setBalance(SUPPLIER, deposited);
@@ -177,11 +177,55 @@ contract ERC4626Test is BaseTest {
 
         assets = bound(assets, deposited + 1, type(uint256).max / (deposited + 10 ** DECIMALS_OFFSET));
 
+        uint256 toAdd = assets - deposited + 1;
+        borrowableToken.setBalance(SUPPLIER, toAdd);
+
+        vm.prank(SUPPLIER);
+        vault.deposit(toAdd, SUPPLIER);
+
         vm.prank(ONBEHALF);
         vm.expectRevert("ERC20: burn amount exceeds balance");
         vault.withdraw(assets, RECEIVER, ONBEHALF);
     }
 
+    function testWithdrawMoreThanTotalAssets(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);
+
+        assets = bound(assets, deposited + 1, type(uint256).max / (deposited + 10 ** DECIMALS_OFFSET));
+
+        vm.prank(ONBEHALF);
+        vm.expectRevert(bytes(ErrorsLib.WITHDRAW_FAILED_MORPHO));
+        vault.withdraw(assets, RECEIVER, ONBEHALF);
+    }
+
+    function testWithdrawMoreThanBalanceButLessThanLiquidity(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);
+
+        assets = bound(assets, deposited + 1, type(uint256).max / (deposited + 10 ** DECIMALS_OFFSET));
+
+        collateralToken.setBalance(BORROWER, type(uint128).max);
+
+        // Borrow liquidity.
+        vm.startPrank(BORROWER);
+        morpho.supplyCollateral(allMarkets[0], type(uint128).max, BORROWER, hex"");
+        morpho.borrow(allMarkets[0], 1, 0, BORROWER, BORROWER);
+        vm.stopPrank();
+
+        vm.prank(ONBEHALF);
+        vm.expectRevert(bytes(ErrorsLib.WITHDRAW_FAILED_MORPHO));
+        vault.withdraw(assets, RECEIVER, ONBEHALF);
+    }
+
     function testTransfer(uint256 deposited, uint256 toTransfer) public {
         deposited = bound(deposited, MIN_TEST_ASSETS, MAX_TEST_ASSETS);