Venus Prime - 0xTheC0der's results

Lines of code

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

Vulnerability details

Description

The core markets (vToken) of the Venus Protocol typically have 8 decimals, see e.g. vUSDT, vBNB and vETH on BscScan.
However, the Venus Prime protocol extension seems to be developed and tested for markets (vToken) with 18 decimals.

Proof of Concept

In this case, it's better to immediately begin with the PoC in order to demonstrate the impacts.
The following diff changes all markets from 18 to their real-world 8 decimals within the test file for the Prime contract. Furthermore, the initial exchange mantissa (vToken <-> underlying token) is changed from 1e18 to a more realistic value of 2e22, see deplyoment transaction of vUSDT.

diff --git a/tests/hardhat/Prime/Prime.ts b/tests/hardhat/Prime/Prime.ts
index 809c48c..3bb1241 100644
--- a/tests/hardhat/Prime/Prime.ts
+++ b/tests/hardhat/Prime/Prime.ts
@@ -30,6 +30,7 @@ chai.use(smock.matchers);
 
 export const bigNumber18 = BigNumber.from("1000000000000000000"); // 1e18
 export const bigNumber16 = BigNumber.from("10000000000000000"); // 1e16
+export const initExchangeMantissa = BigNumber.from("200000000000000000000000000"); // 2e22
 
 type SetupProtocolFixture = {
   oracle: FakeContract<ResilientOracleInterface>;
@@ -97,30 +98,30 @@ async function deployProtocol(): Promise<SetupProtocolFixture> {
     usdt.address,
     comptroller.address,
     InterestRateModelHarness.address,
-    bigNumber18,
+    initExchangeMantissa,
     "VToken usdt",
     "vusdt",
-    BigNumber.from(18),
+    BigNumber.from(8),
     wallet.address,
   )) as VBep20Harness;
   const veth = (await vTokenFactory.deploy(
     eth.address,
     comptroller.address,
     InterestRateModelHarness.address,
-    bigNumber18,
+    initExchangeMantissa,
     "VToken eth",
     "veth",
-    BigNumber.from(18),
+    BigNumber.from(8),
     wallet.address,
   )) as VBep20Harness;
   const vbnb = (await vTokenFactory.deploy(
     wbnb.address,
     comptroller.address,
     InterestRateModelHarness.address,
-    bigNumber18,
+    initExchangeMantissa,
     "VToken bnb",
     "vbnb",
-    BigNumber.from(18),
+    BigNumber.from(8),
     wallet.address,
   )) as VBep20Harness;
 
@@ -597,10 +598,10 @@ describe("PrimeScenario Token", () => {
           bnb.address,
           comptroller.address,
           InterestRateModelHarness.address,
-          bigNumber18,
+          initExchangeMantissa,
           "VToken bnb",
           "vbnb",
-          BigNumber.from(18),
+          BigNumber.from(8),
           deployer.getAddress(),
         )) as VBep20Harness;
 
@@ -863,10 +864,10 @@ describe("PrimeScenario Token", () => {
         matic.address,
         comptroller.address,
         InterestRateModelHarness.address,
-        bigNumber18,
+        initExchangeMantissa,
         "VToken matic",
         "vmatic",
-        BigNumber.from(18),
+        BigNumber.from(8),
         wallet.address,
       )) as VBep20Harness;
 

Running the modified test with npx hardhat test tests/hardhat/Prime/Prime.ts leads to 6 failed cases:

  PrimeScenario Token
    protocol setup
      ✔ markets added
      ✔ borrow balance
      ✔ get markets in prime
    mint and burn
      ✔ stake and mint (179ms)
      ✔ stake and unstake (130ms)
      ✔ burn revocable token (286ms)
      ✔ cannot burn irrevocable token (189ms)
      ✔ manually burn irrevocable token (152ms)
      ✔ issue (177ms)
      ✔ upgrade (131ms)
    boosted yield
      ✔ calculate score (84ms)
      1) accrue interest - prime token minted after market is added
      2) claim interest
      3) track asset state
      update score
        4) add existing market after issuing prime tokens - update score gradually
        5) add existing market after issuing prime tokens - update score manually
    PLP integration
      6) claim interest
      ✔ APR Estimation
      ✔ Hypothetical APR Estimation (130ms)


  13 passing (12s)
  6 failing

Impact

