Venus Prime Audit

We audited the VenusProtocol/venus-protocol repository at commit f31a054. More specifically, we audited pull request #196.

We trust the Protocol Share Reserve (PSR) to handle all accounting functionalities correctly. The system also interacts with the broader lending protocol by reading user’s vToken and borrowed balances which are used to compute scores.

Every time interest is accrued, the total rewards are divided by the total sum of the user's scores to get the rewards entitled per score. This assumes that the total score for each user is the same when interest is collected as when it is calculated. This invariant is usually maintained as executeBoost collects the interest from users before any actions that change their XVS or VToken balances (which play a role in score calculation).

However, updateAlpha and updateMultipliers update the total interest for every market but do not first collect the interest for every individual user. This means that when users collect interest after the alpha or multiplier update, the new score is multiplied by the interest accrued since the last time executeBoost was called. However, that score is different from the score used to calculate the interest at the time. Additionally, the sumOfScores now does not equal the sumOfScores during interest calculation.

If the update increases the sum total of the user's scores, there would not be enough funds in the reserve to pay out all the accounted-for reward amounts. This would mean that users who collect interest early on will get a larger reward than they should, while later claimants will have zero rewards as the reward pool will become insolvent.

If the Venus team attempts to mint an irrevocable token for a user already holding a revocable Prime token, the minting process fails. This is due to the internal function _mint, which requires that the recipient must not hold any form of Prime token, whether it is revocable or irrevocable.

Given that irrevocable Prime tokens are more desirable as they remain even when the user unstakes, this behavior suggests an unintended restriction in the minting process.

alphaDenominator should never exceed alphaNumerator, as specified by the audit documentation as well as this error which would be thrown in the calculateScore function:

assert(alphaNumerator <= alphaDenominator);

Consider ensuring that alphaNumerator <= alphaDenominator in the initialize and updateAlpha functions.

Throughout the codebase, there are several parts that do not have docstrings:

       • Line 572 in Prime.sol
       • Line 105 in Prime.sol

Consider thoroughly documenting all functions (and their parameters) that are part of any contract's public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).

The divisor argument of uintDiv and the multiplier argument of uintMul in the FixedMath library are both signed integers.

The results of multiplication and division operations involving these arguments will therefore be signed, and the results of both the uintDiv and uintMul functions are downcast to uint256.

While this may produce the correct results when the multiplier and divisor arguments are properly-encoded fixed-point numbers, it will produce erroneous results when the high bit of either of these arguments is set.

Consider adding a check to ensure that the multiplier argument of uintMul and the divisor argument of uintDiv are well-formed fixed-point numbers.

Pragma directives should be fixed to clearly identify the Solidity version with which the contracts will be compiled.

The file IPrime.sol has the solidity ^0.5.16 floating pragma directive which is an outdated Solidity version.

Consider using the same fixed pragma version used in Prime.sol.

A valid poolId for a token must be less than the length of the poolInfos array for a particular rewardToken . A prime poolId should also match a pre-existing poolId, so this condition should also be true for Prime pools. However, the setPrimeToken function does not have this check.

Consider adding a check in setPrimeToken to ensure that it matches a pre-existing poolId.

The use of non-explicit imports in the codebase can decrease the clarity of the code, and may create naming conflicts between locally defined and imported variables. This is particularly relevant when multiple contracts exist within the same Solidity files or when inheritance chains are long.

Throughout the codebase, there are multiple instances where int/uint is used as opposed to int256/uint256:

• Line 9 of Prime.sol
• Line 10 of Prime.sol
• Line 11 of Prime.sol
• Line 118 of Prime.sol
• Line 185 of Prime.sol
• Line 190 of Prime.sol
• Line 208 of Prime.sol
• Line 223 of Prime.sol
• Line 249 of Prime.sol

In favor of explicitness, consider replacing all instances of int/uint with int256/ uint256.

The MAXIMUM_BPS state variable in the PrimeStorageV1 contract is never used.

To improve the overall clarity, intentionality, and readability of the codebase, consider removing any unused state variables.

Throughout the codebase, there are files that lack SPDX license identifiers. For instance:

       • IPrime.sol
       • Prime.sol
       • PrimeStorage.sol

To avoid legal issues regarding copyright and follow best practices, consider adding SPDX license identifiers to files as suggested by the Solidity documentation.

Providing a specific security contact, such as an email or ENS, within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice proves beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. Additionally, if the contract incorporates third-party libraries and a bug surfaces in these, it becomes easier for the creators of those libraries to make contact, inform the code owners about the problem, and provide mitigation instructions.

Neither of the contracts in scope has a security contact.

Consider adding a NatSpec comment on top of the contracts' definitions with a security contact. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.

Since Solidity 0.8.18, developers can utilize named parameters in mappings. This means mappings can take the form of mapping(KeyType KeyName? => ValueType ValueName?). This updated syntax provides a more transparent representation of the mapping's purpose.

Throughout the codebase, there are multiple mappings without named parameters:

• The tokens state variable in the PrimeStorageV1 contract.
• The stakedAt state variable in the PrimeStorageV1 contract.
• The markets state variable in the PrimeStorageV1 contract.
• The interests state variable in the PrimeStorageV1 contract.
• The isScoreUpdated state variable in the PrimeStorageV1 contract.
• The vTokenForAsset state variable in the PrimeStorageV1 contract.
• The unreleasedIncome state variable in the PrimeStorageV1 contract.

Consider adding named parameters to the mappings to improve the readability and maintainability of the code.

The addition of Prime tokens had a relatively low impact on the existing codebase. The audit uncovered 1 critical-severity and 2 medium-severity issues. Several recommendations were proposed to follow best practices.