Harvest Strategy - Base - Harvest Finance

In multiple strategies, when converting reward tokens back to the underlying asset, the code commonly passes a minimum output of 1:

IUniversalLiquidator(...).swap(rewardToken, underlying, balance, 1, address(this));

This does not protect against front-running or sandwich attacks. If a large slippage event occurs, the strategy might receive far fewer tokens than expected, negatively impacting user yields.

In the RewardForwarder contract, when calling IUniversalLiquidator(liquidator).swap(...), the code sets amountOutMin = 1. This effectively means “accept any non-zero swap output,” which can lead to highly unfavorable slippage if the market shifts or if the liquidator is front-run. Attackers could exploit this to cause near-zero returns on token swaps:

    function _notifyFee(
        address _token,
        uint256 _profitSharingFee,
        uint256 _strategistFee,
        uint256 _platformFee
    ) internal {
        address _controller = controller();
        address liquidator = IController(_controller).universalLiquidator();

        uint totalTransferAmount = _profitSharingFee.add(_strategistFee).add(_platformFee);
        require(totalTransferAmount > 0, "totalTransferAmount should not be 0");
        
        IERC20(_token).safeTransferFrom(msg.sender, address(this), totalTransferAmount);

        address _targetToken = IController(_controller).targetToken();

        if (_token != _targetToken) {
            IERC20(_token).safeApprove(liquidator, 0);
            IERC20(_token).safeApprove(liquidator, _platformFee);

            uint amountOutMin = 1;

            if (_platformFee > 0) {
                IUniversalLiquidator(liquidator).swap(
                    _token,
                    _targetToken,
                    _platformFee,
                    amountOutMin,
                    IController(_controller).protocolFeeReceiver()
                );
            }
        } else {
            IERC20(_targetToken).safeTransfer(IController(_controller).protocolFeeReceiver(), _platformFee);
        }

        if (_token != iFARM) {
            IERC20(_token).safeApprove(liquidator, 0);
            IERC20(_token).safeApprove(liquidator, _profitSharingFee.add(_strategistFee));

            uint amountOutMin = 1;

            if (_profitSharingFee > 0) {
                IUniversalLiquidator(liquidator).swap(
                    _token,
                    iFARM,
                    _profitSharingFee,
                    amountOutMin,
                    IController(_controller).profitSharingReceiver()
                );
            }
            if (_strategistFee > 0) {
                IUniversalLiquidator(liquidator).swap(
                    _token,
                    iFARM,
                    _strategistFee,
                    amountOutMin,
                    IStrategy(msg.sender).strategist()
                );
            }
        } else {
            if (_strategistFee > 0) {
                IERC20(iFARM).safeTransfer(IStrategy(msg.sender).strategist(), _strategistFee);
            }
            IERC20(iFARM).safeTransfer(IController(_controller).profitSharingReceiver(), _profitSharingFee);
        }
    }

Within the _liquidateReward() function, the code calls the Universal Liquidator with minAmountOut set to 1:

IUniversalLiquidator(_universalLiquidator).swap(
  _rewardToken,
  _underlying,
  remainingRewardBalance,
  1,
  address(this)
);

A minAmountOut of 1 leaves no meaningful protection against large price slippage or sandwich attacks. In a hostile environment, a manipulated or malicious swap could cause the strategy to receive far fewer underlying tokens than expected, resulting in lost yield for strategy users.

Seen In:

• CompoundStrategy (in _liquidateReward())

• AerodromeStableStrategy / AerodromeVolatileStrategy (in _liquidateReward())

• ExtraFiLendStrategy (in _liquidateRewards())

• FluidLendStrategy (in _liquidateRewards())

• MoonwellFoldStrategyV2 (in _liquidateRewards())

To solve issues related to missing slippage checks, it is recommended to:

• Replace amountOutMin = 1 with a slippage-aware calculation. For example, pass in a minimum expected output from off-chain price checks or keep track of a tolerance limit.

• If dynamic slippage is not practical, at least allow the caller (strategy or governance) to specify a suitable amountOutMin to protect against front-running or illiquid markets.

