xiaoming90 - Malicious users could lock in the NAV/Share of the DV to cause the loss of fees

[sherlock-admin2]

xiaoming90

high

Malicious users could lock in the NAV/Share of the DV to cause the loss of fees

Summary

Malicious users could lock in the NAV/Share of the destination vaults, resulting in a loss of fees.

Vulnerability Detail

The _collectFees function only collects fees whenever the NAV/Share exceeds the last NAV/Share.

During initialization, the navPerShareHighMark is set to 1, effectively 1 ETH per share (1:1 ratio). Assume the following:

• It is at the early stage, and only a few shares (0.5 shares) were minted in the LMPVault
• There is a sudden increase in the price of an LP token in a certain DV (Temporarily)
• performanceFeeBps is 10%

In this case, the debt value of DV's shares will increase, which will cause LMPVault's debt to increase. This event caused the currentNavPerShare to increase to 1.4 temporarily.

Someone calls the permissionless updateDebtReporting function. Thus, the profit will be 0.4 ETH * 0.5 Shares = 0.2 ETH, which is small due to the number of shares (0.5 shares) in the LMPVault at this point. The fee will be 0.02 ETH (~40 USD). Thus, the fee earned is very little and almost negligible.

At the end of the function, the navPerShareHighMark will be set to 1.4, and the highest NAV/Share will be locked forever. After some time, the price of the LP tokens fell back to its expected price range, and the currentNavPerShare fell to around 1.05. No fee will be collected from this point onwards unless the NAV/Share is raised above 1.4.

It might take a long time to reach the 1.4 threshold, or in the worst case, the spike is temporary, and it will never reach 1.4 again. So, when the NAV/Share of the LMPVault is 1.0 to 1.4, the protocol only collects 0.02 ETH (~40 USD), which is too little.

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

function _collectFees(uint256 idle, uint256 debt, uint256 totalSupply) internal {
    address sink = feeSink;
    uint256 fees = 0;
    uint256 shares = 0;
    uint256 profit = 0;

    // If there's no supply then there should be no assets and so nothing
    // to actually take fees on
    if (totalSupply == 0) {
        return;
    }

    uint256 currentNavPerShare = ((idle + debt) * MAX_FEE_BPS) / totalSupply;
    uint256 effectiveNavPerShareHighMark = navPerShareHighMark;

    if (currentNavPerShare > effectiveNavPerShareHighMark) {
        // Even if we aren't going to take the fee (haven't set a sink)
        // We still want to calculate so we can emit for off-chain analysis
        profit = (currentNavPerShare - effectiveNavPerShareHighMark) * totalSupply;
        fees = profit.mulDiv(performanceFeeBps, (MAX_FEE_BPS ** 2), Math.Rounding.Up);
        if (fees > 0 && sink != address(0)) {
            // Calculated separate from other mints as normal share mint is round down
            shares = _convertToShares(fees, Math.Rounding.Up);
            _mint(sink, shares);
            emit Deposit(address(this), sink, fees, shares);
        }
        // Set our new high water mark, the last nav/share height we took fees
        navPerShareHighMark = currentNavPerShare;
        navPerShareHighMarkTimestamp = block.timestamp;
        emit NewNavHighWatermark(currentNavPerShare, block.timestamp);
    }
    emit FeeCollected(fees, sink, shares, profit, idle, debt);
}

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#L800

Tool used

Manual Review

Recommendation

Consider implementing a sophisticated off-chain algorithm to determine the right time to lock the navPerShareHighMark and/or restrict the access to the updateDebtReporting function to only protocol-owned addresses.

[sherlock-admin2]

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

Trumpero commented:

low, collecting fee based on NAV is protocol design, so I think it's fine when the NAV is too large and make the fee can't be accured for the protocol, it's a risk from protocol's choice.
Furthermore, in theory more deposits is equivalent with more rewards, so it's likely when the protocol grows bigger, the NAV should increase and the fee can be active again.

[xiaoming9090]

Escalate

First of all, a risk from the protocol's choice does not mean that the issue is invalid/low. Any risk arising from the protocol's design/architecture and its implementation should be highlighted during an audit.

I have discussed this with the protocol team during the audit period as shown below, and the impact of this issue is undesirable.

Using the example in my report, the period where it takes for the NAV/Share of the LMPVault to increase from 1.0 to 1.4 after the attack, the protocol only collects 0.02 ETH (~40 USD), which shows that the design of the accounting and collection of fees is fundamentally flawed. The protocol team might not have been aware of this attack/issue when designing this fee collection mechanism and assumed that the NAV would progressively increase over a period of time, but did not expect this edge case.

Thus, this is a valid High issue due to a loss of fee.

[sherlock-admin2]

Escalate

First of all, a risk from the protocol's choice does not mean that the issue is invalid/low. Any risk arising from the protocol's design/architecture and its implementation should be highlighted during an audit.

I have discussed this with the protocol team during the audit period as shown below, and the impact of this issue is undesirable.

Using the example in my report, the period where it takes for the NAV/Share of the LMPVault to increase from 1.0 to 1.4 after the attack, the protocol only collects 0.02 ETH (~40 USD), which shows that the design of the accounting and collection of fees is fundamentally flawed. The protocol team might not have been aware of this attack/issue when designing this fee collection mechanism and assumed that the NAV would progressively increase over a period of time, but did not expect this edge case.

Thus, this is a valid High issue due to a loss of fee.

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 Could you please share your thoughts about this issue? Do you think it is a real issue of the current protocol's fee model which causes a loss of fee, or is it simply another choice of the fee model?

[codenutt]

@codenutt Could you please share your thoughts about this issue? Do you think it is a real issue of the current protocol's fee model which causes a loss of fee, or is it simply another choice of the fee model?

There are a lot of ways this issue can creep up. It could be something nefarious. It could just be a temporary price spike that would occur normally even if updateDebtReporting was permissioned. We have to do debt reporting fairly frequenting or stale data starts affecting the strategy. However it happens though, we do generally recognize it as an issue and a change we have planned (something we were still solidifying going into the audit). We'll be implementing some decay logic around the high water mark so if we don't see a rise after say 90 days it starts to lower.

[Evert0x]

Planning to accept escalation and make issue medium as the issue rests on some assumptions, it's also unclear how significant the potential losses exactly are.

[Evert0x]

Result:
Medium
Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• xiaoming9090: accepted

[Evert0x]

Update #469 is a duplicate