Venus Prime - Testerbot's results

Lines of code

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

Vulnerability details

Impact

Protocol specifications state that a user cannot have less than the minimum xvs staked if they are not irrevocable prime token users. In other words, only holders of irrevocables prime tokens can have less than the minimum xvs staked.

The problem arises when a user is manually given a prime token via the function issue because the stakedAt value is not cleared:

else {
    // @audit-issue Does not clean stakedAt
    _mint(true, users[i]);
    _initializeMarkets(users[i]);
}

If the same user gets the irrevocable prime token burned, claim() can still be called to get a normal prime token even if the user does not have the minimum staked, which should not be allowed.

PoC

In the mint and burn section we describe:

it.only("Possible to have less than minimum XVS and not being irrevocable", async () => {
      await xvs.connect(user3).approve(xvsVault.address, bigNumber18.mul(2000));
      await xvsVault.connect(user3).deposit(xvs.address, 0, bigNumber18.mul(2000));

      await mine(90 * 24 * 60 * 60);

      // User 3 is granted an irrevocable token.
      await prime.issue(true, [user3.getAddress()]);

      let token = await prime.tokens(user3.getAddress());
      expect(token.isIrrevocable).to.be.equal(true);
      expect(token.exists).to.be.equal(true);

      // User will now withdraw 1500 XVS, only 500 left which is less than the minimum.
      await xvsVault.connect(user3).requestWithdrawal(xvs.address, 0, bigNumber18.mul(1500));

      // Because user was irrevocable his token was not burned.
      token = await prime.tokens(user3.getAddress());
      expect(token.isIrrevocable).to.be.equal(true);
      expect(token.exists).to.be.equal(true);

      // Admin manually burned user3 prime token
      await prime.burn(user3.getAddress());
      token = await prime.tokens(user3.getAddress());
      expect(token.isIrrevocable).to.be.equal(false);
      expect(token.exists).to.be.equal(false);

      // User 3 can claim because stakedAt was not deleted during manual issue.
      // This means that user 3 has a normal prime token but it has less that the minimum xvs staked.
      await prime.connect(user3).claim();
      token = await prime.tokens(user3.getAddress());
      expect(token.exists).to.be.equal(true);
      expect(token.isIrrevocable).to.be.equal(false);      
});

Tools Used

Manual Review

Recommended Mitigation Steps

Add delete stakedAt[users[i]] when manual issuing an irrevocable token.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The updateScores function is used to manually update users scores, devlopers have shared their reasoning of this in the documentation. Any change in the alpha and the multipliers will unbalace the reward system because the change cannot be propagated to all users, the updateScores allow to propagate this change.

The issue is that this system can be bricked. If a user that did not have a prime token during the change of multipliers or alpha mints a prime token and then burn it, it will decrement the variable pendingScoreUpdates which means that one of the real pending scores to be updated will not get updated. For example, if 10 users do this, then 10 pending scores could not be updated.

PoC

Add the following in the update score tests of Prime.ts:

it.only("Update score system can be bricked", async () => {
        await xvs.connect(user3).approve(xvsVault.address, bigNumber18.mul(2000));
        await xvsVault.connect(user3).deposit(xvs.address, 0, bigNumber18.mul(2000));

        await xvs.connect(user2).approve(xvsVault.address, bigNumber18.mul(2000));
        await xvsVault.connect(user2).deposit(xvs.address, 0, bigNumber18.mul(2000));

        await mine(90 * 24 * 60 * 60);
        await prime.connect(user3).claim();

        await prime.updateAlpha(4, 5);
       
        const totalScoreUpdatesRequired = await prime.totalScoreUpdatesRequired();
        const pendingScoreUpdates = await prime.pendingScoreUpdates();
        
        // User1 and User3 scores are pending to be updated.
        expect(totalScoreUpdatesRequired).to.be.equal(2);
        expect(pendingScoreUpdates).to.be.equal(2);
        
        // Update score system working properly
        await prime.updateScores([user1.getAddress(), user3.getAddress()]);

        await prime.updateAlpha(1, 2);

        const totalScoreUpdatesRequired2 = await prime.totalScoreUpdatesRequired();
        const pendingScoreUpdates2 = await prime.pendingScoreUpdates();
        
        // User1 and User3 scores are pending to be updated again.
        expect(totalScoreUpdatesRequired2).to.be.equal(2);
        expect(pendingScoreUpdates2).to.be.equal(2);

        await prime.connect(user2).claim();
        await xvsVault.connect(user2).requestWithdrawal(xvs.address, 0, bigNumber18.mul(2000));

        // Now update score system is broken. You can only update either user1 or user3 but not both.
        await prime.updateScores([user1.getAddress(), user3.getAddress()]);
      })