In the PotPool contract, pushAllRewards(address recipient) calls updateRewards(recipient) and transfers rewards to recipient. Unlike getAllRewards or withdraw, pushAllRewards does not have the nonReentrant or defense modifiers. Although it is restricted to governance only, if recipient is a malicious contract, it could potentially re-enter via token callbacks (depending on the token used):

    function pushAllRewards(address recipient) external updateRewards(recipient) onlyGovernance {
      bool rewardPayout = (!smartContractStakers[recipient] || !IController(controller()).greyList(recipient));
      for(uint256 i = 0 ; i < rewardTokens.length; i++ ){
        uint256 reward = earned(rewardTokens[i], recipient);
        if (reward > 0) {
            rewardsForToken[rewardTokens[i]][recipient] = 0;
            if (rewardPayout) {
                IERC20(rewardTokens[i]).safeTransfer(recipient, reward);
                emit RewardPaid(recipient, rewardTokens[i], reward);
            } else {
                emit RewardDenied(recipient, rewardTokens[i], reward);
            }
        }
      }

Moving forward, most strategies do not implement a nonReentrant guard. Although many DeFi protocols do not expose arbitrary callbacks, if a reward token or external lending/pool contract allowed reentrant calls, the strategy’s state-changing logic could be re-invoked unexpectedly.

Seen in:

• CompoundStrategy (internal calls to IComet, no nonReentrant)

• Aerodrome*Strategy (calls IGauge, IPool, no nonReentrant)

• ExtraFiLendStrategy, FluidLendStrategy (calls external vaults)

• MoonwellFoldStrategyV2 (flash loan from Balancer bVault, no standard reentrancy guard)

In order to solve this issue, it is recommended to:

• If liquidator is trusted and never calls back into RewardForwarder, re-entrancy is likely not an issue. However, if future changes might allow untrusted or user-supplied liquidators, consider adding a nonReentrant modifier (from OpenZeppelin’s ReentrancyGuard) in all functions across the code-base performing external calls to potentially untrusted contracts.

• Keep documentation indicating that only a trusted liquidator is intended.

If external protocols are trusted and have no callbacks, the risk is low. Otherwise, consider adding ReentrancyGuard to state-changing functions.

Keep track of potential tokens or new protocol features that could introduce callback functions in the future.

The VaultV1 contract, which inherits or acts as an upgradeable parent contract, does not reserve a storage gap at the end of its contract variables. In the context of upgradeable contracts, a storage gap is critical to preventing storage layout conflicts when future state variables are added in subsequent contract versions. Without a storage gap, any future modifications to the contract’s storage layout could overwrite existing state variables, leading to unpredictable behavior or loss of state.

In order to resolve this issue, it is recommended to:

• Add a reserved storage gap array (e.g., uint256[50] private __gap;) at the end of the contract to preserve the current storage layout.

• Ensure all future variables are added above this gap array, maintaining the existing layout for previously deployed contract instances.

Several contracts utilize the older Solidity 0.6.x compiler version, which lacks the newest language features, improvements in error handling, and security enhancements introduced in later releases. Continuing to use a legacy compiler version may expose these contracts to known bugs and limit compatibility with modern tooling and best practices.

To solve this issue, it is recommended to:

• Migrate the codebase to a more recent Solidity version (e.g., 0.8.x), after confirming no breaking changes in dependencies.

• Leverage updated compiler checks, safe math built-ins (unchecked blocks in Solidity 0.8+), and other improvements to enhance the contracts’ reliability and maintainability.

Several strategies do not emit events when critical parameters change (e.g., changing gauges, adding reward tokens, toggling folding, or pausing investments). This hinders off-chain analytics and transparency.

Seen In:

• CompoundStrategy (no events for certain changes like market() updates)

• AerodromeStable/VolatileStrategy (no events for setGauge, addRewardToken, etc.)

• ExtraFiLendStrategy (no events for adjusting fees or market updates)

• FluidLendStrategy (no events for addRewardToken, emergency exit, etc.)

• MoonwellFoldStrategyV2 (no events on factor updates, toggling folding)

To solve this issue, it is recommended to:

• Emit events (e.g., GaugeChanged, RewardTokenAdded, FoldingToggled, etc.) with relevant parameters (old/new values).

• Include indexed parameters to facilitate off-chain querying.