Venus Prime - Brenzee's results

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L339-L342 https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L377-L378 https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L397-L405

Vulnerability details

Impact

When a user gets issued an irrevocable Prime token, it is possible that the user can claim the Prime token after the irrevocable token is burned by the Access Control Manager, which contradicts the logic of the contract.

Proof of Concept

User Bob:

• active Venus Protocol user, has supplied and borrowed multiple tokens through Venus.
• does not have any XVS tokens staked in XVSVault
• does not have a Prime token.

Bob decides to participate in the Prime rewards program and deposits 1000 XVS into XVSVault. When Bob deposits XVS tokens into the XVSVault, the contract calls the Prime.xvsUpdated function, that updates the stakedAt variable for Bob to the current block number.

Prime.sol:L377-L378

    } else if (stakedAt[user] == 0 && isAccountEligible && !tokens[user].exists) {
        stakedAt[user] = block.timestamp;

30 days after Bob has staked 1000 XVS into the vault, Venus Protocol decides to issue an irrevocable Prime token to Bob. Prime.issue is called by the Access Control Manager and does the following:

• mints irrevocable Prime token to Bob
• initializes all the markets that are supported by Prime
• does not reset the stakedAt[Bob] to 0

Prime.sol:L336-L342

    Token storage userToken = tokens[users[i]];
    if (userToken.exists && !userToken.isIrrevocable) {
        _upgrade(users[i]);
    } else {
        _mint(true, users[i]);
        _initializeMarkets(users[i]);
    }

When comparing all the possible ways to get/claim a Prime token, stakedAt[user] value always gets set to 0, but it is not done when the irrevocable token is issued:

Prime:claim - stakedAt[user] is set to 0:

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

Prime:issue - if a revocable token is issued, stakedAt[user] is set to 0:

    for (uint256 i = 0; i < users.length; ) {
        _mint(false, users[i]);
        _initializeMarkets(users[i]);
        delete stakedAt[users[i]];

        unchecked {
            i++;
        }
    }

Since stakedAt[user] is not reset to 0 when an irrevocable token is issued, Bob has the ability to exploit the logic.

As a result, this means Bob can:

• Withdraw some of the tokens so the staked balance is under the 1000 XVS threshold and not get his stakedAt value reset.
• Claim the Prime token immediately if Access Control Manager burns the irrevocable token and 90 days have passed from his initial XVS deposit into the XVSVault

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend resetting the stakedAt[user] to 0 when an irrevocable token is issued to the user. In this way, the user cannot abuse the logic.

    Token storage userToken = tokens[users[i]];
    if (userToken.exists && !userToken.isIrrevocable) {
        _upgrade(users[i]);
    } else {
        _mint(true, users[i]);
        _initializeMarkets(users[i]);
        delete stakedAt[users[i]];
    }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Users earned rewards are calculated incorrectly because of the incorrect decimals value used to calculate user's score and markets sumOfMembersScore, which impacts the delta that is added to market's rewardIndex when Prime.accrueInterest function is called.

Proof of Concept

All users rewards are calculated with the following formula:

rewards = (rewardIndex - userRewardIndex) * scoreOfUser;

This means that for user to earn rewards, market's rewardIndex needs to periodically increase.  

markets[vToken].rewardIndex is updated (increased), when Prime.accrueInterest is called.

Prime.sol:L583-L588

    uint256 delta;
    if (markets[vToken].sumOfMembersScore > 0) {
        delta = ((distributionIncome * EXP_SCALE) / markets[vToken].sumOfMembersScore);
    }

    markets[vToken].rewardIndex = markets[vToken].rewardIndex + delta;

From the code snippet above it is assumed that markets[vToken].sumOfMembersScore is precision of 18 decimals.

To ensure that user's score and markets sumOfMemberScore are correctly calculated, in Prime._calculateScore function capital is adjusted to 18 decimals. After that capital is used in Scores.calculateScore function. Note: capital precision from _capitalForScore function is in precision of underlying token decimals.

Prime.sol:660-L663

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

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

The mistake is made when vToken.decimals() is used instead of vToken.underlying().decimals().  

To prove that this is the case, here are vTokens deployed on Binance Smart chain, their decimals and underlying token decimals:

Since vToken.decimals() is used, this means the precision of capital is 18 + (18 - 8) = 28 decimals instead of 18 decimals, which makes the score calculation from Score.calculateScore function incorrect, since the function expects capital to be in precision of 18 decimals.

As a result, delta for market's rewardIndex is incorrectly calculated and it can be 0 even though it shouldn't be, which means that users will not accrue any rewards.

Test

1. Update current test with correct decimals for vTokens

Developers have made a mistake when writing the tests for Prime.sol - in the tests they have set vToken decimals to 18 instead of 8, which makes the tests pass, but on the Binance Smart Chain all of the vToken decimals are 8.

If the decimal value of vToken is set to 8 in the tests, then the tests will fail.

Change the vusdt, veth and vbnb decimals to 8 and run:

npx hardhat test tests/hardhat/Prime/*.ts tests/hardhat/integration/index.ts

This will make the current tests fail.

2. PoC test

Here is a test where it shows, that rewardIndex is still 0 after Prime.accrueInterest is called, even though it should be > 0.

<details> <summary>PoC test</summary>

To run the following test:

1. Create PoC.ts file under the tests/hardhat/Prime path.
2. Copy the code below and paste it into the PoC.ts file.
3. Run npx hardhat test tests/hardhat/Prime/PoC.ts

import { FakeContract, MockContract, smock } from "@defi-wonderland/smock";
import { loadFixture, mine } from "@nomicfoundation/hardhat-network-helpers";
import chai from "chai";
import { BigNumber, Signer } from "ethers";
import { ethers, upgrades } from "hardhat";

import { convertToUnit } from "../../../helpers/utils";
import {
  BEP20Harness,
  ComptrollerLens,
  ComptrollerLens__factory,
  ComptrollerMock,
  ComptrollerMock__factory,
  IAccessControlManager,
  IProtocolShareReserve,
  InterestRateModelHarness,
  PrimeLiquidityProvider,
  PrimeScenario,
  ResilientOracleInterface,
  VBep20Harness,
  XVS,
  XVSStore,
  XVSVault,
  XVSVaultScenario,
} from "../../../typechain";

const { expect } = chai;
chai.use(smock.matchers);

export const bigNumber18 = BigNumber.from("1000000000000000000"); // 1e18
export const bigNumber16 = BigNumber.from("10000000000000000"); // 1e16
export const vTokenDecimals = BigNumber.from(8);

type SetupProtocolFixture = {
  oracle: FakeContract<ResilientOracleInterface>;
  accessControl: FakeContract<IAccessControlManager>;
  comptrollerLens: MockContract<ComptrollerLens>;
  comptroller: MockContract<ComptrollerMock>;
  usdt: BEP20Harness;
  vusdt: VBep20Harness;
  eth: BEP20Harness;
  veth: VBep20Harness;
  xvsVault: XVSVaultScenario;
  xvs: XVS;
  xvsStore: XVSStore;
  prime: PrimeScenario;
  protocolShareReserve: FakeContract<IProtocolShareReserve>;
  primeLiquidityProvider: PrimeLiquidityProvider;
};

async function deployProtocol(): Promise<SetupProtocolFixture> {
  const [wallet, user1, user2, user3] = await ethers.getSigners();

  const oracle = await smock.fake<ResilientOracleInterface>("ResilientOracleInterface");
  const protocolShareReserve = await smock.fake<IProtocolShareReserve>("IProtocolShareReserve");
  const accessControl = await smock.fake<IAccessControlManager>("AccessControlManager");
  accessControl.isAllowedToCall.returns(true);
  const ComptrollerLensFactory = await smock.mock<ComptrollerLens__factory>("ComptrollerLens");
  const ComptrollerFactory = await smock.mock<ComptrollerMock__factory>("ComptrollerMock");
  const comptroller = await ComptrollerFactory.deploy();
  const comptrollerLens = await ComptrollerLensFactory.deploy();
  await comptroller._setAccessControl(accessControl.address);
  await comptroller._setComptrollerLens(comptrollerLens.address);
  await comptroller._setPriceOracle(oracle.address);
  await comptroller._setLiquidationIncentive(convertToUnit("1", 18));
  await protocolShareReserve.MAX_PERCENT.returns("100");

  const tokenFactory = await ethers.getContractFactory("BEP20Harness");
  const usdt = (await tokenFactory.deploy(
    bigNumber18.mul(100000000),
    "usdt",
    BigNumber.from(18),
    "BEP20 usdt",
  )) as BEP20Harness;

  const eth = (await tokenFactory.deploy(
    bigNumber18.mul(100000000),
    "eth",
    BigNumber.from(18),
    "BEP20 eth",
  )) as BEP20Harness;

  const wbnb = (await tokenFactory.deploy(
    bigNumber18.mul(100000000),
    "wbnb",
    BigNumber.from(18),
    "BEP20 wbnb",
  )) as BEP20Harness;

  const interestRateModelHarnessFactory = await ethers.getContractFactory("InterestRateModelHarness");
  const InterestRateModelHarness = (await interestRateModelHarnessFactory.deploy(
    BigNumber.from(18).mul(5),
  )) as InterestRateModelHarness;

  const vTokenFactory = await ethers.getContractFactory("VBep20Harness");
  const vusdt = (await vTokenFactory.deploy(
    usdt.address,
    comptroller.address,
    InterestRateModelHarness.address,
    bigNumber18,
    "VToken usdt",
    "vusdt",
    vTokenDecimals,
    wallet.address,
  )) as VBep20Harness;
  const veth = (await vTokenFactory.deploy(
    eth.address,
    comptroller.address,
    InterestRateModelHarness.address,
    bigNumber18,
    "VToken eth",
    "veth",
    vTokenDecimals,
    wallet.address,
  )) as VBep20Harness;
  const vbnb = (await vTokenFactory.deploy(
    wbnb.address,
    comptroller.address,
    InterestRateModelHarness.address,
    bigNumber18,
    "VToken bnb",
    "vbnb",
    vTokenDecimals,
    wallet.address,
  )) as VBep20Harness;

  //0.2 reserve factor
  await veth._setReserveFactor(bigNumber16.mul(20));
  await vusdt._setReserveFactor(bigNumber16.mul(20));

  oracle.getUnderlyingPrice.returns((vToken: string) => {
    if (vToken == vusdt.address) {
      return convertToUnit(1, 18);
    } else if (vToken == veth.address) {
      return convertToUnit(1200, 18);
    }
  });

  oracle.getPrice.returns((token: string) => {
    if (token == xvs.address) {
      return convertToUnit(3, 18);
    }
  });

  const half = convertToUnit("0.5", 18);
  await comptroller._supportMarket(vusdt.address);
  await comptroller._setCollateralFactor(vusdt.address, half);
  await comptroller._supportMarket(veth.address);
  await comptroller._setCollateralFactor(veth.address, half);

  await eth.transfer(user1.address, bigNumber18.mul(100));
  await usdt.transfer(user2.address, bigNumber18.mul(10000));

  await comptroller._setMarketSupplyCaps([vusdt.address, veth.address], [bigNumber18.mul(10000), bigNumber18.mul(100)]);

  await comptroller._setMarketBorrowCaps([vusdt.address, veth.address], [bigNumber18.mul(10000), bigNumber18.mul(100)]);

  const xvsFactory = await ethers.getContractFactory("XVS");
  const xvs: XVS = (await xvsFactory.deploy(wallet.address)) as XVS;

  const xvsStoreFactory = await ethers.getContractFactory("XVSStore");
  const xvsStore: XVSStore = (await xvsStoreFactory.deploy()) as XVSStore;

  const xvsVaultFactory = await ethers.getContractFactory("XVSVaultScenario");
  const xvsVault: XVSVaultScenario = (await xvsVaultFactory.deploy()) as XVSVaultScenario;

  await xvsStore.setNewOwner(xvsVault.address);
  await xvsVault.setXvsStore(xvs.address, xvsStore.address);
  await xvsVault.setAccessControl(accessControl.address);

  await xvs.transfer(xvsStore.address, bigNumber18.mul(1000));
  await xvs.transfer(user1.address, bigNumber18.mul(1000000));
  await xvs.transfer(user2.address, bigNumber18.mul(1000000));
  await xvs.transfer(user3.address, bigNumber18.mul(1000000));

  await xvsStore.setRewardToken(xvs.address, true);

  const lockPeriod = 300;
  const allocPoint = 100;
  const poolId = 0;
  const rewardPerBlock = bigNumber18.mul(1);
  await xvsVault.add(xvs.address, allocPoint, xvs.address, rewardPerBlock, lockPeriod);

  const primeLiquidityProviderFactory = await ethers.getContractFactory("PrimeLiquidityProvider");
  const primeLiquidityProvider = await upgrades.deployProxy(
    primeLiquidityProviderFactory,
    [accessControl.address, [xvs.address, usdt.address, eth.address], [10, 10, 10]],
    {},
  );

  const primeFactory = await ethers.getContractFactory("PrimeScenario");
  const prime: PrimeScenario = await upgrades.deployProxy(
    primeFactory,
    [
      xvsVault.address,
      xvs.address,
      0,
      1,
      2,
      accessControl.address,
      protocolShareReserve.address,
      primeLiquidityProvider.address,
      comptroller.address,
      oracle.address,
      10,
    ],
    {
      constructorArgs: [wbnb.address, vbnb.address, 10512000],
      unsafeAllow: ["constructor"],
    },
  );

  await xvsVault.setPrimeToken(prime.address, xvs.address, poolId);
  await prime.setLimit(1000, 1000);
  await prime.addMarket(vusdt.address, bigNumber18.mul("1"), bigNumber18.mul("1"));
  await prime.addMarket(veth.address, bigNumber18.mul("1"), bigNumber18.mul("1"));
  await comptroller._setPrimeToken(prime.address);
  await prime.togglePause();

  return {
    oracle,
    comptroller,
    comptrollerLens,
    accessControl,
    usdt,
    vusdt,
    eth,
    veth,
    xvsVault,
    xvs,
    xvsStore,
    prime,
    protocolShareReserve,
    primeLiquidityProvider,
  };
}

describe("PoC", () => {
  let deployer: Signer;
  let user1: Signer;
  let user2: Signer;
  let user3: Signer;
  let comptroller: MockContract<ComptrollerMock>;
  let prime: PrimeScenario;
  let vusdt: VBep20Harness;
  let veth: VBep20Harness;
  let usdt: BEP20Harness;
  let eth: BEP20Harness;
  let xvsVault: XVSVault;
  let xvs: XVS;
  let oracle: FakeContract<ResilientOracleInterface>;
  let protocolShareReserve: FakeContract<IProtocolShareReserve>;
  let primeLiquidityProvider: PrimeLiquidityProvider;
  let vbnb: VBep20Harness;
  let bnb: BEP20Harness;

  before(async () => {
    [deployer, user1, user2, user3] = await ethers.getSigners();
    ({
      comptroller,
      prime,
      vusdt,
      veth,
      usdt,
      eth,
      xvsVault,
      xvs,
      oracle,
      protocolShareReserve,
      primeLiquidityProvider,
    } = await loadFixture(deployProtocol));

    await protocolShareReserve.getUnreleasedFunds.returns("0");
    await protocolShareReserve.getPercentageDistribution.returns("100");

    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 xvs.connect(user2).approve(xvsVault.address, bigNumber18.mul(100));
    await xvsVault.connect(user2).deposit(xvs.address, 0, bigNumber18.mul(100));

    await eth.connect(user1).approve(veth.address, bigNumber18.mul(90));
    await veth.connect(user1).mint(bigNumber18.mul(90));

    await usdt.connect(user2).approve(vusdt.address, bigNumber18.mul(9000));
    await vusdt.connect(user2).mint(bigNumber18.mul(9000));

    await comptroller.connect(user1).enterMarkets([vusdt.address, veth.address]);

    await comptroller.connect(user2).enterMarkets([vusdt.address, veth.address]);

    await vusdt.connect(user1).borrow(bigNumber18.mul(5));
    await veth.connect(user2).borrow(bigNumber18.mul(1));

    const tokenFactory = await ethers.getContractFactory("BEP20Harness");
    bnb = (await tokenFactory.deploy(
      bigNumber18.mul(100000000),
      "bnb",
      BigNumber.from(18),
      "BEP20 bnb",
    )) as BEP20Harness;

    const interestRateModelHarnessFactory = await ethers.getContractFactory("InterestRateModelHarness");
    const InterestRateModelHarness = (await interestRateModelHarnessFactory.deploy(
      BigNumber.from(18).mul(5),
    )) as InterestRateModelHarness;

    const vTokenFactory = await ethers.getContractFactory("VBep20Harness");
    vbnb = (await vTokenFactory.deploy(
      bnb.address,
      comptroller.address,
      InterestRateModelHarness.address,
      bigNumber18,
      "VToken bnb",
      "vbnb",
      BigNumber.from(8),
      deployer.getAddress(),
    )) as VBep20Harness;

    await vbnb._setReserveFactor(bigNumber16.mul(20));
    await primeLiquidityProvider.initializeTokens([bnb.address]);

    oracle.getUnderlyingPrice.returns((vToken: string) => {
      if (vToken == vusdt.address) {
        return convertToUnit(1, 18);
      } else if (vToken == veth.address) {
        return convertToUnit(1200, 18);
      } else if (vToken == vbnb.address) {
        return convertToUnit(300, 18);
      }
    });

    oracle.getPrice.returns((token: string) => {
      if (token == xvs.address) {
        return convertToUnit(3, 18);
      }
    });

    const half = convertToUnit("0.5", 8);
    await comptroller._supportMarket(vbnb.address);
    await comptroller._setCollateralFactor(vbnb.address, half);

    bnb.transfer(user3.getAddress(), bigNumber18.mul(100));

    await comptroller._setMarketSupplyCaps([vbnb.address], [bigNumber18.mul(100)]);
    await comptroller._setMarketBorrowCaps([vbnb.address], [bigNumber18.mul(100)]);

    await bnb.connect(user3).approve(vbnb.address, bigNumber18.mul(90));
    await vbnb.connect(user3).mint(bigNumber18.mul(90));

    await vbnb.connect(user2).borrow(bigNumber18.mul(1));

    await comptroller._setPrimeToken(prime.address);
  });

  it("PoC", async () => {
    const bob = user3;
    // Bob deposits XVS in the vault
    await xvs.connect(bob).approve(xvsVault.address, bigNumber18.mul(2000));
    await xvsVault.connect(bob).deposit(xvs.address, 0, bigNumber18.mul(2000));
    await prime.issue(false, [bob.getAddress()]);
    await prime.addMarket(vbnb.address, bigNumber18.mul(1), bigNumber18.mul(1));

    // Bob mints vBNB/deposits BNB. This calls Prime.accrueInterestAndUpdateScore
    await bnb.connect(bob).approve(vbnb.address, bigNumber18.mul(90));
    await vbnb.connect(bob).mint(bigNumber18.mul(1));

    let market = await prime.markets(vbnb.address);

    // Expect that market.sumOfMembersScore is not 0. This means that the score was updated
    expect(market.sumOfMembersScore).to.not.equal(0);

    // We make the PSR.getUnreleasedFunds to return 103683. This lets Prime contract know that
    // there are unreleased funds and rewardIndex should be updated.
    await protocolShareReserve.getUnreleasedFunds.returns(103683);

    // Call accrueInterest manually.
    //
    // Since there are unreleased funds AND the sumOfMembersScore !== 0,
    // the reward index should be updated.
    await prime.accrueInterest(vbnb.address);
    market = await prime.markets(vbnb.address);

    // The reward index should be > 0, but it is not updated.
    expect(market.rewardIndex).to.equal(0);
  });
});

</details>

Tools Used

Manual Review

Recommended Mitigation Steps

Make sure that underlying token decimals are used instead of vToken decimals when calculating capital in Prime._calculateScore function.

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

Assessed type

Decimal

[L-01] Prime.updateScores can be griefed if a User frontruns the transaction and burns his Prime token.

Link to the code

updateScores function allows anyone to update a user's scores once if _startScoreUpdateRound() is called. This function reverts if a user does not exist.

Proof of Concept

Let's say admin wants to update scores for 100 users. Admin calls the Prime.updateScores and the transaction goes to mempool. At the same time, one of the users decides that he wants to burn his Prime Token and withdraws XVS from the XVSVault. Users transaction is completed first, meaning that now in the 100 user list there is one user, who does not exist anymore and the transaction will revert.

            address user = users[i];

            // @audit-issue Skipping here is preffered.
            // Because if there are 100 users and 1 user is invalid, then the whole transaction will revert
            // For example: Venus Devs call updateScores for 100 users, but one user burns his Prime token -> tx reverts
            if (!tokens[user].exists) revert UserHasNoPrimeToken();
            if (isScoreUpdated[nextScoreUpdateRoundId][user]) continue;

Recommended Solution

Instead of reverting if a user does not exist, skip.

            if (!tokens[user].exists || isScoreUpdated[nextScoreUpdateRoundId][user]) continue;