Venus Prime - dirk_y's results

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L237-L255 https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L818-L822 https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L200-L230 https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L397-L405 https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L704-L719

Vulnerability details

Impact

When the alpha value is changed or the multiplier of a market is updated, user scores have to be recalculated. As stated in the Prime README, this will be handled by a script written by the Venus team that will call updateScores repeatedly (to loop through all the users).

However there is an issue with the current logic where an eligible user can deliberately/accidentally claim a new Prime token after an alpha/multiplier update has occurred but before the script has called updateScores for all the users. Since updateScores is an unprotected method, anyone can now call updateScores with the address of the new Prime token holder. This will result in 1 Prime token holder that existed before the alpha/multiplier not having their score updated, which impacts not only their interest calculation, but also the interest calculation for all the relevant markets (since they use the sum of user scores for share calculations). The script could also revert on the last updateScores call as pendingScoreUpdates will be 0 earlier than intended, depending on how often the script reads pendingScoreUpdates.

Proof of Concept

When the timelock performs an alpha/multiplier update, a method like updateAlpha is called (we'll consider an alpha update for this report):

    function updateAlpha(uint128 _alphaNumerator, uint128 _alphaDenominator) external {
        _checkAccessAllowed("updateAlpha(uint128,uint128)");
        _checkAlphaArguments(_alphaNumerator, _alphaDenominator);

        emit AlphaUpdated(alphaNumerator, alphaDenominator, _alphaNumerator, _alphaDenominator);

        alphaNumerator = _alphaNumerator;
        alphaDenominator = _alphaDenominator;

        for (uint256 i = 0; i < allMarkets.length; ) {
            accrueInterest(allMarkets[i]);

            unchecked {
                i++;
            }
        }

        _startScoreUpdateRound();
    }

As you can see, at the bottom of this method is a call to _startScoreUpdateRound:

    function _startScoreUpdateRound() internal {
        nextScoreUpdateRoundId++;
        totalScoreUpdatesRequired = totalIrrevocable + totalRevocable;
        pendingScoreUpdates = totalScoreUpdatesRequired;
    }

This method increments the score update round id and then sets the pendingScoreUpdates variable to the sum of all the revocable and irrevocable Prime tokens. An external script then loops through all the users that hold Prime tokens and calls updateScores repeatedly to update their score for each market based on the updated alpha value:

    function updateScores(address[] memory users) external {
        if (pendingScoreUpdates == 0) revert NoScoreUpdatesRequired();
        if (nextScoreUpdateRoundId == 0) revert NoScoreUpdatesRequired();

        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++;
                }
            }

            pendingScoreUpdates--;
            isScoreUpdated[nextScoreUpdateRoundId][user] = true;

            unchecked {
                i++;
            }

            emit UserScoreUpdated(user);
        }
    }

As you can see, if the score for the user has not been updated for the current round, then the interest is accrued (in the executeBoost call) and the user + market score updated in updateScore. The pendingScoreUpdates variable is then decremented. Also, you can see from the first line of the method that it will revert if called when pendingScoreUpdates = 0.

Now, the problem occurs when an eligible user decides to claim their Prime token by calling claim:

    function claim() external {
        if (stakedAt[msg.sender] == 0) revert IneligibleToClaim();
        if (block.timestamp - stakedAt[msg.sender] < STAKING_PERIOD) revert WaitMoreTime();

        stakedAt[msg.sender] = 0;

        _mint(false, msg.sender);
        _initializeMarkets(msg.sender);
    }

This calls _mint:

    function _mint(bool isIrrevocable, address user) internal {
        if (tokens[user].exists) revert IneligibleToClaim();

        tokens[user].exists = true;
        tokens[user].isIrrevocable = isIrrevocable;

        if (isIrrevocable) {
            totalIrrevocable++;
        } else {
            totalRevocable++;
        }

        if (totalIrrevocable > irrevocableLimit || totalRevocable > revocableLimit) revert InvalidLimit();

        emit Mint(user, isIrrevocable);
    }

