Description

The strategy contract's balances() function exhibits discontinuous behavior (that is, the function value "jumps") stemming from two different sources.

The first is in the balances() function itself:

function balances() public view returns (uint256 token0Bal, uint256 token1Bal) {
    (uint256 thisBal0, uint256 thisBal1) = balancesOfThis();
    (uint256 poolBal0, uint256 poolBal1,,,,) = balancesOfPool();
    (uint256 locked0, uint256 locked1) = lockedProfit();

    uint256 total0 = thisBal0 + poolBal0 - locked0;
    uint256 total1 = thisBal1 + poolBal1 - locked1;
    uint256 unharvestedFees0 = fees0;
    uint256 unharvestedFees1 = fees1;

    // If pair is so imbalanced that we no longer have any enough tokens to pay fees, we set them to 0.
    if (unharvestedFees0 > total0) unharvestedFees0 = 0;
    if (unharvestedFees1 > total1) unharvestedFees1 = 0;

    // For token0 and token1 we return balance of this contract + balance of positions - locked profit - feesUnharvested.
    return (total0 - unharvestedFees0, total1 - unharvestedFees1);
}

The crucial lines here are the two conditional adjustments to unharvestedFees0 and unharvestedFees1.

if (unharvestedFees0 > total0) unharvestedFees0 = 0;
if (unharvestedFees1 > total1) unharvestedFees1 = 0;

We will drop the 0 and 1 suffix here, as the two cases are completely analogous. Then the balance returned is of the form

If, say, $fees$ stayed fixed and a large number, and $total$ approaches $fees$ from below, then the value of $balance$ will also approach $fees$ and then suddenly drop to $0$ when $total=fees$. The following graph of this function depicts this discontinuity as well.

Such discontinuities are dangerous as an attacker might engineer a situation that is very close to the discontinuity and is then able to make the balance jump by only using a comparatively small amount of funds, for example a single Wei.

In this particular case, an attacker can also indeed engineer such a situation. If we start out in the case where fees0 < total0, as will usually be the case, then the attacker can buy token0 for token1 in the Uniswap pool. Doing this will not change fees0, and will decrease total0. Eventually, the attacker would have converted the strategy's entire position to token1, so that total0 = 0, but we would still have fees0 > 0. As total0 acts sufficiently continuous, there is thus a point in between at which the attacker can stop, not having converted quite all of the strategy's token0 to token1, at which total0 is just under fees0 (e.g., just one Wei smaller).

By stopping at such a spot, the attacker is now in a situation in which, by trading a miniscule amount of tokens on the Uniswap pool, they can repeatedly move back and forth between the discontinuity. Other ways to influence fees0 or total0 can also cause a jump across the discontinuity, for example by sending the strategy contract a single Wei of token0.

The most dangerous entry point to profit from such an attack would be in the vault's deposit function. To calculate how much the caller contributed, and thus how many shares they should get, that function compares the difference in balances() from before to after the caller's deposit:

(uint256 _bal0, uint256 _bal1) = balances();
if (_amount0 > 0) IERC20Upgradeable(token0).safeTransferFrom(msg.sender, address(strategy), _amount0);
if (_amount1 > 0) IERC20Upgradeable(token1).safeTransferFrom(msg.sender, address(strategy), _amount1);
(uint256 _after0, uint256 _after1) = balances();
strategy.deposit();

_amount0 = _after0 - _bal0;
_amount1 = _after1 - _bal1;

An attacker could now call deposit in a situation in which adding a miniscule mount of token0 will cause the discontinuous jump in balances(), which would cause _after0 to differ much more from _bal0 than the miniscule amount of tokens used for the call. If the discontinuity were such that the value of balances() could be caused to jump up by increasing total0 across the discontinuity, then this would mean the attacker would have to pay essentially nothing but get a decent amount of shares. Repeating this again and again, trading back across the discontinuity on the Uniswap pool in between calls to deposit, the attacker could gain a large number of shares and then cash out at the end for a large profit. However, crossing the discontinuity by increasing total0 causes a jump down. This means that the caller to deposit would get less shares than they should get, so an attacker cannot profit from this directly.

With the amounts properly arranged, _after0 could be made to be smaller than _bal0, however. This would cause the line _amount0 = _after0 - _bal0; to revert. An attacker might be able to front-run a deposit to engineer a value of fees0 and a composition of the strategy's position that will cause such a revert. Regarding the contracts that are in scope themselves, this is only a griefing risk, but it cannot be ruled out that an attacker could profit from this in more complex interactions with other (third-party) contracts (as in, being able to make another contract's call to deposit revert opens up an attack vector on that other contract).

Furthermore, an attacker might be able to front-run a deposit transaction (to simplify, let us say one that only deposits token0) with a sandwich attack as follows to reduce the amount of shares the caller receives to _minShares. The transactions executed will be the following:

1. The attacker buys enough token0 for token1 so that total0 and fees0 are arranged in such a way that the following deposit will cross the discontinuity, with values such that the deposit will not revert but so that _amount0 = _after0 - _bal0 will be small.

