Beefy Hedera Contracts - Bonzo Finance

The SaucerSwapCLMLib::getPoolPrice() function operates as shown below:

function getPoolPrice(address pool) external view returns (uint256 _price) {
    (uint160 sqrtPriceX96, , , , , , ) = IUniswapV3Pool(pool).slot0();
    uint256 scaledPrice = FullMath.mulDiv(uint256(sqrtPriceX96), SQRT_PRECISION, (2 ** 96));
    _price = FullMath.mulDiv(scaledPrice, scaledPrice, SQRT_PRECISION);
}

The handling of decimal precision in this implementation is incorrect, leading to unexpected results. For example, in the DAI/WETH pool, the function returns the amount of DAI obtainable for 1 WETH, approximately 3600e18 (note that this is for an inverted pool price; otherwise, the price would represent how much WETH one can get for 1 DAI, which is less intuitive to interpret). In the USDC/USDT pool, the function returns a value close to 1e18, reflecting the near 1:1 exchange rate between these stablecoins.

However, due to differences in token decimals, USDT and USDC have 6 decimals, the function scales this value by approximately 1e12.

This inconsistency indicates that the function mishandles token decimals, resulting in one pool's price being returned with no additional precision, while another includes an extra 12 decimals, causing significant discrepancies and confusion.

Remove the last division by SQRT_PRECISION.

Solved: The Benzo team resolved the issue by removing the final division by SQRT_PRECISION in SaucerSwapCLMLib::getPoolPrice().

Added another solution that also handles different decimals of LP tokens case, also updated PRECISION in BonzoVaultConcLiq to 18.

The StrategyPassiveManagerSaucerSwap functions by deploying two positions within a SaucerSwap pool, which is a fork of UniswapV3.

The first position is based on the current pool price and aims to provide liquidity in both tokens. To achieve this, the code selects lower and upper ticks such that the current pool tick lies between them.

The second position aims to utilize any remaining token after adding liquidity to the main position. This is done by selecting pool ticks that are both above or below the current pool tick, resulting in single-sided liquidity provision.

The alternate position is configured in StrategyPassiveManagerSaucerSwap::_setAltTick():

if (amount0 < bal1) {
    (positionAlt.tickLower, ) = TickUtils.baseTicks(tick, width, distance);
    (, positionAlt.tickUpper) = TickUtils.baseTicks(tick, distance, distance);
}

For illustration, consider the following parameter values:

• tickSpacing = 10

• positionWidth = 10

• tick = 1000

Since the tick spacing is 10 and the position width is 10, the width parameter is calculated as 100. Ticks are obtained using the following function of Tickutils contract:

function baseTicks(
    int24 currentTick,
    int24 baseThreshold,
    int24 tickSpacing
) internal pure returns (int24 tickLower, int24 tickUpper) {
    int24 tickFloor = floor(currentTick, tickSpacing);

    tickLower = tickFloor - baseThreshold;
    tickUpper = tickFloor + baseThreshold;
}

Given that tickFloor is 1000, matching the current tick (since the tick is exactly divisible by the tick spacing), the first call uses width as the baseThreshold. Therefore, it returns tickLower as 1000 - 100 = 900. The second call uses distance as the baseThreshold, resulting in tickUpper being 1000 + 10 = 1010.

Note that the distance value corresponds to the tick spacing.

This situation creates an issue: the upper tick surpasses the current pool tick. Consequently, the position is set to provide liquidity in both tokens. However, since the pool already fully utilized one token during the main position addition, deploying the secondary position with both tokens can cause a revert because it attempts an imbalanced liquidity addition.

Use the first return value in the second baseTicks() call (the lower tick).

Solved: The Benzo team resolved the issue by using the first return value in the second baseTicks() call in StrategyPassiveManagerSaucerSwap's _setAltTick().

The function SaucerSwapCLMLib::calculateLiquidityWithPriceCheck relies on the Uniswap pool’s slot0 value to fetch the current adjustedSqrtPrice during deposits.

