Venus Prime - oakcobalt's results

Lines of code

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

Vulnerability details

Impact

According to docs, the protocol may update the market multiplier often and modify alpha when needed at any time. When multiplier and alpha update, all users' scores need to be revised which is a gradual process where total score will be inaccurate which allows some users' to benefit unfairly from such uneven score distribution. To mitigate, protocol designed a safeguard mechanism - Venus will supply a script that will use the permission-less function updateScores to update the scores of all users.

However, a malicious user can easily front-run protocol's updateScores() intended to sync all user scores and cause the protocol's transactions to revert repeatedly, indefinitely DOS protocols' safeguard transactions. In the meantime, the malicious user will benefit from the uneven score distribution during the slow and arbitrary process of individual user score update calls.

Proof of Concept

The vulnerability lies in the fact the for-loop in Prime.sol - updateScores() is incorrectly implemented. When a user from the argument array has already been updated for a given round, the intention is to skip one iteration for that user. However, the iteration will be skipped with continue keyword but without incrementing the iterator due to faulty gas optimization of the iterator i. This will create an infinite loop whenever the continue condition is reached.

//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;
...
        unchecked {
|>              i++;
            }
...

(https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L208) As seen from above, due to gas optimization,i++ is placed with unchecked block at the bottom of the for-loop body instead of for-loop condition. When continue runs, it effectively skipped i++, resulting in i not incrementing for the next loop, creating an infinite loop, causing updateScores() to fail due to out of gas.

A malicious user could easily exploit the vulnerability by front-run protocol script's updateScores() call with their own updateScores() call due to the permissionless nature of the function. As long as the malicious user update the score with one user which is a subset of the protocol's updateScores() call, it will effectively trigger the protocol's call to revert due to an infinite loop. And the malicious user can perform this attack repeatedly for every single protocol's attempt to update score, effectively reducing the score update to a slow and arbitrary process, taking advantage of the incorrect total scores to get a higher ratio of rewards during the process.

See here for the full test file for POC: Test modified from tests/hardhat/Prime/Prime.ts

    it.only("malicious front run update scores", async () => {
      await prime.accrueInterestAndUpdateScore(user1.getAddress(), vmatic.address);
      await prime.accrueInterestAndUpdateScore(user2.getAddress(), vmatic.address);
      // update multiplier
      await prime.updateMultipliers(vmatic.address, bigNumber18.mul("3"), bigNumber18.mul("3"));
      // user3 malicious front run protocol off-chain mechanism's updateScores() 
      await prime.connect(user3).updateScores([user1.getAddress()]);
      // protocol updateScores() revert
      await expect(prime.updateScores([user1.getAddress(), user2.getAddress()])).to.be.rejected;
    });

  PrimeScenario Token
The Hardhat Network tracing engine could not be initialized. Run Hardhat with --verbose to learn more.
    manipulation of capped users score
      ✔ malicious front run update scores (31938ms)


  1 passing (40s)

I think this is high severity due to the attack is easy and cheap to perform and effectively disable protocol's safeguard mechanism for unfair profit.

Tools Used

Manual Review Hardhat

Recommended Mitigation Steps

In the for-loop, add incrementing i before continue to properly skip an iteration.

Assessed type

Loop

Low 01: Prime.sol will not support market with an underlying token with more than 18 decimals. Capital calculation will revert.

Markets are added at the discretion of selected protocol roles or through voting, however, when a market with an underlying token with more than 18 decimals is created. They will not be supported by current Prime.sol and will cause key score-related functions to revert.

In addition, adding one unsupported market to Prime.sol through addMarket() will paralyze the Prime.sol, due to the fact that _calculateScore() will revert, which causes key functions such as _initializeMarkets to revert, effectively paralyzing Prime.sol contract.

