Venus Prime - Breeje's results

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L661

Vulnerability details

Impact

Scores are getting incorrectly calculated.

Proof of Concept

Scores are the critical part of Prime codebase as Scores are directly correlated to the interest users can claim.

As per the Natspec in Scores.sol:

14:   * @param xvs amount of xvs (xvs, 1e18 decimal places)
15:   * @param capital amount of capital (1e18 decimal places)

The xvs and capital passed in calculateScore function must have 18 decimal places.

Now let's focus on _calculateScore function in Prime.sol. This internal function calculates the score based on market and user and returns the actual score.

    function _calculateScore(address market, address user) internal returns (uint256) {
        uint256 xvsBalanceForScore = _xvsBalanceForScore(_xvsBalanceOfUser(user));

        IVToken vToken = IVToken(market);
        uint256 borrow = vToken.borrowBalanceStored(user); 
        uint256 exchangeRate = vToken.exchangeRateStored(); 
        uint256 balanceOfAccount = vToken.balanceOf(user);
        uint256 supply = (exchangeRate * balanceOfAccount) / EXP_SCALE;

        address xvsToken = IXVSVault(xvsVault).xvsAddress();
        oracle.updateAssetPrice(xvsToken);
        oracle.updatePrice(market);

        (uint256 capital, , ) = _capitalForScore(xvsBalanceForScore, borrow, supply, market);
        capital = capital * (10 ** (18 - vToken.decimals()));

        return Scores.calculateScore(xvsBalanceForScore, capital, alphaNumerator, alphaDenominator);
    }

Now let's understand decimals precision of each variable in this function:

1. xvsBalanceForScore has the decimals of xvs token which is 18 decimals.

2. borrow has the number of decimals of the underlying token (for example 18 for USDT and 6 for TRX).

3. balanceOfAccount has the number of decimals of the VTokens (every VToken in Venus has 8 decimals now)

4. exchangeRate has the decimals of the underlying token + 10 (for example, 28 for USDT and 16 for TRX).

5. supply which is (exchangeRate * balanceOfAccount) / EXP_SCALE will have always the decimals of the underlying token.

Now there is a call on _capitalForScore to calculate the capital required for calculation of score.

  function _capitalForScore(
        uint256 xvs, 
        uint256 borrow,
        uint256 supply, 
        address market
    ) internal view returns (uint256, uint256, uint256) {
        address xvsToken = IXVSVault(xvsVault).xvsAddress();

        uint256 xvsPrice = oracle.getPrice(xvsToken)
        uint256 borrowCapUSD = (xvsPrice * ((xvs * markets[market].borrowMultiplier) / EXP_SCALE)) / EXP_SCALE;
        uint256 supplyCapUSD = (xvsPrice * ((xvs * markets[market].supplyMultiplier) / EXP_SCALE)) / EXP_SCALE;

        uint256 tokenPrice = oracle.getUnderlyingPrice(market); 
        uint256 supplyUSD = (tokenPrice * supply) / EXP_SCALE
        uint256 borrowUSD = (tokenPrice * borrow) / EXP_SCALE;

        if (supplyUSD >= supplyCapUSD) {
            supply = supplyUSD > 0 ? (supply * supplyCapUSD) / supplyUSD : 0;
        }

        if (borrowUSD >= borrowCapUSD) {
            borrow = borrowUSD > 0 ? (borrow * borrowCapUSD) / borrowUSD : 0;
        }

        return ((supply + borrow), supply, borrow);
    }

Here, the response of the ResilientOracle has 36 - the decimals of the underlying token (for example 18 for USDT and 30 for TRX). This way (tokenPrice * supply) / EXP_SCALE will have always 18 decimals (same for the borrow).

As supply and borrow both have same decimals as underlying token, the return value of capital will also have same decimals of underlying token.

Now focus on this last 3 lines of _calculateScore:

      (uint256 capital, , ) = _capitalForScore(xvsBalanceForScore, borrow, supply, market);
@->   capital = capital * (10 ** (18 - vToken.decimals()));

      return Scores.calculateScore(xvsBalanceForScore, capital, alphaNumerator, alphaDenominator);

