Universal Vault

Currently function maxMint returns previewMint on the maxDeposit

function maxMint() public view returns (uint256) {
     return previewMint(maxDeposit());
}

However there is an issue with that as previewMint is meant to accept shares and maxDeposit returns assets:

function previewMint(uint256 _shares) public view returns (uint256) {
     UniversalVaultStorage storage $ = _getUniversalVaultStorage();
      return _convertToAssets(
           _shares, $.oracle.getLatestPrice(address(this)), Math.Rounding.Ceil
      );
}

This would mean that previewMint would convert our assets into assets (thinking they were shares) which would break the whole max assets/shares that anyone is able to deposit.

Depending on the ratio between the two, this can either significantly decrease the cap for which mint can deposit up to or in the more dangerous scenario - significantly increase it.

Change previewMint to previewDeposit within maxMint.

Alongside Team: The issue was resolved in commit 9f0ea52.

Function mint(uint256 _shares, address _receiver, uint256 _minAssets) takes a _minAssets parameter as a form of slippage protection:

if (_minAssets = 0 && assets < _minAssets) {
    revert SlippageError(assets, _minAssets);
}

However, for proper slippage control, mint must revert if minting _shares costs more than a maxAssets of underlying tokens, rather than less than minAssets.

This is because if the shares become more expensive, more assets may be required than the user expected.

Change function mint to use maxAssets instead of a _minAssets parameter, and update the inequality.

Alongside Team: The issue was resolved in commit 7636b28.

Function _checkPriceChange restricts price changes to 20 bps per update interval (at least 1 hour).

Although this may function normally for most strategies, if the manager is utilizing a strategy that handles external trading positions, a much larger price movement can occur in that time period that may not be appropriately delta neutral.

Afterwards, the price change will be exceeded and the oracle updater will be unable to update the price, progress the epochs forward, and process redemptions.

Consider turning MAX_PRICE_CHANGE in a mutable variable that can be updated by the admin.

Alongside Team: The issue was resolved in commit 79d1d417.

Appointing a new vault manager does not automatically transfer funds from the old manager, potentially leaving the new manager without assets to process withdrawals and trapping users' funds.

The new manager would need control of the assets as well as ownership of any external positions.

Ensure all necessary assets are transferred to the new manager.

Alongside Team: Acknowledged, this was already considered.

Although the Universal Vault will be primarily used for universal assets, the Vault was designed to be token-agnostic.

However, low decimal tokens may suffer excessive precision loss (e.g., 2 decimals like GUSD), such that oracle price updates are limited or entirely prevented.

In the case of GUSD, maxDiff would floor to 0 and DoS Oracle/Queue operations: uint256 maxDiff = (lastPrice * MAX_PRICE_CHANGE) / 1e18;

Carefully select which assets will be used with the Vault and document this risk.

Alongside Team: Acknowledged. We are going to use these vaults with uAssets (standard ERC20 tokens with 18 decimals) only. We are also considering USDC in the future, but not confirmed.

Currently there is a minimum withdrawal amount but not a minimum deposit amount. Non-malicious users typically do not deposit a couple wei of assets, and having that symmetrical validation would help ensure a user does not deposit and become stuck instantaneously.

Consider adding a minimum deposit amount of assets, or clearly document this behavior.

Alongside Team: Acknowledged. This behavior is intended, we let users to deposit any amount as long as shares are not zero. Therefore, users have two options, wait until their invest have surpassed the threshold or deposit more uAssets, since the restriction to withdraw was implemented merely to prevent from spamming attacks in the WithdrawalQueue. This might be considered in the future.

maxDeposit is calculated as totalStakedLimit() - totalAssets(), where totalAssets() is calculated with FLOOR rounding. Because totalAssets is used to subtract from totalStakedLimit, but totalAssets is rounded down, this would increase the overall value of the maxDeposit.

Note that behaviour and set the limit accordingly, or roundup stakedAssets specifically within function maxDeposit().

