xiaoming90 - navPerShareHighMark not reset to 1.0

[sherlock-admin2]

xiaoming90

high

navPerShareHighMark not reset to 1.0

Summary

The navPerShareHighMark was not reset to 1.0 when a vault had been fully exited, leading to a loss of fee.

Vulnerability Detail

The LMPVault will only collect fees if the current NAV (currentNavPerShare) is more than the last NAV (effectiveNavPerShareHighMark).

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L800

File: LMPVault.sol
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) {
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;

Assume the current LMPVault state is as follows:

• totalAssets = 15 WETH
• totalSupply = 10 shares
• NAV/share = 1.5
• effectiveNavPerShareHighMark = 1.5

Alice owned all the remaining shares in the vault, and she decided to withdraw all her 10 shares. As a result, the totalAssets and totalSupply become zero. It was found that when all the shares have been exited, the effectiveNavPerShareHighMark is not automatically reset to 1.0.

Assume that at some point later, other users started to deposit into the LMPVault, and the vault invests the deposited WETH to profitable destination vaults, resulting in the real/actual NAV rising from 1.0 to 1.49 over a period of time.

The system is designed to collect fees when there is a rise in NAV due to profitable investment from sound rebalancing strategies. However, since the effectiveNavPerShareHighMark has been set to 1.5 previously, no fee is collected when the NAV rises from 1.0 to 1.49, resulting in a loss of fee.

Impact

Loss of fee. Fee collection is an integral part of the protocol; thus the loss of fee is considered a High issue.

Code Snippet

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L448

Tool used

Manual Review

Recommendation

Consider resetting the navPerShareHighMark to 1.0 whenever a vault has been fully exited.

function _withdraw(
    uint256 assets,
    uint256 shares,
    address receiver,
    address owner
) internal virtual returns (uint256) {
..SNIP..
    _burn(owner, shares);
    
+	if (totalSupply() == 0) navPerShareHighMark = MAX_FEE_BPS;

    emit Withdraw(msg.sender, receiver, owner, returnedAssets, shares);

    _baseAsset.safeTransfer(receiver, returnedAssets);

    return returnedAssets;
}

[sherlock-admin2]

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

Trumpero commented:

invalid, the fee is only collected when navPerShareHighMark increases. In the example, it should collect fee only at the first rising of NAV from 1.0 to 1.5; the second rising (even after withdrawing all) isn't counted.

[xiaoming9090]

Escalate

Please double-check with the protocol team. The protocol should always collect a fee when a profit is earned from the vaults. The fact that it only collects fees during the first rise of NAV, but not the second rise of NAV is a flaw in the design and leads to loss of protocol fee. Thus, this is a valid High issue.

[sherlock-admin2]

Escalate

Please double-check with the protocol team. The protocol should always collect a fee when a profit is earned from the vaults. The fact that it only collects fees during the first rise of NAV, but not the second rise of NAV is a flaw in the design and leads to loss of protocol fee. Thus, this is a valid High issue.

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.

[Trumpero]

@codenutt, I believe this issue revolves around the protocol's intention regarding how to handle the protocol fee. Given that @xiaoming9090 and I hold differing viewpoints on this intention, I would greatly appreciate hearing your thoughts on the matter.

[codenutt]

Agree with @xiaoming9090 here. We should be resetting the high water mark. Do believe it should only be a Medium, though. We've actually already fixed it as it was brought up as a Low issue in our parallel Halborn audit as well. If there was a complete outflow of funds and the high water mark was high enough that we wouldn't see us over coming it any time soon, there'd be no reason we wouldn't just shut down the vault and spin up a new one.

[xiaoming9090]

@codenutt Thanks for your response!

@hrishibhat @Trumpero Fee collection is an integral part of the protocol. Based on Sherlock's judging criteria and historical decisions, loss of protocol fee revenue is considered a Medium and above.

[Evert0x]

Planning to accept escalation and make issue medium.

If @xiaoming9090 can provide a strong argument for high, I will consider assigning high severity. But it's unclear if the losses are material.

[Evert0x]

Result:
Medium
Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• xiaoming9090: accepted