yAudit Asymmetry Review

Review Resources:

code repository

Auditors:

• spalen
• panda

Table of Contents

1. Review Summary
2. Scope
3. Code Evaluation Matrix
4. Findings Explanation
5. Critical Findings
   1. 1. Critical - requestUnlock() doesn’t take into consideration the amount of unlocking already requested
   2. 2. Critical - totalValue() reverts, breaking the protocol when unlockObligations grow
6. Medium Findings
   1. 1. Medium - setOperator and setProtocolFeeCollector can’t be used to update the address
   2. 2. Medium - maxRequestUnlock()doesn’t take into consideration pending unlocks
   3. 3. Medium - totalValue() returns overestimated value
7. Low Findings
   1. 1. Low - Contracts can receive ETH but do not implement any function for withdrawal
   2. 2. Low - maxWithdraw() should take into consideration the amount of CVX available to withdraw in the Convex reward pool
   3. 3. Low - Return 0 for max deposit and max withdraw values when paused
   4. 4. Low - Instant share value increase can be sandwiched if the instant withdrawal fee is lower than the profit
8. Gas Saving Findings
   1. 1. Gas - Cache array length outside of the loop
   2. 2. Gas - Split deposit() into two function
   3. 3. Gas - Remove double-checks
   4. 4. Gas - Unnecessary variable creation
   5. 5. Gas - Calculate variable only if needed
   6. 6. Gas - PirexMigrator multiRedeem should group redemption and deposits together
   7. 7. Gas - Skip claiming extra rewards from CVX_REWARDS_POOL
9. Informational Findings
   1. 1. Informational - Recommended array length check
   2. 2. Informational - Prefer using Ownable2Step or Ownable2StepUpgradeable instead of Ownable
   3. 3. Informational - TODO left in the code
   4. 4. Informational - public functions not called by the contract should be declared external instead
   5. 5. Informational - Refactor duplicated require()/revert() checks into a modifier or function
   6. 6. Informational - Unnecessary integer variable initialization
   7. 7. Informational - Misleading modifier name
   8. 8. Informational - PirexMigrator prevents the receiver from being the PirexMigrator contract
   9. 9. Informational - Use memory instead of storage
   10. 10. Informational - The price of AfCVX can be manipulated atomically
   11. 11. Informational - Upgrade dependencies
10. Final remarks

Review Summary

Asymmetry

Asymmetry provides a vault built on top of Convex staker and ClevCVX contracts. The vault allows users to deposit CVX and earn more yield by depositing funds to CLever and Convex. The CVX gets locked for a higher yield, meaning they must wait some time before withdrawing CVX. The protocol handles this by using withdraw queues in the strategy contract.

The contracts of the Asymmetry Repo were reviewed over four days. Two auditors performed the code review between April 22nd and April 25th, 2024. The repository was under active development during the review, but the review was limited to the latest commit at the start. This was commit 57cb6d5162a526bce6b34209a13c0bcfd36f86cb for the asymmetry repo.

Scope

The scope of the review consisted of the following contracts at the specific commit:

• AfCvx.sol
• PirexMigrator.sol

After the findings were presented to the asymmetry team, fixes were made and included in several PRs.

This review is a code review to identify potential vulnerabilities in the code. The reviewers did not investigate security practices or operational security and assumed that privileged accounts could be trusted. The reviewers did not evaluate the security of the code relative to a standard or specification. The review may not have identified all potential attack vectors or areas of vulnerability.

yAudit and the auditors make no warranties regarding the security of the code and do not warrant that the code is free from defects. yAudit and the auditors do not represent nor imply to third parties that the code has been audited nor that the code is free from defects. By deploying or using the code, asymmetry, and users of the contracts agree to use the code at their own risk.

Code Evaluation Matrix

Findings Explanation

Findings are broken down into sections by their respective impact:

• Critical, High, Medium, Low Impact
  • These findings range from attacks that may cause loss of funds, impact control/ownership of the contracts, or cause any unintended consequences/actions that are outside the scope of the requirements.
• Gas savings
  • Findings that can improve the gas efficiency of the contracts.
• Informational
  • Findings including recommendations and best practices.

Critical Findings

1. Critical - requestUnlock() doesn’t take into consideration the amount of unlocking already requested

The function requestUnlock() in the CLeverCVXStrategy is designed to handle unlock requests from users by accessing locked funds and scheduling them for release. However, it does not consider concurrent unlock requests from multiple users targeting the same epoch. The function retrieves locked amounts and schedules unlocks without checking the available balance left to be unlocked per epoch based on other users’ ongoing unlock requests.

Technical Details

This oversight can lead to the over-commitment of funds in a single unlock epoch, where the actual unlockable amount might be less than the sum of requests if multiple users attempt to unlock funds for the same epoch. Such a scenario could lead to transaction failures or incorrect processing of unlock requests, impacting user trust and the integrity of the fund management process.

CLeverCVXStrategy.sol#L123-L150

Here is the test you can run to try out:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.25;

import {CVX} from "src/interfaces/convex/Constants.sol";
import {ICleverCvxStrategy} from "src/interfaces/afCvx/ICleverCvxStrategy.sol";
import {BaseForkTest} from "test/utils/BaseForkTest.sol";
import {CLEVER_CVX_LOCKER, EpochUnlockInfo} from "src/interfaces/clever/ICLeverCvxLocker.sol";
import "forge-std/console.sol";

contract AfCvxUnlockForkTest is BaseForkTest {
    function test_withdrawUnlockedPoc() public {
        uint256 assets = 100e18;
        address userA = _createAccountWithCvx(assets);
        address userB = _createAccountWithCvx(assets);
        address userC = _createAccountWithCvx(assets * 10); // Add more assets.
        // First userA deposits.
        _deposit(userA, assets);
        _distributeAndBorrow();

        // Now we wait two week.
        skip(2 weeks);
        vm.roll(block.number + 1);

        // Now userB deposits.
        _deposit(userB, assets);
        _deposit(userC, assets * 10);
        _distributeAndBorrow();

        // We now have two locks.
        (EpochUnlockInfo[] memory locks, ) = CLEVER_CVX_LOCKER.getUserLocks(
            address(cleverCvxStrategy)
        );
        for (uint i = 0; i < locks.length; i++) {
            console.log("pendingUnlock", locks[i].pendingUnlock);
            console.log("unlockEpoch", locks[i].unlockEpoch);
        }
        // User A will request a unlock equal to 100% of the first unlock.
        uint256 firstUnlock = locks[0].pendingUnlock;

        vm.startPrank(userA);
        afCvx.approve(address(afCvx), afCvx.previewWithdraw(firstUnlock));
        (uint256 unlockEpochA, ) = afCvx.requestUnlock(
            firstUnlock,
            userA,
            userA
        );
        vm.stopPrank();

        // User B also asks for a unlock equal to the first unlock.
        vm.startPrank(userB);
        afCvx.approve(address(afCvx), afCvx.previewWithdraw(firstUnlock));
        (uint256 unlockEpochB, ) = afCvx.requestUnlock(
            firstUnlock,
            userB,
            userB
        );
        vm.stopPrank();

        // Let's read the strategy locks.
        ICleverCvxStrategy.UnlockRequest[] memory unlocksA = cleverCvxStrategy
            .getRequestedUnlocks(userA);
        ICleverCvxStrategy.UnlockRequest[] memory unlocksB = cleverCvxStrategy
            .getRequestedUnlocks(userB);
        console.log("unlocksA", unlocksA[0].unlockAmount, unlocksA[0].unlockEpoch);
        console.log("unlocksB", unlocksB[0].unlockAmount, unlocksB[0].unlockEpoch);
        // The two are equal but shouldn't be.
        assert(unlocksA[0].unlockEpoch == unlocksB[0].unlockEpoch);
    }
}

