June 13, 2023
This security assessment was prepared by OpenZeppelin.
We audited the VenusProtocol/oracle repository at the 78b1a41 commit.
contracts
├── interfaces
│ ├── FeedRegistryInterface.sol
│ ├── OracleInterface.sol
│ ├── PublicResolverInterface.sol
│ ├── PythInterface.sol
│ ├── SIDRegistryInterface.sol
│ └── VBep20Interface.sol
├── libraries
│ └── PancakeLibrary.sol
├── oracles
│ ├── BinanceOracle.sol
│ ├── BoundValidator.sol
│ ├── ChainlinkOracle.sol
│ ├── PythOracle.sol
│ └── TwapOracle.sol
└── ResilientOracle.sol
The Venus protocol is a lending and borrowing solution on the Binance Smart Chain. As part of this protocol, oracles are required to obtain current fair market prices for certain assets. These oracles will be used for pricing core and isolated pools on Venus. The tokens that these oracles will be used for will be determined through governance. Any tokens that have more than 18 decimals, as well as fee-on-transfer and ERC-777 tokens will not be allowed.
Rather than relying on one single data feed, Venus has a ResilientOracle that obtains prices from various oracles, analyzes if these prices are trustworthy using the BoundValidator contract, and returns the resulting price. The specifics of how the ResilientOracle works are detailed below. The various other oracles used by this ResilientOracle are the BinanceOracle, ChainlinkOracle, PythOracle, and TwapOracle. Each of these oracle contracts designed by Venus calls their corresponding data feeds to obtain prices (e.g., ChainlinkOracle calls the oracle contract from Chainlink), and the TwapOracle uses the pool data from PancakeSwap.
The ResilientOracle, BinanceOracle, BoundValidator, ChainlinkOracle, PythOracle, and TwapOracle contracts are all meant to be used as implementation contracts and sit behind transparent proxies. This allows for upgradeability in the future. However, there are no plans to build more contracts that inherit from any of these oracles, nor the BoundValidator. Each of these contracts has a state variable named _accessControlManager, in which there is an address with default admin privileges that can determine which addresses are allowed to call the different functions in these oracles. In addition, all of these contracts inherit from OpenZeppelin's Ownable2Step contract, and the owner has the power to change the access control manager.
As mentioned, the ResilientOracle contract obtains prices from other oracles and compares them to check whether a price is within certain boundaries, finally deciding whether the price is valid or not. These oracles are categorized as MAIN, PIVOT, and FALLBACK. For someone to get a price of a listed asset, the getUnderlyingPrice function must be called, which works as follows:
BoundValidator contract with the FALLBACK price. If it is, it returns the MAIN price. Otherwise, it reverts.BoundValidator against the PIVOT price.The MAIN, PIVOT, and FALLBACK oracles could be any of the oracle contracts mentioned above. Nevertheless, the ResilientOracle defines the updateOracle function, which assumes that the PIVOT contract will always be the TwapOracle by default.
The owner is a single address that can perform the critical administrative action of setting the access control manager. According to Venus, the owner is a time-locked contract that is used through the governance process. The owner is considered a trusted entity.
The access control manager is a contract, in which there is an address with default admin privileges that can perform the critical administrative action of giving and revoking roles for different addresses. These roles provide permissions for addresses to call privileged functions within the different oracle contracts as well as the BoundValidator. These functions include setting and enabling the oracle addresses, setting bounds configurations, adding or removing tokens from the oracles, setting data feed configurations, and even pausing and unpausing the contract. According to Venus, the address with the default admin privileges for the access control manager is a time-locked contract that is used through the governance process. Currently, this address is the same one as the owner.
The data feeds of Chainlink, Binance, and Pyth that are relied upon by the oracles are considered trusted. While ultimately the ResilientOracle returns the price to the user, if a malicious adversary were to be able to control one or more of these data feeds, they could implement a denial-of-service attack, or even worse, provide incorrect data to the system. The security of the system relies on the difficulty of taking control of one or more of these relatively centralized sources.
In addition, Venus will rely on PancakeSwap pools to provide the data feed for the TwapOracle. This source of data is decentralized and thus is more vulnerable to price manipulation, hence the requirement for TWAP. The difficulty of price manipulations for these pools is directly related to their liquidity. Therefore, the security of this oracle model is dependent on using tokens that are more popular and have a larger pool depth. With the decline in popularity of PancakeSwap V2 and the migration to V3, it will be important for the protocol to closely monitor the tokens of interest.
Lastly, it is worth noting that all the mentioned oracle contracts feature a getUnderlyingPrice function. However, regardless of the number of decimals associated with the underlying token or the vToken address, these functions consistently return prices with 36 decimals minus the number of decimals of the underlying token. This applies to the ResilientOracle as well, which serves as the primary point of interaction for other contracts. It is assumed that this consideration is taken into account throughout the out-of-scope codebase when invoking the getUnderlyingPrice functions and performing calculations based on the returned results.
In line 201 of ChainlinkOracle, the interface AggregatorV2V3Interface is used, which inherits from both the AggregatorInterface and the AggregatorV3Interface. Chainlink recommends using the AggregatorV3Interface, as shown in its documentation. This prevents the usage of deprecated functions in the AggregatorInterface, which do not throw errors if no answer has been reached, but instead return 0. The dependence on this unexpected behavior increases the attack surface for the calling contract.
Consider updating the interface used in the ChainlinkOracle from AggregatorV2V3Interface to AggregatorV3Interface.
Update: Resolved in pull request #84 at commit ddd4b02.
The following documentation is misleading:
BoundValidator says "range error thrown if lower bound is greater than upper bound" instead of "range error thrown if lower bound is greater than or equal to upper bound".TwapOracle says that the baseUnit signifies the "decimals of the underlying asset", but it actually takes the value of 1e{decimals} of the asset.TwapOracle says that "XVS-WBNB is not reversed, while WBNB-XVS is", when in reality the opposite is true.Consider updating these misleading comments to match the code's intention.
Update: Resolved in pull request #84 at commit f4352f1.
UPPER_CASE FormatIn TwapOracle.sol the following constants are not using UPPER_CASE format:
According to the Solidity Style Guide, constants should be named with all capital letters with underscores separating words. For better readability, consider following this convention.
Update: Resolved in pull request #84 at commit 70a2211.
According to the Solidity Style Guide, elements inside of contracts, libraries, and interfaces should be in the following order: type declarations, state variables, events, errors, modifiers, and finally functions.
In addition, the guide states that "functions should be grouped according to their visibility and ordered" first by the constructor, followed by the receive function (if it exists), the fallback function (if it exists), public functions, internal functions, and finally private functions. In addition, since the contracts are implementation contracts, it is idiomatic to place the initialize function directly below the constructor.
Throughout the codebase, the layouts of the files do not follow the Solidity Style Guide and are not consistent with each other. Consider moving the modifiers, events, errors, and functions in these files to match the Solidity Style Guide.
Update: Acknowledged, not resolved. The Venus team stated:
Ignoring the Pyth Interface because it was copied from the original project and linting would make the diff more complicated when updating.
In line 305 and line 307 of TwapOracle, the parameter vToken, which is already of type address, is unnecessarily cast with the address keyword.
In addition, in lines 182 and 207, the value 18 is unnecessarily cast with the uint256 keyword.
Consider removing the superfluous casting in order to improve the readability of the codebase.
Update: Resolved in pull request #84 at commit 66707bd.
In BoundValidator, ChainlinkOracle, PythOracle, and TwapOracle, there are structs defined outside of the contracts and therefore in the global namespace. This pattern is used when there are multiple contract implementations within one source file that need to reference the same structs. However, in these files in the codebase, there is only one implementation contract per file, so this methodology is unnecessary.
Consider either placing these structs inside the contracts' definitions or within the corresponding interfaces to reduce global namespace pollution.
Update: Resolved in pull request #84 at commit 28f4924.
To favor explicitness and readability, the following locations in the contracts may benefit from better naming:
The interface file names as well as the interfaces themselves could take the format of I{...} as opposed to {...}Interface. For example, VBep20Interface.sol could be named IVBep20.sol.
In ResilientOracle:
struct field enableFlagsForOracles could be renamed to isEnabled.enableOracle could be changed to updateOracleEnablement to reflect both the enablement and disablement of the oracle.updatePrice could be changed to updatePivotPrice to provide clarity as to which oracle is being updated.tokenConfigs could be changed to _tokenConfigs since it is a private variable.In BinanceOracle:
decimalDelta could be renamed to decimals to avoid confusion with other parts of the codebase, as decimalDelta typically refers to the value 18 - decimals.compare could be renamed to _compare since it is an internal function.In BoundValidator:
ValidateConfig could be renamed to ValidatorConfig or BoundValidatorConfig for consistency.In TwapOracle:
struct field isBnbBased could be renamed to isWbnbBased.pokeWindowValues could be renamed to _pokeWindowValues since it is an internal function.Consider renaming as suggested above to improve the consistency and readability of the codebase.
Update: Acknowledged, not resolved. The Venus team stated:
We prefer to reduce the number of changes.
When the function _getUnderlyingAsset in ResilientOracle is called, if the passed vToken does not equal that of vBnb nor vai, it calls the function VBep20Interface(vToken).underlying(). If the address passed in asvToken does not have the underlying method, it does not fail gracefully with an error message.
Similarly, there are instances of ungraceful handling in getUnderlyingPrice in BinanceOracle, _getUnderlyingAsset in BoundValidator, setUnderlyingPrice and _getUnderlyingPriceInternal in ChainlinkOracle, getUnderlyingPrice in PythOracle, and _getUnderlyingAsset in TwapOracle.
Consider handling these cases gracefully using try-catch and returning descriptive error messages.
Update: Acknowledged, not resolved. The Venus team stated:
We prefer to reduce the number of changes.
virtual and Visibility KeywordsThroughout the codebase, there are extraneous virtual and visibility keywords. From what is understood, there are no plans to inherit from existing contracts. Thus, it is best practice to limit the scope to what is designed, thereby removing extraneous virtual keywords and using more restrictive visibility keywords.
Places with unnecessary virtual keywords:
BoundValidator, in the functions setValidateConfigs, setValidateConfig, and validatePriceWithAnchorPriceTwapOracle, in the function _updateTwapInternalPlaces with unnecessary internal keywords (replace with private):
ResilientOracle, in the functions _getMainOraclePrice, _getFallbackOraclePrice and _getUnderlyingAssetBinanceOracle, in the function compareBoundValidator, in the functions _isWithinAnchor and _getUnderlyingAssetChainlinkOracle, in the functions _getUnderlyingPriceInternal and _getChainlinkPriceTwapOracle, in the functions _updateTwapInternal, pokeWindowValues, and _getUnderlyingAssetPlaces with unnecessary public keywords (replace with external):
ResilientOracle, in the function initializeBinanceOracle, in the functions initialize and getUnderlyingPriceBoundValidator, in the functions initialize and validatePriceWithAnchorPriceChainlinkOracle, in the functions initialize and getUnderlyingPricePythOracle, in the functions initialize and getUnderlyingPriceTwapOracle, in the functions initialize and updateTwapConsider removing extraneous virtual keywords and using more restrictive visibility keywords following the recommendations above.
Update: Resolved in pull request #84 at commit e11f135.
In the file PythInterface, there is a contract PythStructs, an interface IPyth, and an abstract contract AbstractPyth. To improve understandability and readability for developers and reviewers, it is recommended to include one contract or interface per file.
The contract PythStructs contains no functions, and only two structs. Consider moving these structs into PythOracle. Consider removing AbstractPyth completely, since no contracts inherit it.
The file PancakeLibrary also contains more than one library and interface declaration in the same file. Consider separating the interface and libraries into their own files to improve the readability of the codebase.
Update: Acknowledged, not resolved. The Venus team stated:
We prefer to reduce the number of changes.
We recommend the following high-level refactors to improve the readability, composability and overall quality of the codebase:
ResilientOracle contract, the _getFallbackOraclePrice and _getMainOraclePrice functions have very similar code. They can be consolidated into one function to prevent changes in two different places, which increases the space for error in future changes to the codebase._getUnderlyingAsset function is very similar or the same in many contracts, such as ResilientOracle and BoundValidator. Consider moving this functionality to a separate library.setTokenConfigs and setTokenConfig functions in the ResilientOracle, ChainlinkOracle, TwapOracle, and PythOracle are practically the same and can be moved to a library.setDirectPrice and setUnderlyingPrice functions in the ChainlinkOracle contract share almost all their functionality. The setDirectPrice function could be called from the setUnderlyingPrice function after looking up the asset address from the vToken address, or alternatively an internal _setPrice(asset) function that is called from both functions could be introduced.Update: Acknowledged, not resolved. The Venus team stated:
We prefer to reduce the number of changes.
In the ChainlinkOracle contract, the PricePosted event defined has four fields. This event is only used in line 80 and line 95. Since in both cases the last two fields (requestedPriceMantissa and newPriceMantissa) are always equal, consider removing one of them.
Update: Resolved in pull request #84 at commit 5d811fd.
The following suggestions are provided to improve the ability of off-chain services to search and filter for specific events:
PythOracleSet event in the PythOracle contract should emit both the old and new oracle addresses. In the initializer function, it should emit (0, underlyintPythOracle_) and in the setUnderlyingPythOracle function it should emit (old, new), similar to the PricePosted event of the ChainlinkOracle contract, which emits both the old and new oracle price.BatchPriceFeedUpdate event in PythInterface does not have any parameters indexed. The chainId field could be indexed.Update: Partially resolved in pull request #84 at commit cf6a66c. Regarding the first bullet point, the Venus team updated the PythOracleSet event to emit both the old and new oracle addresses. Furthermore, the initializer and setUnderlyingPythOracle functions, which emit this event, have been updated correspondingly. Regarding the second bullet point, the Venus team stated:
Ignoring the Pyth Interface because it was copied from the original project and linting would make the diff more complicated when updating.
We recommend implementing the following input validations:
setOracle function of the ResilientOracle contract, it is advisable to check that for non-MAIN oracles (i.e., PIVOT and FALLBACK), the specified address is different from the MAIN oracle address.setTokenConfig function of the ResilientOracle contract, there are currently no checks in place to ensure that a new configuration does not override an existing one. To address this, it is recommended to verify that the existing token asset address in the tokenConfigs is a zero address. By implementing this validation, updating the oracle for a specific token would only be possible through the setOracle and enableOracle functions, reducing the likelihood of errors.While this issue does not pose a security risk, the absence of validation on user-controlled parameters may lead to erroneous transactions, particularly if some clients default to sending null parameters when none are specified.
Update: Acknowledged, not resolved. The Venus team stated:
These functions will be executed via Governance, with a timelock of 3 days, and the vote of the community, so there should not be errors in the inputs.
The usage of the constant 100e16 on line 123 of BoundValidator.sol lacks sufficient explanation regarding its origin and purpose.
Developers should define a constant variable for every magic value used (including booleans), giving it a clear and self-explanatory name. Additionally, for complex values, inline comments explaining how they were calculated or why they were chosen are highly recommended.
Consider following the Solidity Style Guide to define the constant in UPPER_CASE_WITH_UNDERSCORES format with the corresponding specific public getter to read it.
Update: Partially resolved in pull request #84 at commit c420a18. The Venus team stated:
Mitigated. We added a comment to explain why the multiplication by the constant
1e18is needed.
The contracts ResilientOracle, TwapOracle, PythOracle, and ChainlinkOracle define a TokenConfig struct. This struct is utilized in the tokenConfigs mappings to track various properties of each token's configuration. However, these mappings already use the asset address as the key for each asset, while also including an asset field within the structs. This repetition results in storing unnecessary information.
A similar pattern can be observed in the ValidateConfig struct within the BoundValidator contract.
To optimize storage and avoid redundant data, it is recommended to remove the asset field from the structs. Instead, consider utilizing an existing field within the struct or introducing a boolean field to check the configuration's validity.
Update: Acknowledged, not resolved. The Venus team stated:
We prefer to reduce the number of changes.
initializeThe boundValidator variable in the ResilientOracle contract is currently set in the initialize function. However, since this variable remains unchanged after initialization, it can be optimized by declaring it as an immutable variable in the constructor.
By making boundValidator immutable, initialization inconsistencies can be avoided, and gas costs can be reduced when accessing it. This is especially beneficial when calling the getUnderlyingPrice function, where accessing boundValidator may occur up to three times.
To improve efficiency and maintain consistency, consider declaring boundValidator as an immutable variable in the constructor of the ResilientOracle contract.
Update: Resolved in pull request #84 at commit 4f56661.
An in-depth review of the oracles was performed, and several low-severity issues and notes were identified in the system. The Venus Protocol team provided dedicated documentation, which clarified the system's design. The Venus team was very responsive throughout the audit.
While audits help in identifying code-level issues in the current implementation and potentially the code deployed in production, the Venus Protocol team is encouraged to consider incorporating monitoring activities in the production environment. Ongoing monitoring of deployed contracts helps identify potential threats and issues affecting production environments. With the goal of providing a complete security assessment, the monitoring recommendations section raises several actions addressing trust assumptions and out-of-scope components that can benefit from on-chain monitoring. Consider monitoring the following items: