AssetVault Manual Review Findings

AVT-01M: Improper Dependency Utilization

Type	Severity	Location
Language Specific		AssetVault.sol:L10

Description:

The AssetVault is meant to be deployed via a proxy system, however, it inherits a non-upgradeable variant of the ReentrancyGuard.

Impact:

While the ReentrancyGuard::constructor does contain logic within it, it simply assigns a value to _status as a matter of optimization rather than functionality.

In any case, the contract should be standardized and utilize upgradeable variants as it is meant to be deployed as a cloned instance via the VaultFactory contract.

Example:

Copy
5import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
6import "@openzeppelin/contracts/token/ERC1155/IERC1155.sol";
7import "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";
8import "@openzeppelin/contracts/token/ERC1155/utils/ERC1155Holder.sol";
9import "@openzeppelin/contracts/proxy/utils/Initializable.sol";
10import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
11import "@openzeppelin/contracts/utils/Address.sol";

Recommendation:

As the ReentrancyGuard::constructor does assign an initial value to _status, we advise its upgradeable variant to be utilized from the relevant OpenZeppelin dependency to ensure code consistency. We advise the same dependency to be utilized for all other assets that the AssetVault contract imports as well.

Alleviation (7a4e1dc948e94ded7385dbb74818bcf93ecc207c):

All dependencies of the AssetVault contract were updated to be of their upgradeable OpenZeppelin variant and the ReentrancyGuardUpgradeable contract is properly initialized via its ReentrancyGuardUpgradeable::__ReentrancyGuard_init hook during the contract's AssetVault::initialize function.

As such, we consider this exhibit fully alleviated.