From 07e00baf4b28dbcfc1e1fafd639dfd09db9865e9 Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Thu, 2 Nov 2023 17:08:56 +0100 Subject: [PATCH] fix(reallocate): prevent slippage --- src/MetaMorpho.sol | 17 +++--- src/interfaces/IMetaMorpho.sol | 2 - test/forge/ReallocateIdleTest.sol | 37 +++++++----- test/forge/ReallocateWithdrawTest.sol | 83 ++++++++++++--------------- test/hardhat/MetaMorpho.spec.ts | 17 ++---- 5 files changed, 77 insertions(+), 79 deletions(-) diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index 0987b492..d2101829 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -351,19 +351,22 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph uint256 totalWithdrawn; for (uint256 i; i < withdrawn.length; ++i) { MarketAllocation memory allocation = withdrawn[i]; + Id id = allocation.marketParams.id(); if (allocation.marketParams.loanToken != asset()) { - revert ErrorsLib.InconsistentAsset(allocation.marketParams.id()); + revert ErrorsLib.InconsistentAsset(id); } // Guarantees that unknown frontrunning donations can be withdrawn, in order to disable a market. - if (allocation.shares == type(uint256).max) { - allocation.shares = MORPHO.supplyShares(allocation.marketParams.id(), address(this)); + uint256 shares; + if (allocation.assets == type(uint256).max) { + shares = MORPHO.supplyShares(id, address(this)); + + allocation.assets = 0; } - (uint256 withdrawnAssets,) = MORPHO.withdraw( - allocation.marketParams, allocation.assets, allocation.shares, address(this), address(this) - ); + (uint256 withdrawnAssets,) = + MORPHO.withdraw(allocation.marketParams, allocation.assets, shares, address(this), address(this)); totalWithdrawn += withdrawnAssets; } @@ -377,7 +380,7 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph if (supplyCap == 0) revert ErrorsLib.UnauthorizedMarket(id); (uint256 suppliedAssets,) = - MORPHO.supply(allocation.marketParams, allocation.assets, allocation.shares, address(this), hex""); + MORPHO.supply(allocation.marketParams, allocation.assets, 0, address(this), hex""); if (_supplyBalance(allocation.marketParams) > supplyCap) { revert ErrorsLib.SupplyCapExceeded(id); diff --git a/src/interfaces/IMetaMorpho.sol b/src/interfaces/IMetaMorpho.sol index 99ce58b6..ae7cb0f7 100644 --- a/src/interfaces/IMetaMorpho.sol +++ b/src/interfaces/IMetaMorpho.sol @@ -31,8 +31,6 @@ struct MarketAllocation { MarketParams marketParams; /// @notice The amount of assets to allocate. uint256 assets; - /// @notice The amount of shares to allocate. - uint256 shares; } interface IMetaMorpho is IERC4626 { diff --git a/test/forge/ReallocateIdleTest.sol b/test/forge/ReallocateIdleTest.sol index fb0c9000..a3e898f7 100644 --- a/test/forge/ReallocateIdleTest.sol +++ b/test/forge/ReallocateIdleTest.sol @@ -31,26 +31,37 @@ contract ReallocateIdleTest is IntegrationTest { vault.deposit(INITIAL_DEPOSIT, ONBEHALF); } - function testReallocateSupplyIdle(uint256[3] memory suppliedShares) public { - suppliedShares[0] = bound(suppliedShares[0], SharesMathLib.VIRTUAL_SHARES, CAP2 * SharesMathLib.VIRTUAL_SHARES); - suppliedShares[1] = bound(suppliedShares[1], SharesMathLib.VIRTUAL_SHARES, CAP2 * SharesMathLib.VIRTUAL_SHARES); - suppliedShares[2] = bound(suppliedShares[2], SharesMathLib.VIRTUAL_SHARES, CAP2 * SharesMathLib.VIRTUAL_SHARES); + function testReallocateSupplyIdle(uint256[3] memory suppliedAssets) public { + suppliedAssets[0] = bound(suppliedAssets[0], 1, CAP2); + suppliedAssets[1] = bound(suppliedAssets[1], 1, CAP2); + suppliedAssets[2] = bound(suppliedAssets[2], 1, CAP2); - supplied.push(MarketAllocation(allMarkets[0], 0, suppliedShares[0])); - supplied.push(MarketAllocation(allMarkets[1], 0, suppliedShares[1])); - supplied.push(MarketAllocation(allMarkets[2], 0, suppliedShares[2])); + supplied.push(MarketAllocation(allMarkets[0], suppliedAssets[0])); + supplied.push(MarketAllocation(allMarkets[1], suppliedAssets[1])); + supplied.push(MarketAllocation(allMarkets[2], suppliedAssets[2])); uint256 idleBefore = vault.idle(); vm.prank(ALLOCATOR); vault.reallocate(withdrawn, supplied); - assertEq(morpho.supplyShares(allMarkets[0].id(), address(vault)), suppliedShares[0], "morpho.supplyShares(0)"); - assertEq(morpho.supplyShares(allMarkets[1].id(), address(vault)), suppliedShares[1], "morpho.supplyShares(1)"); - assertEq(morpho.supplyShares(allMarkets[2].id(), address(vault)), suppliedShares[2], "morpho.supplyShares(2)"); - - uint256 expectedIdle = idleBefore - suppliedShares[0] / SharesMathLib.VIRTUAL_SHARES - - suppliedShares[1] / SharesMathLib.VIRTUAL_SHARES - suppliedShares[2] / SharesMathLib.VIRTUAL_SHARES; + assertEq( + morpho.supplyShares(allMarkets[0].id(), address(vault)), + suppliedAssets[0] * SharesMathLib.VIRTUAL_SHARES, + "morpho.supplyShares(0)" + ); + assertEq( + morpho.supplyShares(allMarkets[1].id(), address(vault)), + suppliedAssets[1] * SharesMathLib.VIRTUAL_SHARES, + "morpho.supplyShares(1)" + ); + assertEq( + morpho.supplyShares(allMarkets[2].id(), address(vault)), + suppliedAssets[2] * SharesMathLib.VIRTUAL_SHARES, + "morpho.supplyShares(2)" + ); + + uint256 expectedIdle = idleBefore - suppliedAssets[0] - suppliedAssets[1] - suppliedAssets[2]; assertApproxEqAbs(vault.idle(), expectedIdle, 3, "vault.idle() 1"); } } diff --git a/test/forge/ReallocateWithdrawTest.sol b/test/forge/ReallocateWithdrawTest.sol index 2e901133..e8bb47d9 100644 --- a/test/forge/ReallocateWithdrawTest.sol +++ b/test/forge/ReallocateWithdrawTest.sol @@ -32,24 +32,10 @@ contract ReallocateWithdrawTest is IntegrationTest { vault.deposit(INITIAL_DEPOSIT, ONBEHALF); } - function testReallocateWithdrawAll() public { - withdrawn.push(MarketAllocation(allMarkets[0], 0, morpho.supplyShares(allMarkets[0].id(), address(vault)))); - withdrawn.push(MarketAllocation(allMarkets[1], 0, morpho.supplyShares(allMarkets[1].id(), address(vault)))); - withdrawn.push(MarketAllocation(allMarkets[2], 0, morpho.supplyShares(allMarkets[2].id(), address(vault)))); - - vm.prank(ALLOCATOR); - vault.reallocate(withdrawn, supplied); - - assertEq(morpho.supplyShares(allMarkets[0].id(), address(vault)), 0, "morpho.supplyShares(0)"); - assertEq(morpho.supplyShares(allMarkets[1].id(), address(vault)), 0, "morpho.supplyShares(1)"); - assertEq(morpho.supplyShares(allMarkets[2].id(), address(vault)), 0, "morpho.supplyShares(2)"); - assertEq(vault.idle(), INITIAL_DEPOSIT, "vault.idle() 1"); - } - function testReallocateWithdrawMax() public { - withdrawn.push(MarketAllocation(allMarkets[0], 0, type(uint256).max)); - withdrawn.push(MarketAllocation(allMarkets[1], 0, type(uint256).max)); - withdrawn.push(MarketAllocation(allMarkets[2], 0, type(uint256).max)); + withdrawn.push(MarketAllocation(allMarkets[0], type(uint256).max)); + withdrawn.push(MarketAllocation(allMarkets[1], type(uint256).max)); + withdrawn.push(MarketAllocation(allMarkets[2], type(uint256).max)); vm.prank(ALLOCATOR); vault.reallocate(withdrawn, supplied); @@ -63,39 +49,42 @@ contract ReallocateWithdrawTest is IntegrationTest { function testReallocateWithdrawInconsistentAsset() public { allMarkets[0].loanToken = address(1); - withdrawn.push(MarketAllocation(allMarkets[0], 0, 0)); + withdrawn.push(MarketAllocation(allMarkets[0], 0)); vm.prank(ALLOCATOR); vm.expectRevert(abi.encodeWithSelector(ErrorsLib.InconsistentAsset.selector, allMarkets[0].id())); vault.reallocate(withdrawn, supplied); } - function testReallocateWithdrawSupply(uint256[3] memory withdrawnShares, uint256[3] memory suppliedAssets) public { + function testReallocateWithdrawSupply(uint256[3] memory withdrawnAssets, uint256[3] memory suppliedAssets) public { uint256[3] memory sharesBefore = [ morpho.supplyShares(allMarkets[0].id(), address(vault)), morpho.supplyShares(allMarkets[1].id(), address(vault)), morpho.supplyShares(allMarkets[2].id(), address(vault)) ]; - withdrawnShares[0] = bound(withdrawnShares[0], 0, sharesBefore[0]); - withdrawnShares[1] = bound(withdrawnShares[1], 0, sharesBefore[1]); - withdrawnShares[2] = bound(withdrawnShares[2], 0, sharesBefore[2]); - uint256[3] memory totalSupplyAssets; uint256[3] memory totalSupplyShares; (totalSupplyAssets[0], totalSupplyShares[0],,) = morpho.expectedMarketBalances(allMarkets[0]); (totalSupplyAssets[1], totalSupplyShares[1],,) = morpho.expectedMarketBalances(allMarkets[1]); (totalSupplyAssets[2], totalSupplyShares[2],,) = morpho.expectedMarketBalances(allMarkets[2]); - uint256[3] memory withdrawnAssets = [ - withdrawnShares[0].toAssetsDown(totalSupplyAssets[0], totalSupplyShares[0]), - withdrawnShares[1].toAssetsDown(totalSupplyAssets[1], totalSupplyShares[1]), - withdrawnShares[2].toAssetsDown(totalSupplyAssets[2], totalSupplyShares[2]) + withdrawnAssets[0] = + bound(withdrawnAssets[0], 0, sharesBefore[0].toAssetsDown(totalSupplyAssets[0], totalSupplyShares[0])); + withdrawnAssets[1] = + bound(withdrawnAssets[1], 0, sharesBefore[1].toAssetsDown(totalSupplyAssets[1], totalSupplyShares[1])); + withdrawnAssets[2] = + bound(withdrawnAssets[2], 0, sharesBefore[2].toAssetsDown(totalSupplyAssets[2], totalSupplyShares[2])); + + uint256[3] memory withdrawnShares = [ + withdrawnAssets[0].toSharesUp(totalSupplyAssets[0], totalSupplyShares[0]), + withdrawnAssets[1].toSharesUp(totalSupplyAssets[1], totalSupplyShares[1]), + withdrawnAssets[2].toSharesUp(totalSupplyAssets[2], totalSupplyShares[2]) ]; - if (withdrawnShares[0] > 0) withdrawn.push(MarketAllocation(allMarkets[0], 0, withdrawnShares[0])); - if (withdrawnAssets[1] > 0) withdrawn.push(MarketAllocation(allMarkets[1], withdrawnAssets[1], 0)); - if (withdrawnShares[2] > 0) withdrawn.push(MarketAllocation(allMarkets[2], 0, withdrawnShares[2])); + if (withdrawnAssets[0] > 0) withdrawn.push(MarketAllocation(allMarkets[0], withdrawnAssets[0])); + if (withdrawnAssets[1] > 0) withdrawn.push(MarketAllocation(allMarkets[1], withdrawnAssets[1])); + if (withdrawnAssets[2] > 0) withdrawn.push(MarketAllocation(allMarkets[2], withdrawnAssets[2])); totalSupplyAssets[0] -= withdrawnAssets[0]; totalSupplyAssets[1] -= withdrawnAssets[1]; @@ -122,9 +111,9 @@ contract ReallocateWithdrawTest is IntegrationTest { suppliedAssets[2].toSharesDown(totalSupplyAssets[2], totalSupplyShares[2]) ]; - if (suppliedShares[0] > 0) supplied.push(MarketAllocation(allMarkets[0], suppliedAssets[0], 0)); - if (suppliedAssets[1] > 0) supplied.push(MarketAllocation(allMarkets[1], 0, suppliedShares[1])); - if (suppliedShares[2] > 0) supplied.push(MarketAllocation(allMarkets[2], suppliedAssets[2], 0)); + if (suppliedAssets[0] > 0) supplied.push(MarketAllocation(allMarkets[0], suppliedAssets[0])); + if (suppliedAssets[1] > 0) supplied.push(MarketAllocation(allMarkets[1], suppliedAssets[1])); + if (suppliedAssets[2] > 0) supplied.push(MarketAllocation(allMarkets[2], suppliedAssets[2])); vm.prank(ALLOCATOR); vault.reallocate(withdrawn, supplied); @@ -148,18 +137,20 @@ contract ReallocateWithdrawTest is IntegrationTest { assertApproxEqAbs(vault.idle(), expectedIdle, 1, "vault.idle() 1"); } - function testReallocateUnauthorizedMarket(uint256 amount) public { - amount = bound(amount, 1, CAP2); + function testReallocateUnauthorizedMarket(uint256[3] memory suppliedAssets) public { + suppliedAssets[0] = bound(suppliedAssets[0], 1, CAP2); + suppliedAssets[1] = bound(suppliedAssets[1], 1, CAP2); + suppliedAssets[2] = bound(suppliedAssets[2], 1, CAP2); _setCap(allMarkets[1], 0); - withdrawn.push(MarketAllocation(allMarkets[0], 0, type(uint256).max)); - withdrawn.push(MarketAllocation(allMarkets[1], 0, type(uint256).max)); - withdrawn.push(MarketAllocation(allMarkets[2], 0, type(uint256).max)); + withdrawn.push(MarketAllocation(allMarkets[0], type(uint256).max)); + withdrawn.push(MarketAllocation(allMarkets[1], type(uint256).max)); + withdrawn.push(MarketAllocation(allMarkets[2], type(uint256).max)); - supplied.push(MarketAllocation(allMarkets[0], amount, 0)); - supplied.push(MarketAllocation(allMarkets[1], amount, 0)); - supplied.push(MarketAllocation(allMarkets[2], amount, 0)); + supplied.push(MarketAllocation(allMarkets[0], suppliedAssets[0])); + supplied.push(MarketAllocation(allMarkets[1], suppliedAssets[1])); + supplied.push(MarketAllocation(allMarkets[2], suppliedAssets[2])); vm.prank(ALLOCATOR); vm.expectRevert(abi.encodeWithSelector(ErrorsLib.UnauthorizedMarket.selector, allMarkets[1].id())); @@ -167,11 +158,11 @@ contract ReallocateWithdrawTest is IntegrationTest { } function testReallocateSupplyCapExceeded() public { - withdrawn.push(MarketAllocation(allMarkets[0], 0, type(uint256).max)); - withdrawn.push(MarketAllocation(allMarkets[1], 0, type(uint256).max)); - withdrawn.push(MarketAllocation(allMarkets[2], 0, type(uint256).max)); + withdrawn.push(MarketAllocation(allMarkets[0], type(uint256).max)); + withdrawn.push(MarketAllocation(allMarkets[1], type(uint256).max)); + withdrawn.push(MarketAllocation(allMarkets[2], type(uint256).max)); - supplied.push(MarketAllocation(allMarkets[0], CAP2 + 1, 0)); + supplied.push(MarketAllocation(allMarkets[0], CAP2 + 1)); vm.prank(ALLOCATOR); vm.expectRevert(abi.encodeWithSelector(ErrorsLib.SupplyCapExceeded.selector, allMarkets[0].id())); @@ -188,7 +179,7 @@ contract ReallocateWithdrawTest is IntegrationTest { _setCap(allMarkets[0], type(uint192).max); - supplied.push(MarketAllocation(allMarkets[0], CAP2 + rewards, 0)); + supplied.push(MarketAllocation(allMarkets[0], CAP2 + rewards)); vm.prank(ALLOCATOR); vm.expectRevert(ErrorsLib.InsufficientIdle.selector); diff --git a/test/hardhat/MetaMorpho.spec.ts b/test/hardhat/MetaMorpho.spec.ts index d9ac068b..8953a544 100644 --- a/test/hardhat/MetaMorpho.spec.ts +++ b/test/hardhat/MetaMorpho.spec.ts @@ -250,29 +250,25 @@ describe("MetaMorpho", () => { const market = await expectedMarket(marketParams); const position = await morpho.position(identifier(marketParams), metaMorphoAddress); - const liquidity = market.totalSupplyAssets - market.totalBorrowAssets; - const liquidityShares = liquidity.toSharesDown(market.totalSupplyAssets, market.totalSupplyShares); - return { marketParams, market, - liquidShares: position.supplyShares.min(liquidityShares), + liquidity: market.totalSupplyAssets - market.totalBorrowAssets, + supplyAssets: position.supplyShares.toAssetsDown(market.totalSupplyAssets, market.totalSupplyShares), }; }), ); const withdrawn = allocation - .map(({ marketParams, liquidShares }) => ({ + .map(({ marketParams, liquidity, supplyAssets }) => ({ marketParams, - assets: 0n, // Always withdraw all, up to the liquidity. - shares: liquidShares, + assets: liquidity > supplyAssets ? MaxUint256 : liquidity, })) - .filter(({ shares }) => shares > 0n); + .filter(({ assets }) => assets > 0n); const withdrawnAssets = allocation.reduce( - (total, { market, liquidShares }) => - total + liquidShares.toAssetsDown(market.totalSupplyAssets, market.totalSupplyShares), + (total, { supplyAssets, liquidity }) => total + supplyAssets.min(liquidity), 0n, ); @@ -284,7 +280,6 @@ describe("MetaMorpho", () => { marketParams, // Always supply evenly on each market 90% of what the vault withdrawn in total. assets: marketAssets, - shares: 0n, })) .filter(({ assets }) => assets > 0n);