From cb9366449241860d1a2f006a1a2466a84fce87c3 Mon Sep 17 00:00:00 2001 From: MerlinEgalite Date: Wed, 20 Sep 2023 12:06:21 +0200 Subject: [PATCH 1/5] refactor: roll back withdraw and deposit logic --- src/MetaMorpho.sol | 29 ++--------------------------- test/forge/ERC4626Test.sol | 2 +- 2 files changed, 3 insertions(+), 28 deletions(-) diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index 97cdf6e9..ed8b92f2 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -368,20 +368,9 @@ 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 { - // 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. @@ -389,23 +378,9 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho { 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..d3869a7a 100644 --- a/test/forge/ERC4626Test.sol +++ b/test/forge/ERC4626Test.sol @@ -178,7 +178,7 @@ contract ERC4626Test is BaseTest { assets = bound(assets, deposited + 1, type(uint256).max / (deposited + 10 ** DECIMALS_OFFSET)); vm.prank(ONBEHALF); - vm.expectRevert("ERC20: burn amount exceeds balance"); + vm.expectRevert(ErrorsLib.WITHDRAW_FAILED_MORPHO); vault.withdraw(assets, RECEIVER, ONBEHALF); } From bdc501c73ff645ca2269f033c50d01456e4e1a5b Mon Sep 17 00:00:00 2001 From: MerlinEgalite Date: Wed, 20 Sep 2023 14:24:20 +0200 Subject: [PATCH 2/5] test: fix error --- test/forge/ERC4626Test.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/forge/ERC4626Test.sol b/test/forge/ERC4626Test.sol index d3869a7a..abe5e9a3 100644 --- a/test/forge/ERC4626Test.sol +++ b/test/forge/ERC4626Test.sol @@ -178,7 +178,7 @@ contract ERC4626Test is BaseTest { assets = bound(assets, deposited + 1, type(uint256).max / (deposited + 10 ** DECIMALS_OFFSET)); vm.prank(ONBEHALF); - vm.expectRevert(ErrorsLib.WITHDRAW_FAILED_MORPHO); + vm.expectRevert(bytes(ErrorsLib.WITHDRAW_FAILED_MORPHO)); vault.withdraw(assets, RECEIVER, ONBEHALF); } From ce2f29f64fb9538c620793254573e9c30f0ed1c5 Mon Sep 17 00:00:00 2001 From: MerlinEgalite Date: Wed, 20 Sep 2023 17:43:39 +0200 Subject: [PATCH 3/5] feat: add comments + specific tests --- src/MetaMorpho.sol | 5 +++++ test/forge/ERC4626Test.sol | 46 +++++++++++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index ed8b92f2..2936f6ee 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -374,6 +374,11 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho { } /// @dev Used in redeem or withdraw to withdraw the underlying asset from Blue markets. + /// @dev When withdrawing "too much", the error logged depends on 3 cases: + /// 1. when withdrawing more than balance but less than vault's total assets "ERC20: burn amount exceeds balance" is + /// logged. + /// 2. when withdrawing more than vault's total assets "withdraw failed on Morpho" is logged. + /// 3. when withdrawing more than balance but less than liquidity "withdraw failed on Morpho" is logged. function _withdraw(address caller, address receiver, address owner, uint256 assets, uint256 shares) internal override diff --git a/test/forge/ERC4626Test.sol b/test/forge/ERC4626Test.sol index abe5e9a3..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,6 +177,50 @@ 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); From a5776ac20e33f961edd1601758b66e93b8375a24 Mon Sep 17 00:00:00 2001 From: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com> Date: Wed, 20 Sep 2023 17:53:13 +0200 Subject: [PATCH 4/5] docs: apply suggestion Co-authored-by: Romain Milon Signed-off-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com> --- src/MetaMorpho.sol | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index 2936f6ee..04133a89 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -374,11 +374,10 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho { } /// @dev Used in redeem or withdraw to withdraw the underlying asset from Blue markets. - /// @dev When withdrawing "too much", the error logged depends on 3 cases: - /// 1. when withdrawing more than balance but less than vault's total assets "ERC20: burn amount exceeds balance" is - /// logged. - /// 2. when withdrawing more than vault's total assets "withdraw failed on Morpho" is logged. - /// 3. when withdrawing more than balance but less than liquidity "withdraw failed on Morpho" is logged. + /// @dev Reverts when withdrawing "too much", depending on 3 cases: + /// 1. "ERC20: burn amount exceeds balance" when withdrawing more than balance but less than total assets. + /// 2. "withdraw failed on Morpho" when withdrawing more than vault's total assets. + /// 3. "withdraw failed on Morpho" when withdrawing more than balance but less than liquidity. function _withdraw(address caller, address receiver, address owner, uint256 assets, uint256 shares) internal override From 2f59f62fd353dca0d75333be129c9252f68fd2d1 Mon Sep 17 00:00:00 2001 From: MerlinEgalite Date: Fri, 22 Sep 2023 10:35:36 +0200 Subject: [PATCH 5/5] docs: improve comment --- src/MetaMorpho.sol | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index 04133a89..2d61cf7e 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -375,9 +375,11 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho { /// @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 than balance but less than total assets. + /// 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 balance but less than liquidity. + /// 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