diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index 86bc71ae..711a9da9 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -434,44 +434,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 index a494f3c9..6bfa179a 100644 --- 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);