function calculateLiquidityWithPriceCheck(
        address pool,
        uint160 initialSqrtPrice,
        int24 tickLower,
        int24 tickUpper,
        uint256 amount0,
        uint256 amount1,
        uint256 priceDeviationTolerance
) external view returns (uint128 liquidity, uint160 adjustedSqrtPrice) {
        (adjustedSqrtPrice, , , , , , ) = IUniswapV3Pool(pool).slot0();

Function flow:

1. calculateLiquidityWithPriceCheck retrieves adjustedSqrtPrice from slot0.

2. It is called within _calculateLiquidityWithPriceCheck in StrategyPassiveManagerSaucerSwap.

3. _calculateLiquidityWithPriceCheck is then invoked by _addLiquidity().

4. _addLiquidity() is executed during the deposit() function.

The issue arises because slot0 reflects the current spot price, which can be manipulated by attackers. There is no protection against price manipulation, and no slippage checks are enforced when using this price to compute liquidity amounts.

Theoretical attack scenario:

A classical sandwich attack would normally follow this sequence:

1. A user submits a deposit() transaction.

2. An attacker or MEV bot observes this transaction in the mempool.

3. The attacker front-runs the transaction with a large swap, temporarily shifting the pool price (slot0).

4. When the user's deposit() executes, calculateLiquidityWithPriceCheck uses the manipulated slot0 price to determine liquidity placement.

5. The attacker then back-runs with an opposite swap, restoring the price and securing a profit at the expense of the user.

This attack relies entirely on front-running capabilities. On Hedera, front-running is not possible because there is no public mempool and transaction ordering is determined by consensus. This scenario is documented under a defense in depth approach: while infeasible on Hedera, if the same code were deployed on EVM-compatible chains, it could expose users to sandwich attacks.

Instead of relying on slot0 (which reflects the instantaneous spot price), use a time-weighted average price (TWAP) or an external trusted oracle for adjustedSqrtPrice. This mitigates short-term price manipulation, as TWAP/oracle-based prices cannot be skewed by a single large swap within a block.

Additionally, enforce slippage checks on add/remove liquidity operations to ensure that deposits are executed only within acceptable price bounds.

Acknowledged: The Bonzo team acknowledged this finding with no code changes, stating that:

No change since there is no front running possible on Hedera.

The stakeWithPermit function in BeefyRewardPool uses a user-provided signature to call IERC20PermitUpgradeable.permit() before staking tokens.

function stakeWithPermit(
        address _user,
        uint256 _amount,
        uint256 _deadline,
        uint8 _v,
        bytes32 _r,
        bytes32 _s
) external update(_user) {        
IERC20PermitUpgradeable(address(stakedToken)).permit(
   _user, address(this), _amount, _deadline, _v, _r, _s);
        _stake(_user, _amount);
}

ERC20 Permit uses nonces for replay protection. Once a signature is used, the nonce increments, preventing the same signature from being reused.An attacker monitoring the mempool can front-run the user’s transaction, call permit() themselves with the same signature, and increase the nonce.

This causes the victim’s pending stakeWithPermit transaction to fail, effectively blocking them from staking.

Theoretical attack scenario

Front-running is not possible on the Hedera network because there is no public mempool and transaction ordering is determined by consensus. However, this scenario could be relevant on other EVM-based chains with a public mempool. The sequence would be as follows

1. A user generates an ERC20 Permit signature (v, r, s) that allows BeefyRewardPool to spend their tokens.

2. The user then submits a stakeWithPermit transaction to the mempool with this signature.

3. The attacker sees the pending stakeWithPermit transaction.

4. They extract the permit signature (v, r, s) and transaction details.

5. Attacker front-runs with permit()Before the victim’s transaction is confirmed, the attacker broadcasts a transaction directly calling and increments the victim’s nonce.

6. When the victim’s stakeWithPermit transaction executes, the permit() call reverts because the nonce is no longer valid. As a result, the entire stakeWithPermit reverts and the victim cannot stake.

Users can still stake through the normal stake function (with prior approval), so the impact is limited to the stakeWithPermit flow.

In the BeefyRewardPool's stakeWithPermit function, check if it has the approval it needs. If not, then only submit the permit signature.

function stakeWithPermit(
    address _user,
    uint256 _amount,
    uint256 _deadline,
    uint8 _v,
    bytes32 _r,
    bytes32 _s
) external update(_user) {
    if (IERC20(stakedToken).allowance(_user, address(this)) < _amount) {
        IERC20PermitUpgradeable(address(stakedToken)).permit(
            _user, address(this), _amount, _deadline, _v, _r, _s
        );
    }

    _stake(_user, _amount);
}

Solved: The Benzo team resolved the issue by checking the allowances() of the token before calling the permit() function.

The internal YieldLoopConfigurable::_leverage() function implements the following logic:

IERC20(want).approve(lendingPool, _amount * (leverageLoops + 1));
ILendingPool(lendingPool).deposit(want, _amount, address(this), 0);

uint256 currentCollateral = _amount;
uint256 totalBorrowed = 0;
// Loop for additional leverage
for (uint256 i = 0; i < leverageLoops - 1; i++) {
    // Looping logic...
}

Considering the case where leverageLoops equals 1, the process is as follows:

• The code approves an amount of _amount * 2, effectively doubling the initial amount.

• The subsequent deposit uses only _amount of the approved funds.

• Since 0 < 1 - 1 evaluates to false, the loop does not execute.

This results in the lending pool being approved for an extra iteration that is not performed.

Modify IERC20(want).approve to set the allowance precisely to _amount when leverageLoops == 1, and to _amount * (leverageLoops + 1) only when leverageLoops > 1, in order to prevent granting unnecessary token approvals.

Acknowledged: The Benzo team acknowledged this finding, stating the following:

We have added loop+1 approval amount because borrowed amount is again deposited into Lending Pool. So, while doing second deposit there is no need of approval again.

In the BonzoSupplyStrategy::_harvest function, the StratHarvest event is emitted using msg.sender instead of tx.origin:

function harvest() external virtual  {
    _harvest(tx.origin);
}

function _harvest(address callFeeRecipient) internal whenNotPaused {
    emit StratHarvest(msg.sender, wantHarvested, balanceOf());
}

Since msg.sender refers to the vault or router contract calling harvest(), the event does not correctly represent the original user who triggered the transaction.

In BonzoSupplyStrategy::_harvest() Use tx.origin instead of msg.sender when emitting the event to correctly capture the user initiating the harvest:

emit StratHarvest(tx.origin, balanceOfToken0(), balanceOfToken1());

Solved: The Benzo team resolved the issue by adding tx.origin in the Harvest event emit.