Fix increasing performance fee from zero value

foundry.toml

@@ -27,8 +27,9 @@ runs = 1_000
 [invariant]
 runs = 256
 depth = 500
-fail_on_revert = true
 shrink_run_limit = 5_000
+show_metrics = true
+fail_on_revert = false
 
 [dependencies]
 "@openzeppelin-contracts" = "5.0.2"

src/contracts/AbstractARM.sol

@@ -689,6 +689,11 @@ abstract contract AbstractARM is OwnableOperable, ERC20Upgradeable {
         // Accrue any performance fees up to this point
         (fees, newAvailableAssets) = _feesAccrued();
 
+        // Save the new available assets back to storage less the collected fees.
+        // This needs to be done before the fees == 0 check to cover the scenario where the performance fee is zero
+        // and there has been an increase in assets since the last time fees were collected.
+        lastAvailableAssets = SafeCast.toInt128(SafeCast.toInt256(newAvailableAssets) - SafeCast.toInt256(fees));
+
         if (fees == 0) return 0;
 
         // Check there is enough liquidity assets (WETH) that are not reserved for the withdrawal queue
@@ -700,9 +705,6 @@ abstract contract AbstractARM is OwnableOperable, ERC20Upgradeable {
         // a failed WETH transfer so we spend the extra gas to check and give a meaningful error message.
         require(fees <= IERC20(liquidityAsset).balanceOf(address(this)), "ARM: insufficient liquidity");
 
-        // Save the new available assets back to storage less the collected fees.
-        lastAvailableAssets = SafeCast.toInt128(SafeCast.toInt256(newAvailableAssets) - SafeCast.toInt256(fees));
-
         IERC20(liquidityAsset).transfer(feeCollector, fees);
 
         emit FeeCollected(feeCollector, fees);

src/contracts/LidoARM.sol

@@ -26,6 +26,9 @@ contract LidoARM is Initializable, AbstractARM {
     /// @notice The amount of stETH in the Lido Withdrawal Queue
     uint256 public lidoWithdrawalQueueAmount;
 
+    /// @notice stores the requested amount for each Lido withdrawal
+    mapping(uint256 id => uint256 amount) public lidoWithdrawalRequests;
+
     event RequestLidoWithdrawals(uint256[] amounts, uint256[] requestIds);
     event ClaimLidoWithdrawals(uint256[] requestIds);
 
@@ -83,6 +86,9 @@ contract LidoARM is Initializable, AbstractARM {
         uint256 totalAmountRequested = 0;
         for (uint256 i = 0; i < amounts.length; i++) {
             totalAmountRequested += amounts[i];
+
+            // Store the amount of each withdrawal request
+            lidoWithdrawalRequests[requestIds[i]] = amounts[i];
         }
 
         // Increase the Ether outstanding from the Lido Withdrawal Queue
@@ -95,21 +101,32 @@ contract LidoARM is Initializable, AbstractARM {
      * @notice Claim the ETH owed from the redemption requests and convert it to WETH.
      * Before calling this method, caller should check on the request NFTs to ensure the withdrawal was processed.
      */
-    function claimLidoWithdrawals(uint256[] calldata requestIds) external {
-        uint256 etherBefore = address(this).balance;
-
+    function claimLidoWithdrawals(uint256[] memory requestIds) external {
         // Claim the NFTs for ETH.
         uint256 lastIndex = lidoWithdrawalQueue.getLastCheckpointIndex();
         uint256[] memory hintIds = lidoWithdrawalQueue.findCheckpointHints(requestIds, 1, lastIndex);
         lidoWithdrawalQueue.claimWithdrawals(requestIds, hintIds);
 
-        uint256 etherAfter = address(this).balance;
+        // Reduce the amount outstanding from the Lido Withdrawal Queue.
+        // The amount of ETH claimed from the Lido Withdrawal Queue can be less than the requested amount
+        // in the event of a mass slashing event of Lido validators.
+        uint256 totalAmountRequested = 0;
+        for (uint256 i = 0; i < requestIds.length; i++) {
+            // Read the requested amount from storage
+            uint256 requestAmount = lidoWithdrawalRequests[requestIds[i]];
+
+            // Validate the request came from this Lido ARM contract and not
+            // transferred in from another account.
+            require(requestAmount > 0, "LidoARM: invalid request");
+
+            totalAmountRequested += requestAmount;
+        }
 
-        // Reduce the Ether outstanding from the Lido Withdrawal Queue
-        lidoWithdrawalQueueAmount -= etherAfter - etherBefore;
+        // Store the reduced amount outstanding from the Lido Withdrawal Queue
+        lidoWithdrawalQueueAmount -= totalAmountRequested;
 
         // Wrap all the received ETH to WETH.
-        weth.deposit{value: etherAfter}();
+        weth.deposit{value: address(this).balance}();
 
         emit ClaimLidoWithdrawals(requestIds);
     }

test/fork/LidoFixedPriceMultiLpARM/Deposit.t.sol

@@ -616,4 +616,169 @@ contract Fork_Concrete_LidoARM_Deposit_Test_ is Fork_Shared_Test_ {
         if (ac) assertEq(capManager.liquidityProviderCaps(address(this)), 0, "user cap"); // All the caps are used
         assertEqQueueMetadata(receivedAssets, 0, 1);
     }
+
+    /// @notice Test the following scenario:
+    /// 1. ARM gain assets in WETH after small initial deposit
+    /// 2. User deposit liquidity
+    /// 3. Operator collects the performance fees
+    /// Check depositor hasn't lost value
+    function test_Deposit_WithAssetGain()
+        public
+        deal_(address(weth), address(lidoARM), 2 * MIN_TOTAL_SUPPLY)
+        disableCaps
+    {
+        // Assertions Before
+        uint256 expectedTotalSupplyBeforeDeposit = MIN_TOTAL_SUPPLY;
+        uint256 expectTotalAssetsBeforeDeposit = MIN_TOTAL_SUPPLY + (MIN_TOTAL_SUPPLY * 80 / 100);
+        uint256 assetsPerShareBefore = expectTotalAssetsBeforeDeposit * 1e18 / expectedTotalSupplyBeforeDeposit;
+        assertEq(lidoARM.totalSupply(), expectedTotalSupplyBeforeDeposit, "total supply before deposit");
+        assertEq(lidoARM.totalAssets(), expectTotalAssetsBeforeDeposit, "total assets before deposit");
+        assertEq(lidoARM.feesAccrued(), MIN_TOTAL_SUPPLY * 20 / 100, "fees accrued before deposit");
+
+        // shares = assets * total supply / total assets
+        uint256 expectShares = DEFAULT_AMOUNT * expectedTotalSupplyBeforeDeposit / expectTotalAssetsBeforeDeposit;
+
+        // Expected events
+        vm.expectEmit({emitter: address(weth)});
+        emit IERC20.Transfer(address(this), address(lidoARM), DEFAULT_AMOUNT);
+        vm.expectEmit({emitter: address(lidoARM)});
+        emit IERC20.Transfer(address(0), address(this), expectShares);
+
+        // Main calls
+        // 2. User mint shares
+        uint256 shares = lidoARM.deposit(DEFAULT_AMOUNT);
+
+        assertEq(shares, expectShares, "shares after deposit");
+        assertEq(lidoARM.totalAssets(), expectTotalAssetsBeforeDeposit + DEFAULT_AMOUNT, "total assets after deposit");
+        assertEq(lidoARM.totalSupply(), expectedTotalSupplyBeforeDeposit + shares, "total supply after deposit");
+        assertEq(lidoARM.feesAccrued(), MIN_TOTAL_SUPPLY * 20 / 100, "fees accrued after deposit");
+        assertEq(
+            lidoARM.lastAvailableAssets(),
+            int256(MIN_TOTAL_SUPPLY + DEFAULT_AMOUNT),
+            "last available assets after deposit"
+        );
+        assertGe(
+            lidoARM.totalAssets() * 1e18 / lidoARM.totalSupply(), assetsPerShareBefore, "assets per share after deposit"
+        );
+        assertGe(lidoARM.convertToAssets(shares), DEFAULT_AMOUNT - 1, "depositor has not lost value after deposit");
+
+        // 3. collect fees
+        lidoARM.collectFees();
+
+        // Assertions after collect fees
+        assertEq(lidoARM.totalSupply(), expectedTotalSupplyBeforeDeposit + shares, "total supply after collect fees");
+        assertApproxEqRel(
+            lidoARM.totalAssets(),
+            expectTotalAssetsBeforeDeposit + DEFAULT_AMOUNT,
+            1e6,
+            "total assets after collect fees"
+        );
+        assertEq(lidoARM.feesAccrued(), 0, "fees accrued after collect fees");
+        assertEq(
+            lidoARM.lastAvailableAssets(),
+            int256(expectTotalAssetsBeforeDeposit + DEFAULT_AMOUNT),
+            "last available assets after collect fees"
+        );
+        assertGe(
+            lidoARM.convertToAssets(shares), DEFAULT_AMOUNT - 1, "depositor has not lost value after collected fees"
+        );
+    }
+
+    /// @notice Test the following scenario:
+    /// 1. Alice deposits 800 WETH
+    /// 2. Set fee to zero
+    /// 3. Swap 500 stETH for WETH
+    /// 4. Bob deposits 600 WETH
+    /// 5. Owner sets fee to 33%
+    /// Check depositor hasn't lost value
+    function test_Deposit_AfterSwapWithZeroFees()
+        public
+        disableCaps
+        /// Give 500 stETH to tester for swapping
+        deal_(address(steth), address(this), 600e18)
+        /// 1. Alice deposits 800 WETH
+        depositInLidoARM(address(alice), 800e18)
+        /// 2. Set buy, cross and sell prices
+        setPrices(0.998e36, 0.999e36, 1e36)
+    {
+        // Assertions Before
+        uint256 aliceDeposit = 800e18;
+        uint256 expectedTotalSupplyBeforeSwap = MIN_TOTAL_SUPPLY + aliceDeposit;
+        uint256 expectTotalAssetsBeforeSwap = MIN_TOTAL_SUPPLY + aliceDeposit;
+        uint256 assetsPerShareBeforeSwap = expectedTotalSupplyBeforeSwap * 1e18 / expectedTotalSupplyBeforeSwap;
+        assertEq(lidoARM.totalSupply(), expectedTotalSupplyBeforeSwap, "total supply before swap");
+        assertEq(lidoARM.totalAssets(), expectTotalAssetsBeforeSwap, "total assets before swap");
+        assertEq(lidoARM.feesAccrued(), 0, "fees accrued before swap");
+
+        // Main calls
+        // 2. Owner sets the fee
+        lidoARM.setFee(0);
+
+        // 3. Swap 500 stETH for WETH
+        uint256 swapInAmount = 500e18;
+        lidoARM.swapExactTokensForTokens(
+            steth, // inToken
+            weth, // outToken
+            swapInAmount,
+            0,
+            address(this) // to
+        );
+
+        uint256 expectedTotalSupplyBeforeDeposit = expectTotalAssetsBeforeSwap;
+        uint256 expectTotalAssetsBeforeDeposit = expectTotalAssetsBeforeSwap - 1
+        // steth in discounted to the cross price
+        + ((swapInAmount * 0.999e36) / 1e36)
+        // weth out discounted by the buy price
+        - ((swapInAmount * 0.998e36) / 1e36);
+        assertEq(lidoARM.totalSupply(), expectedTotalSupplyBeforeDeposit, "total supply before deposit");
+        assertEq(lidoARM.totalAssets(), expectTotalAssetsBeforeDeposit, "total assets before deposit");
+        assertEq(lidoARM.feesAccrued(), 0, "fees accrued before swap");
+
+        /// 4. Bob deposits 600 WETH
+        uint256 bobDeposit = 600e18;
+        // shares = assets * total supply / total assets
+        uint256 expectShares = bobDeposit * expectedTotalSupplyBeforeDeposit / expectTotalAssetsBeforeDeposit;
+
+        // Expected events
+        vm.expectEmit({emitter: address(weth)});
+        emit IERC20.Transfer(address(this), address(lidoARM), bobDeposit);
+        vm.expectEmit({emitter: address(lidoARM)});
+        emit IERC20.Transfer(address(0), address(this), expectShares);
+
+        uint256 bobShares = lidoARM.deposit(bobDeposit);
+
+        assertEq(bobShares, expectShares, "shares after deposit");
+        assertEq(lidoARM.totalAssets(), expectTotalAssetsBeforeDeposit + bobDeposit, "total assets after deposit");
+        assertEq(lidoARM.totalSupply(), expectedTotalSupplyBeforeDeposit + bobShares, "total supply after deposit");
+        assertEq(lidoARM.feesAccrued(), 0, "fees accrued after deposit");
+        assertEq(
+            lidoARM.lastAvailableAssets(),
+            int256(expectTotalAssetsBeforeSwap + bobDeposit),
+            "last available assets after deposit"
+        );
+        assertGe(
+            lidoARM.totalAssets() * 1e18 / lidoARM.totalSupply(),
+            assetsPerShareBeforeSwap,
+            "assets per share after deposit"
+        );
+        assertGe(lidoARM.convertToAssets(bobShares), bobDeposit - 1, "depositor has not lost value after deposit");
+
+        // 5. Owner sets fee to 33%
+        lidoARM.setFee(3300);
+
+        // Assertions after collect fees
+        assertEq(lidoARM.totalSupply(), expectedTotalSupplyBeforeDeposit + bobShares, "total supply after collect fees");
+        assertApproxEqRel(
+            lidoARM.totalAssets(), expectTotalAssetsBeforeDeposit + bobDeposit, 1e6, "total assets after collect fees"
+        );
+        assertEq(lidoARM.feesAccrued(), 0, "fees accrued after collect fees");
+        assertEq(
+            lidoARM.lastAvailableAssets(),
+            int256(expectTotalAssetsBeforeDeposit + bobDeposit),
+            "last available assets after collect fees"
+        );
+        assertGe(
+            lidoARM.convertToAssets(bobShares), bobDeposit - 1, "depositor has not lost value after collected fees"
+        );
+    }
 }