Tools Used

Manual Review

Recommended Mitigation Steps

When burning someone prime token do not decrement pendingScoreUpdates

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Incorrect computation of the capital variable due to an incorrect decimal scaling. This directly impacts the computation of user's score.

PoC

The function _calculateScore calculates the score for a given user and a given market. One of the core variables is the capital variable:

// @audit-info Returns with decimals of underlying.
(uint256 capital, , ) = _capitalForScore(xvsBalanceForScore, borrow, supply, market);
// @audit-issue It should be vToken.underlying.decimals.
capital = capital * (10 ** (18 - vToken.decimals()));

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

As shown above, the code obtains the capital variable using the _capitalForScore function. It is important to know that capital is in supply and borrow units, which are the underlying token of a given market.

Since underlying tokens can have different decimals, they try to convert capital variable to 18 decimals, in order to be used in conjunction with xvsBalanceForScore to calculate the user's score. The conversion is incorrect because it should be using the decimals of the underlying token and not the decimals of the vToken.

One clear example is the case of the Venus USDC Market, the vUSDC has 8 decimals:

https://bscscan.com/address/0xecA88125a5ADbe82614ffC12D0DB554E2e2867C8#readProxyContract

But the underlying token USDC has 18 decimals:

https://bscscan.com/address/0x8ac76a51cc950d9822d68b83fe1ad97b32cd580d#readProxyContract

Tools Used

Manual Review

Recommended Mitigation Steps

Rewrite the line as:

capital = capital * (10 ** (18 - _getUnderlying(vToken).decimals()));



## Assessed type

Decimal

Lines of code

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

Vulnerability details

Impact

The formula for a user's score depends on the xvs staked and the capital. One core variable in the calculation of a user's score is alpha which represents the weight for xvs and capital. It has been stated in the documentation that alpha can range from 0 to 1 depending on what kind of incentive the protocol wants to drive (XVS Staking or supply/borrow). Please review Significance of α subsection.

When alpha is 1, xvs has all the weight when calculating a user's score, and capital has no weight. If we see the Cobb-Douglas function, the value of capital doesn't matter, it will always return 1 since capital^0 is always 1. So, a user does not need to borrow or lend in a given market since capital does not have any weight in the score calculation.

The issue is an inconsistency in the implementation of the Cobb-Douglas function.

Developers have added an exception: if (xvs == 0 || capital == 0) return 0;
Because of this the code will always return 0 if either the xvs or the capital is zero, but we know this should not be the case when the alpha value is 1.

PoC

To check how it works: In describe('boosted yield') add the following code:

it.only("calculate score only staking", async () => {
      const xvsBalance = bigNumber18.mul(5000);
      const capital = bigNumber18.mul(0);

      await prime.updateAlpha(1, 1); // 1

      //  5000^1 * 0^0 = 5000
      // BUT IS RETURNING 0!!!
      expect((await prime.calculateScore(xvsBalance, capital)).toString()).to.be.equal("0");
});

Tools Used

Manual Review

Recommended Mitigation Steps

Only return 0 when (xvs = 0 or capital = 0) *and alpha is not 1.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The Prime.sol contract doesn't have any function to remove added markets. This could lead to 2 different scenarios:

1. Cannot remove a vulnerable market
2. Cannot add new markets when the markets list is full.

In the case #2 it is because the protocol limits the amount of markets that can be added with _ensureMaxLoops.

PoC

Case #1 is pretty straight forward: Prime.sol doesn't have any function to remove markets.

Case #2 is a special case, since the addMarket() function calls _ensureMaxLoops(allMarkets.length), this in order to avoid DOS as per their own code:
https://github.com/code-423n4/2023-05-venus/blob/main/contracts/MaxLoopsLimitHelper.sol

However the code of _ensureMaxLoops() states the following:

 function _ensureMaxLoops(uint256 len) internal view {
        if (len > maxLoopsLimit) {
            revert MaxLoopsLimitExceeded(maxLoopsLimit, len);
        }

So in the case that a market is added so that allMarkets.length > maxLoopsLimit the function would revert thus reverting the addMarket() call. Admin won`t be able to add a new market.

Tools Used

Manual Review

Recommended Mitigation Steps

1. Case #1 Add a removeMarket() function that allows the admin to remove a specific market.
2. Case #2 either increase maxLoopsLimit++ or add a removeMarket function to remove an "obsolete" or "deprecated" market without updating _ensureMaxLoops prior adding a new market. This way a slot would be available to add the new market.

Assessed type

Other