In line above (@->), capital which has decimals of underlying token is multiplied by:

• 10 ** (18 - vToken.decimals())
• 10 ** (18 - 8) [As vToken.decimals() = 8]
• 10 ** 10

As shown earlier, capital passed in Scores.calculateScore must have 18 decimals But the decimal of capital passed will be:

For underlying tokens having 18 Decimals (USDT): 18 + 10 = 28 Decimals. For underlying tokens having 6 Decimals (TRX): 6 + 10 = 16 Decimals.

This means the value of xvs will have 18 decimal precision but capital will have precision ranging from 16 to 28 decimals depending on underlying token.

This will lead to incorrect calculation of Scores.

Tools Used

VS Code

Recommended Mitigation Steps

Update the codebase in such a way that you subtract decimal of underlying token instead of vToken.decimals().

    function _calculateScore(address market, address user) internal returns (uint256) {
        uint256 xvsBalanceForScore = _xvsBalanceForScore(_xvsBalanceOfUser(user));

        IVToken vToken = IVToken(market);
        uint256 borrow = vToken.borrowBalanceStored(user); 
        uint256 exchangeRate = vToken.exchangeRateStored(); 
        uint256 balanceOfAccount = vToken.balanceOf(user);
        uint256 supply = (exchangeRate * balanceOfAccount) / EXP_SCALE;

        address xvsToken = IXVSVault(xvsVault).xvsAddress();
        oracle.updateAssetPrice(xvsToken);
        oracle.updatePrice(market);

        (uint256 capital, , ) = _capitalForScore(xvsBalanceForScore, borrow, supply, market);
-       capital = capital * (10 ** (18 - vToken.decimals()));
+       capital = capital * (10 ** (18 - decimalOfUnderlyingToken));

        return Scores.calculateScore(xvsBalanceForScore, capital, alphaNumerator, alphaDenominator);
    }

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L208

Vulnerability details

Proof of Concept

As per the Readme Doc inside Prime Folder:

Market multipliers and alpha can be updated at anytime and need to be propagated to all users. Changes will be gradually applied to users as they borrow/supply assets and their individual scores are recalculated. This strategy has limitations because the scores will be wrong in aggregate.

To mitigate this issue, Venus will supply a script that will use the permission-less function updateScores to update the scores of all users. This script won’t pause the market or Prime contract. Scores need to be updated in multiple transactions because it will run out of gas trying to update all scores in 1 transaction.

Venus is going to use a script to call updateScores to update scores for all the users.

Consider the following Situation now:

1. Before running the script, Alice calls the updateScores function to update her score for round x.

File: Prime.sol

    function updateScores(address[] memory users) external {
        ***

        for (uint256 i = 0; i < users.length; ) {
            address user = users[i];

            if (!tokens[user].exists) revert UserHasNoPrimeToken();
 @->        if (isScoreUpdated[nextScoreUpdateRoundId][user]) continue; // @audit Infinite Loop

            ***

 @->        isScoreUpdated[nextScoreUpdateRoundId][user] = true;

            unchecked {
 @->            i++;
            }

            emit UserScoreUpdated(user);
        }
    }

Link to code

2. After Alice's call, her isScoreUpdated status is set to true.
3. When the script runs, it checks if a user's score has already been updated with the line: if (isScoreUpdated[nextScoreUpdateRoundId][user]) continue;. The idea is not to update scores again if they've already been updated.
4. However, there's an issue here. To optimize gas usage, i++ is used with the unchecked keyword at the end. But because of the continue keyword, i++ is effectively skipped, resulting in the same user being processed repeatedly, creating an infinite loop.

In simple terms, the code tries to be efficient with gas usage but makes a mistake. Instead of increasing the counter i within the for loop itself, it's done later with unchecked to save gas. However, it ends up getting stuck in an infinite loop for some users like Alice.

Tools Used

VS Code

Recommended Mitigation Steps

Update the code as following:

