Code maturity
Various areas of the codebase can be improved to increase maintainability, robustness, and readability.
Testsuite
The code as reviewed lacks a comprehensive testsuite. The Voyage team was already aware and planning to expand the testsuite with both positive and negative tests, ensuring the contracts appropriately handle both correct and incorrect inputs and state.
Missing events
The protocols lack sufficient event triggers. For example, there are no events triggered in critical VToken functions added to the base ERC4626 contract.
The overall codebase should be reviewed for missing events and updated accordingly.
Input validation
The codebase exhibits a general lack of input validation. One pervasive example is the retrieval of elements from maps without explicit checks to ensure the key (provided by the msg.sender) exists in the map. We strongly encourage the Voyage team to perform explicit checks against every external input and to extend the testsuite to ensure those checks are and remain effective as the codebase evolves.
The lack of input validation has been the root cause of critical issues in the codebase, such as finding 3.3 on the missing consistency check between the _tokenId argument and the token ID embedded in the _data argument provided to the buyNow function, allowing to buy an NFT at the cost of only the down payment.
Unused code
The codebase contains multiple pieces of unused code, ranging from individual lines, functions, and even whole files (such as the subvault-related features). This makes it harder to understand the codebase, increases code size (and hence deployment cost), and increases the available attack surface. Some unused functions even proved dangerous, such as PeripheryPayments::approve. Our recommendation is to remove any unused code from the main branch of the codebase.
Mix of generic ERC20 and hardcoded WETH code
At the time of review, the codebase contained both code designed to handle any generic ERC20 asset and code that assumes usage of WETH. The Voyage team communicated they are evaluating plans to support other ERC20 assets, such as various stablecoins, to enable processing NFT orders using these assets. Since the support for assets other than WETH is incomplete, we reviewed the code under the assumption that only WETH is used. If multiple assets are to be allowed, we recommend to review carefully the code interacting with the generic assets to ensure the contract being interacted with is consistent throughout the whole process of buying, repaying, and liquidating an NFT as well as depositing and withdrawing said asset.
Fail-open design of some core functions
Some functions critical to the operation of the protocol are designed with a fail-open behavior. For example, withdrawNFT relies on the isCollateral flag to be set in the nftIndex map of the shared diamond storage to prevent NFTs being held as collateral from being withdrawn.
It is arguably easier to forget to set or mistakenly set to the default value a variable than it is to set it to a nondefault value. We suggest to consider adopting a fail-closed design, in which dangerous actions are prevented by default. In the specific case of withdrawNFT, an isWithdrawable variable could be introduced, which would have to be set to true (a nondefault value) to allow an NFT to be transferred.
The fail-open design of withdrawNFT made possible to exploit the issue presented in finding 3.3, where a lack of input validation in buyNow led to setting the incorrect entry in nftIndex, allowing to withdraw the NFT bought with funds from the reserve without repaying it in full.
Functions marked as payable
The codebase contains multiple functions marked as payable without requiring them to be marked as such. Examples include
PaymentsFacet::refundETHPeripheryPayments::pullTokenPeripheryPayments::approveall
SelfPermitfunctions