# Aladdin DAO aCVX Security Audit Report
<center>
Copyright © 2023 by Supremacy. All rights reserved.
</center>
## Contents
[TOC]
## Introduction
Given the opportunity to review the design document and related codebase of the Aladdin DAO Concentrator aCVX, we outline in the report our systematic approach to evaluate potential security issues in the smart contract(s) implementation, and provide additional suggestions or recommendations for improvement. Our results show that the given version of smart contracts can be further improved due to the presence of several issues related to either security or performance. This document outlines our audit results.
### About Client
---
Aladdin DAO is a decentralized builder and incubator of cutting edge DeFi protocols. Our current focus is in building new approaches to yield farming optimization and automation within the Curve ecosystem and with our latest product, f(x) Protocol, we are expanding to scalable decentralized stablecoins.
To date Aladdin has built three products: Concentrator, Clever, and f(x) Protocol. Each of these protocols offers powerful new tools for DeFi users and adds new, composable DeFi money legos which will help form the basis of what we believe will be the decentralized future of global finance.
Concentrator is a yield enhancer that boosts yields on Convex vaults by concentrating all rewards into auto-compounding top-tier tokens like aCRV (cvxCRV), aFXS (cvxFXS), aCVX (CVX), and asdCRV (sdCRV).
| Item | Description |
| --------- | --------------------------------- |
| Client | Aladdin DAO |
| Project | Concentrator aCVX |
| Website | https://concentrator.aladdin.club |
| Type | Smart Contract |
| Languages | Solidity |
| Platform | EVM-compatible |
### Audit Scope
---
In the following, we show the Git repository of reviewed file and the commit hash used in this security audit:
- https://github.com/AladdinDAO/aladdin-v3-contracts/tree/main/contracts
- Commit Hash: 2ebd6bfbabd5d9ddd2e3ddc9d1bb6fba4316f10e
Below are the files in scope for this security audit and their corresponding SHA256 hashes.
| Filename | SHA256 |
| ----------------------------------------------------------- | ---------------------------------------------------------------- |
| ./concentrator/cvx/CvxCompounder.sol | fe116bffafe0535d85a292db96d88849926177725dda361f9f19a6c5ab4be0da |
| ./concentrator/cvx/CvxStakingStrategy.sol | 709cf34b078c9b9fc5588bea598a6d0671d5518301ddcf363565c82de1c2e8da |
| ./common/rewards/distributor/LinearRewardDistributor.sol | d31db66aae8f9b5c0772b3159ae4fecdcbac1e420c6976afc6a3b2995fbd9840 |
| ./concentrator/ConcentratorCompounderBase.sol | ff7d6f0e92b270d8aaa3cab78716f4fe171cac91d32d12d566639e977fc98265 |
| ./concentrator/strategies/AutoCompoundingStrategyBaseV2.sol | 024a59b4d6e3d46c170be4dae1387eeed5c1ec1af04a3a8d48b6a4714685c96b |
| ./concentrator/strategies/ConcentratorStrategyBase.sol | 03e3399efd6bacb7254f203001952c66147e67560921de9457e97839aa41756e |
| ./concentrator/strategies/ConcentratorStrategyBaseV2.sol | 7dd11b1a278f2ce7b2ea1a34ac60b998c5e1e4897ea5756104eaca9a0529304f |
### Changelogs
---
| Version | Date | Description |
|:-------:|:---------------:|:--------------------:|
| 0.1 | December 18, 2023 | Initial Draft |
### About Us
---
[Supremacy](https://Supremacy.team) is a leading blockchain security agency, composed of industry hackers and academic researchers, provide top-notch security solutions through our technology precipitation and innovative research.
We are reachable at Twitter (https://twitter.com/SupremacyHQ), or Email ([contact@supremacy.email](mailto:contact@supremacy.email)).
#### Terminology
---
For the purpose of this assessment, we adopt the following terminology. To classify the severity of our findings, we determine the likelihood and impact (according to the CVSS risk rating methodology).
* Likelihood represents the likelihood of a finding to be triggered or exploited in practice
* Impact specifies the technical and business-related consequences of a finding
* Severity is derived based on the likelihood and the impact
We categorize the findings into four distinct categories, depending on their severity. These severities are derived from the likelihood and the impact using the following table, following a standard risk assessment procedure.
As seen in the table above, findings that have both a high likelihood and a high impact are classified as critical. Intuitively, such findings are likely to be triggered and cause significant disruption. Overall, the severity correlates with the associated risk. However, every finding's risk should always be closely checked, regardless of severity.
## Findings
The table below summarizes the findings of the audit, including status and severity details.
| ID | Severity | Description | Status |
|:---:|:-------------:|:--------------------------------------------:|:--------------:|
| 1 | Medium | `Centralization risk` | `Undetermined` |
| 2 | Medium | `The potential freezing of funds` | `Undetermined` |
| 3 | Low | `Non-compliance with EIP-4626 specification` | `Undetermined` |
| 4 | Low | `The potential denial-of-service` | `Undetermined` |
| 5 | Informational | `Lack of pause feature` | `Undetermined` |
| 6 | Informational | `Lack of address validation` | `Undetermined` |
| 7 | Informational | `Lack of event records` | `Undetermined` |
| 8 | Informational | `Lack of comments` | `Undetermined` |
### Medium
---
1. Centralization risk **[Medium]**
• **Severity**: Medium      • **Likelihood**: Low      • **Impact**: High
• **Status**: Undetermined
**Description**: In the Concentrator aCVX, `migrateStrategy()` is used to migrate of strategies pool, and its main logic is to transfer assets from `oldStrategy` to `newStrategy`. However, the access control for this function is based on the `onlyRole(DEFAULT_ADMIN_ROLE)` modifier implementation. But, the `DEFAULT_ADMIN_ROLE` privileged account is held by the deployer, which would normally be an EOA account.
Our analysis shows that privileged accounts need to be scrutinized. In the following, we will examine privileged accounts and the associated privileged access in the current contract.
Note that if the privileged owner account is a plain EOA, this may be worrisome and pose counter-party risk to the protocol users. A multi-sig account could greatly alleviate this concern, though it is still far from perfect. Specifically, a better approach is to eliminate the administration key concern by transferring the role to a community-governed DAO. In the meantime, a timelock-based mechanism can also be considered as mitigation.
```solidity=276
/************************
* Restricted Functions *
************************/
/// @inheritdoc IConcentratorCompounder
function migrateStrategy(address _newStrategy) external onlyRole(DEFAULT_ADMIN_ROLE) {
if (_newStrategy == address(0)) revert StrategyIsZero();
// This is the actual assets deposited into strategy.
(uint256 _distributable, uint256 _undistributed) = pendingRewards();
uint256 _totalAssets = totalAssets + _distributable + _undistributed + rewardData.queued;
address _oldStrategy = strategy;
strategy = _newStrategy;
IConcentratorStrategy(_oldStrategy).prepareMigrate(_newStrategy);
IConcentratorStrategy(_oldStrategy).withdraw(_newStrategy, _totalAssets);
IConcentratorStrategy(_oldStrategy).finishMigrate(_newStrategy);
IConcentratorStrategy(_newStrategy).deposit(address(this), _totalAssets);
emit Migrate(_oldStrategy, _newStrategy);
}
```
<center> ConcentratorCompounderBase.sol </center>
<br>
```solidity=23
function initialize(
string memory _name,
string memory _symbol,
address _treasury,
address _harvester,
address _converter,
address _strategy
) external initializer {
__AccessControl_init(); // from AccessControlUpgradeable
__ReentrancyGuard_init(); // from ReentrancyGuardUpgradeable
__ERC20_init(_name, _symbol); // from ERC20Upgradeable
__ERC20Permit_init(_name); // from ERC20PermitUpgradeable
__ConcentratorBaseV2_init(_treasury, _harvester, _converter); // from ConcentratorBaseV2
__LinearRewardDistributor_init(CVX); // from LinearRewardDistributor
__ConcentratorCompounderBase_init(_strategy); // from ConcentratorCompounderBase
// access control
_grantRole(DEFAULT_ADMIN_ROLE, _msgSender());
}
```
<center> CvxCompounder.sol </center>
<br>
**Recommendation**: Add a new address parameter to the `initialize()` function to make it a `DEFAULT_ADMIN_ROLE` privileged account.
Initially onboarding could can use multisign wallets or timelocks to initially mitigate centralization risks, but as a long-running protocol, we recommend eventually transfer the privileged account to the intended DAO-like governance contract. All changed to privileged operations may need to be mediated with necessary timelocks.
Eventually, activate the normal on-chain community-based governance life-cycle and ensure the intended trustless nature and high-quality distributed governance.
---
2. The potential freezing of funds **[Medium]**
• **Severity**: Medium      • **Likelihood**: Medium      • **Impact**: Medium
• **Status**: Undetermined
**Description**: `_sweepToken()` is used to sweep specified assets within a contract, in the current scenario `_transferToken()` takes into account the possibility of an native token, but its `#L152` code's way of fetching the balance of an asset is not compatible with native token. As a result, the desired `_sweepToken()` will always fail to perform as expected if native token are transferred into the contract in the future.
In this way, a denial-of-service will be created.
```solidity=145
/// @dev Internal function to sweep tokens from this contract.
///
/// @param _tokens The list of tokens to sweep.
function _sweepToken(address[] memory _tokens) internal {
address _stash = stash;
for (uint256 i = 0; i < _tokens.length; i++) {
address _token = _tokens[i];
uint256 _balance = IERC20(_token).balanceOf(address(this));
if (_balance > 0) {
_transferToken(_token, _stash, _balance);
}
}
}
```
<center> ConcentratorStrategyBase.sol </center>
<br>
```solidity=159
/// @dev Internal function to transfer ETH or ERC20 tokens to some `_receiver`.
///
/// @param _token The address of token to transfer, user `_token=address(0)` if transfer ETH.
/// @param _receiver The address of token receiver.
/// @param _amount The amount of token to transfer.
function _transferToken(
address _token,
address _receiver,
uint256 _amount
) internal {
if (_token == address(0)) {
Address.sendValue(payable(_receiver), _amount);
} else {
IERC20(_token).safeTransfer(_receiver, _amount);
}
}
```
<center> ConcentratorStrategyBase.sol </center>
<br>
**Recommendation**: Add a new branch so that it correctly gets the contract's current native token balance.
### Low
---
3. Non-compliance with EIP-4626 specification **[Low]**
• **Severity**: Medium      • **Likelihood**: Medium      • **Impact**: Medium
• **Status**: Undetermined
**Description**: Contracts that integrate with the `ConcentratorCompounderBase` may wrongly assume that the functions are EIP-4626 specification, which it might cause integration problems in the future, that can lead to a wide range of issues for both parties, including loss of funds.
```solidity=86
/// @inheritdoc IERC4626Upgradeable
function maxDeposit(address) external pure override returns (uint256) {
return type(uint256).max;
}
```
<center> ConcentratorStrategyBase.sol </center>
<br>
```solidity=96
/// @inheritdoc IERC4626Upgradeable
function maxMint(address) external pure override returns (uint256) {
return type(uint256).max;
}
```
<center> ConcentratorStrategyBase.sol </center>
<br>
```solidity=106
/// @inheritdoc IERC4626Upgradeable
function maxWithdraw(address) external pure override returns (uint256) {
return type(uint256).max;
}
```
<center> ConcentratorStrategyBase.sol </center>
<br>
```solidity=126
/// @inheritdoc IERC4626Upgradeable
function maxRedeem(address) external pure override returns (uint256) {
return type(uint256).max;
}
```
<center> ConcentratorStrategyBase.sol </center>
<br>
EIP-4626 specification defines the withdraw function:
```
Maximum amount of the underlying asset that can be withdrawn from the owner balance in the Vault, through a withdraw call.
MUST return the maximum amount of assets that could be transferred from owner through withdraw and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary).
MUST factor in both global and user-specific limits, like if withdrawals are entirely disabled (even temporarily) it MUST return 0.
MUST NOT revert.
```
<br>
**Recommendation**: The related functions should be modified to meet the specifications of EIP-4626.
---
4. The potential denial-of-service **[Low]**
• **Severity**: Low      • **Likelihood**: Low      • **Impact**: Medium
• **Status**: Undetermined
**Description**: Typically, the contract’s owner is the account that deploys the contract. As a result, the owner is able to perform certain privileged activities.
The Openzeppelin’s Ownable used in `ConcentratorStrategyBase` contract implements `renounceOwnership()`. This can represent a certain risk if the ownership is renounced for any other reason than by design. Renouncing ownership will leave the contract without an owner, thereby removing any functionality that is only available to the owner.
```solidity=13
// solhint-disable func-name-mixedcase
// solhint-disable no-empty-blocks
abstract contract ConcentratorStrategyBase is Initializable, Ownable2Step, IConcentratorStrategy {
```
<center> ConcentratorStrategyBase.sol </center>
<br>
```solidity=8
/**
* @dev Contract module which provides access control mechanism, where
* there is an account (an owner) that can be granted exclusive access to
* specific functions.
*
* By default, the owner account will be the one that deploys the contract. This
* can later be changed with {transferOwnership} and {acceptOwnership}.
*
* This module is used through inheritance. It will make available all functions
* from parent (Ownable).
*/
abstract contract Ownable2Step is Ownable {
```
<center> Ownable2Step.sol </center>
<br>
```solidity=8
/**
* @dev Leaves the contract without owner. It will not be possible to call
* `onlyOwner` functions. Can only be called by the current owner.
*
* NOTE: Renouncing ownership will leave the contract without an owner,
* thereby disabling any functionality that is only available to the owner.
*/
function renounceOwnership() public virtual onlyOwner {
_transferOwnership(address(0));
}
```
<center> Ownable.sol </center>
<br>
**Recommendation**: Either reimplement the function to disable it or to clearly specify if it is part of the contract design.
### Informational
---
5. Lack of pause feature **[Informational]**
**Status**: Undetermined
**Description**: The pause feature is a key risk control component of smart contracts, and it is necessary to add it. In this way, once malicious behavior occurs against the smart contract, access restrictions on key features can be completed in a timely manner. At the same time, there are already relatively mature libraries on the market, and it is recommended to use pausable utils to enable this feature.
---
6. Lack of address validation **[Informational]**
**Status**: Undetermined
**Description**: The `initialize()` function lacks zero address checking for the address parameters of multiple external contracts, and the same is true for the corresponding function implementation.
```solidity=23
function initialize(
string memory _name,
string memory _symbol,
address _treasury,
address _harvester,
address _converter,
address _strategy
) external initializer {
__AccessControl_init(); // from AccessControlUpgradeable
__ReentrancyGuard_init(); // from ReentrancyGuardUpgradeable
__ERC20_init(_name, _symbol); // from ERC20Upgradeable
__ERC20Permit_init(_name); // from ERC20PermitUpgradeable
__ConcentratorBaseV2_init(_treasury, _harvester, _converter); // from ConcentratorBaseV2
__LinearRewardDistributor_init(CVX); // from LinearRewardDistributor
__ConcentratorCompounderBase_init(_strategy); // from ConcentratorCompounderBase
// access control
_grantRole(DEFAULT_ADMIN_ROLE, _msgSender());
}
```
<center> CvxCompounder.sol </center>
<br>
```solidity=55
// solhint-disable-next-line func-name-mixedcase
function __LinearRewardDistributor_init(address _rewardToken) internal onlyInitializing {
rewardToken = _rewardToken;
}
```
<center> LinearRewardDistributor.sol </center>
<br>
```solidity=51
/***************
* Constructor *
***************/
function __ConcentratorCompounderBase_init(address _strategy) internal onlyInitializing {
strategy = _strategy;
}
```
<center> ConcentratorCompounderBase.sol </center>
<br>
**Recommendation**: Add zero address validation.
---
7. Lack of event records **[Informational]**
**Status**: Undetermined
**Description**: Many smart contract(s) generally lack event records. However, events are important because off-chain monitoring tools rely on them to index important state changes to the smart contract(s).
**Recommendation**: Always ensure that all functions that trigger state changes have event logging capabilities.
---
8. Lack of comments **[Informational]**
**Status**: Undetermined
**Description**: Throughout the codebase there are numerous functions missing or lacking documentation. This hinders reviewers' understanding of the code's intention, which is fundamental to correctly assess not only security, but also correctness. Additionally, comments improve readability and ease maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, the values returned and the events emitted.
Consider thoroughly documenting all functions (and their parameters) that are part of the smart contracts' public interfaces. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing comments, consider following the Ethereum Natural Specification Format (NatSpec).
## Disclaimer
This audit report does not constitute investment advice or a personal recommendation. It does not consider, and should not be interpreted as considering or having any bearing on, the potential economics of a token, token sale or any other product, service or other asset. Any entity should not rely on this report in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. This audit report is not an endorsement of any particular project or team, and the report does not guarantee the security of any particular project. This audit does not give any warranties on discovering all security issues of the smart contracts, i.e., the evaluation result does not guarantee the nonexistence of any further findings of security issues, also cannot make guarantees about any additional code added to the assessed project after the audit version. As one audit-based assessment cannot be considered comprehensive, we always recommend proceeding with independent audits and a public bug bounty program to ensure the security of smart contract(s). Unless explicitly specified, the security of the language itself (e.g., the solidity language), the underlying compiling toolchain and the computing infrastructure are out of the scope.
Sign in with Wallet
Connect another wallet