The test will show the unlock times for both requests, which have the same epoch.

Impact

Critical. With the user’s AfCVX tokens burnt the funds are locked in the strategy.

Recommendation

Track the total amount of funds to be unlocked.

Developer Response

The issue was fixed in the commit.

2. Critical - totalValue() reverts, breaking the protocol when unlockObligations grow

While testing, we uncovered an error with totalValue(). The call reverts due to the borrowedClever and unlockObligations sum being higher than the depositedClever.

Technical Details

File: CleverCvxStrategy.sol
 74 |       deposited = depositedClever - borrowedClever + unrealisedFurnace - unlockObligations;

CLeverCVXStrategy.sol#L74

Here is a test you can add to your test suite:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.25;

import {CVX} from "src/interfaces/convex/Constants.sol";
import {ICleverCvxStrategy} from "src/interfaces/afCvx/ICleverCvxStrategy.sol";
import {BaseForkTest} from "test/utils/BaseForkTest.sol";

contract AfCvxReverts is BaseForkTest {
      function test_totalAssets() public {
        uint256 assets = 100e18;
        address userA = _createAccountWithCvx(assets);
        address userB = _createAccountWithCvx(assets);
        // First userA deposits.
        _deposit(userA, assets);
        _distributeAndBorrow();

        // Now we wait two week.
        skip(2 weeks);
        // Now userB deposits.
        _deposit(userB, assets);
        _distributeAndBorrow();
        // User A requests unlock.
        vm.startPrank(userA);
        uint256 maxUnlock = afCvx.maxRequestUnlock(userA);
        afCvx.approve(address(afCvx), afCvx.previewWithdraw(maxUnlock));
        (uint256 unlockEpochA, ) = afCvx.requestUnlock(maxUnlock, userA, userA);
        // totalAssets is reverting.
        afCvx.totalAssets();
    }
}

forge test --match-test test_totalAssets
Failing tests:
Encountered 1 failing test in test/AfCvx.unlock.t.sol:AfCvxUnlockForkTest
[FAIL. Reason: panic: arithmetic underflow or overflow (0x11)] test_totalAssets() (gas: 1492587)

Impact

Critical. totalValue() reverting breaks the vault.

Recommendation

Rethink totalValue() so that borrowedClever and unlockObligations don’t underflow the deposited calculation.

Developer Response

I fixed totalValue calculation to avoid the overflow in the following commit.

Medium Findings

1. Medium - setOperator and setProtocolFeeCollector can’t be used to update the address

The function to set the new operator or new protocolFeeCollector will always revert except if the operator has a zero address, making updating the address impossible.

Technical Details

File: AfCvx.sol
484 |    function setProtocolFeeCollector(address newProtocolFeeCollector) external onlyOwner {
485 |        if (newProtocolFeeCollector != address(0)) revert InvalidAddress();
486 |        protocolFeeCollector = newProtocolFeeCollector;
487 |        emit ProtocolFeeCollectorSet(newProtocolFeeCollector);
488 |    }
489 |
490 |    function setOperator(address newOperator) external onlyOwner {
491 |        if (newOperator != address(0)) revert InvalidAddress();
492 |         operator = newOperator;
493 |         emit OperatorSet(newOperator);
494 |     }

AfCvx.sol#L484-L494

File: CLeverCVXStrategy.sol
228 |    function setOperator(address newOperator) external onlyOwner {
229 |        if (newOperator != address(0)) revert InvalidAddress();
230 |        operator = newOperator;
231 |        emit OperatorSet(newOperator);
232 |    }

CLeverCVXStrategy.sol#L228-L232

Impact

Medium. The operator or the protocolFeeCollector can’t be updated.

Recommendation

Fix the condition.

Developer Response

Fixed in the commit.

2. Medium - maxRequestUnlock()doesn’t take into consideration pending unlocks

The function maxRequestUnlock() should return the maximum amount of assets that can be unlocked by the owner but it doesn’t take into consideration that there can pending unlocks.

