Prime contract incompatible with underlying assets differing from 18 decimals

[c4-submissions]

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

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #588

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[c4-judge]

fatherGoose1 changed the severity to 3 (High Risk)