berndartmueller - Protocol fees are not collected for a while after the LMPVault got emptied

[sherlock-admin2]

berndartmueller

medium

Protocol fees are not collected for a while after the LMPVault got emptied

Summary

If an existing LMPVault with deposits gets emptied, the protocol fees are not collected until the NAV per share surpasses the previous high mark or the high mark is manually reset.

Vulnerability Detail

Protocol fees are collected on the LMPVault's profits in the following three occasions:

1. Rebalancing via the LMPVault.rebalance in line 703
2. Rebalancing via the LMPVault.flashRebalance in line 727
3. Updating the debt reporting via LMPVault._updateDebtReporting in line 797

The current NAV (i.e., net asset value) per share (currentNavPerShare) is calculated and used to determine if the LMPVault made a profit and compared to the previously cached value (navPerShareHighMark). If the current NAV per share is higher than the cached value, the difference is considered as profit, and the protocol fee is calculated based on the profit. Thereafter, the current NAV value is cached as the new high mark.

However, if the destination vaults experience losses and the NAV per share decreases, the high mark will remain unchanged. If the LMPVault gets fully withdrawn and later gets deposits from users again, no protocol fees are collected as navPerShareHighMark is still the highest NAV value previously recorded. Even though the LMPVault can be considered reset (meaning that any price increases of the underlying tokens of destination vaults are considered a profit) after all funds are withdrawn, and profits are realized again, fees are not collected until the current NAV per share surpasses the previous high mark.

The following test case demonstrates the inability to collect protocol fees after the LMPVault got emptied:

Test case (click to reveal)

diff --git a/v2-core-audit-2023-07-14/test/vault/LMPVault-Withdraw.t.sol b/v2-core-audit-2023-07-14/test/vault/LMPVault-Withdraw.t.sol
index 47b238e..9cb9a98 100644
--- a/v2-core-audit-2023-07-14/test/vault/LMPVault-Withdraw.t.sol
+++ b/v2-core-audit-2023-07-14/test/vault/LMPVault-Withdraw.t.sol
@@ -2063,6 +2063,72 @@ contract LMPVaultMintingTests is Test {
         _lmpVault.updateDebtReporting(_destinations);
     }