2. The original transaction. The value of _amount0 = _after0 - _bal0 will be smaller than the amount of tokens paid by the caller due to the preceding transaction's manipulation.

3. The attacker sells their token0 for token1 to roll back their Uniswap pool manipulation.

Note that while an attacker can cause _amount0 = _after0 - _bal0 to be miniscule, the deposit would then usually revert due to the number of shares minted being less than _minShares. If the caller were to set _minShares=0, then this attack could be used by an attacker that owns shares in the vault themselves in order to make a deposit act as a donation, from which they profit due to their shares being worth more afterwards.

There is a second discontinuity that is though more difficult to control by an attacker. The lockedProfit function contains a similar discontinuity. As lockedProfit is used to calculate total0 in balances(), this discontinuity impacts balances() again.

function lockedProfit() public override view returns (uint256 locked0, uint256 locked1) {
    (uint256 balThis0, uint256 balThis1) = balancesOfThis();
    (uint256 balPool0, uint256 balPool1,,,,) = balancesOfPool();
    uint256 totalBal0 = balThis0 + balPool0;
    uint256 totalBal1 = balThis1 + balPool1;

    uint256 elapsed = block.timestamp - lastHarvest;
    uint256 remaining = elapsed < DURATION ? DURATION - elapsed : 0;

    // Make sure we don't lock more than we have.
    if (totalBal0 > totalLocked0) locked0 = totalLocked0 * remaining / DURATION;
    else locked0 = 0;

    if (totalBal1 > totalLocked1) locked1 = totalLocked1 * remaining / DURATION; 
    else locked1 = 0;
}

Note that here totalBal0 increasing across the discontinuity causes locked0 to jump up, but as the value of lockedProfits() is subtracted in balances(), the resulting discontinuity in balances() still jumps down, as the one previously discussed.

Impact

Discontinuous behavior for balances in this kind of context can be very dangerous in general. In this case, the jump appears to be in the safer direction, and while there are a number of scenarios where an attacker might cause unintended behavior, we did not find a likely exploit in which the attacker can easily profit. However, more complicated attacks, or attacks involving other contracts that make use of and interact with the StrategyPassiveManagerUniswap or BeefyVaultConcLiq, cannot be ruled out. Changes to the vault contract that appear locally correct, assuming that balances() is continuous, could also potentially introduce serious vulnerabilities.

Recommendations

Consider removing the discontinuities in the two functions, for example by making the following changes. Firstly, for the balances function:

    function balances() public view returns (uint256 token0Bal, uint256 token1Bal) {
        (uint256 thisBal0, uint256 thisBal1) = balancesOfThis();
        (uint256 poolBal0, uint256 poolBal1,,,,) = balancesOfPool();
        (uint256 locked0, uint256 locked1) = lockedProfit();

        uint256 total0 = thisBal0 + poolBal0 - locked0;
        uint256 total1 = thisBal1 + poolBal1 - locked1;
        uint256 unharvestedFees0 = fees0;
        uint256 unharvestedFees1 = fees1;

        // If pair is so imbalanced that we no longer have any enough tokens to pay fees, we set them to 0.
-        if (unharvestedFees0 > total0) unharvestedFees0 = 0;
-        if (unharvestedFees1 > total1) unharvestedFees1 = 0;
+        if (unharvestedFees0 > total0) unharvestedFees0 = total0;
+        if (unharvestedFees1 > total1) unharvestedFees1 = total1;

        // For token0 and token1 we return balance of this contract + balance of positions - locked profit - feesUnharvested.
        return (total0 - unharvestedFees0, total1 - unharvestedFees1);
    }

Here is a graph showing how the first return value of this changed function depends on total0 and fees0, showing visually that the function is now continuous.

Secondly, the lockedProfit function:

    function lockedProfit() public override view returns (uint256 locked0, uint256 locked1) {
        (uint256 balThis0, uint256 balThis1) = balancesOfThis();
        (uint256 balPool0, uint256 balPool1,,,,) = balancesOfPool();
        uint256 totalBal0 = balThis0 + balPool0;
        uint256 totalBal1 = balThis1 + balPool1;

        uint256 elapsed = block.timestamp - lastHarvest;
        uint256 remaining = elapsed < DURATION ? DURATION - elapsed : 0;

        // Make sure we don't lock more than we have.
        if (totalBal0 > totalLocked0) locked0 = totalLocked0 * remaining / DURATION;
-        else locked0 = 0;
+        else locked0 = totalBal0  * remaining / DURATION;

        if (totalBal1 > totalLocked1) locked1 = totalLocked1 * remaining / DURATION; 
-        else locked1 = 0;
+        else locked1 = totalBal1  * remaining / DURATION;
    }

In this case it may also make more semantic sense sense to also change the condition slightly to totalBal0 > totalLocked0 * remaining / DURATION instead of totalBal0 > totalLocked0 and analogously for the second if.

Remediation

This issue has been acknowledged by Beefy, and a fix was implemented in commit b949cf21↗.