# VaultCraft Security Review // 16-02-2024

### Scope

| Type | File | Logic Contracts | Interfaces | Lines | nLines | nSLOC | Comment Lines | Complex. Score | Capabilities |
| ---- | ------ | --------------- | ---------- | ----- | ------ | ----- | ------------- | -------------- | ------------ |
| 📝 | src/vaults/MultiStrategyVault.sol | 1 | **** | 891 | 832 | 503 | 170 | 426 | **<abbr title='Uses Hash-Functions'>🧮</abbr><abbr title='Handles Signatures: ecrecover'>🔖</abbr><abbr title='Unchecked Blocks'>Σ</abbr>** |
| 📝 | src/vault/adapter/aura/AuraCompounder.sol | 1 | **** | 284 | 254 | 160 | 50 | 149 | **<abbr title='TryCatch Blocks'>♻️</abbr>** |
| 📝 | src/vault/adapter/curve/gauge/other/CurveGaugeSingleAssetCompounder.sol | 1 | **** | 217 | 192 | 118 | 36 | 131 | **<abbr title='TryCatch Blocks'>♻️</abbr>** |
| 📝 | src/vault/adapter/curve/gauge/mainnet/CurveGaugeCompounder.sol | 1 | **** | 230 | 201 | 122 | 36 | 140 | **<abbr title='TryCatch Blocks'>♻️</abbr>** |
| 📝 | src/vault/adapter/convex/ConvexCompounder.sol | 1 | **** | 340 | 303 | 178 | 68 | 154 | **<abbr title='TryCatch Blocks'>♻️</abbr>** |
| 📝 | **Totals** | **5** | **** | **1962** | **1782** | **1081** | **360** | **1000** | **<abbr title='Uses Hash-Functions'>🧮</abbr><abbr title='Handles Signatures: ecrecover'>🔖</abbr><abbr title='TryCatch Blocks'>♻️</abbr><abbr title='Unchecked Blocks'>Σ</abbr>** |