//Prime.sol
    function _calculateScore(address market, address user) internal returns (uint256) {
...
        (uint256 capital, , ) = _capitalForScore(xvsBalanceForScore, borrow, supply, market);
|>      capital = capital * (10 ** (18 - vToken.decimals()));
...

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

As seen above, when decimals() is greater than 18, a negative exponential will cause EVM to revert.

//Prime.sol
    function _initializeMarkets(address account) internal {
        address[] storage _allMarkets = allMarkets;
        for (uint256 i = 0; i < _allMarkets.length; ) {
            address market = _allMarkets[i];
            accrueInterest(market);

            interests[market][account].rewardIndex = markets[market].rewardIndex;

|>          uint256 score = _calculateScore(market, account);
...

(https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L631) Recommendations: (1) Adding bypass scaling when a token decimal is more than 18 decimals. (2) Explicitly prohibiting adding market with underlying token with greater than 18 decimals.

Low 02: No methods to remove or disable a market accounting in Prime.sol, susceptible to a single-point failure

In Prime.sol, all the markets added to Prime.sol will have to be queried in a for-loop through multiple interest or capital-related accounting methods. Since there is no mechanism to remove or disable a single market, if any one of the active markets needs to be halted or excluded for any reason, the entire contract needs to be paused. And the only option to truly remove a problematic market from the loop is a contract upgrade.

Like any defi market, there could be many reasons why a lending/borrowing market needs to be put on pause or even completely removed. The reason can be specific to the issues related to the underlying token protocol. When such a case happens, there is no remove or disable market mechanism in Prime.sol to reduce such impacts.

//Prime.sol
    function addMarket(address vToken, uint256 supplyMultiplier, uint256 borrowMultiplier) external {
...
        markets[vToken].rewardIndex = 0;
        markets[vToken].supplyMultiplier = supplyMultiplier;
        markets[vToken].borrowMultiplier = borrowMultiplier;
        markets[vToken].sumOfMembersScore = 0;
        markets[vToken].exists = true;
|>      allMarkets.push(vToken);
...}

(https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L303) As seen from above, a new market is added to the markets mapping.

//Prime.sol
    function _initializeMarkets(address account) internal {
|>          address[] storage _allMarkets = allMarkets;
        for (uint256 i = 0; i < _allMarkets.length; ) {
            address market = _allMarkets[i];
            accrueInterest(market);

            interests[market][account].rewardIndex = markets[market].rewardIndex;

            uint256 score = _calculateScore(market, account);
...

(https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L624) Important accounting functions will always loop through all the markets.

Recommendations: Add a remove market method to allow a single market being removed from the for-loop without impacting the whole contract.

Low 03: Unnecessary code in Prime.sol initialize()

nextScoreUpdateRoundId is declared as a state variable. In initialize(), it is assign as '0' value which is a redundant state update. Note that the winning bot report didn't include this instance in their report.

//Prime.sol
    function initialize(
        address _xvsVault,
        address _xvsVaultRewardToken,
        uint256 _xvsVaultPoolId,
        uint128 _alphaNumerator,
        uint128 _alphaDenominator,
        address _accessControlManager,
        address _protocolShareReserve,
        address _primeLiquidityProvider,
        address _comptroller,
        address _oracle,
        uint256 _loopsLimit
    ) external virtual initializer {
...
|>      nextScoreUpdateRoundId = 0;
...

(https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L156)

//Prime.sol
    /// @notice unique id for next round
    uint256 public nextScoreUpdateRoundId;

Recommendation: Delete nextScoreUpdateRoundId = 0;

Low 04: Incorrect natspec @notice for lastAccruedBlock

Natspec @notice is important when generating doc for codebase. Current @notice for lastAccruedBlock is incorrect or misleading. It's currently marked as the rate at which token is distributed to the Prime contract, this is not a rate, but the block number of the last accrual block.

//PrimeLiquidityProvider.sol
    /// @notice The rate at which token is distributed to the Prime contract
    mapping(address => uint256) public lastAccruedBlock;

(https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L23)

Recommendation: Being more precise on documentation increases code readability.

Gas 01: Wasteful gas operation in updateScores() for-loop

In Prime.sol - updateScores(), contains two for-loops - (1) user for-loop (2) market for-loop. The second for-loop can be optimized.

Under current implementation, the market for-loop contains _executeBoost() and _updateScore() calls for each market and each user. Note that under the hood _executeBoost() will always call a more general accrueInterest(vToken) function to sync vToken rewards. Since accrueInterest(vToken) is not user-specific, this effectively means that for each user the exact same call will be performed even though one call accrueInterest() already fully updates distributionIncome for a given vToken.

//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;

            address[] storage _allMarkets = allMarkets;
            for (uint256 j = 0; j < _allMarkets.length; ) {
                address market = _allMarkets[j];
 |>             _executeBoost(user, market);
                _updateScore(user, market);

                unchecked {
                    j++;
                }
            }
...

(https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L213)

//Prime.sol
    function _executeBoost(address user, address vToken) internal {
        if (!markets[vToken].exists || !tokens[user].exists) {
            return;
        }
|>       accrueInterest(vToken);
        interests[vToken][user].accrued += _interestAccrued(vToken, user);
        interests[vToken][user].rewardIndex = markets[vToken].rewardIndex;

(https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L784) As seen from above, accureInterest(vToken) is nested in (2) for-loops and executed once per each user and each market. This is quite wasteful.

Recommendations: Consider breaking down _executeBoost() and only iterating accrueInterest(vToken) once, since every user participates in all markets. See a sample below:

//Prime.sol
    function updateScores(address[] memory users) external {
...
|>      bool allMarketsAccrued;
        for (uint256 i = 0; i < users.length; ) {
...
            for (uint256 j = 0; j < _allMarkets.length; ) {
                address market = _allMarkets[j];
|>              if (!allMarketsAccrued) {
                    accrueInterest(market);
                    if (j == _allMarkets.length - 1) {
                        allMarketsAccrued = true;
                    }
                }
|>              if (tokens[user].exists) {
                    interests[market][user].accrued += _interestAccrued(market, user);
                    interests[market][user].rewardIndex = markets[market].rewardIndex;
                }
                _updateScore(user, market);
...
}

Gas 02: Wasteful state update in _startScoreUpdateRound()

In _startScoreUpdateRound(), any time a new round begins, both pendingScoreUpdates and totalScoreUpdatesRequired will be rewritten without knowing if totalScoreUpdatesRequired changed.

//Prime.sol
    function _startScoreUpdateRound() internal {
        nextScoreUpdateRoundId++;
|>      totalScoreUpdatesRequired = totalIrrevocable + totalRevocable;
|>      pendingScoreUpdates = totalScoreUpdatesRequired;
    }

(https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L818-L821) When totalScoreUpdatesRequired hasn't changed, storage writing twice is a waste of gas.

Recommendations: Consider checking whether totalScoreUpdateRequired differs from totalIrrevocable + totalRevocable first, and only re-write to storage when the value changes.

Gas 03: Waste of gas to rewrite the '0' value to '0' in Prime.sol - initialize()

In Prime.sol - initialize(), state variable nextScoreUpdateRoundId will be rewritten to 0. This is wasteful storage writing.

//Prime.sol
    function initialize(
...
|>      nextScoreUpdateRoundId = 0;
...

(https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L156)

//PrimeStorage.sol
    /// @notice unique id for next round
    uint256 public nextScoreUpdateRoundId;

Recommendations: Delete nextScoreUpdateRoundId = 0;

Gas 04: accrueInterest() wasts gas when _primeLiquidityProvider already accured tokens in the same block

In Prime.sol accrueInterest(), accureTokens() call to _primeLiquidityProvider is performed every time, and subsequent math operation of totalAccruedInPLP - unreleasedPLPIncome[underlying]; will be performed, then subsequent storage writing to unreleasedPSRIncome[underlying] and unreleasedPLPIncome[underlying] are performed without verifying if both values have changed.

The above math operation plus storage operation are wasteful if (1)accrueInterest() is just called in the same block which means totalAccruedInPLP - unreleasedPLPIncome[underlying]; returns 0 and 0 is added to distributionIncome ;(2)OR if either unreleasedPSRIncome[underlying] didn't change or unreleasedPLPIncome[underlying] didn't change, which resulting in updating storge variable with the same value.

//Prime.sol
    function accrueInterest(address vToken) public {
...
|>   _primeLiquidityProvider.accrueTokens(underlying);
        uint256 totalAccruedInPLP = _primeLiquidityProvider.tokenAmountAccrued(underlying);
|>      uint256 unreleasedPLPAccruedInterest = totalAccruedInPLP - unreleasedPLPIncome[underlying];

|>      distributionIncome += unreleasedPLPAccruedInterest;

        if (distributionIncome == 0) {
            return;
        }
|>      unreleasedPSRIncome[underlying] = totalIncomeUnreleased;
|>      unreleasedPLPIncome[underlying] = totalAccruedInPLP;
...

(https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L554)

//PrimeLiquiityProvider.sol
    function accrueTokens(address token_) public {
...
        uint256 blockNumber = getBlockNumber();
        uint256 deltaBlocks = blockNumber - lastAccruedBlock[token_];
        //@audit-info `accrueTokens` will not update when called again in the same block.
        if (deltaBlocks > 0) {
...

(https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L257) In addition, accrueInteret() is one of the most frequently called functions in many user flows in Prime.sol. For example, it is nested under _executeBoost() and most importantly the most gas-intensive function updateScores(). This significantly increases the total gas wasted by any function that nests accrueInterest() in a for-loop.

Recommendations: Consider add check for unreleasedPLPAccruedInterest, if unreleasedPLPAccruedInterest is '0', we don't add to distributionIncome and also don't re-write to unreleasedPLPIncome[underlying];

Various cases of arbitrary and incorrect score distribution

The core of Prime.sol is score and rewards accounting for users. There are several factors affecting a user's score or rewards, but based on current implementations they are not handled equally and fairly.

The most truthful way of updating rewards or scores is to strictly match a time period for accrual to the score derived from the state variables(balance, multipliers, alpha, etc.) from that time period. But this is not always implemented strictly, which opens up room for error or exploits at different severity.

• Case 1: Only balance changed. And balance is not capped.

This is the most straightforward scenario and is correctly handled. When a user's balance - either XVS or market token changes and their balance is not capped based on their usd value in _capitalForScore(), a user score will always be updated at the point of user balance change. The user's old score will be first used to accrue any rewards prior to balance change before writing the new score to storage.

• Case 2: Only balance changed. But balance is capped.

This is not currently handled. When a user's balance is capped, current implementation in _captialForScore() is converting both XVS and market underlying token values into USD values, although seems rational this introduces a new dynamic factor to score calculation that is almost impossible to track - token price change.

//Prime.sol
    function _capitalForScore(
        uint256 xvs,
        uint256 borrow,
        uint256 supply,
        address market
    ) internal view returns (uint256, uint256, uint256) {
...
        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);
...

A token price can change at any time, which results in a user's capped capital token amount fluctuate with token price, Or a user who is not previously capped became caped due to price change and now their score will vary.

This creates a situation where the score in storage will be inaccurate relative to the actual token price at the time and the rewards update for a user will create an incorrect and arbitrary score distribution - some users' scores are updated based on the new token price, and some others may not.

Furthermore, this allows malicious users to exploit the incorrect score distribution and game the rule. When some users can target other users to either change score distribution or decide not to change the score when it's to their benefit, and repeatedly take advantage of price change opportunities, the reward system is rigged in that it allows opportunistic gains at the expense of others. See details in my HM submission.

Recommendations: This issue can be resolved by changing the denomination of the cap from USD value to token amounts in _captialForScore(), which eliminates the token price factor altogether.

• Case 3: Only alpha or multiplier changed.

This will require score revisions for every user due to alpha and multiplier are baked into score calculations as seen in _capitalForScore()(Prime.sol) and calculateScore()(Scores.sol).

//Scores.sol
    function calculateScore(
        uint256 xvs,
        uint256 capital,
        uint256 alphaNumerator,
        uint256 alphaDenominator
    ) internal pure returns (uint256) {
...
        // e ^ ( ln(ratio) * 𝝰 )
        int256 exponentiation = FixedMath.exp(
 |>         (FixedMath.ln(ratio) * alphaNumerator.toInt256()) / alphaDenominator.toInt256()
        );
        if (lessxvsThanCapital) {
            // capital * e ^ (𝝰 * ln(xvs / capital))
            return FixedMath.uintMul(capital, exponentiation);
        }
        // capital / e ^ (𝝰 * ln(capital / xvs))
        return FixedMath.uintDiv(capital, exponentiation);
...

However, the current implementation is (1) vulnerable to attacks (2) current mitigation logics (protocols' script to mass update users scores through updateScores() (see docs) is faulty and gas intensive (3) Only mitigate but not resolve the issue.

See details on (1) and (2) in my HM and GAS submissions. Briefly, updateScores() is vulnerable to an infinite-loop condition which can be easily exploited by a malicious actor to disable protocol safeguards.

Even if the vulnerability in (1) & (2) are fixed. The protocol is still dealing with (3) which means the protocol will always have to pay for a lot of gas to sync every user score update to date, which will be an increasingly difficult process as the Prime.sol gains more traction. In addition, the uneven and incorrect score distribution will still exist for some period of time.

Recommendations: Apart from the fixes proposed in (1) & (2), (3) is much harder to completely resolved without rewriting the logic of score calculation. If it's possible, may consider separate multiplier and alpha as a global scaler instead of having it baked into individual score calculations, such that as long as all user balances are up to date (which is already taken care of in balance change flow) total score can be deduced from sum of balance. But the trade-off would be user score will not maintain a geometric weighted mean relationship as currently implemented in calculateScore() that allows concave control.

Time spent:

30 hours