Closer examination of the failed test assertions reveals that score and interest computations are off by multiple orders of magnitude. The consequences are loss of yield for the user or loss of funds for the protocol (depending on the case) when interest is claimed.
In general, one can see that the Venus Prime protocol is not ready to work with the markets of the Venus protocol although it's supposed to.

Tools Used

Manual review

Recommended Mitigation Steps

In the method Prime._calculateScore(address market, address user), there is L661 which is intended to account for markets (vToken) having less than 18 decimals:

    function _calculateScore(address market, address user) internal returns (uint256) {
        ...

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

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

However, closer examination of that line revealed that it was actually meant to account for underlying tokens (not the vToken itself) having less than 18 decimals. (Correct handling of underlying tokens with != 18 decimals is discussed in a separate report.)

Consequently, in case of 18 decimals (underlying token) and 8 decimals (vToken), which is the standard case (vUSDT, vBNB, vETH), the mentioned line skews the underlying capital for the score computation by multiple orders of magnitude and its removal leads to all test cases passing again:

diff --git a/contracts/Tokens/Prime/Prime.sol b/contracts/Tokens/Prime/Prime.sol
index 2be244f..53d1ed2 100644
--- a/contracts/Tokens/Prime/Prime.sol
+++ b/contracts/Tokens/Prime/Prime.sol
@@ -658,7 +658,6 @@ contract Prime is IIncomeDestination, AccessControlledV8, PausableUpgradeable, M
         oracle.updatePrice(market);
 
         (uint256 capital, , ) = _capitalForScore(xvsBalanceForScore, borrow, supply, market);
-        capital = capital * (10 ** (18 - vToken.decimals()));
 
         return Scores.calculateScore(xvsBalanceForScore, capital, alphaNumerator, alphaDenominator);
     }

Assessed type

Decimal

Lines of code

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

Vulnerability details

Description

The underlying assets of the core markets (vToken) of the Venus Protocol typically have 18 decimals on Binance Smart Chain, e.g. USDT, BNB, WETH. However, there are exceptions like the vUST market (also a core market) with the underlying UST token which has just 6 decimals.
Moreover, the Venus Prime protocol extension seems to be developed and tested for underyling assets with 18 decimals only.

Issues:

1. The MAX_DISTRIBUTION_SPEED constant is 1e18 and therefore implicitly allows faster assets distributions for assets having less than 18 decimals, see also PrimeLiquidityProvider._setTokenDistributionSpeed(...).
2. The Prime._calculateScore(...) method wrongly accounts for the decimals in L661 by using the market's (vToken) decimals instead of the underlying asset's decimals. This would be necessary to upscale the underlying capital allocation to 18 decimals for the subsequent score computation.

   function _calculateScore(address market, address user) internal returns (uint256) {
       ...

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

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

(Please note that there is also an issue when markets (vToken) differ from 18 decimals, but this is discussed in a separate report.)

Proof of Concept

In this case, it's better to immediately begin with the PoC in order to demonstrate the impacts.
The following diff changes the MATIC token (18 decimals) to the UST token (6 decimals) within all the PrimeLiquidityProvider integration test cases of the Prime test file. Note that this also includes the reduction of all related values to 6 decimals, i.e. mints, approvals, transfers, limits, test assertions and the distributions speed.

diff --git a/tests/hardhat/Prime/Prime.ts b/tests/hardhat/Prime/Prime.ts
index 809c48c..959c140 100644
--- a/tests/hardhat/Prime/Prime.ts
+++ b/tests/hardhat/Prime/Prime.ts
@@ -30,6 +30,7 @@ chai.use(smock.matchers);
 
 export const bigNumber18 = BigNumber.from("1000000000000000000"); // 1e18
 export const bigNumber16 = BigNumber.from("10000000000000000"); // 1e16
+export const bigNumber6 = BigNumber.from("1000000"); // 1e6
 
 type SetupProtocolFixture = {
   oracle: FakeContract<ResilientOracleInterface>;
@@ -821,7 +822,7 @@ describe("PrimeScenario Token", () => {
     });
   });
 