File: Prime.sol

    function updateScores(address[] memory users) external {
        ***

        for (uint256 i = 0; i < users.length; ) {
            address user = users[i];

            if (!tokens[user].exists) revert UserHasNoPrimeToken();
-           if (isScoreUpdated[nextScoreUpdateRoundId][user]) continue;
+           if (isScoreUpdated[nextScoreUpdateRoundId][user]) {
+               unchecked {
+                   i++;
+               }
+               continue;
+           }

            ***

            unchecked {
                i++;
            }

            emit UserScoreUpdated(user);
        }
    }

Assessed type

Loop

QA Report

Low Risk Issues

[L-01] Solidity Version 0.8.13 used is vulnerable to optimizer issues

The solidity version 0.8.13 has below two issues:

1. Vulnerability related to ABI-encoding. ref : https://blog.soliditylang.org/2022/05/18/solidity-0.8.14-release-announcement/
2. Vulnerability related to 'Optimizer Bug Regarding Memory Side Effects of Inline Assembly' ref : https://blog.soliditylang.org/2022/06/15/solidity-0.8.15-release-announcement/

Use recent Solidity version or atleast above 0.8.15 version which has the fix for these issues.

[L-02] No functionality to change maxLoopsLimit can be problematic in long term

In initialize function of Prime.sol contract, maxLoopsLimit is set to _loopsLimit value through calling function _setMaxLoopsLimit.

File: Prime.sol

`initialize` function:

164:    _setMaxLoopsLimit(_loopsLimit);

Issue here is that _setMaxLoopsLimit function is internal access function which cannot be called from outside and there is no other method in the contract which uses this function to change the value of maxLoopsLimit again in future to make sure no DoS issues happens.

Not having this access can lead to issues in future. So it is recommended to add a function that allows priviledged account to update this value.

[L-03] issue doesn't check the 90 days staking condition to issue Prime Token

issue is a priviledged function used to Directly issue prime tokens to users.

But in implementation, the condition is not checked to make sure that user has staked the xvs tokens for 90 Days. This can lead to issuing of tokens even if user fails to satisfy the condition required to get the token.