Alongside Team: The issue was resolved in commit e930413.

The vaults should be agnostic and be able to use any tokens, however if used with rebasing tokens the withdraw queue will experience rebases inside of it, as there would be time gaps between the manager calling completeFinalizeWithdraw and all users collecting their withdraws with claimWithdraw.

During those time gaps rebases may occur, which would result in that rebase being bricked inside the contract.

If negative rebases occur the manager may be required to separately send extra tokens in order to allow for all users to withdraw.

It's not recommended to use tokens such as stETH or any other rebasing tokens.

Alongside Team: Acknowledged. We are going to use these vaults with uAssets (standard ERC20 tokens with 18 decimals) only. We are also considering USDC in the future, but not confirmed.

Modifier onlyVaultOwner is defined but never used within the WithdrawalQueue.

Consider removing the extraneous modifier definition.

Alongside Team: The issue was resolved in commit de78e5d.

If a Vault is deployed without being activated in the VaultOracle in-tandem, user deposits will panic underflow when calling getLatestPrice since the epoch will be 0 for the vault:

$.prices[vaultAddr][$.vaults[vaultAddr].epoch - 1];

This may be an unexpected error and a more verbose custom error may be preferred.

Clearly document this or consider adding a more verbose error such as 'VaultNotActiveYet'.

Alongside Team: The issue was resolved in commit 62836ff.

Users of the Universal Vault system must trust a set privileged actors to act in good faith, including but not limited to:

Manager Trust Assumptions:

• Securely manages and protects user deposited assets.
• Uses assets appropriately in yield-generating strategies.
• Approves an allowance to the WithdrawalQueue and returns assets when users request

withdrawals.

• Requests withdraw finalization and completes batches in a timely, proper manner.

Oracle Updater Trust Assumption:

• Accurately prices in each epoch and handle price volatility.
• Updates prices in a timely manner to ensure smooth withdrawal flow and without excess gas usage

on claim.

Vault Owner Trust Assumptions:

• Pauses the Vault when necessary
• Sets parameters such that the Vault operates safely, e.g. enforcing a large enough minWithdraw to

prevent spam and likely malicious withdrawal requests.

Clearly document trust assumptions for privileged actors as well as the specs for the offchain system.

Alongside Team: Acknowledged. Everything mentioned here will be properly documented prior to Vault's launch.

Although event VaultDeployed is defined it is not used which may negatively impact frontends relying on these emitted events.

Emit the event within function deployVault.

Alongside Team: The issue was resolved in commit 12d0f04.

The totalStakedLimit does not account for pending withdrawal assets, which are still held by the manager post-share burn but pre-finalization, allowing new deposits to exceed the effective limit and potentially overcommitting the strategy.

Although this may be the intended behavior by the Vault Owner and Manager, it should be clearly documented.

Clearly document this behavior.

Alongside Team: Acknowledged. As you mentioned, this is intended as we don't consider those pending withdrawals as regular positions since at some point they will be withdrawn but there will be a period in between where those “positions” will remain exposed to loses in the vault. We will documents this behavior more explicitly.

Function __Pausable_init() is not called within the initialize function which goes against best practices to call __{ContractName}_init functions for all directly inherited contracts.

Consider adding __Pausable_init() in the initialize function.

Alongside Team: The issue was resolved in commit 6b12a9a.

• IERC20Metadata is imported within the UniversalVault but never used.
• Math is imported within the Withdrawal Queue but never used.

Consider removing the unused import.

Alongside Team: The issue was resolved in commit 3504d5d.

When the factory initializes a UniversalVault and Withdrawal Queue, it sets the owner of these entities as the owner() of the Factory itself.

However, if the Factory updates its owner via Ownable2Step, it doesn't update the owner for previously initialized Vaults and Withdrawal Queues.

This creates a situation where outdated/incorrect owners exist for entities created via the factory.

Be aware of this scenario and update the owners of vaults and queues accordingly.