### Commit hash and branch
[3c75d5c4c078a5bff90cad5bd56a9de3fb3348f5](https://github.com/Popcorn-Limited/contracts/commit/3c75d5c4c078a5bff90cad5bd56a9de3fb3348f5))

## [H-1] Total assets can be manipulated in CurveGaugeSingleAssetCompounder

### Summary
`totalAssets()` calculation inside the template is using `calc_withdraw_one_coin` function from curve pool which is easy to be manipulated.

### Vulnerability Details
There are two possible exploits:

1. **Inflate the pool such that the underlying is cheaper:**
Assume the underlying curve pool has 10% U (underlying token) and 90% O (other token).
Total supply = 10
Total assets = 10
Normally, the shares to be minted for a user depositing 10 tokens would be calculated as:
10 * (10 / 10) = 10
Just before the deposit, the attacker inflates the pool by executing a flash swap or flash liquidity action. Now, the balances in the curve pool are 90% U and 10% O, meaning that the underlying token is cheaper in the pool. Hence, the total assets will be higher. Assume the total assets are now calculated as "100"; thus, the shares to be minted will be:
scss
10 * (100 / 10) = 100
shares. Then, the attacker immediately sells back the flash swap (the other token bought by the inflation attack previously executed) and recovers some portion of the attack cost. As a result, the attacker now has 100 shares, although the attacker actually deposited only 10 tokens into the adapter.

2. **Inflate the pool such that the underlying token is expensive:**
Using the same numbers and scenario as above, let's assume that the pool is 50-50% and there are 50 total assets and 50 total supply.
Now, the attacker has 10 shares and just before redeeming, they execute a flash swap, adding a large amount of another token to the pool. This action increases the price of the underlying token in the pool, resulting in overpriced total assets. Assume the total assets are now counted as "100"; hence, the underlying tokens that the attacker will claim will be calculated as:
scss
10 * (100 / 50) = 20
underlying tokens. After the redemption, the attacker will swap back and normalize the pool as much as possible.

Both of these attack cases can occur depending on the pool's liquidity and the adapter's total assets. There will be mathematically profitable occasions in which the above scenarios can be executed by anyone with no cost, thanks to flash loans.

### Links to code
https://github.com/Popcorn-Limited/contracts/blob/06edf653ebcf1772aa0ed91c6e3a58208f614d70/src/vault/adapter/curve/gauge/other/CurveGaugeSingleAssetCompounder.sol#L88-L94
### Impact
Draining of the vault or minting unfairly inflated shares

**Some examples of the attack irl and other findings similiar to this:**
https://medium.com/haechi-audit/harvest-finance-economic-attack-deep-dive-part-1-420bc5d5ac0c
https://twitter.com/bantg/status/1320738907869372418
https://solodit.xyz/issues/calc_withdraw_one_coin-is-vulnerable-to-manipulation-trailofbits-frax-solidity-pdf
### Recommendation
How to price balancer LP tokens:
https://github.com/Revest-Finance/ResonateContracts/blob/a114ea0b3f462008099076397345359114cfbcd2/hardhat/contracts/oracles/adapters/balancer/BalancerV2WeightedPoolPriceOracle.sol#L102-L142

How to price Curve LP tokens:
https://news.curve.fi/chainlink-oracles-and-curve-pools/

In general how to price stable swap LP tokens:
https://blog.chain.link/using-chainlink-oracles-to-securely-utilize-curve-lp-pools/

Detailed Python implementation of how to calculate stable swap lp tokens:
https://github.com/ConicFinance/POC_curve_lp_token_pricing/blob/main/conic/curve_token_pricing/token_pricing.py

- [ ] Fixed
- [ ] Will not fix

## [H-2] CurveGaugeSingleAssetCompounder deposits can be frontrunned
### Summary
When a user or a vault deposits into the CurveGaugeSingleAssetCompounder adapter, the adapter deploys the underlying asset to Curve, effectively performing a single-sided liquidity addition to the pool. The adapter executes this action with a minOut parameter set to "0," which is highly susceptible to sandwich attacks by any MEV bot.

### Vulnerability Details
Adding single-sided liquidity to a Curve pool essentially involves a swap followed by balanced liquidity addition. Since it's "like a swap", adding liquidity in a way that deviates from the pool's current balances will always result in either a loss or a bonus in the LP token amount received by the user. If the minOut is not correctly, the swapping part can be easily frontrunned by a MEV bot.

function _protocolDeposit(uint256 amount, uint256) internal override {
        uint256[] memory amounts = new uint256[](nCoins);
        amounts[uint256(uint128(indexIn))] = amount;
        // dev: can be frontrunned ! 
        -> ICurveLp(lpToken).add_liquidity(amounts, 0);
        gauge.deposit(IERC20(lpToken).balanceOf(address(this)));
    }

### Links to code
https://github.com/Popcorn-Limited/contracts/blob/06edf653ebcf1772aa0ed91c6e3a58208f614d70/src/vault/adapter/curve/gauge/other/CurveGaugeSingleAssetCompounder.sol#L105-L111
### Impact
Since anyone can deposit any amount to the adapter, this poses a high severity risk. If someone were to deposit a large amount to the adapter, the sandwiching that occurs when adding liquidity would be quite significant. This would result in an immediate loss for both the adapter and the deposited user.
### Recommendation
soon

- [ ] Fixed
- [ ] Will not fix

## [H-3] Withdrawals in MultiStrategyVault does not transfer the assets if the amount is available in the vault

### Summary
When the requested amount is available in the vault the vault does not transfers the amount back to user but burns the full shares.

### Vulnerability Details
`_withdrawStrategyFunds` function does not transfers the assets if the "amount <= float" and ends the withdrawal flow.

function _withdrawStrategyFunds(uint256 amount, address receiver) internal {
        // caching
        IERC20 asset_ = IERC20(asset());

        // Get the Vault's floating balance.
        uint256 float = asset_.balanceOf(address(this));

        // If the amount is greater than the float, withdraw from strategies.
        if (amount > float) {
            .
            .
        }
}

### Links to code
https://github.com/Popcorn-Limited/contracts/blob/06edf653ebcf1772aa0ed91c6e3a58208f614d70/src/vaults/MultiStrategyVault.sol#L293-L320
### Impact
Users shares will be burned but no assets will be send back in exchange
### Recommendation
If the amount is available in the vault, transfer it to the recipient

- [x] [Fixed](https://github.com/Popcorn-Limited/contracts/commit/f988a80d068484555a7693f94fdcec66ebbba28f)
- [ ] Will not fix

## [M-1] Selling rewards for underlying can be frontrunned

### Summary
When both Convex and Curve compounders claim rewards and sell them for more underlying assets, both contracts use a "minOut" set to "0" in the CurveRouter contract. Making a swap with "minOut" = 0 is easily sandwiched if the amount to be sold increases. Frontrunners will cause the yield to earn less.

### Vulnerability Details
When rewards are sold the internal `_exchange` function is used which is as follows:

function _exchange(
        ICurveRouter router,
        CurveSwap memory swap,
        uint256 amount
    ) internal {
        if (amount == 0) revert ZeroAmount();
        router.exchange(swap.route, swap.swapParams, amount, 0, swap.pools);
    }

As we can notice, the third parameter, which is the minimum amount out, is set to 0 by default. Compounding with a "0" minOut will have a continuous MEV threat.

### Links to code
https://github.com/Popcorn-Limited/contracts/blob/06edf653ebcf1772aa0ed91c6e3a58208f614d70/src/vault/adapter/convex/ConvexCompounder.sol#L237
https://github.com/Popcorn-Limited/contracts/blob/06edf653ebcf1772aa0ed91c6e3a58208f614d70/src/vault/adapter/curve/gauge/mainnet/CurveGaugeCompounder.sol#L207
https://github.com/Popcorn-Limited/contracts/blob/06edf653ebcf1772aa0ed91c6e3a58208f614d70/src/vault/adapter/curve/gauge/other/CurveGaugeSingleAssetCompounder.sol#L196
### Impact
Lesser yield for the users
### Recommendation
In general, this is a big concern for yield aggregators, and the fix for it is not as straightforward as we think. I'd suggest introducing "maxTradeAmounts" for every reward token and getting rid of "minTradeAmounts" that are currently used by the code. When rewards are too small, swapping is not a problem; the actual problem arises when the rewards are too substantial. If there is a "maxTradeAmount" for each token, which is set by the owner and is a well-thought-out number such that selling that much amount will not be a MEV threat, then we can guarantee that the harvest will not be sandwiched..

Also, by solving this issue you will be automatically solving the other frontrunnable part of the code which is
`ICurveLp(asset_).add_liquidity(amounts, 0);`

since the amount sold was capped we will believe that the LP is liquid enough and the amount to be added liquidity is not a big proportion such that there will be no frontrunning the curve add LP tx threat.

for (uint256 i = 0; i < rewLen; i++) {
    address rewardToken = _rewardTokens[i];
    amount = IERC20(rewardToken).balanceOf(address(this));
    // dev: only sell the maximum viable amount that we assure 
    // is not sandwichable. If the reward to be sold is too much then 
    // the next harvest can cover the remaining parts of the rewards.
    -> if (amount > maxTradeAmounts[i]) {
        _exchange(router_, swaps[rewardToken], maxTradeAmounts[i]);
    }
}

- [ ] Fixed
- [x] Will not fix

## [M-2] Reward token might also be an underlying token
### Summary
Convex compounder can have different reward tokens at any time. If the new reward token is an underlying token then selling the token will not be possible in curve router.

### Vulnerability Details
If the reward token is the underlying "depositAsset" then curve router will not be able to swap the asset since the input and output tokens are the same address.

### Links to code
https://github.com/Popcorn-Limited/contracts/blob/06edf653ebcf1772aa0ed91c6e3a58208f614d70/src/vault/adapter/convex/ConvexCompounder.sol#L198-L238
### Impact
Reward token will not be able to compounded back to the strategy
### Recommendation
If the reward token is the deposit asset then don't execute the exchange and keep looping.

function harvest() public override takeFees {
        if ((lastHarvest + harvestCooldown) < block.timestamp) {
            claim();

            ICurveRouter router_ = curveRouter;
            uint256 amount;
            uint256 rewLen = _rewardTokens.length;
            for (uint256 i = 0; i < rewLen; i++) {
                // dev: keep looping if the reward token is deposit asset
                -> if (rewardToken == depositAsset) continue;
                address rewardToken = _rewardTokens[i];
                amount = IERC20(rewardToken).balanceOf(address(this));
                if (amount > minTradeAmounts[i]) {
                    _exchange(router_, swaps[rewardToken], amount);
                }
            }

            amount = IERC20(depositAsset).balanceOf(address(this));
            if (amount > 0) {
                uint256[] memory amounts = new uint256[](nCoins);
                amounts[uint256(uint128(indexIn))] = amount;

                address asset_ = asset();

                ICurveLp(asset_).add_liquidity(amounts, 0);

                _protocolDeposit(IERC20(asset_).balanceOf(address(this)), 0);
            }

            lastHarvest = block.timestamp;
        }

        emit Harvested();
    }

- [ ] Fixed
- [x] Will not fix

## [M-3] Fees are taken incorrectly

### Summary
When the management and performance fee is charged, contract uses hardcoded "1e18" value to convert from shares to asset. However, most of the time this won't be correct since the decimals for shares are:
underlyingTokenDecimals + decimalsOffset.

### Vulnerability Details
This how the accrued performance fee is calculated:

function accruedPerformanceFee() public view returns (uint256) {
        uint256 highWaterMark_ = highWaterMark;
        uint256 shareValue = convertToAssets(1e18);
        uint256 performanceFee = fees.performance;

        return
            performanceFee > 0 && shareValue > highWaterMark_
                ? performanceFee.mulDiv(
                    (shareValue - highWaterMark_) * totalSupply(),
                    1e36,
                    Math.Rounding.Floor
                )
                : 0;
    }

as we can observe, the "shareValue" is calculated by converting "1e18" shares to assets. However, if the underlying token is say, 18 decimals, then the vaults decimals will be 27. Hence, the `convertToAssets()` value should be expecting a value with 27 decimals, not 18.

Same in here:
https://github.com/Popcorn-Limited/contracts/blob/1b19ecda4371d90ccc7c8c098ac2f921b25b6f28/src/vaults/MultiStrategyVault.sol#L571

### Links to code
https://github.com/Popcorn-Limited/contracts/blob/1b19ecda4371d90ccc7c8c098ac2f921b25b6f28/src/vaults/MultiStrategyVault.sol#L535-L548

https://github.com/Popcorn-Limited/contracts/blob/1b19ecda4371d90ccc7c8c098ac2f921b25b6f28/src/vaults/MultiStrategyVault.sol#L568-L591
### Impact
Miscalculation of fees
### Recommendation
Use the correct decimal precised value
For example:

uint256 shareValue = convertToAssets(10 ** decimals());

- [x] [Fixed](https://github.com/Popcorn-Limited/contracts/commit/167fae7aeb79ab490984257033cf9d964f96ef9b)
- [ ] Will not fix

## [M-4] `highWaterMark` is initialized with a wrong value

### Summary
The performance fee is calculated as the difference between the new share price and the previous share price before the update. Since there is no previous share price for the very first calculation, the `highWaterMark` value is utilized. However, this value should represent 1 unit of vault shares, but the code in scope uses a hardcoded value of 1e9, which is significantly undervalued assuming the underlying token has 18 decimals.

### Vulnerability Details
When the first fee is charged, the `highWaterMark` is set to 1e9. Hence, the below snippet of how performance fee is calculated will be too much because the `shareValue` will be way too much than the `highWaterMark`

performanceFee > 0 && shareValue > highWaterMark_
                ? performanceFee.mulDiv(
                    (shareValue - highWaterMark_) * totalSupply(),
                    1e36,
                    Math.Rounding.Floor
                )
                : 0;

Since this is the first fee taken, the `highWaterMark` should be the value 1 in vault tokens decimals.
### Links to code
https://github.com/Popcorn-Limited/contracts/blob/1b19ecda4371d90ccc7c8c098ac2f921b25b6f28/src/vaults/MultiStrategyVault.sol#L109

https://github.com/Popcorn-Limited/contracts/blob/1b19ecda4371d90ccc7c8c098ac2f921b25b6f28/src/vaults/MultiStrategyVault.sol#L540-L548
### Impact
The first fee taken will be very high
### Recommendation
Initialized the `highWaterMark` as 1 units in vaults decimals

highWaterMark = 10 ** decimals();

- [x] [Fixed](https://github.com/Popcorn-Limited/contracts/commit/167fae7aeb79ab490984257033cf9d964f96ef9b)
- [ ] Will not fix

## [M-5] Management fee can be taken right after deployment

### Summary
When the vault is deployed, the management fee can be taken right away.

### Vulnerability Details
When the vault is deployed, the feesUpdated is default to "0". If there is a management fee, then calling `takeManagementAndPerformanceFees` will immediately mint tokens to the fee recipient.

function accruedManagementFee() public view returns (uint256) {
        uint256 managementFee = fees.management;
        return
            managementFee > 0
                ? managementFee.mulDiv(
                    totalAssets() * (block.timestamp - feesUpdatedAt),
                    SECONDS_PER_YEAR,
                    Math.Rounding.Floor
                ) / 1e18
                : 0;
    }

From above snippet, someone can donate some assets to vault such that the `totalAssets()` will be non zero. Then, since the `feesUpdated` is "0" the fee will be succesfully taken although there are no assets.

### Links to code
https://github.com/Popcorn-Limited/contracts/blob/1b19ecda4371d90ccc7c8c098ac2f921b25b6f28/src/vaults/MultiStrategyVault.sol#L517-L527
### Impact
Unfair initialization of vault and potentailly opens up new exploits.
### Recommendation
soon..
- [x] [Fixed](https://github.com/Popcorn-Limited/contracts/commit/167fae7aeb79ab490984257033cf9d964f96ef9b)
- [ ] Will not fix

## [M-6] Uneven accrual of performance/management fees

### Summary
In order to fairly charge fees, all the actions that are changing the total supply and total assets should be accruing fees before changing state.
### Vulnerability Details

Since `totalAssets()` and `totalSupply()` deposits everytime withdrawals/deposits and yield compounding occurs, the management/performance fee can be taken lesser than it supposed to.

function accruedManagementFee() public view returns (uint256) {
        uint256 managementFee = fees.management;
        return
            managementFee > 0
                ? managementFee.mulDiv(
                    totalAssets() * (block.timestamp - feesUpdatedAt),
                    SECONDS_PER_YEAR,
                    Math.Rounding.Floor
                ) / 1e18
                : 0;
    }

function accruedPerformanceFee() public view returns (uint256) {
        uint256 highWaterMark_ = highWaterMark;
        uint256 shareValue = convertToAssets(1e18);
        uint256 performanceFee = fees.performance;

        return
            performanceFee > 0 && shareValue > highWaterMark_
                ? performanceFee.mulDiv(
                    (shareValue - highWaterMark_) * totalSupply(),
                    1e36,
                    Math.Rounding.Floor
                )
                : 0;
    }

### Links to code
https://github.com/Popcorn-Limited/contracts/blob/1b19ecda4371d90ccc7c8c098ac2f921b25b6f28/src/vaults/MultiStrategyVault.sol#L517-L548
### Impact
Unfair fee accrual for the vault owner
### Recommendation
Add the modifier to withdraw/deposit/harvest functions.
- [x] [Fixed](https://github.com/Popcorn-Limited/contracts/commit/167fae7aeb79ab490984257033cf9d964f96ef9b)
- [ ] Will not fix
## Low Severity Issues and Best Practices
* Curve and Convex compounders are not adaptable to every curve pool there are several places where the adaptability issue arises, I will list all here in this issue

1. A serious assumption is that `asset()` is the LP token of the curve and also the pool itself. However, in some curve pool implementations the LP token and the pool are different addresses.
For example:
https://etherscan.io/address/0xdc24316b9ae028f1497c275eb9192a3ea0f67022#readContract
Especially the convex compounder does the following to derive the LP token from the booster contract:

    .
    .
    (address _asset, , , address _convexRewards, , ) = IConvexBooster(.
                registry
            ).poolInfo(_pid);
    .
    .
    if (_asset != asset()) revert AssetMismatch();

- [x] [Fixed](https://github.com/Popcorn-Limited/contracts/commit/02f3e6c60111621a137e322542200427de3f5f29)
- [ ] Will not fix

`_asset` value will always be the Curve Pool's LP token and if the LP token is not the pool too, then the initialization will never go through.

2. Deposit asset address(0) check is used to see if the underlying asset is ETH or not. However, ETH is hardcoded as:"0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE" on majority of the pools.
For example:
https://etherscan.io/address/0xdc24316b9ae028f1497c275eb9192a3ea0f67022#readContract

    .
    .
    if (depositAsset != address(0)) IERC20(depositAsset).approve(asset_, 0);
            IERC20(depositAsset_).approve(asset_, type(uint256).max);
    .
    .

- [ ] Fixed
- [x] Will not fix

3. Some curve pools uses native ETH as underlying asset. In such pools, the "depositAsset" can be picked the ETH and the addLiquidity tx has to send value to the pool and also be able to receive ETH too. Current code does not covers the handling of native ETH in such cases.
For example:
https://etherscan.io/address/0xdc24316b9ae028f1497c275eb9192a3ea0f67022#readContract

    function harvest() public override takeFees {
        .
        .
        // dev: add_liquidity{value: amounts}(amounts,0);
        -> ICurveLp(asset_).add_liquidity(amounts, 0);
        .
        emit Harvested();
    }

    function _exchange(
            ICurveRouter router,
            CurveSwap memory swap,
            uint256 amount
        ) internal {
            if (amount == 0) revert ZeroAmount();
            // dev: router.exchange{value: amount}(swap.route, swap.swapParams, amount, 0, swap.pools);
            -> router.exchange(swap.route, swap.swapParams, amount, 0, swap.pools);
    }

- [ ] Fixed
- [x] Will not fix

* Previous allowances to the previous router is not revoked
New router is set immediately before revoking the previous routers allowances. If the intend here was to revoke the previous routers allowances then it is not correctly implemented.
https://github.com/Popcorn-Limited/contracts/blob/06edf653ebcf1772aa0ed91c6e3a58208f614d70/src/vault/adapter/convex/ConvexCompounder.sol#L157-L162
- [x] [Fixed](https://github.com/Popcorn-Limited/contracts/commit/02f3e6c60111621a137e322542200427de3f5f29)
- [ ] Will not fix

* MultiStrategyVault contract name is blank and domain seperator is calculated wrong
`name()` is empty because the contract didn't yet constructed the name hence, the contractName uses the blank bytes for the "name"
https://github.com/Popcorn-Limited/contracts/blob/06edf653ebcf1772aa0ed91c6e3a58208f614d70/src/vaults/MultiStrategyVault.sol#L105-L107
**Recommendation:**

    function initialize(
            IERC20 asset_,
            IERC4626[] calldata strategies_,
            VaultFees calldata fees_,
            address feeRecipient_,
            uint256 depositLimit_,
            address owner
        ) external initializer {
            .
            .
            .
            .
            // dev: first set the name and symbol then 
            // set the domain seperator and contractName
            _name = string.concat(
                "VaultCraft ",
                IERC20Metadata(address(asset_)).name(),
                " Vault"
            );
            _symbol = string.concat(
                "vc-",
                IERC20Metadata(address(asset_)).symbol()
            );
            
            contractName = keccak256(
                abi.encodePacked("VaultCraft ", name(), block.timestamp, "Vault")
            );
            
            INITIAL_CHAIN_ID = block.chainid;
            INITIAL_DOMAIN_SEPARATOR = computeDomainSeparator();
        }

- [x] [Fixed](https://github.com/Popcorn-Limited/contracts/commit/02f3e6c60111621a137e322542200427de3f5f29)
- [ ] Will not fix

* These can be deleted since they are the same as the ERC4626 functions
https://github.com/Popcorn-Limited/contracts/blob/1b19ecda4371d90ccc7c8c098ac2f921b25b6f28/src/vaults/MultiStrategyVault.sol#L460-L484
- [x] [Fixed](https://github.com/Popcorn-Limited/contracts/blob/1b19ecda4371d90ccc7c8c098ac2f921b25b6f28/src/vaults/MultiStrategyVault.sol#L460-L484)
- [ ] Will not fix

* Multi strategy vault suggestions:
1. Don't withdraw from the strategy when ordering the withdrawal queue.
2. Don't deposit to strategy[0] as default, instead make a storage pointer that which strategy the funds should be deposited, way easier to manage
3. Add a functionality to add a strategy
4. Add a functionality to remove a strategy
- [x] Fixed
- [ ] Will not fix