alpha-contracts-v2.1
Cantina Security Report
Organization
- @charmfinance
Engagement Type
Cantina Reviews
Period
-
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
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
Severity
- Severity: Low
≈
Likelihood: Low×
Impact: Low Submitted by
cergyk
Description
To evaluate shares to mint for given deposit amounts,
AlphaProVault::deposit
uses agetPositionAmounts(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:
} 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
forgetPositionAmounts
:(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
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 inAlphaProVaultFactory
to set protocol fee for vaults created in the future:/** * @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
:// 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
Severity
- Severity: Low
≈
Likelihood: Low×
Impact: High Submitted by
cergyk
Description
An implicit assumption in
AlphaProVault
is thatwide
andbase
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 methodsdeposit
,rebalance
andwithdraw
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:
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
// 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 rebalanceDuring 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%.// 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: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); }
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
Severity
- Severity: Low
Submitted by
cergyk
Description
To deposit on AlphaProVault, users provide
amount0Desired
andamount1Desired
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 caseamount0
andamount1
pulled amount calculation in_calcAmountsAndShares
: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:
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:
// 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.// 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:
// 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 torebalance
, we make sure that the new boundaries for thebase
andlimit
ranges are not out of bounds:// 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
andnewBaseUpper <= maxTick
, so the function will revert ifnewBaseLower == -maxTick
ornewBaseUpper == maxTick
which could be allowedFee 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 wherefees0
becomes bigger thantotalLiquidity
, the due fees may be non-zero even if theliquidity == 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
/// @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:/// @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
Severity
- Severity: Informational
Submitted by
cergyk
-
period
could be renamed torebalancePeriod
to be more specificAlphaProVault.sol?lines=118,118:
uint32 public override period;
-
maxTwapDeviation
could be renamed tomaxTwapDeviationTicks
to make unit clear:AlphaProVault.sol?lines=129,129:
int24 public override maxTwapDeviation;
Additionally
maxTwapDeviation
can be madeuint24
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 themint
operation on the uniswap v3 pool, because during rebalancing theburn
andmint
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 restrictdeposit
andrebalance
actions, with a short cooldown after which the emergencyState can be deactivated._verifyTick is a misnomer
State
Severity
- Severity: Informational
Submitted by
Kaden
Description
AlphaProVault._verifyTick
takes atick
and a_maxTick
and clamps thetick
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
Severity
- Severity: Informational
Submitted by
Kaden
Description
In each of
_calcSharesAndAmounts
,withdraw
, and_burnLiquidityShare
, we usetotalSupply
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 theAlphaProVaultFactory
. It can alternatively be deployed withcreate2
directly orClonesWithImmutableArgs
(seeClones.cloneDeterministicWithImmutableArgs
) to allow for the use of immutables.Recommendation
Consider making these storage variables immutable, either via:
- Use
create2
to deploy the vaults inAlphaProVaultFactory
, then mark the aforementioned storage variables asimmutable
, initializing them in the constructor instead ofinitialize
. - Use
cloneDeterministicWithImmutableArgs
to deploy the vaults inAlphaProVaultFactory
, passing the aforementioned storage variables as arguments and updating logic to access them as necessary.
Storage variables can be packed
State
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 inrebalance
, 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.