Technical Details

CLEVER_CVX_LOCKER.getUserInfo(address(cleverCvxStrategy)) returns the totalLocked in CLever locker that can withdraw. If some user requests unlock, the function should subtract that request from totalLocked value because pending requests will withdraw from the same Clever locker. This can lead to users burning more assets than it is possible to unlock.

Impact

Medium. Users could burn more assets than they can request to unlock.

Recommendation

In CleverCvxStrategy add a function in CleverCvxStrategy to return the maximum amount of assets that can be unlocked which is totalLocked minus pending unlock amount.

function maxUnlock() external view returns (uint256) {
    (uint256 totalLocked,,,,) = CLEVER_CVX_LOCKER.getUserInfo(address(this));
    return totalLocked - unlockObligations;
}

Use this new function to calculate the correct value for max request unlock:

function maxRequestUnlock(address owner) public view returns (uint256 maxAssets) {
-   (uint256 totalLocked,,,,) = CLEVER_CVX_LOCKER.getUserInfo(address(cleverCvxStrategy));
+   uint256 totalLocked = cleverCvxStrategy.maxUnlock();
    return super.previewRedeem(balanceOf(owner)).min(totalLocked);
}

Developer Response

Related to findings 2. Fixed it in the same commit.

3. Medium - totalValue() returns overestimated value

totalValue() returns the total value of the strategy’s funds, but it doesn’t account for the fee it has to pay, meaning the return value will be overestimated.

Technical Details

The strategy deposits CVX into CLever and borrows clevCVX. To withdraw all funds from CLever, it must repay the whole borrowed amount plus the additional fee. This means that the strategy will have to pay that fee in the end, which should be subtracted from the total value of the strategy.

The CLever repay fee is calculated as a percentage of the repay clevCVX amount.

Impact

Medium. The strategy overestimates its total value.

Recommendation

Subtract the CLever repay fee from the total value of the strategy. Be aware that borrow() call will decrease the total value.

Developer Response

Fixed in the commit

Low Findings

1. Low - Contracts can receive ETH but do not implement any function for withdrawal

The AfCvx contract can receive ETH, but can not withdraw.

Technical Details

File: src/AfCvx.sol