As you can see isScoreUpdated isn't set to true for the current round id. As such, the user can be successfully passed into a call to updateScores to perform an unnecessary score update and cause the pendingScoreUpdates variable to be decremented. However this now means that another user will not be able to have their score updated (when they should).

The number of users that claim Prime tokens between an alpha/multiplier update and before all the existing holders' scores are updated with updateScores will be how many users cannot have their scores updated (e.g. 5 claims = 5 users who can't have their score updated). This impacts not only the interest rate of the user, but also the interest rate for all the other users in the market (the scope of the impact depends on the alpha/multiplier change).

Below is a diff to the existing test suite that can be executed with yarn test. This test shows how a user can deliberately/accidentally claim their Prime token after an operation that requires a score update (e.g. adding a market, updating a market's multipliers or updating alpha). This causes the script discussed in the README to fail because pendingScoreUpdates has reached 0 earlier than expected. Some users will not be able to have their score updated which will affect the interest of the entire market going forwards:

diff --git a/tests/hardhat/Prime/Prime.ts b/tests/hardhat/Prime/Prime.ts
index 809c48c..056c3ee 100644
--- a/tests/hardhat/Prime/Prime.ts
+++ b/tests/hardhat/Prime/Prime.ts
@@ -708,6 +708,29 @@ describe("PrimeScenario Token", () => {
         expect(await prime.callStatic.getInterestAccrued(vusdt.address, user1.getAddress())).to.be.equal(207283);
       });
 
+      it.only("update scores can be accidentlly/deliberately manipulated", async() => {
+        await xvs.connect(user3).approve(xvsVault.address, bigNumber18.mul(10000));
+        await xvsVault.connect(user3).deposit(xvs.address, 0, bigNumber18.mul(10000));
+        await mine(90 * 24 * 60 * 60);
+        // At this point user3 can claim their prime token
+
+        // Now we add a new market which requires a score update
+        await prime.addMarket(vbnb.address, bigNumber18.mul(1), bigNumber18.mul(1));
+
+        // At the moment we actually only expect 1 score update because user1 is the only user
+        // that has a prime token
+        let pendingScoreUpdates = await prime.pendingScoreUpdates();
+        expect(pendingScoreUpdates).to.be.equal(1);
+        
+        // User3 claims their prime token and then performs a score update
+        await prime.connect(user3).claim();
+        await prime.connect(user3).updateScores([user3.getAddress()]);
+
+        // Now the script tries to update the scores for users that existed before the new
+        // market was added. This now fails.
+        await expect(prime.updateScores([user1.getAddress()])).to.be.revertedWithCustomError(prime, "NoScoreUpdatesRequired");
+      });
+
       it("add existing market after issuing prime tokens - update score manually", async () => {
         await xvs.connect(user3).approve(xvsVault.address, bigNumber18.mul(2000));
         await xvsVault.connect(user3).deposit(xvs.address, 0, bigNumber18.mul(2000));

Tools Used

Manual review + Hardhat test

Recommended Mitigation Steps

When a new Prime token is minted, the isScoreUpdated mapping for the user for the current score update round id should be set to true to prevent the user from decrementing pendingScoreUpdates by calling updateScores. Below is a suggested diff:

diff --git a/contracts/Tokens/Prime/Prime.sol b/contracts/Tokens/Prime/Prime.sol
index 2be244f..5bb6b0e 100644
--- a/contracts/Tokens/Prime/Prime.sol
+++ b/contracts/Tokens/Prime/Prime.sol
@@ -706,6 +706,7 @@ contract Prime is IIncomeDestination, AccessControlledV8, PausableUpgradeable, M
 
         tokens[user].exists = true;
         tokens[user].isIrrevocable = isIrrevocable;
+        isScoreUpdated[nextScoreUpdateRoundId][user] = true;
 
         if (isIrrevocable) {
             totalIrrevocable++;

Assessed type

Other