Alongside Team: Acknowledged. We can update the owner by calling directly to the vaults we want to change their owner.

Although the contracts are not intended to be fully compliant with ERC-4626, a few minor modifications could bring the vault closer to compliance:

1. Rename the underlyingAsset function to asset().
2. Ensure that the max functions (maxDeposit, maxMint, maxWithdraw, and maxRedeem) return 0

when the contract is paused. Currently, deposit and withdraw operations revert as expected during a pause, meaning that users can technically deposit or withdraw 0 assets. However, the max functions return non-zero values, which creates an inconsistency.

Consider implementing those changes in order to make it easier for other projects to integrate.

Alongside Team: We acknowledge this, we consider it will be almost impossible to be 100% compliant with ERC4626 due to the async withdrawal mechanism.

Function recalculateAccumulatedPrice had the upper bound validation changed from if(_upperBound > $.nextFinalizedWithdrawalId) revert InvalidUpperBound(); to if(_upperBound > $.nextWithdrawalId) revert InvalidUpperBound();

Because the nextWithdrawalId can be much greater than the maximum id that has been requested for finalization, recalculateAccumulatedPrice will provide an inaccurate accumulation for a particular interval.

Consider the following example:

1. 5 requests to withdraw for 100e18 have been made and the withdrawal ids are as following: [0, 1, 2, 3, 4, 5]
2. Manager calls requestFinalizeWithdraw with _upToWithdrawalId = 0
3. nextFinalizationRequestId is now 1
4. Off-chain script triggers recalculateAccumulatedPrice and passes upper bound with id 4
5. Accumulated amount (assuming price of 1e18) is 100e18 * 5 rather than 100e18
6. During claims too much will be withdrawn by the user and consequent users will experience ERC20InsufficientBalance reverts.

Be extremely careful with the inputs from the off-chain scripts, or update the validation accordingly.

Alongside Team: The issue was resolved in PR#19. 31

NAME_SUFIX and SYMBOL_SUFIX both have a typo in the variable names. It should be NAME_SUFFIX and SYMBOL_SUFFIX respectively.

Update the variable naming.

Alongside Team: The issue was resolved in commit acd7a89.

The underscore that prefixes the function name in the _getBytecode function indicates that the function will be either internal or private. However, the function is public.

Remove the underscore to follow the same visibility conventions used elsewhere in the contract.

Alongside Team: The issue was resolved in commit 3ba7d0b.

The NatSpec comment for the registerNewVault function indicates that the function is internal. However, the function is actually external.

Update the comment to indicate that the function is external.

Alongside Team: The issue was resolved in commit 9ffdf75.

The code below the binary search in _findCheckpointEpoch can not be hit. After many fuzzing runs, this section of code had not achieved execution.

Consider removing the code if verified to be unreachable.

Alongside Team: The issue was resolved in commit 1b326e7.

Function deployVault deploys the Vault and WithdrawalQueue with salts based on the _name and _symbol, hence any attempted deployment with the same name and symbol and on the same chain would lead to a CREATE2 collision.

This is not an issue since only the owner can utilize the factory and existing deployments can be upgraded, but should be clearly communicated internally.

Be aware of this behavior.

Alongside Team: Acknowledged.

The validation if ($.vaults[_vaultAddr].active = false) revert VaultNotActive(); was added to function deactivateVault, but it already has modifier onlyActiveVault.

Remove the redundant validation.

Alongside Team: The issue was resolved in commit 8285c91.

When a vault is deactivated within the Vault Oracle, price updates, finalization requests, and completions are prevented.

When a vault is paused, users cannot deposit nor initiate withdrawals from the vault, cannot request, withdraw nor claim from the queue, but prices can continue to be updated and epochs can advance.

Because there is more than one way to prevent the same functionality with slight differences, it should be clearly defined when the vault is expected to be paused and when it is expected to be deactivated through the oracle.

Clearly document this behavior.

Alongside Team: Acknowledged. We will document this properly in the front-end.