File: Prime.sol

  function issue(bool isIrrevocable, address[] calldata users) external {

    ***

            for (uint256 i = 0; i < users.length; ) {
                _mint(false, users[i]);
                _initializeMarkets(users[i]);
                delete stakedAt[users[i]]; // @audit no check for 90 Days staking condition.

                unchecked {
                    i++;
                }
     ***
    }

[L-4] Adding Multiple Markets with same underlying Tokens can be problematic

addMarket function in Prime contract is used to Add a market to prime program.

Also there is a possibility that multiple markets have same underlying tokens.

If case it happens, the line below will overwrite any existing market.

File: Prime.sol

    function addMarket(address vToken, uint256 supplyMultiplier, uint256 borrowMultiplier) external {

      ***

@->   vTokenForAsset[_getUnderlying(vToken)] = vToken;

      ***

    }

This can lead to issues as there will now be 2 markets in existance (markets[vToken].exists = true) with same underlying token but vTokenForAsset will only point to new market added.

Analysis

Summary

Introduction

This technical analysis report examines the security aspects of Venus Prime, a vital component of the Venus Protocol's incentive program. The analysis explores the underlying smart contracts and different components, including reward distribution calculations, claiming mechanism, and unique Soulbound Tokens, while identifying security insights and potential vulnerabilities within the Venus Prime contracts.

High Level Contract Flow

Audit Approach

1. Initial Scope and Documentation Review: Thoroughly examined the Contest Readme File and Venus Prime Readme file to understand the contract's objectives and functionalities.

2. High-level Architecture Understanding: Performed an initial architecture review of the codebase by skimming through all files without exploring function details.

3. Test Environment Setup and Analysis: Set up a test environment and executed all tests.

4. Comprehensive Code Review: Conducted a line-by-line code review focusing on understanding code functionalities.

   • Understand Codebase Functionalities: Began by comprehending the functionalities of the codebase to gain a clear understanding of its operations.
   • Analyze Value Transfer Functions: Adopted an attacker's mindset while inspecting value transfer functions, aiming to identify potential vulnerabilities that could be exploited.
   • Identify Access Control Issues: Thoroughly examined the codebase for any access control problems that might allow unauthorized users to execute critical functions.
   • Detect Reentrancy Vulnerabilities: Looked for potential reentrancy attacks where malicious contracts could exploit reentrant calls to manipulate the contract's state and behavior.
   • Evaluate Function Execution Order: Examined the sequence of function executions to ensure the protocol's logic cannot be disrupted by changing the order of calls.
   • Assess State Variable Handling: Identified state variables and assessed the possibility of breaking assumptions to manipulate them into exploitable states, leading to unintended exploitation of the protocol's functionality.

5. Report Writing: Write Report by compiling all the insights I gained throughout the line by line code review.

Codebase Quality

Architecture Recommendation

• Block.number Vs Block.timestamp: Since the contracts are planned for deployment on multiple chains, including those like Optimism where block.number may not accurately represent time, it is advisable to use block.timestamp consistently. In the PrimeLiquidityProvider (PLP) contract, variations in average block time can lead to fluctuations in the rate at which tokens accrue.

• Solidity Version Used: Consider upgrading the Solidity version from 0.8.13 to 0.8.15 or a higher version. Although the current version does not impact any contracts within scope, it's a important to avoid any potential issues which 0.8.13 version brings related to ABI-encoding and optimizer bugs regarding memory side effects of inline assembly.

• Handling Multiple Markets with Same Underlying Token: Address the limitation in the Prime contract, which currently cannot handle multiple markets with the same underlying token. This issue could have long-term implications, especially if multiple markets with identical underlying tokens need to be added to the Prime program simultaneously. It's recommended to enhance the contract's ability to handle such scenarios for improved flexibility and scalability.

Centralization Risk

• Token Sweep Power: The owner has the authority to sweep all tokens held in the PrimeLiquidityProvider contract. This centralized control could potentially pose serious risk.

• Claim Functionality Pause: The owner retains the capability to temporarily "pause" the claim functionality whenever necessary. This centralized ability to halt operations could affect user access and functionality.

• Privileged Role Impact: Privileged roles have the ability to modify the values of alpha and multiplier, which can have a substantial impact on the interest earned by users.

Systemic risks

• Sybil Risk: The contract employs a cap named MAXIMUM_XVS_CAP to prevent users from earning interest on amounts exceeding this limit. However, in practice, a user can potentially exploit this by using multiple accounts to accumulate more interest. As a result, the existing protection measures in place are not entirely immune to Sybil attacks.

• DoS in Key Functionities: sweepToken function is intended to recover accidentally transferred ERC-20 tokens to the contract. While this function is restricted to the contract owner (onlyOwner modifier), it does not account for previously accrued tokens, which poses a significant risk to the protocol's stability. In case it sweeps the token which are previously added to total accrued tokens but not released to Prime yet, then it will lead to DoS across both this contracts.

• Gas Limit Issues: There is no way to update Max Loops value in Future. While addMarket controls the number of markets that can be add so it not that big a risk. But still as there is no way to even remove the market, it is a possible risk if block limit is reached.

Code Review Insights

Key Contracts:

1. Prime.sol: Soulbound token, the central contract of this audit, allows Prime holders to accumulate rewards, which they can later claim at their convenience.

2. PrimeLiquidityProvider.sol: This contract offers liquidity to the Prime token consistently over a specific duration. This liquidity serves as one of the two fund sources supported by Prime tokens.

Insights

• The rate at which tokens accumulate in PrimeLiquidityProvider may not be consistent across different blockchain networks.

• The sweepToken function has the potential to sweep already accrued rewards, which could result in Denial-of-Service (DoS) vulnerabilities and financial losses for users.

• There is a risk of encountering an infinite loop within the updateScores function.

• There is a decimal precision error which is leading to incorrect calculation of Score.

• The value of BLOCKS_PER_YEAR may be unpredictable during deployment, particularly on blockchain networks with inconsistent block times.

• Tokens that have fees associated with them may lead to exaggerated interest claims for some users, potentially causing issues for the remaining users.

• Irrevocable tokens can be burned.

• There is currently no mechanism in place to update the maximum loop count in the future.

• The issue function allows the issuance of Prime Tokens to users without enforcing the 90-day staking condition.

• The Prime contract does not support the use of multiple markets with the same underlying tokens.

Time Spent

Time spent:

16 hours