+    function test_updateDebtReporting_FeesAreNotCollectedAfterVaultGotEmptied() public {
+        _accessController.grantRole(Roles.SOLVER_ROLE, address(this));
+        _accessController.grantRole(Roles.LMP_FEE_SETTER_ROLE, address(this));
+
+        // User is going to deposit 1000 asset
+        _asset.mint(address(this), 1000);
+        _asset.approve(address(_lmpVault), 1000);
+        _lmpVault.deposit(1000, address(this));
+
+        // At time of writing LMPVault always returned true for verifyRebalance
+        // Rebalance 1000 baseAsset for 500 underlyerOne+destVaultOne (price is 2:1)
+        _underlyerOne.mint(address(this), 250);
+        _underlyerOne.approve(address(_lmpVault), 250);
+        _lmpVault.rebalance(
+            address(_destVaultOne),
+            address(_underlyerOne), // tokenIn
+            250,
+            address(0), // destinationOut, none when sending out baseAsset
+            address(_asset), // baseAsset, tokenOut
+            500
+        );
+
+        // Setting a sink but not an actual fee yet
+        address feeSink = vm.addr(555);
+        _lmpVault.setFeeSink(feeSink);
+
+        // Dropped 1000 asset in and just did a rebalance. There's no slippage or anything
+        // atm so assets are just moved around, should still be reporting 1000 available
+        uint256 shareBal = _lmpVault.balanceOf(address(this));
+        assertEq(_lmpVault.totalDebt(), 500);
+        assertEq(_lmpVault.totalIdle(), 500);
+        assertEq(_lmpVault.convertToAssets(shareBal), 1000);
+
+        // Underlyer1 is currently worth 2 ETH a piece
+        // Lets update the price to 4 ETH and trigger a debt reporting
+        // and verify our totalDebt and asset conversions match the increase in price
+        _mockRootPrice(address(_underlyerOne), 4e18);
+        _lmpVault.updateDebtReporting(_destinations);
+
+        // No change in idle
+        assertEq(_lmpVault.totalIdle(), 500);
+        // Debt value per share went from 2 to 4 so a 100% increase
+        // Was 500 before
+        assertEq(_lmpVault.totalDebt(), 1000);
+        // So overall I can get 500 + 1000 back
+        shareBal = _lmpVault.balanceOf(address(this));
+        assertEq(_lmpVault.convertToAssets(shareBal), 1500);
+
+        assertEq(_lmpVault.navPerShareHighMark(), 15000);
+
+        _lmpVault.withdraw(1500, address(this), address(this));
+        assertEq(_asset.balanceOf(address(this)), 500 + 1500);
+        assertEq(_lmpVault.balanceOf(address(this)), 0);
+        assertEq(_lmpVault.totalSupply(), 0);
+
+        assertEq(_lmpVault.navPerShareHighMark(), 15000);
+
+        _asset.approve(address(_lmpVault), 1000);
+        _lmpVault.deposit(1000, address(this));
+
+        assertEq(_lmpVault.balanceOf(address(this)), 1000);
+        assertEq(_lmpVault.totalSupply(), 1000);
+
+        assertEq(_lmpVault.navPerShareHighMark(), 15000); // nav per share high mark is unchanged and did not get automatically reset, leading to no fees collected unless the high mark is surpassed or resetted
+    }
+
     function test_updateDebtReporting_FlashRebalanceFeesAreTakenWithoutDoubleDipping() public {
         _accessController.grantRole(Roles.SOLVER_ROLE, address(this));
         _accessController.grantRole(Roles.LMP_FEE_SETTER_ROLE, address(this));

How to run this test case:

Save git diff to a file named test.patch and run with

git apply test.patch
forge test --match-test "test_updateDebtReporting_FeesAreNotCollectedAfterVaultGotEmptied"

Result:

Running 1 test for test/vault/LMPVault-Withdraw.t.sol:LMPVaultMintingTests
[PASS] test_updateDebtReporting_FeesAreNotCollectedAfterVaultGotEmptied() (gas: 1118963)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 12.54ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

The protocol misses out on collecting fees.

Code Snippet

src/vault/LMPVault.sol#L815

800: function _collectFees(uint256 idle, uint256 debt, uint256 totalSupply) internal {
801:     address sink = feeSink;
802:     uint256 fees = 0;
803:     uint256 shares = 0;
804:     uint256 profit = 0;
805:
806:     // If there's no supply then there should be no assets and so nothing
807:     // to actually take fees on
808:     if (totalSupply == 0) {
809:         return;
810:     }
811:
812:     uint256 currentNavPerShare = ((idle + debt) * MAX_FEE_BPS) / totalSupply;
813:     uint256 effectiveNavPerShareHighMark = navPerShareHighMark;
814:
815:     if (currentNavPerShare > effectiveNavPerShareHighMark) { // @audit-info Only collect fees if the current NAV per share is higher than the high mark
816:         // Even if we aren't going to take the fee (haven't set a sink)
817:         // We still want to calculate so we can emit for off-chain analysis
818:         profit = (currentNavPerShare - effectiveNavPerShareHighMark) * totalSupply;
819:         fees = profit.mulDiv(performanceFeeBps, (MAX_FEE_BPS ** 2), Math.Rounding.Up);
820:         if (fees > 0 && sink != address(0)) {
821:             // Calculated separate from other mints as normal share mint is round down
822:             shares = _convertToShares(fees, Math.Rounding.Up);
823:             _mint(sink, shares);
824:             emit Deposit(address(this), sink, fees, shares);
825:         }
826:         // Set our new high water mark, the last nav/share height we took fees
827:         navPerShareHighMark = currentNavPerShare;
828:         navPerShareHighMarkTimestamp = block.timestamp;
829:         emit NewNavHighWatermark(currentNavPerShare, block.timestamp);
830:     }
831:     emit FeeCollected(fees, sink, shares, profit, idle, debt);
832: }

Tool used

Manual Review

Recommendation

Consider resetting the high mark navPerShareHighMark to MAX_FEE_BPS during withdrawals if the total supply is zero.

duplicate of #661

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

Trumpero commented:

invalid, fee should be collected when the NAV increase, so I don't think reseting the navPerShareHighMark when totalSupply == 0 is a good option for the vault

[berndartmueller]

Escalate

I agree with the judge's comment that fees should be collected when the NAV increases, compared to the previous NAV high mark threshold.

My point is that a vault can be considered "reset" (i.e., the vault should behave as if it is newly deployed) if the total supply is zero after users have withdrawn their funds. Any realized profits from the newly deposited funds should be considered for the protocol fee calculation.

Currently, if all funds are withdrawn, i.e., total supply is zero, and the debt and idle funds are zero, it will not reset the previous (potentially high) NAV high mark threshold (NAV is effectively the ratio between the debt + idle funds and totalSupply)

Consequently, if users start depositing funds again into the vault, the protocol will miss out on fees on realized profits as long as the previous NAV high mark threshold is not surpassed again.

[sherlock-admin2]

Escalate

I agree with the judge's comment that fees should be collected when the NAV increases, compared to the previous NAV high mark threshold.

My point is that a vault can be considered "reset" (i.e., the vault should behave as if it is newly deployed) if the total supply is zero after users have withdrawn their funds. Any realized profits from the newly deposited funds should be considered for the protocol fee calculation.

Currently, if all funds are withdrawn, i.e., total supply is zero, and the debt and idle funds are zero, it will not reset the previous (potentially high) NAV high mark threshold (NAV is effectively the ratio between the debt + idle funds and totalSupply)

Consequently, if users start depositing funds again into the vault, the protocol will miss out on fees on realized profits as long as the previous NAV high mark threshold is not surpassed again.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[xiaoming9090]

This issue is valid and similar to the one I have reported at Issue #661. Refer to my escalation (#661 (comment)) for more details.

[Evert0x]

Planning to accept escalation and duplicate with #661. Same root cause.

[Evert0x]

Result:
Medium
Duplicate of #661

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• berndartmueller: accepted