-  describe("PLP integration", () => {
+  describe.only("PLP integration", () => {
     let comptroller: MockContract<ComptrollerMock>;
     let prime: PrimeScenario;
     let vusdt: VBep20Harness;
@@ -846,10 +847,10 @@ describe("PrimeScenario Token", () => {
 
       const tokenFactory = await ethers.getContractFactory("BEP20Harness");
       matic = (await tokenFactory.deploy(
-        bigNumber18.mul(100000000),
-        "matic",
-        BigNumber.from(18),
-        "BEP20 MATIC",
+        bigNumber6.mul(100000000),
+        "ust",
+        BigNumber.from(6),
+        "BEP20 UST",
       )) as BEP20Harness;
 
       await primeLiquidityProvider.initializeTokens([matic.address]);
@@ -864,8 +865,8 @@ describe("PrimeScenario Token", () => {
         comptroller.address,
         InterestRateModelHarness.address,
         bigNumber18,
-        "VToken matic",
-        "vmatic",
+        "VToken ust",
+        "vust",
         BigNumber.from(18),
         wallet.address,
       )) as VBep20Harness;
@@ -886,23 +887,23 @@ describe("PrimeScenario Token", () => {
 
       await comptroller._setCollateralFactor(vmatic.address, half);
 
-      await comptroller._setMarketSupplyCaps([vmatic.address], [bigNumber18.mul(10000)]);
-      await comptroller._setMarketBorrowCaps([vmatic.address], [bigNumber18.mul(10000)]);
+      await comptroller._setMarketSupplyCaps([vmatic.address], [bigNumber6.mul(10000)]);
+      await comptroller._setMarketBorrowCaps([vmatic.address], [bigNumber6.mul(10000)]);
 
-      await prime.addMarket(vmatic.address, bigNumber18.mul("1"), bigNumber18.mul("1"));
+      await prime.addMarket(vmatic.address, bigNumber6.mul("1"), bigNumber6.mul("1"));
 
       await xvs.connect(user1).approve(xvsVault.address, bigNumber18.mul(10000));
       await xvsVault.connect(user1).deposit(xvs.address, 0, bigNumber18.mul(10000));
       await mine(90 * 24 * 60 * 60);
       await prime.connect(user1).claim();
 
-      await matic.transfer(user1.getAddress(), bigNumber18.mul(90));
-      await matic.connect(user1).approve(vmatic.address, bigNumber18.mul(90));
-      await vmatic.connect(user1).mint(bigNumber18.mul(90));
+      await matic.transfer(user1.getAddress(), bigNumber6.mul(90));
+      await matic.connect(user1).approve(vmatic.address, bigNumber6.mul(90));
+      await vmatic.connect(user1).mint(bigNumber6.mul(90));
 
-      const speed = convertToUnit(1, 18);
+      const speed = convertToUnit(1, 6); // important to also reduce distribution speed to 6 decimals
       await primeLiquidityProvider.setTokensDistributionSpeed([matic.address], [speed]);
-      await matic.transfer(primeLiquidityProvider.address, bigNumber18.mul(10000));
+      await matic.transfer(primeLiquidityProvider.address, bigNumber6.mul(10000));
     });
 
     it("claim interest", async () => {
@@ -917,7 +918,7 @@ describe("PrimeScenario Token", () => {
       await mine(100);
       await primeLiquidityProvider.accrueTokens(matic.address);
       plpAccrued = await primeLiquidityProvider.tokenAmountAccrued(matic.address);
-      expect(plpAccrued).to.be.equal(bigNumber18.mul(102)); // (1 * 100) + 2 = 102
+      expect(plpAccrued).to.be.equal(bigNumber6.mul(102)); // (1 * 100) + 2 = 102
 
       await prime.accrueInterest(vmatic.address);
       interest = await prime.interests(vmatic.address, user1.getAddress());
@@ -931,20 +932,20 @@ describe("PrimeScenario Token", () => {
       // 103000000000000000000 / 948683298050513937723 = 108571532999114341
       // 1000000000000000000 / 948683298050513937723 = 1054092553389459
       // 108571532999114341 + 1054092553389459 = 109625625552503800
-      expect(market.rewardIndex).to.be.equal("109625625552503800");
+      expect(market.rewardIndex).to.be.equal("109625"); // cut 18-6 = 12 decimals
 
       interest = await prime.interests(vmatic.address, user1.getAddress());
       expect(interest.score).to.be.equal("948683298050513937723");
       //109625625552503800 * 948683298050513937723 = 103999999999999999163
-      expect(interest.accrued).to.be.equal("103999999999999999163");
-      expect(interest.rewardIndex).to.be.equal("109625625552503800");
+      expect(interest.accrued).to.be.equal("103999406"); // cut 18-6 = 12 decimals
+      expect(interest.rewardIndex).to.be.equal("109625"); // cut 18-6 = 12 decimals
 
       const beforeBalance = await matic.balanceOf(user1.getAddress());
       expect(beforeBalance).to.be.equal(0);
       await prime["claimInterest(address,address)"](vmatic.address, user1.getAddress());
       const afterBalance = await matic.balanceOf(user1.getAddress());
       // 103999999999999999163 + 1000000000000000000 = 104999999999999998571
-      expect(afterBalance).to.be.equal("104999999999999998571");
+      expect(afterBalance).to.be.equal("104999318"); // cut 18-6 = 12 decimals
     });
 
     it("APR Estimation", async () => {
@@ -956,8 +957,8 @@ describe("PrimeScenario Token", () => {
       let apr = await prime.estimateAPR(
         vmatic.address,
         user1.getAddress(),
-        bigNumber18.mul(100),
-        bigNumber18.mul(100),
+        bigNumber6.mul(100),
+        bigNumber6.mul(100),
         bigNumber18.mul(1000000),
       );
       expect(apr.supplyAPR.toString()).to.be.equal("525600000");
@@ -966,8 +967,8 @@ describe("PrimeScenario Token", () => {
       apr = await prime.estimateAPR(
         vmatic.address,
         user1.getAddress(),
-        bigNumber18.mul(100),
-        bigNumber18.mul(50),
+        bigNumber6.mul(100),
+        bigNumber6.mul(50),
         bigNumber18.mul(1000000),
       );
       expect(apr.supplyAPR.toString()).to.be.equal("700800000");
@@ -976,8 +977,8 @@ describe("PrimeScenario Token", () => {
       apr = await prime.estimateAPR(
         vmatic.address,
         user1.getAddress(),
-        bigNumber18.mul(100),
-        bigNumber18.mul(0),
+        bigNumber6.mul(100),
+        bigNumber6.mul(0),
         bigNumber18.mul(1000000),
       );
       expect(apr.supplyAPR.toString()).to.be.equal("0");

Running the modified PrimeLiquidityProvider integration test with npx hardhat test tests/hardhat/Prime/Prime.ts leads to 1 failed case:

  PrimeScenario Token
    PLP integration
      1) claim interest
      ✔ APR Estimation (44ms)
      ✔ Hypothetical APR Estimation (144ms)


  2 passing (7s)
  1 failing

  1) PrimeScenario Token
       PLP integration
         claim interest:

      AssertionError: expected 948683298050514 to equal 948683298050513937723. The numerical values of the given "ethers.BigNumber" and "string" inputs were compared, and they differed.
      + expected - actual

      -948683298050514
      +948683298050513937723

      at Context.<anonymous> (tests\hardhat\Prime\Prime.ts:911:36)

Impact

Closer examination of the failed test case reveals that the score and subsequently the interest computation are off by multiple orders of magnitude. The consequence is loss of yield for the user when interest is claimed.
In general, one can see that the Venus Prime protocol is not ready to work with all core markets of the Venus protocol.

Tools Used

Manual review

Recommended Mitigation Steps

Accounting for the underlying asset's decimals instead of the market's (vToken) decimals in Prime._calculateScore(...) fixes the issue and lets all Prime cases pass.

diff --git a/contracts/Tokens/Prime/Prime.sol b/contracts/Tokens/Prime/Prime.sol
index 2be244f..024da03 100644
--- a/contracts/Tokens/Prime/Prime.sol
+++ b/contracts/Tokens/Prime/Prime.sol
@@ -2,6 +2,7 @@
 pragma solidity 0.8.13;
 
 import { SafeERC20Upgradeable, IERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
+import { IERC20MetadataUpgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/IERC20MetadataUpgradeable.sol";
 import { AccessControlledV8 } from "@venusprotocol/governance-contracts/contracts/Governance/AccessControlledV8.sol";
 import { ResilientOracleInterface } from "@venusprotocol/oracle/contracts/interfaces/OracleInterface.sol";
 import { PausableUpgradeable } from "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";
@@ -658,7 +659,7 @@ contract Prime is IIncomeDestination, AccessControlledV8, PausableUpgradeable, M
         oracle.updatePrice(market);
 
         (uint256 capital, , ) = _capitalForScore(xvsBalanceForScore, borrow, supply, market);
-        capital = capital * (10 ** (18 - vToken.decimals()));
+        capital = capital * (10 ** (18 - IERC20MetadataUpgradeable(_getUnderlying(market)).decimals()));
 
         return Scores.calculateScore(xvsBalanceForScore, capital, alphaNumerator, alphaDenominator);
     }

Assessed type

Decimal

Summary

• Low 1: Owner of PrimeLiquidityProvider contract can drain all tokens
• Low 2: BLOCKS_PER_YEAR is immuatble
• Low 3: MAX_DISTRIBUTION_SPEED is only intended for underlying assets with 18 decimals
• Low 4: Use of outdated OpenZeppelin library version
• Low 5: Method xvsUpdated(...) is callabe by anyone
• Low 6: Method accrueInterestAndUpdateScore(...) is callabe by anyone
• Low 7: Missing check for stalling oracle in score computation
• Non-critical 1: Missing method removeMarket(...)
• Non-critical 2: No support for markets (vToken) with more than 18 decimals
• Non-critical 3: Hypothetical underflow

Low 1: Owner of PrimeLiquidityProvider contract can drain all tokens

The method PrimeLiquidityProvider.sweepToken(...) allows the contract owner to dain all its tokens.

    function sweepToken(IERC20Upgradeable token_, address to_, uint256 amount_) external onlyOwner {
        uint256 balance = token_.balanceOf(address(this));
        if (amount_ > balance) {
            revert InsufficientBalance(amount_, balance);
        }

        emit SweepToken(address(token_), to_, amount_);

        token_.safeTransfer(to_, amount_);
    }

Mitigation: Add a check to exclude underlying market tokens and therefore only allow stranded tokens to be withdrawn.

Low 2: BLOCKS_PER_YEAR is immuatble

Having Prime.BLOCKS_PER_YEAR as an immutable value leads to the following concerns, since the average amount of blocks per time should be constant, but the reality looks like this: Binance Smart Chain Blocks Per Day.

1. Although the amount of blocks per time should be constant, we cannot safely assume that this value is not subject to change during the course of a future upgrade. I recommend to add a setter function.
2. This value is an estimate at best and therefore only suitable for subsquent estimations, but not for calculations see Prime._incomeDistributionYearly(...). I suggest to re-evaluate if BLOCKS_PER_YEAR is good enough for subsequent APR computations.

Low 3: MAX_DISTRIBUTION_SPEED is only intended for underlying assets with 18 decimals

The PrimeLiquidityProvider.MAX_DISTRIBUTION_SPEED constant is 1e18 and therefore implicitly allows faster assets distributions for assets having less than 18 decimals, see also PrimeLiquidityProvider._setTokenDistributionSpeed(...).

Low 4: Use of outdated OpenZeppelin library version

According to package.json, openzeppelin-contracts v4.8.3 and openzeppelin-contracts-upgradeable v4.8.0 is used. Both versions are outdated and have known vulnerabilities, see Snyk Vulnerability Database.

Low 5: Method xvsUpdated(...) is callabe by anyone

The method Prime.xvsUpdated(...) is callable by anyone although only meant to be invoked by the XVSVault contract.

Low 6: Method accrueInterestAndUpdateScore(...) is callabe by anyone

The method Prime.accrueInterestAndUpdateScore(...) is callable by anyone although only meant to be invoked by the PolicyFacet contract.

Low 7: Missing check for stalling oracle in score computation

The oracle calls in L657-658 of Prime._calculateScore(...) do not perform any kind of check to revert on stalling oracle prices.

Non-critical 1: Missing method removeMarket(...)

There is a method Prime.addMarket(...). However, it might be required in some occasions to remove a market (vToken) from Prime.

Non-critical 2: No support for markets (vToken) with more than 18 decimals

The vToken initializer allows to create markets with user-defined decimals. However, L661 in Prime._calculateScore(...) allows the usage of markets with less than or equal to 18 decimals only. Underflow error in case of more than 18 decimals.

    function _calculateScore(address market, address user) internal returns (uint256) {
        ...
        capital = capital * (10 ** (18 - vToken.decimals()));
        ...
    }

Non-critical 3: Hypothetical underflow

In line L800 of Prime._updateScore(...)

    function _updateScore(address user, address market) internal {
        ...
        markets[market].sumOfMembersScore = markets[market].sumOfMembersScore - interests[market][user].score + score;
        ...
    }

there might happen an underflow leading to DoS in case interests[market][user].score > markets[market].sumOfMembersScore. Although such a case seems not possible due to the current contract logic, I still recommend to rewrite as:

    function _updateScore(address user, address market) internal {
        ...
        markets[market].sumOfMembersScore = (markets[market].sumOfMembersScore + score) - interests[market][user].score;
        ...
    }