Velvet Capital

When initialized, updated, or used, the vault tokens are not verified whether they are enabled or not by the TokenManagement smart contract and its function isTokenEnabled(). The only instance where it is being validated is within the function getVaultValueInUSD() which is only called when the Asset Manager calls the function chargePerformanceFee(). This means that every interaction with vault tokens on the platform is not tracked and verified whether or not the tokens are enabled.

The functions _init() and _init_unchained() are an implementation of OpenZeppelin's Upgradeable Proxies design standard and are used to serve as the constructor of upgradeable contracts. Despite being both used as initializers, they possess slightly different use cases to correctly implement the analog linearization of smart contract constructors.The function _init() should be used and implemented to embed the linearized calls to all parent initializers. The function _init_unchained() should be used to perform the initialization of the variables for the current contract only.As a consequence of this design, it is possible that, due to the lack of automatic linearization, the calling of _init() functions initializes the same contract multiple times. For these cases, the _init_unchained() function may be used manually to avoid double initialization, however it will break the upgradeability design and compatibility between smart contracts on the blockchain.Additionally, the inherited smart contracts should be initialized according to their inheritance order, from the most base-like to the most derived.There are multiple instances where contracts do not follow the design standard in the following account:_init_unchained() function is not used.Initialization according to inheritance order is incorrectMissing initialization of certain contracts

The ERC165 standard defines a supportsInterface function which is designed to verify that a particular address has a contract deployed that implements expected functionalities. Using ERC165 checks is considered a good practice as it eliminates most of the user errors related to providing invalid addresses. Furthermore, the supportsInterface check also indirectly assures that no zero-address was provided.

The PriceOracle contract received a price for a given asset using Chainlink oracle's latestRoundData function. Then, it then assures that the price is not expired using the updatedAt value returned from Chainlink's oracle. However, the time threshold used to verify the data staleness is set to 25 hours and is used for every asset. It must be noted that the different assets in Chainlink's infrastructure have different heartbeat intervals. Assets used more often have the interval of 1 hour, while less commonly used ones can have the interval of 24 hours. Using a 25 hour threshold for assets with smaller heartbeat interval will make the Velvet's Oracle accept the price even if it is stale. If such price would be accepted, it would make the swap calculations incorrect and might result in a loss of funds for protocol, and in consequence for the users.The heartbeat intervals are known for each asset and they do not change often during normal operation.

The AccessController contract defines a transferSuperAdminOwnership. This function is responsible for revoking the SUPER_ADMIN role from the previous holder and assigning it to a new user. The function internally calls the revokeRole and _setupRole and functions from the OpenZeppelin's AccessControl contract. Both of these functions execute their logic only if the a given address holds the role or does not respectively. As a consequence, it was observed that two specific scenarios are possible:The supposed previous role holder does not actually hold the role. In this case, the revokeRole function will do nothing, while _setupRole function will assign the SUPER_ADMIN role to a new address. This scenario leads to potentially unwittingly creating new SUPER_ADMIN users without revoking previous role owners.If the transferSuperAdminOwnership function is called with the same address provided as a _oldAccount and _newAccount parameters, first the code will attempt to assign new role to this user. This action will be effectively a no-op, as this user already holds this role. However, right after that, the revokeRole function will revoke the SUPER_ADMIN privileges from this user. As a consequence, the protocol would be left with no SUPER_ADMIN.

The function transferSuperAdminOwnership() is used to transfer the SUPER_ADMIN role from one account to another. When successful, the account of the address identified by the variable _newAccount will be the new SUPER_ADMIN, and the variable _oldAccount will identify the account that was stripped of this role. However, it should be noted that only the ownership of this role is transferred, which means that, previous roles that may have been assigned by the previous SUPER_ADMIN to themselves or others will stay in place.In cases the SUPER_ADMIN role is being transferred due to a compromise of the respective account, this issue becomes more relevant and impactful as the malicious actor may assign important roles on the protocol that will serve as a backdoor for future use.