Venus Prime - ether_sky's results

Lines of code

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

Vulnerability details

Impact

When the system tries to update Alpha or Market etc, the number of current users is stored as pendingScoreUpdates. And this value is deducted every time we update the user's score. Problems arise when new users are added to the system before all users' scores have been updated. Anyone can call the updateScores function, so if someone calls this function with the just added user, pendingScoreUpdates will also be deducted. So the user trying to update his score at last will not be able to do so because pendingScoreUpdates will already be 0. If this user's stake amount is large, this will be a serious problem. (Of course, each user has the option to update his or her score by calling the xvsUpdated function or the accrueInterestAndUpdateScore function, but this requires more knowledge from the user. If the pendingScoreUpdates value is 0, you can easily assume that all users' scores have been updated.)

Proof of Concept

Add test below to tests/hardhat/Prime/Prime.ts. I added user1 and user2 and updated Alpha. And added another user (user3) before updating these user's score. Then updated the scores of user1 and user3. At this point, user2 cannot update his score.

  describe("update scores test", () => {
    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 protocolShareReserve: FakeContract<IProtocolShareReserve>;

    beforeEach(async () => {
      ({ comptroller, prime, vusdt, veth, usdt, eth, xvsVault, xvs, protocolShareReserve } = 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(10000));
      await xvsVault.connect(user2).deposit(xvs.address, 0, bigNumber18.mul(10000));
      await mine(90 * 24 * 60 * 60);
      await prime.connect(user2).claim();

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

    it("update alpha and add new user", 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);

      await prime.updateAlpha(4, 5);
      // update Alpha, so need to update scores of user1, user2.
      expect(await prime.pendingScoreUpdates()).to.be.equal(2);

      // add user3 to the system
      await prime.connect(user3).claim();
      // system has 3 users now and one updates score of user1, user3
      await prime.updateScores([user1.getAddress(), user3.getAddress()]);

      // oh, there is no pending score updates,
      expect(await prime.pendingScoreUpdates()).to.be.equal(0);

      // so we can't update score of user2. this is accidient
      await expect(prime.updateScores([user2.getAddress()])).to.be.revertedWithCustomError(
        prime,
        "NoScoreUpdatesRequired",
      );

      // please uncomment after applying changes to '_mint' function and run test
      // expect(await prime.pendingScoreUpdates()).to.be.equal(1);
      // await prime.updateScores([user2.getAddress()]);
    });
  });

Tools Used

Recommended Mitigation Steps

Add code below to _mint function.

if (pendingScoreUpdates > 0) {
   totalScoreUpdatesRequired ++;
   pendingScoreUpdates ++;
}

You can confirm that the test will be passed by uncommenting the last 2 lines in the test above and commenting the 2 lines above.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

If the decimals of the vToken is not equal to 18, the estimateAPR function returns an incorrect value.

Proof of Concept

In Prime, all scores were calculated using the _calculateScore function, which performed the calculations below for normalization.

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

However, this is missing from the user score calculation in estimateAPR function.

As a result, estimateAPR function may return incorrect values Add the test code below to tests/hardhat/Prime/Prime.ts. For testing, I deployed vMatic token as 8-decimals token and added 2 users to the system. Then, for one user, calculate the APR using calculateAPR and estimateAPR. These two values should be the same because I used the same input values.

  describe("Estimate APR", () => {
    let comptroller: MockContract<ComptrollerMock>;
    let prime: PrimeScenario;
    let vusdt: VBep20Harness;
    let veth: VBep20Harness;
    let vmatic: VBep20Harness;
    let matic: BEP20Harness;
    let xvsVault: XVSVault;
    let xvs: XVS;
    let oracle: FakeContract<ResilientOracleInterface>;
    let protocolShareReserve: FakeContract<IProtocolShareReserve>;
    let primeLiquidityProvider: PrimeLiquidityProvider;

    beforeEach(async () => {
      const [wallet, user1] = await ethers.getSigners();

      ({ comptroller, prime, vusdt, veth, xvsVault, xvs, oracle, protocolShareReserve, primeLiquidityProvider } =
        await loadFixture(deployProtocol));

      await protocolShareReserve.getUnreleasedFunds.returns("0");
      await protocolShareReserve.getPercentageDistribution.returns("100");
      await primeLiquidityProvider.setPrimeToken(prime.address);

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

      await primeLiquidityProvider.initializeTokens([matic.address]);
      const interestRateModelHarnessFactory = await ethers.getContractFactory("InterestRateModelHarness");
      const InterestRateModelHarness = (await interestRateModelHarnessFactory.deploy(
        BigNumber.from(18).mul(5),
      )) as InterestRateModelHarness;

      const vTokenFactory = await ethers.getContractFactory("VBep20Harness");
      vmatic = (await vTokenFactory.deploy(
        matic.address,
        comptroller.address,
        InterestRateModelHarness.address,
        bigNumber18,
        "VToken matic",
        "vmatic",
        // 8-decimals
        BigNumber.from(8),
        wallet.address,
      )) as VBep20Harness;

      const half = convertToUnit("0.5", 18);
      await vmatic._setReserveFactor(bigNumber16.mul(20));
      await comptroller._supportMarket(vmatic.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 == vmatic.address) {
          return convertToUnit(1, 18);
        }
      });

      await comptroller._setCollateralFactor(vmatic.address, half);

      await comptroller._setMarketSupplyCaps([vmatic.address], [bigNumber18.mul(10000)]);
      await comptroller._setMarketBorrowCaps([vmatic.address], [bigNumber18.mul(10000)]);

      await prime.addMarket(vmatic.address, bigNumber18.mul("1"), bigNumber18.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 xvs.connect(user2).approve(xvsVault.address, bigNumber18.mul(10000));
      await xvsVault.connect(user2).deposit(xvs.address, 0, bigNumber18.mul(10000));
      await mine(90 * 24 * 60 * 60);
      await prime.connect(user2).claim();

      await matic.transfer(user2.getAddress(), bigNumber18.mul(90));
      await matic.connect(user2).approve(vmatic.address, bigNumber18.mul(90));
      await vmatic.connect(user2).mint(bigNumber18.mul(90));

      const speed = convertToUnit(1, 18);
      await primeLiquidityProvider.setTokensDistributionSpeed([matic.address], [speed]);
      await matic.transfer(primeLiquidityProvider.address, bigNumber18.mul(10000));
    });

    it("Discrepancies between 'calculateAPR' and 'estimateAPR'", async () => {
      const decimals_m = await vmatic.decimals();
      expect(decimals_m).to.be.equal(8);
      const borrow_m = await vmatic.borrowBalanceStored(user1.getAddress());
      const exchangeRate_m = await vmatic.exchangeRateStored();
      const balanceOfAccount_m = await vmatic.balanceOf(user1.getAddress());
      // bigNumber18 = EXP_SCALE
      const supply_m = exchangeRate_m.mul(balanceOfAccount_m).div(bigNumber18);
      const userInfo_m = await xvsVault.getUserInfo(xvs.address, 0, user1.getAddress());
      // xvs - pendingWithdrawals
      const xvsBalanceOfUser_m = userInfo_m.amount.sub(userInfo_m.pendingWithdrawals);

      const apr1_m = await prime.calculateAPR(vmatic.address, user1.getAddress());
      const apr2_m = await prime.estimateAPR(
        vmatic.address,
        user1.getAddress(),
        borrow_m,
        supply_m,
        xvsBalanceOfUser_m,
      );
      // two values should be the same, but the test will be failed due to miscalculation of 'estimateAPR'
      expect(apr1_m.supplyAPR.toString()).to.be.equal(apr2_m.supplyAPR.toString());
    });
  });

