alpha-contracts-v2.1

Cantina Security Report

Organization: @charmfinance
Engagement Type: Cantina Reviews
Period: 09 June 2025 - 12 June 2025
Researchers: Kaden
cergyk

Findings

Medium Risk

1 findings

1 fixed

0 acknowledged

Low Risk

6 findings

4 fixed

2 acknowledged

Informational

6 findings

2 fixed

4 acknowledged

Gas Optimizations

2 findings

1 fixed

1 acknowledged

Medium Risk1 finding

Unsufficient factory check during vault creation

State: Fixed
https://github.com/charmfinance/alpha-contracts-v2.1/commit/4932841d93a5b7e1cc47624768b0b768dba67a65
Severity: Severity: Medium
≈
Likelihood: Medium
×
Impact: Medium
Submitted by: cergyk

Description

When creating a vault in AlphaProVaultFactory, it is checked that the contract has a field factory which is whitelisted:

AlphaProVaultFactory.sol?lines=46,47:

/**     * @notice Create a new Alpha Pro Vault     * @param params InitizalizeParams Underlying Uniswap V3 pool address     */    function createVault(VaultParams calldata params) external returns (address vaultAddress) {        address poolFactory = IUniswapV3Pool(params.pool).factory();        require(allowedFactories[poolFactory], "allowedFactories");

Unfortunately, anybody can deploy a contract which has a field factory which is whitelisted by governance; So this means this check is not sufficient to prove that the pool has been created by the factory and is not malicious;

Recommendation

One should also check that the factory "knows" the pool by checking that the user provided address is indeed the address for the pool with (token0, token1, fee);

AlphaProVaultFactory.sol?lines=46,47:

/**     * @notice Create a new Alpha Pro Vault     * @param params InitizalizeParams Underlying Uniswap V3 pool address     */    function createVault(VaultParams calldata params) external returns (address vaultAddress) {        IUniswapV3Pool pool = IUniswapV3Pool(params.pool);        address poolFactory = pool.factory();        require(allowedFactories[poolFactory], "allowedFactories");+       require(params.pool == IUniswapV3PoolFactory(poolFactory).getPool(pool.token0(), pool.token1(), pool.fee());

Low Risk6 findings

Fee rounding consistency in getPositionAmounts

State: Fixed
https://github.com/charmfinance/alpha-contracts-v2.1/commit/b398a433d44c864f893dd5a8819494cbaf18535f
Severity: Severity: Low
≈
Likelihood: Low
×
Impact: Low
Submitted by: cergyk

Description

To evaluate shares to mint for given deposit amounts, AlphaProVault::deposit uses a getPositionAmounts(bool roundUp) function adapted from UniswapV3 library. It enables to compute these amounts with rounding up if necessary. In the case of a roundup however, division related to the evaluation of fees is not rounded up:

AlphaProVault.sol?lines=586,590:

// Subtract protocol and manager fees    uint256 oneMinusFee = uint256(1e6) - protocolFee - managerFee;
    //@audit Amount left after fee is rounded down here    amount0 = amount0 + ((uint256(tokensOwed0) * oneMinusFee) / 1e6);    amount1 = amount1 + ((uint256(tokensOwed1) * oneMinusFee) / 1e6);

As a result the amount returned here due for the vault can be rounded down overall

Since the total amount is later used to evaluate the amount of shares minted to the user, it is crucial to have it rounded up overall:

AlphaProVault.sol#L272-L283:

} else {        uint256 cross0 = amount0Desired * total1;        uint256 cross1 = amount1Desired * total0;        uint256 cross = cross0 > cross1 ? cross1 : cross0;        require(cross > 0, "cross");
        // Round up amounts        amount0 = ((cross - 1) / (total1)) + 1;        amount1 = ((cross - 1) / (total0)) + 1;        //@audit shares evaluated here need to have a rounded up total0 and total1, to be rounded down overall        shares = ((cross * totalSupply) / total0) / total1;    }

Recommendation

Use the same formula to compute pending fees as the one used in _burnAndCollect for getPositionAmounts:

(amount0, amount1) = _amountsForLiquidity(tickLower, tickUpper, liquidity, roundUp);          // Subtract protocol and manager fees-        uint256 oneMinusFee = uint256(1e6) - protocolFee - managerFee;+        uint256 _managerFee = managerFee;+        uint256 _protocolFee = protocolFee;+        uint256 protocolFee0 = (_protocolFee * tokensOwed0) / 1e6;+        uint256 protocolFee1 = (_protocolFee * tokensOwed1) / 1e6;+        uint256 managerFee0 = (_managerFee * tokensOwed0) / 1e6;+        uint256 managerFee1 = (_managerFee * tokensOwed1) / 1e6; -        amount0 = amount0 + ((uint256(tokensOwed0) * oneMinusFee) / 1e6);-        amount1 = amount1 + ((uint256(tokensOwed1) * oneMinusFee) / 1e6);+        amount0 = amount0 + (uint256(tokensOwed0) - protocolFee0 - managerFee0);+        amount1 = amount1 + (uint256(tokensOwed1) - protocolFee1 - managerFee1);     }

Protocol fee could be fetched from factory after rebalance

State: Fixed
https://github.com/charmfinance/alpha-contracts-v2.1/commit/4932841d93a5b7e1cc47624768b0b768dba67a65
Severity: Severity: Low
≈
Likelihood: Low
×
Impact: Low
Submitted by: cergyk

Description

Currently in AlphaProVault, there is a dedicated setter to modify protocol fee, whereas one is also present in AlphaProVaultFactory to set protocol fee for vaults created in the future:

AlphaProVault.sol#L684-L693:

/**     * @notice Change the protocol fee charged on pool fees earned from     * Uniswap, expressed as multiple of 1e-6. Fee is hard capped at 25%.     */    function setProtocolFee(uint24 _pendingProtocolFee) external {        require(msg.sender == factory.governance(), "governance");        require(_pendingProtocolFee <= 25e4, "protocolFee must be <= 250000");        pendingProtocolFee = _pendingProtocolFee;        emit UpdateProtocolFee(_pendingProtocolFee);    }

AlphaProVaultFactory.sol?lines=64,72:

/**     * @notice Change the protocol fee charged on pool fees earned from     * Uniswap, expressed as multiple of 1e-6. Fee is hard capped at 25%.     */    function setProtocolFee(uint24 _protocolFee) external onlyGovernance {        require(_protocolFee <= 25e4, "protocolFee must be <= 250000");        protocolFee = _protocolFee;        emit UpdateProtocolFee(_protocolFee);    }

A more practical approach would be to pull the protocol fee from factory right after rebalance, as this would avoid the need to call on every vault created historically to update protocol fee.

Recommendation

Fetch protocol fee from factory at the end of rebalance, and remove dedicated setter and pendingProtocolFee:

AlphaProVault.sol#L433-L441:

// Update fee only at each rebalance, so that if fee is increased    // it won't be applied retroactively to current open positions-   uint24 _protocolFee = protocolFee = pendingProtocolFee;+   uint24 _protocolFee = protocolFee = factory.protocolFee();    // Manager + protocol fee must be <= 100%    if (pendingManagerFee + _protocolFee <= HUNDRED_PERCENT) {        managerFee = pendingManagerFee;    } else {        managerFee = HUNDRED_PERCENT - _protocolFee;    }

Manager can allow himself to rebalance unrestricted

State: Acknowledged
Severity: Severity: Low
≈
Likelihood: Medium
×
Impact: Low
Submitted by: cergyk

Description

There are checks implemented in checkCanRebalance to rate limit the number of rebalances that are made, to avoid slow leakage of funds through successive rebalancings;

AlphaProVault.sol?lines=444,463:

function checkCanRebalance() public view override {        checkPriceNearTwap();        uint256 _lastTimestamp = lastTimestamp;
        // check enough time has passed        require(block.timestamp >= (_lastTimestamp + period), "PE");
        // check price has moved enough        (, int24 tick,,,,,) = pool.slot0();        int24 tickMove = tick > lastTick ? tick - lastTick : lastTick - tick;        require(_lastTimestamp == 0 || tickMove >= minTickMove, "TM");
        // check price not too close to boundary        int24 maxThreshold = baseThreshold > limitThreshold ? baseThreshold : limitThreshold;        require(            tick >= TickMath.MIN_TICK + maxThreshold + tickSpacing                && tick <= TickMath.MAX_TICK - maxThreshold - tickSpacing,            "PB"        );    }

Unfortunately all of these checks can be lifted by the manager by resetting the values:

AlphaProVault.sol?lines=740,761:

function setPeriod(uint32 _period) external onlyManager {        period = _period;        emit UpdatePeriod(_period);    }
    function setMinTickMove(int24 _minTickMove) external onlyManager {        require(_minTickMove >= 0, "minTickMove must be >= 0");        minTickMove = _minTickMove;        emit UpdateMinTickMove(_minTickMove);    }
    function setMaxTwapDeviation(int24 _maxTwapDeviation) external onlyManager {        require(_maxTwapDeviation >= 0, "maxTwapDeviation must be >= 0");        maxTwapDeviation = _maxTwapDeviation;        emit UpdateMaxTwapDeviation(_maxTwapDeviation);    }
    function setTwapDuration(uint32 _twapDuration) external onlyManager {        require(_twapDuration > 0, "twapDuration must be > 0");        twapDuration = _twapDuration;        emit UpdateTwapDuration(_twapDuration);    }

Recommendation

Multiple mitigations are possible:

Implement a delay for setting these important values
Implement a minimum non-zero delay between rebalances

Vault can be drained if baseThreshold is equal to wideThreshold

State: Fixed
https://github.com/charmfinance/alpha-contracts-v2.1/commit/4932841d93a5b7e1cc47624768b0b768dba67a65
Severity: Severity: Low
≈
Likelihood: Low
×
Impact: High
Submitted by: cergyk

Description

An implicit assumption in AlphaProVault is that wide and base position are independent, however these can be set to be equal by the manager, causing miscalculations during vault calls. Due to some implementation specifics, the impacts on the methods deposit, rebalance and withdraw are different.

Let's analyse these impacts when adding minting 100% of current supply, and then withdrawing all of minted shares (50% of totalSupply). For simplicity we assume zero liquidity for the limit range and zero balances for tokens directly in vault, which are not impacted by this:

1/ Deposits:

Since liquidities are prefetched, and if we assume L of current liquidity in wide/base range, we will mint for 2*L:

-> Deposits are overestimated by a factor of 2x:

AlphaProVault.sol#L231-L239:

for (uint256 i = 0; i < 3; i++) {        int24 tickLower = positions[i][0];        int24 tickUpper = positions[i][1];
        if (liquidities[i] > 0) {            uint128 liquidityShare = uint128(Math.mulDiv(uint256(liquidities[i]), shares, totalSupply()));            _mintLiquidity(tickLower, tickUpper, liquidityShare);        }    }

2/ Rebalance:

Since liquidities are prefetched, and if we assume L of current liquidity in wide/base range, we will attempt to burn for 2*L:

-> Rebalances will fail when L != 0

AlphaProVault.sol#L361-L371:

// Withdraw all current liquidity from Uniswap pool    {        int24 _wideLower = wideLower;        int24 _wideUpper = wideUpper;        (uint128 wideLiquidity,,,,) = _position(_wideLower, _wideUpper);        (uint128 baseLiquidity,,,,) = _position(baseLower, baseUpper);        (uint128 limitLiquidity,,,,) = _position(limitLower, limitUpper);        _burnAndCollect(_wideLower, _wideUpper, wideLiquidity);        _burnAndCollect(baseLower, baseUpper, baseLiquidity);        _burnAndCollect(limitLower, limitUpper, limitLiquidity);    }

Please note that the manager can always call emergencyBurn to empty vault positions and enable the call to rebalance

During mint however, the vault will only attempt to deposit all of the available tokens, so deposited amounts would be unchanged here.

3/ Withdraw

Liquidities are not prefetched. This means that when trying to withdraw 50% of totalSupply():

we will first withdraw 50% of the sum wide/base when burn/collecting from wide range
then again 50% of the remainder when burn/collecting from base range.

This means we are withdrawing 75% of the liquidity in the wide/base range instead of 50%.

AlphaProVault.sol#L311-L314:

// Withdraw proportion of liquidity from Uniswap pool    //@audit if shares/totalSupply = 50%, we withdraw 50% of position liq here    (uint256 wideAmount0, uint256 wideAmount1) = _burnLiquidityShare(wideLower, wideUpper, shares, totalSupply);    //@audit if shares/totalSupply = 50%, we withdraw 50% of remainder of position liq here, so 25% of initial position liq    (uint256 baseAmount0, uint256 baseAmount1) = _burnLiquidityShare(baseLower, baseUpper, shares, totalSupply);    (uint256 limitAmount0, uint256 limitAmount1) = _burnLiquidityShare(limitLower, limitUpper, shares, totalSupply);

Impact Analysis

Overall we notice that in this situation where baseThreshold == wideThreshold, making a deposit for 100% of current shares and then withdrawing it all is profitable, enabling the drainage of the vault.

Recommendation

Setting baseThreshold == wideThreshold should be protected against in the setters and during vault creation:

AlphaProVault.sol#L716-L738:

function setBaseThreshold(int24 _baseThreshold) external onlyManager {        _checkThreshold(_baseThreshold, tickSpacing);+       require(wideThreshold != _baseThreshold);        baseThreshold = _baseThreshold;        emit UpdateBaseThreshold(_baseThreshold);    }    function setLimitThreshold(int24 _limitThreshold) external onlyManager {        _checkThreshold(_limitThreshold, tickSpacing);        limitThreshold = _limitThreshold;        emit UpdateLimitThreshold(_limitThreshold);    }    function setWideRangeWeight(uint24 _wideRangeWeight) external onlyManager {        require(_wideRangeWeight <= 1e6, "wideRangeWeight must be <= 1e6");        wideRangeWeight = _wideRangeWeight;        emit UpdateWideRangeWeight(_wideRangeWeight);    }    function setWideThreshold(int24 _wideThreshold) external onlyManager {        _checkThreshold(_wideThreshold, tickSpacing);+       require(baseThreshold != _wideThreshold);        wideThreshold = _wideThreshold;        emit UpdateWideThreshold(_wideThreshold);    }

AlphaProVault.sol#L165-L175:

factory = AlphaProVaultFactory(_factory);    pendingProtocolFee = factory.protocolFee();    _checkThreshold(_params.baseThreshold, _tickSpacing);    _checkThreshold(_params.limitThreshold, _tickSpacing);    _checkThreshold(_params.wideThreshold, _tickSpacing);    require(_params.wideRangeWeight <= 1e6, "wideRangeWeight must be <= 1e6");    require(_params.minTickMove >= 0, "minTickMove must be >= 0");    require(_params.maxTwapDeviation >= 0, "maxTwapDeviation must be >= 0");    require(_params.twapDuration > 0, "twapDuration must be > 0");    require(_params.managerFee <= HUNDRED_PERCENT, "managerFee must be <= 1000000");+   require(_params.baseThreshold != _params.wideThreshold, "baseThreshold cannot be equal to wideThreshold");

Rounding error during direct deposit may Dos deposits

State: Fixed
https://github.com/charmfinance/alpha-contracts-v2.1/commit/4932841d93a5b7e1cc47624768b0b768dba67a65
Severity: Severity: Low
Submitted by: cergyk

Description

To deposit on AlphaProVault, users provide amount0Desired and amount1Desired of tokens which is the max amount of either token which will be pulled from the user. One of these amounts is guaranteed to be pulled in the general case

amount0 and amount1 pulled amount calculation in _calcAmountsAndShares:

AlphaProVault.sol#L273-L282:

uint256 cross0 = amount0Desired * total1;        uint256 cross1 = amount1Desired * total0;        uint256 cross = cross0 > cross1 ? cross1 : cross0;        require(cross > 0, "cross");
        // Round up amounts        //@audit either amount0 == amount0Desired or amount1 == amount1Desired        amount0 = ((cross - 1) / (total1)) + 1;        amount1 = ((cross - 1) / (total0)) + 1;        shares = ((cross * totalSupply) / total0) / total1;    }

However since the vault ends up minting in 3 different ranges after the deposit made by the user, the pulled amount from the vault is rounded up 3 times and may exceed the amount pulled from the user and would revert as a result:

AlphaProVault.sol#L231-L240:

for (uint256 i = 0; i < 3; i++) {        int24 tickLower = positions[i][0];        int24 tickUpper = positions[i][1];
        if (liquidities[i] > 0) {            uint128 liquidityShare = uint128(Math.mulDiv(uint256(liquidities[i]), shares, totalSupply()));            //@audit shares has been computed accordingly to amount0Desired or amount1Desired            //@audit since the amounts pulled below are rounded up 3 times, they end up exceeding amount0Desired or amount1Desired by a few wei             _mintLiquidity(tickLower, tickUpper, liquidityShare);        }    }

Recommendation

As Charm Finance team found out, it is sufficient to adapt the minting logic as following:

AlphaProVault.sol#L231-L240:

// Pull in tokens from sender    if (amount0 > 0) token0.safeTransferFrom(msg.sender, address(this), amount0);    if (amount1 > 0) token1.safeTransferFrom(msg.sender, address(this), amount1);
    uint256 _totalSupply = totalSupply();    (uint160 sqrtRatioX96,,,,,,) = pool.slot0();    for (uint256 i = 0; i < 3; i++) {        int24 tickLower = positions[i][0];        int24 tickUpper = positions[i][1];        if (liquidities[i] > 0) {            uint128 liquidityToMint = uint128(Math.mulDiv(uint256(liquidities[i]), shares, _totalSupply));            uint128 liquidityFromAmounts =                _liquidityForAmounts(tickLower, tickUpper, amount0, amount1, sqrtRatioX96);            liquidityToMint = liquidityToMint > liquidityFromAmounts ? liquidityFromAmounts : liquidityToMint;
            (uint256 mintAmount0, uint256 mintAmount1) = _mintLiquidity(tickLower, tickUpper, liquidityToMint);            amount0 -= mintAmount0;            amount1 -= mintAmount1;        }    }

The getAmountsTotal(bool roundUp) function forked from UniswapV3 library must be kept and used to evaluate shares to be minted for the user; As the shares minted to the user should be rounded down.

ERC777 tokens will enable caller to self sandwich mint to extract part of deposit

State: Acknowledged
Severity: Severity: Low
Submitted by: cergyk

Description

ERC777 is a token standard derived from ERC20 and generally compatible with UniswapV3. However the integration of AlphaProVault with UniswapV3 may enable value extraction from vault if one of the tokens of the pool is an ERC777 token;

Indeed the _callTokensToSend hook can be used by caller to swap in the underlying pool of the vault and modify the price. This causes slippage on the mint operation conducted later by the vault.

AlphaProVault.sol#L227-L240:

// Pull in tokens from sender    //@audit _callTokensToSend is called on msg.sender, which will manipulate price of underlying pool    if (amount0 > 0) token0.safeTransferFrom(msg.sender, address(this), amount0);    if (amount1 > 0) token1.safeTransferFrom(msg.sender, address(this), amount1);
    for (uint256 i = 0; i < 3; i++) {        int24 tickLower = positions[i][0];        int24 tickUpper = positions[i][1];
        if (liquidities[i] > 0) {            uint128 liquidityShare = uint128(Math.mulDiv(uint256(liquidities[i]), shares, totalSupply()));            //@audit mint of liquidity to the vault is done at the manipulated price            _mintLiquidity(tickLower, tickUpper, liquidityShare);        }    }

Recommendation

Either document the incompatibility with ERC777 standard, or add the twap check again after token transfer:

AlphaProVault.sol#L227-L240:

// Pull in tokens from sender    if (amount0 > 0) token0.safeTransferFrom(msg.sender, address(this), amount0);    if (amount1 > 0) token1.safeTransferFrom(msg.sender, address(this), amount1);+   checkPriceNearTwap();    for (uint256 i = 0; i < 3; i++) {        int24 tickLower = positions[i][0];        int24 tickUpper = positions[i][1];        if (liquidities[i] > 0) {            uint128 liquidityShare = uint128(Math.mulDiv(uint256(liquidities[i]), shares, totalSupply()));            _mintLiquidity(tickLower, tickUpper, liquidityShare);        }    }

Informational6 findings

Tick boundary check is off by one in checkCanRebalance
State
Acknowledged
Severity
Severity: Informational
Submitted by
cergyk
Description

In the function checkCanRebalance used to approve the call to rebalance, we make sure that the new boundaries for the base and limit ranges are not out of bounds:

AlphaProVault.sol#L456-L462:
```
// check price not too close to boundary    int24 maxThreshold = baseThreshold > limitThreshold ? baseThreshold : limitThreshold;    require(        tick >= TickMath.MIN_TICK + maxThreshold + tickSpacing            && tick <= TickMath.MAX_TICK - maxThreshold - tickSpacing,        "PB"    );
```
Unfortunately this tick is more restrictive than simply restricting newBaseLower >= -maxTick and newBaseUpper <= maxTick, so the function will revert if newBaseLower == -maxTick or newBaseUpper == maxTick which could be allowed

Fee share may not be distributed to user in edge-case

State: Acknowledged
Severity: Severity: Informational
Submitted by: cergyk

Description

During the evaluation of the amounts which should be transferred to user during withdrawal, the burning and collecting of fees is conditioned to liquidity > 0; But in the edge case where fees0 becomes bigger than totalLiquidity, the due fees may be non-zero even if the liquidity == 0.

This is an extreme edge-case because any action on the vault which reduces the liquidity, also collects fees. So the only case when this would happen is if the fees collected on a position grow bigger than the liquidity itself

AlphaProVault.sol#L329-L345:

/// @dev Withdraws share of liquidity in a range from Uniswap pool.    function _burnLiquidityShare(int24 tickLower, int24 tickUpper, uint256 shares, uint256 totalSupply)        internal        returns (uint256 amount0, uint256 amount1)    {        (uint128 totalLiquidity,,,,) = _position(tickLower, tickUpper);        uint256 liquidity = (uint256(totalLiquidity) * shares) / totalSupply;
        if (liquidity > 0) {            (uint256 burned0, uint256 burned1, uint256 fees0, uint256 fees1) =                _burnAndCollect(tickLower, tickUpper, _toUint128(liquidity));
            // Add share of fees            //@audit if fees0 > totalLiquidity, (fees0 * shares) / totalSupply can be non-zero, and liquidity == 0            //@audit In this edge-case, fee is not distributed to user            amount0 = burned0 + ((fees0 * shares) / totalSupply);            amount1 = burned1 + ((fees1 * shares) / totalSupply);        }    }

Recommendation

One could simply remove the liquidity > 0 condition:

AlphaProVault.sol#L329-L345:

/// @dev Withdraws share of liquidity in a range from Uniswap pool.    function _burnLiquidityShare(int24 tickLower, int24 tickUpper, uint256 shares, uint256 totalSupply)        internal        returns (uint256 amount0, uint256 amount1)    {        (uint128 totalLiquidity,,,,) = _position(tickLower, tickUpper);        uint256 liquidity = (uint256(totalLiquidity) * shares) / totalSupply;-        if (liquidity > 0) {-            (uint256 burned0, uint256 burned1, uint256 fees0, uint256 fees1) =-                _burnAndCollect(tickLower, tickUpper, _toUint128(liquidity));--            // Add share of fees-            amount0 = burned0 + ((fees0 * shares) / totalSupply);-            amount1 = burned1 + ((fees1 * shares) / totalSupply);-        }+        (uint256 burned0, uint256 burned1, uint256 fees0, uint256 fees1) =+            _burnAndCollect(tickLower, tickUpper, _toUint128(liquidity));++        // Add share of fees+        amount0 = burned0 + ((fees0 * shares) / totalSupply);+        amount1 = burned1 + ((fees1 * shares) / totalSupply);    }

Minor changes and renaming suggestions
State
Acknowledged
https://github.com/charmfinance/alpha-contracts-v2.1/commit/b398a433d44c864f893dd5a8819494cbaf18535f
Severity
Severity: Informational
Submitted by
cergyk
- period could be renamed to rebalancePeriod to be more specific
  
  AlphaProVault.sol?lines=118,118:
  uint32 public override period;
- maxTwapDeviation could be renamed to maxTwapDeviationTicks to make unit clear:
  
  AlphaProVault.sol?lines=129,129:
  int24 public override maxTwapDeviation;
  Additionally maxTwapDeviation can be made uint24 to avoid unecessary sign checks
Emergency burn action should have cooldown
State
Acknowledged
Severity
Severity: Informational
Submitted by
cergyk
Description

As seen in reported issues #5 and #7 the emergencyBurn endpoint enables the manager to do manipulations on liquidity which otherwise would not be possible during a rebalancing in normal conditions (most notably sandwich the mint operation on the uniswap v3 pool, because during rebalancing the burn and mint action happen at the same price).

It would make sense as such to restrict the call to this operation to exceptional conditions, and disable a repeated or frequent usage of the endpoint.

Recommendation

Introduce a emergencyState which would restrict deposit and rebalance actions, with a short cooldown after which the emergencyState can be deactivated.
_verifyTick is a misnomer
State
Fixed
https://github.com/charmfinance/alpha-contracts-v2.1/commit/4932841d93a5b7e1cc47624768b0b768dba67a65
Severity
Severity: Informational
Submitted by
Kaden
Description

AlphaProVault._verifyTick takes a tick and a _maxTick and clamps the tick to be within the negative and positive _maxTick:
```
/// @dev Verifies that tick is within the range boundariesfunction _verifyTick(int24 tick, int24 _maxTick) internal pure returns (int24) {    if (tick < -_maxTick) {        return -_maxTick;    }    if (tick > _maxTick) {        return _maxTick;    }    return tick;}
```
Given the name of the function alone, this is a bit unexpected. Instead, it might be expected that the function checks if the tick is valid and reverts or returns false if it's not.

Recommendation

Consider renaming the function to something that better represents the behavior, e.g.: _boundTick or _clampTick.
Shadowed function name
State
Fixed
https://github.com/charmfinance/alpha-contracts-v2.1/commit/e47b88536f37f211efe3729f37f0b1526531e9b1
Severity
Severity: Informational
Submitted by
Kaden
Description

In each of _calcSharesAndAmounts, withdraw, and _burnLiquidityShare, we use totalSupply as a variable or parameter name, e.g.:
```
uint256 totalSupply = totalSupply();
```
This shadows the function by the same name. As a result, in any scope which we have defined a variable or parameter as totalSupply, the function will no longer executable. Currently this doesn't pose any issues and is likely to be blocked by the compiler if it does become an issue, but it's best practice to avoid shadowing function names for improved maintainability.

Recommendation

Use a different name for the variables and parameters referencing the totalSupply, e.g.:
```
-uint256 totalSupply = totalSupply();+uint256 _totalSupply = totalSupply();
```

Gas Optimizations2 findings

Storage variables can be immutable
State
Acknowledged
Severity
Severity: Gas optimization
Submitted by
Kaden
Description

In AlphaProVault, we have a few variables defined during initialization which are never reassigned, and are thus effectively immutable:
```
IUniswapV3Pool public override pool;IERC20 public token0;IERC20 public token1;AlphaProVaultFactory public factory;
```
Since these are effectively immutable, we can mark them as immutable to save gas on each read.

Note: This does require switching from use of regular ERC1167 clone to deploy this contract from the AlphaProVaultFactory. It can alternatively be deployed with create2 directly or ClonesWithImmutableArgs (see Clones.cloneDeterministicWithImmutableArgs) to allow for the use of immutables.

Recommendation

Consider making these storage variables immutable, either via:
1. Use create2 to deploy the vaults in AlphaProVaultFactory, then mark the aforementioned storage variables as immutable, initializing them in the constructor instead of initialize.
2. Use cloneDeterministicWithImmutableArgs to deploy the vaults in AlphaProVaultFactory, passing the aforementioned storage variables as arguments and updating logic to access them as necessary.

Storage variables can be packed

State: Fixed
https://github.com/charmfinance/alpha-contracts-v2.1/commit/4932841d93a5b7e1cc47624768b0b768dba67a65
Severity: Severity: Gas optimization
Submitted by: Kaden

Description

We include the following storage variables in AlphaProVault:

uint256 public override accruedProtocolFees0;uint256 public override accruedProtocolFees1;uint256 public override accruedManagerFees0;uint256 public override accruedManagerFees1;uint256 public override lastTimestamp;

We can reduce the size of these uints to pack them into shared storage slots:

// accruedProtocolFees0 and accruedProtocolFees1 are in the same storage slotuint128 public override accruedProtocolFees0;uint128 public override accruedProtocolFees1;// accruedManagerFees0 and accruedManagerFees1 are in the same storage slotuint128 public override accruedManagerFees0;uint128 public override accruedManagerFees1;

Since these storage vars are always used together, this allows us to save gas on expensive SSTORE/SLOAD operations each time we read/write them.

We can also include lastTimestamp in one of these slots since it's only used in rebalance, in which the others are too:

// accruedProtocolFees0 and accruedProtocolFees1 are in the same storage slotuint128 public override accruedProtocolFees0;uint128 public override accruedProtocolFees1;// accruedManagerFees0, accruedManagerFees1, and lastTimestamp are in the same storage slotuint104 public override accruedManagerFees0;uint104 public override accruedManagerFees1;uint40 public override lastTimestamp;

Recommendation

Reduce the size of the aforementioned storage variables as indicated above.

alpha-contracts-v2.1

Findings

Medium Risk1 finding

Unsufficient factory check during vault creation

Description

Recommendation

Low Risk6 findings

Fee rounding consistency in getPositionAmounts

Description

Recommendation

Protocol fee could be fetched from factory after rebalance

Description

Recommendation

Manager can allow himself to rebalance unrestricted

Description

Recommendation

Vault can be drained if baseThreshold is equal to wideThreshold

Description

1/ Deposits:

2/ Rebalance:

3/ Withdraw

Impact Analysis

Recommendation

Rounding error during direct deposit may Dos deposits

Description

Recommendation

ERC777 tokens will enable caller to self sandwich mint to extract part of deposit

Description

Recommendation

Informational6 findings

Tick boundary check is off by one in checkCanRebalance

Description

Fee share may not be distributed to user in edge-case

Description

Recommendation

Minor changes and renaming suggestions

Emergency burn action should have cooldown

Description

Recommendation

_verifyTick is a misnomer

Description

Recommendation

Shadowed function name

Description

Recommendation

Gas Optimizations2 findings

Storage variables can be immutable

Description

Recommendation

Storage variables can be packed

Description

Recommendation