23 | contract AfCvx is IAfCvx, TrackedAllowances, Ownable, ERC4626Upgradeable, ERC20PermitUpgradeable, UUPSUpgradeable {

The contract isn’t sending back the ETH received in the initialize() or the receive() functions, but this ETH hasn’t been sent back.

AfCvx.sol#L23

Impact

Low. ETH can be stuck on the contract.

Recommendation

Remove the payable modifier and remove the receive function.

Developer Response

The receive() function is required to perform the swap. It can only receive ETH from the Curve pool. Removed payable modifier from initialize in the commit

2. Low - maxWithdraw() should take into consideration the amount of CVX available to withdraw in the Convex reward pool

In the scenario where weeklyWithdrawalLimit is higher than the sum of the balance of CVX in the contract and available to withdraw from the convex reward pool, a user/contract calling maxWithdraw() will get an inaccurate value.

Technical Details

File: AfCvx.sol
 165 |       return previewRedeem(balanceOf(owner)).min(weeklyWithdrawalLimit);

AfCvx.sol#L158-L166

Impact

Low. AfCvx doesn’t follow the ERC4626 standard.

Recommendation

Add a check for available CVX to the maxWithdraw() function.

Developer Response

Fixed in commit

3. Low - Return 0 for max deposit and max withdraw values when paused

When the contract is paused, the user cannot deposit or withdraw. In that case, the functions that return max values should return 0.

Technical Details

Override the following ERC4626 functions to return 0 when the AfCvx contract is paused:

• maxDeposit()
• maxMint()
• maxWithdraw()
• maxRedeem()

Returning other values than 0 will show users that they can deposit or withdraw returned values, but if the contract is paused, the call will revert. Returning 0 will imply that deposits and withdrawals cannot performed.

EIP4626 clearly specifies that disabled deposits must return 0: “MUST factor in both global and user-specific limits, like if deposits are entirely disabled (even temporarily), it MUST return 0”.

Impact

Low. AfCvx doesn’t follow the ERC4626 standard.

Recommendation

Return 0 if paused in specified function.

if (paused) return 0;

Developer Response

Fixed in the commit

4. Low - Instant share value increase can be sandwiched if the instant withdrawal fee is lower than the profit

The function harvest(uint256 minAmountOut) in the AfCVX contract triggers the harvesting of rewards from both a Convex staking pool and a clever strategy involving CVX tokens. However, adding rewards directly to the vault without a time-based distribution leads to an immediate increase in share value. This could result in discrepancies in share value for investors entering or exiting around the time of harvest, possibly leading to exploitation or unfair advantage.

Technical Details

The immediate increase in share value post-reward harvest can create opportunities for arbitrage, where users might predict the harvest time. The fee and withdrawal limits mitigate this.

Impact

Low. It’s essential to set accurate limits and fees to prevent this behavior.

Recommendation

If possible, monitor on-chain transactions for a sandwich type harvest() attack.

Developer Response

We are aware of this risk and the withdrawal fee was introduced to mitigate it. The harvest() can also be sandwiched with a deposit and request unlock. We don’t take a withdrawal fee on request unlock, but it’s a very gas-intensive operation. The unit tests here illustrate the sandwich attacks

Gas Saving Findings

1. Gas - Cache array length outside of the loop

When iterating over an array, it is recommended to cache the array length outside of the loop to avoid unnecessary gas costs.

Technical Details

File: src/PirexMigrator.sol

122 | for (uint256 i = 0; i < _unlockTimes.length; i++) {

PirexMigrator.sol#L122

Impact

Gas savings.

Recommendation

Cache the array length outside of the loop.

Developer Response

Fixed.

2. Gas - Split deposit() into two function

Using two functions would save gas, as they do not require a boolean and a minAmountOut when calling deposit on CLEVER_CVX_LOCKER.

Technical Details

94  |    function deposit(uint256 cvxAmount, bool swap, uint256 minAmountOut) external onlyManager unlockNotInProgress {
95  |        address(CVX).safeTransferFrom(msg.sender, address(this), cvxAmount);
96  |        if (swap) {
97  |            uint256 clevCvxAmount = Zap.swapCvxToClevCvx(cvxAmount, minAmountOut);
98  |            FURNACE.deposit(clevCvxAmount);
99  |        } else {
100 |            CLEVER_CVX_LOCKER.deposit(cvxAmount);
101 |        }

Using two functions, you can remove the need for a boolean argument and save some gas.

Impact

Gas savings

Recommendation

Use two functions named deposit with the following function definitions deposit(address) that would deposit into CLEVER_CVX_LOCKER and deposit(address, uint256) for swapping CVX for ClevCVX.

Developer Response

I decided not to make the modifications here as it will require splitting distribute() as well. The gas savings are minimal and the function is restricted, the end-users won’t be calling it. Also, I don’t understand why the suggested recommendation passes address to deposit() function - deposit(address).

3. Gas - Remove double-checks

In some places, there are double checks for the same value. Removing duplicated checks will save gas.

Technical Details

File: src/PirexMigrator.sol

88 |    if (_minSwapReceived == 0) revert ZeroAmount();
89 |    PIREX_LP.swap(IPirexLiquidityPool.Token._PX_CVX, _amount, _minSwapReceived, FROM_INDEX, TO_INDEX);

PirexMigrator.sol#L88 can be removed because because PIREX_LP.swap() function have also check for _minSwapReceived == 0.

File: src/PirexMigrator.sol

106 |    if (_receiver == address(0)) revert ZeroAddress();
112 |    if (_amount > 0) _amount = ASYMMETRY_CVX.deposit(_amount, _receiver);

PirexMigrator.sol#L106 can be removed because because ASYMMETRY_CVX.deposit() function have check inside ERC20Upgradeable._mint() that receiver account is account != address(0).

File: src/PirexMigrator.sol

133 |    if (_for == address(0)) revert ZeroAddress();
147 |    if (_amount > 0) _amount = ASYMMETRY_CVX.deposit(_amount, _for);

PirexMigrator.sol#L133 can be removed because because ASYMMETRY_CVX.deposit() function have check inside ERC20Upgradeable._mint() that receiver account is account != address(0).

Impact

Gas savings.

Recommendation

Remove duplicated checks.

Developer Response

kept PirexMigrator.sol#L106 check because _amount could be 0

4. Gas - Unnecessary variable creation

Unnecessary variable creation increases the gas cost.

Technical Details

Instead of creating the realisedFurnace variable, assign the value directly to rewards.

CLeverCVXStrategy.sol#L70-L76

Impact

Gas savings.

Recommendation

- (uint256 unrealisedFurnace, uint256 realisedFurnace) = FURNACE.getUserInfo(address(this));
+ uint256 unrealisedFurnace;
+ (unrealisedFurnace, rewards) = FURNACE.getUserInfo(address(this));

deposited = depositedClever - borrowedClever + unrealisedFurnace - unlockObligations;
- rewards = realisedFurnace;

Developer Response

The code has changed, and the recommendation isn’t relevant anymore

5. Gas - Calculate variable only if needed

Calculation of some variables can be skipped in some cases.

Technical Details

In the function withdrawUnlocked() variable unlockAmount can be calculated later inside if statement to save some gas.

-          uint256 unlockAmount = unlocks[nextUnlockIndex].unlockAmount;
            if (unlockEpoch <= currentEpoch) {
+              uint256 unlockAmount = unlocks[nextUnlockIndex].unlockAmount;
                delete unlocks[nextUnlockIndex];
                cvxUnlocked += unlockAmount;
             } else {
                break;
             }

Impact

Gas Savings.

Recommendation

Calculate variables only when needed, as suggested above.

Developer Response

Fixed

6. Gas - PirexMigrator multiRedeem should group redemption and deposits together

The contract exposes multiRedeem() and redeem(), and multiRedeem() uses redeem() in a for loop, which significantly increases the gas needed.

Technical Details

File: PirexMigrator.sol
121 |    function multiRedeem(uint256[] calldata _unlockTimes, address[] calldata _fors) external returns (uint256 _amount) {
122 |        for (uint256 i = 0; i < _unlockTimes.length; i++) {
123 |            _amount += redeem(_unlockTimes[i], _fors[i]);
124 |        }
125 |    }

PirexMigrator.sol#L121-L125

On every call to redeem(), the funds from PIREX_CVX are redeemed and deposited into ASYMMETRY_CVX. This process incurs a significant gas cost because it allows for redeems to be made for different users and unlock times. However, a single user with multiple unlock times will face a higher gas cost than necessary to migrate their funds.

Impact

Gas savings.

Recommendation

Since an array is required to call PIREX_CVX.redeem, it is advised to move most of the complexity in the multiRedeem function and have redeem call multiRedeem with a single-entry array. The multiRedeem() function could have the following interface.

struct RedeemRequest {
address account,
uint256[] unlockTimes
}

function multiRedeem(RedeemRequest[] requests) {}

Developer Response

Because this function is designed to potentially be used by Asymmetry team to redeem for multiple users, and not necessarily for a user to redeem several requests.

7. Gas - Skip claiming extra rewards from CVX_REWARDS_POOL

Convex rewards pool provides an option to claim extra rewards. The contract AfCvx is not designed to handle additional tokens so it would be cheaper to skip claiming.

Technical Details

The CVX_REWARDS_POOL has an additional function to skip claiming extra rewards. Use it to save some gas. Currently, there are no extra rewards.

Impact

Gas savings.

Recommendation

Use a different function to claim Convex rewards that skips claiming additional rewards:

- CVX_REWARDS_POOL.getReward(false);
+ CVX_REWARDS_POOL.getReward(address(this), false, false);

Developer Response

Fixed

Informational Findings

1. Informational - Recommended array length check

If the length of the _fors array is smaller than the length of the _unlockTimes array, the transaction will revert and consume the gas used up to that point.

Technical Details

File: PirexMigrator.sol
121 |    function multiRedeem(uint256[] calldata _unlockTimes, address[] calldata _fors) external returns (uint256 _amount) {
122 |        for (uint256 i = 0; i < _unlockTimes.length; i++) {
123 |            _amount += redeem(_unlockTimes[i], _fors[i]);

PirexMigrator.sol#L121-L125

Impact

Informational

Recommendation

Add a requirement that checks if the lengths of the two variables are equal.

Developer Response

Fixed.

2. Informational - Prefer using Ownable2Step or Ownable2StepUpgradeable instead of Ownable

The contracts Ownable2Step or Ownable2StepUpgradeable introduce a mechanism that prevents accidental transfer of contract ownership to an incorrect address. This is achieved by requiring the recipient of owner permission to actively accept the transfer through a contract call of its own.

Technical Details

File: src/AfCvx.sol

23 | contract AfCvx is IAfCvx, TrackedAllowances, Ownable, ERC4626Upgradeable, ERC20PermitUpgradeable, UUPSUpgradeable {

AfCvx.sol#L23

File: src/strategies/CLeverCVXStrategy.sol

17 | contract CleverCvxStrategy is ICleverCvxStrategy, TrackedAllowances, Ownable, UUPSUpgradeable {

CLeverCVXStrategy.sol#L17)

Impact

Informational

Recommendation

Use Ownable2Step instead of Ownable.

Developer Response

Not an issue. Ownable from solady is already a two-step. See https://github.com/Vectorized/solady/blob/main/src/auth/Ownable.sol#L222

3. Informational - TODO left in the code

TODO should be resolved before deployment.

Technical Details

File: src/PirexMigrator.sol

35 | // IERC4626 public constant ASYMMETRY_CVX = IERC4626(0x4e3FBD56CD56c3e72c1403e103b45Db9da5B9D2B); // TODO - make constant

47 | ASYMMETRY_CVX = IERC4626(_afCVX); // TODO

PirexMigrator.sol#L35 PirexMigrator.sol#L47

Impact

Informational

Recommendation

Fix the todos

Developer Response

That TODO will be removed at deployment time, after afCVX is deployed.

4. Informational - public functions not called by the contract should be declared external instead

Using a public function instead of an external function does not necessarily incur more gas costs; however, it is still recommended to use the external modifier instead of the public modifier for clarity. A function override can change the visibility.

Technical Details

File: src/AfCvx.sol

80 | function decimals() public pure override(ERC4626Upgradeable, ERC20Upgradeable, IERC20Metadata) returns (uint8) {

111 | function deposit(uint256 assets, address receiver)

127 | function mint(uint256 shares, address receiver)

158 | function maxWithdraw(address owner)

193 | function withdraw(uint256 assets, address receiver, address owner)

208 | function maxRedeem(address owner)

244 | function redeem(uint256 shares, address receiver, address owner)

AfCvx.sol 80, 111, 127, 158, 193, 208, 244

Impact

Informational

Recommendation

Use the external modifier instead of the public

Developer Response

Not an issue. All mentioned functions are overrides and the function visibility should be the same as in the parent contract.

5. Informational - Refactor duplicated require()/revert() checks into a modifier or function

It is recommended to refactor duplicated require()/revert() checks into a modifier or function.

Technical Details

File: src/AfCvx.sol

453 | if (newShareBps > BASIS_POINT_SCALE) revert InvalidShare();
477 | if (newShareBps > BASIS_POINT_SCALE) revert InvalidShare();

461 | if (newFeeBps > BASIS_POINT_SCALE) revert InvalidFee();
469 | if (newFeeBps > BASIS_POINT_SCALE) revert InvalidFee();

AfCvx.sol 453, 461, 469, 477

File: src/PirexMigrator.sol

79 | if (_receiver == address(0)) revert ZeroAddress();
106 | if (_receiver == address(0)) revert ZeroAddress();

PirexMigrator.sol 79, 106

Impact

Informational.

Recommendation

Use a modifier for clarity.

Developer Response

Refactored in the commit

6. Informational - Unnecessary integer variable initialization

In Solidity, variables are automatically initialized to zero. Therefore, explicitly initializing variables to zero is not mandatory and can be skipped to optimize gas costs.

Technical Details

File: src/PirexMigrator.sol

27 | uint256 public constant TO_INDEX = 0; // CVX

122 | for (uint256 i = 0; i < _unlockTimes.length; i++) {

PirexMigrator.sol#L27, PirexMigrator.sol#L122

Impact

Informational.

Recommendation

Do not initialize to zero.

Developer Response

Removed initialization from PirexMigrator.sol#L122, kept PirexMigrator.sol#L27, for code readability.

7. Informational - Misleading modifier name

The modifier name should clearly explain its function.

Technical Details

AfCvx.onlyOperator() should limit the caller to the operator’s address, but it also enables the owner to bypass this check.

CleverCvxStrategy.onlyOperator() also enables the owner to bypass only operator check.

Impact

Informational.

Recommendation

Rename the modifier to onlyOperatorOrOwner.

Developer Response

Changed.

8. Informational - PirexMigrator prevents the receiver from being the PirexMigrator contract

The PirexMigrator prevents the _receiver from being the zero address. A second address that can be checked is making sure the recipient is not the contract itself.

Technical Details

PirexMigrator.sol#L74 PirexMigrator.sol#L105

Impact

Informational.

Recommendation

Consider adding a check to make sure the _receiver is not the PirexMigrator contract.

Developer Response

Fixed.

9. Informational - Use memory instead of storage

Using memory instead of storage clearly distinguishes that the data cannot be changed and will be used only for reading.

Technical Details

accountUnlocks can be defined as memory:

- UnlockRequest[] storage accountUnlocks = requestedUnlocks[account].unlocks;
+ UnlockRequest[] memory accountUnlocks = requestedUnlocks[account].unlocks;

Impact

Informational.

Recommendation

When the data is not updated, define it as memory instead of storage.

Developer Response

Done

10. Informational - The price of AfCVX can be manipulated atomically

The price of AfCVX based on totalAssets() can be inflated atomically using, for example, a CVX transfer to the afCVX contract or stakeFor from the convex rewards contract. It’s essential to consider the price of AfCVX while integrating with the lending borrowing protocol.

Technical Details

AfCvx.sol#L93-L103

Impact

Informational.

Recommendation

It doesn’t impact the protocol but is highly risky if used in a lending and borrowing market.

Developer Response

acknowledged.

11. Informational - Upgrade dependencies

The Solady contracts dependency is version [0.0.180](, which is outdated. Consider updating to a newer version.

Technical Details

The new version of the Solady library is v0.0.192.

Impact

Informational.

Recommendation

Upgrade Soliday dependency to a newer version.

Developer Response

Upgraded solady to v.0.0194

Final remarks

The afCVX ERC4626 vaults enable the users to earn a yield on the deposited CVX by funneling funds to the Convex rewards pool and CLever. Additionally, CLever is used to borrow clevCVX and route it to Furnace to get more CVX tokens. The strategy and vault require active management to rebalance funds, harvest rewards, and trigger withdrawals from CLever. Both strategy and vault contracts are upgradeable proxies that enable future changes to the deployed contracts. The Pirex migrator contract provides users with a simple way to transfer uCVX, pxCVX, and upxCVX directly to the asymmetry afCVX vault.