And the test was failed with

AssertionError: expected '584000000' to equal '11679'
+ expected - actual
-584000000
+11679

As you can see, there was a significant difference between the two values.

Tools Used

Recommended Mitigation Steps

Add below to the estimateAPR function.

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

Then the test will be passed.

One more. Of course it's not completely bug-free, but capital is based on the decimals of the underlying token. (The denominator and numerator use the same decimals in score calculation, so this does not affect the score/sumOfMembersScore). So, for clarity, I recommend changing capital = capital * (10 ** (18 - vToken.decimals())); to:

address underlying = _getUnderlying(vToken);
IERC20Upgradeable asset = IERC20Upgradeable(underlying);
capital = capital * (10 ** (18 - asset.decimals()));

Assessed type

Decimal

address underlying = _getUnderlying(vToken);
IERC20Upgradeable asset = IERC20Upgradeable(underlying);
capital = capital * (10 ** (18 - asset.decimals()));

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L216-L225 https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L259-L261 https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L237-L239

Vulnerability details

Impact

The owner can withdraw the desired amount by calling the sweepToken function. If the tokenAmountAccrued value becomes greater than the PLP balance for a specific token, the accrueTokens function will be reverted with Arithmatic Underflow. This means that no one can call the accrueInterest and claimInterest functions on Prime until the PLP has sufficient balance. i.e the system may crash accidentally.

And getEffectiveDistributionSpeed function can be reverted also.

if (balance - accrued > 0) {
    return distributionSpeed;
}

Proof of Concept

Add the test function below to the describe("Accrue tokens") section of tests/hardhat/Prime/PrimeLiquidityProvider.ts.

   it("Sweep tokens and accrue amount for tokenA", async () => {
      const balanceBefore = await tokenA.balanceOf(primeLiquidityProvider.address);
      expect(await tokenA.balanceOf(primeLiquidityProvider.address)).to.equal(tokenAInitialFund);

      await mine(10);
      const lastAccruedBlock = await primeLiquidityProvider.lastAccruedBlock(tokenA.address);
      await primeLiquidityProvider.accrueTokens(tokenA.address);
      const currentBlock = await primeLiquidityProvider.getBlockNumber();

      const balanceA = await primeLiquidityProvider.tokenAmountAccrued(tokenA.address);
      const deltaBlocks = Number(currentBlock) - Number(lastAccruedBlock);
      const accrued = deltaBlocks * Number(tokenASpeed);

      expect(Number(balanceA)).to.equal(accrued);

      await mine(10);
      // we sweep tokenA and leave only 1000
      const sweepAmount = balanceBefore.sub(1000);
      const tx = await primeLiquidityProvider.sweepToken(tokenA.address, signer.address, sweepAmount);
      tx.wait();
      expect(await tokenA.balanceOf(primeLiquidityProvider.address)).to.equal(1000);
      // tokenAmountAccrued[tokenA.address] > 1000
      expect(Number(balanceA)).to.greaterThan(1000);

      // The transaction is reverted with an arithmetic underflow. (panic code 0x11)
      await expect(primeLiquidityProvider.accrueTokens(tokenA.address)).to.be.revertedWithPanic(0x11);
    });

Tools Used

Recommended Mitigation Steps

We can change if (amount_ > balance) to if (amount_ > balance - tokenAmountAccrued[address(token_)]) in sweepToken function.

Because tokenAmountAccrued is in use by Prime, we cannot easily change the tokenAmountAccrued value.

Change if (balance - accrued > 0) to if (balance > accrued) in getEffectiveDistributionSpeed function.

Assessed type

Under/Overflow