Venus Prime - said's results

Lines of code

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

Vulnerability details

Impact

When issue is called for minting new Irrevocable Prime Token for users. It is not delete/reset the target user stakedAt, if in the future the Irrevocable Prime Token is burned by protocol, user could immediately mint new Revocable Prime Token via claim().

Proof of Concept

When issue is called for minting new Irrevocable Prime Token for users, stakedAt is not deleted.

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

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

                unchecked {
                    i++;
                }
            }
        }
    }

If user previously stake XVS and trigger xvsUpdated and reach eligibility threshold, it will update stakedAt and start the timer for claim revocable Prime token. The user intentionally not call claim as he know that he is eligible for Irrevocable Prime Token.

    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;
        } else if (tokens[user].exists && isAccountEligible) {
            _accrueInterestAndUpdateScore(user);
        }
    }

Irrevocable token can still be burned by protocol, depend on mint/burn criteria that will be set.

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

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

Now for instance, if the user loss his eligibility for Irrevocable Prime Token, and protocol call burn. The user could immediately call claim to mint Revocable Prime Token if passed the 90 days check.

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

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

The user doesn't have to wait the 90 days time threshold after the Irrevocable Prime Token is burned to mint the new Revocable Prime Token.

Hardhat PoC :

Add this test inside Prime.ts under "mint and burn" test.

    it("immediate mint", async () => {
      const user = user1;

      await xvs.connect(user).approve(xvsVault.address, bigNumber18.mul(10000));
      await xvsVault.connect(user).deposit(xvs.address, 0, bigNumber18.mul(10000));
      
      // protocol issue Irrevocable for user
      await prime.issue(true, [user.getAddress()]);
      await mine(90 * 24 * 60 * 60);
      // after some time, protocol decide to burn the user irrevocable due to loss eligibility
      await prime.burn(user.getAddress());
      // but user can immediately claim again
      await prime.connect(user).claim();
      // total revocable check
      expect(await prime.totalRevocable()).to.be.equal(1);

    });

Run the test :

npx hardhat test --grep "immediate mint"

Tools Used

Manual review + hardhat

Recommended Mitigation Steps

delete stakedAt when issue is called for new Irrevocable Prime 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]];
                }

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

                unchecked {
                    i++;
                }
            }
        }
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

New Prime token minted is not considering if currently inside score updates period (pendingScoreUpdates > 0). When this newly Prime token holder call updateScores, it will decreasing pendingScoreUpdates despite not considered when calculating pendingScoreUpdates initially.

According to docs:

Market multipliers and alpha can be updated at anytime and need to be propagated to all users. Changes will be gradually applied to users as they borrow/supply assets and their individual scores are recalculated. This strategy has limitations because the scores will be wrong in aggregate.

To mitigate this issue, Venus will supply a script that will use the permission-less function updateScores to update the scores of all users. This script won’t pause the market or Prime contract. Scores need to be updated in multiple transactions because it will run out of gas trying to update all scores in 1 transaction.

the script that call updateScores for all users will always fail early when this new Prime token holder call updateScores for himself, leaving some addresses have to be called each manually and breaking the script automation.

Proof of Concept

When _mint is called either via claim or issue, it is not checking if currently inside score updates period (pendingScoreUpdates > 0).

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

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

_initializeMarkets also not checking the same condition.

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

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

Now, this newly minted Prime token holder can call updateScores and decrement the pendingScoreUpdates because pass the isScoreUpdated[nextScoreUpdateRoundId][user] check.

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

    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;

            address[] storage _allMarkets = allMarkets;
            for (uint256 j = 0; j < _allMarkets.length; ) {
                address market = _allMarkets[j];
                _executeBoost(user, market);
                _updateScore(user, market);

                unchecked {
                    j++;
                }
            }

            pendingScoreUpdates--;
            isScoreUpdated[nextScoreUpdateRoundId][user] = true;

            unchecked {
                i++;
            }

            emit UserScoreUpdated(user);
        }
    }

Hardhat PoC :

Add this test inside Prime.ts under "update score" test.

      it("new prime updateScores PoC", async () => {
        await xvs.connect(user3).approve(xvsVault.address, bigNumber18.mul(2000));
        await xvsVault.connect(user3).deposit(xvs.address, 0, bigNumber18.mul(2000));
        await prime.issue(false, [user3.getAddress()]);
        await prime.addMarket(vbnb.address, bigNumber18.mul(1), bigNumber18.mul(1));

        let interest = await prime.interests(vbnb.address, user3.getAddress());
        expect(interest.accrued).to.be.equal(0);
        expect(interest.score).to.be.equal(0);
        expect(interest.rewardIndex).to.be.equal(0);

        let market = await prime.markets(vbnb.address);
        expect(market.supplyMultiplier).to.be.equal(bigNumber18.mul(1));
        expect(market.borrowMultiplier).to.be.equal(bigNumber18.mul(1));
        expect(market.rewardIndex).to.be.equal(0);
        expect(market.sumOfMembersScore).to.be.equal(0);

        let nextScoreUpdateRoundId = await prime.nextScoreUpdateRoundId();
        let totalScoreUpdatesRequired = await prime.totalScoreUpdatesRequired();
        let pendingScoreUpdates = await prime.pendingScoreUpdates();
        let isScoreUpdated = await prime.isScoreUpdated(nextScoreUpdateRoundId, user3.getAddress());
        expect(nextScoreUpdateRoundId).to.be.equal(3);
        expect(totalScoreUpdatesRequired).to.be.equal(2);
        expect(pendingScoreUpdates).to.be.equal(2);
        expect(isScoreUpdated).to.be.equal(false);
        // new prime issued for other user after update initiated
        await prime.issue(true, [user2.getAddress()]);
        // newly user call update
        await prime.updateScores([user2.getAddress()]);
       
        await expect(prime.updateScores([user1.getAddress(), user3.getAddress()])).to.be.reverted;

      });

Run the test :

npx hardhat test --grep "new prime updateScores PoC"

Tools Used

Manual review + hardhat

Recommended Mitigation Steps

When _mint (or _initializeMarkets) is called, check if currently inside update period and mark isScoreUpdated[nextScoreUpdateRoundId][user] to true for that user.

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

+        if (pendingScoreUpdates > 0) {
+           isScoreUpdated[nextScoreUpdateRoundId][user] = true;
+        }

        emit Mint(user, isIrrevocable);
    }

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Infinite loop could happen inside updateScores if one of the user address inside users array already done update in current nextScoreUpdateRoundId (isScoreUpdated[nextScoreUpdateRoundId][user] is true). Causing the call to revert.

This issue is crucial, as stated in docs :

Market multipliers and alpha can be updated at anytime and need to be propagated to all users. Changes will be gradually applied to users as they borrow/supply assets and their individual scores are recalculated. This strategy has limitations because the scores will be wrong in aggregate.

To mitigate this issue, Venus will supply a script that will use the permission-less function updateScores to update the scores of all users. This script won’t pause the market or Prime contract. Scores need to be updated in multiple transactions because it will run out of gas trying to update all scores in 1 transaction.

the script that call updateScores for all users will always fail when some users previously already update the score themself, making the automation for updating score harder to maintain.

Proof of Concept

This is the updateScores implementation, it can be observed that if isScoreUpdated[nextScoreUpdateRoundId][user] is true, then it will hit continue, skip the rest of operation inside the loop and start again the for loop from top.

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

    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;

            address[] storage _allMarkets = allMarkets;
            for (uint256 j = 0; j < _allMarkets.length; ) {
                address market = _allMarkets[j];
                _executeBoost(user, market);
                _updateScore(user, market);

                unchecked {
                    j++;
                }
            }

            pendingScoreUpdates--;
            isScoreUpdated[nextScoreUpdateRoundId][user] = true;

            unchecked {
@>>              i++;
            }

            emit UserScoreUpdated(user);
        }
    }

The problem is, i++ is located at the bottom of loop operation, if continue ever hit the for loop, it will stuck forever inside current i because increment will never reached.

Hardhat PoC :

Add this test inside Prime.ts under "update score" test.

      it("infinite loop", async () => {
        await xvs.connect(user3).approve(xvsVault.address, bigNumber18.mul(2000));
        await xvsVault.connect(user3).deposit(xvs.address, 0, bigNumber18.mul(2000));
        await prime.issue(false, [user3.getAddress()]);
        await prime.addMarket(vbnb.address, bigNumber18.mul(1), bigNumber18.mul(1));

        let interest = await prime.interests(vbnb.address, user3.getAddress());
        expect(interest.accrued).to.be.equal(0);
        expect(interest.score).to.be.equal(0);
        expect(interest.rewardIndex).to.be.equal(0);

        let market = await prime.markets(vbnb.address);
        expect(market.supplyMultiplier).to.be.equal(bigNumber18.mul(1));
        expect(market.borrowMultiplier).to.be.equal(bigNumber18.mul(1));
        expect(market.rewardIndex).to.be.equal(0);
        expect(market.sumOfMembersScore).to.be.equal(0);

        let nextScoreUpdateRoundId = await prime.nextScoreUpdateRoundId();
        let totalScoreUpdatesRequired = await prime.totalScoreUpdatesRequired();
        let pendingScoreUpdates = await prime.pendingScoreUpdates();
        let isScoreUpdated = await prime.isScoreUpdated(nextScoreUpdateRoundId, user3.getAddress());
        expect(nextScoreUpdateRoundId).to.be.equal(3);
        expect(totalScoreUpdatesRequired).to.be.equal(2);
        expect(pendingScoreUpdates).to.be.equal(2);
        expect(isScoreUpdated).to.be.equal(false);
        // updateScore twice for user1
        await prime.updateScores([user1.getAddress()]);
        await expect(prime.updateScores([user1.getAddress(), user3.getAddress()])).to.be.reverted;

      });

Run the test :

npx hardhat test --grep "infinite loop"

Tools Used

Manual review + hardhat

Recommended Mitigation Steps

Fix the for loop to this :

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

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

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

            address[] storage _allMarkets = allMarkets;
            for (uint256 j = 0; j < _allMarkets.length; ) {
                address market = _allMarkets[j];
                _executeBoost(user, market);
                _updateScore(user, market);

                unchecked {
                    j++;
                }
            }

            pendingScoreUpdates--;
            isScoreUpdated[nextScoreUpdateRoundId][user] = true;

-            unchecked {
-                i++;
-            }

            emit UserScoreUpdated(user);
        }
    }

Assessed type

Loop

QA Report

Low Risk Issues

Low Risk Issues

[L-01] getPendingInterests will always revert if market contain VBNB

Calling getPendingInterests will always revert if market contain VBNB because VBNB don't have underlying() read functions.

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

    function getPendingInterests(address user) external returns (PendingInterest[] memory pendingInterests) {
        address[] storage _allMarkets = allMarkets;
        PendingInterest[] memory pendingInterests = new PendingInterest[](_allMarkets.length);

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

Need to be updated using _getUnderlying so it will work for all VToken including VBNB.

    function getPendingInterests(address user) external returns (PendingInterest[] memory pendingInterests) {
        address[] storage _allMarkets = allMarkets;
        PendingInterest[] memory pendingInterests = new PendingInterest[](_allMarkets.length);

        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(),
+                market: _getUnderlying(market),
                amount: interestAccrued + accrued
            });

            unchecked {
                i++;
            }
        }

        return pendingInterests;
    }

[L-02] updateMultipliers, updateAlpha and addMarket can be called even pendingScoreUpdates not 0

The mentioned functions can still be called even when inside score update period (pendingScoreUpdates not 0). Check need to be performed to finish previous pendingScoreUpdates to avoid unexpected protocol state.

https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L237-L255 https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L263-L280 https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L288-L309

    function addMarket(address vToken, uint256 supplyMultiplier, uint256 borrowMultiplier) external {
        _checkAccessAllowed("addMarket(address,uint256,uint256)");
        if (markets[vToken].exists) revert MarketAlreadyExists();
+       if (pendingScoreUpdates > 0) revert InsideUpdatePeriod();

        bool isMarketExist = InterfaceComptroller(comptroller).markets(vToken);
        if (!isMarketExist) revert InvalidVToken();

        markets[vToken].rewardIndex = 0;
        markets[vToken].supplyMultiplier = supplyMultiplier;
        markets[vToken].borrowMultiplier = borrowMultiplier;
        markets[vToken].sumOfMembersScore = 0;
        markets[vToken].exists = true;

        vTokenForAsset[_getUnderlying(vToken)] = vToken;

        allMarkets.push(vToken);
        _startScoreUpdateRound();

        _ensureMaxLoops(allMarkets.length);

        emit MarketAdded(vToken, supplyMultiplier, borrowMultiplier);
    }

[L-03] sweepToken can break tokenAmountAccrued accounting

When sweepToken is called, it can withdraw reward token, but it doesn't check and decrease tokenAmountAccrued. Could potentially break the accounting and cause issue. Consider to check and update tokenAmountAccrued if necessary.

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

    function sweepToken(IERC20Upgradeable token_, address to_, uint256 amount_) external onlyOwner {
        uint256 balance = token_.balanceOf(address(this));
        if (amount_ > balance) {
            revert InsufficientBalance(amount_, balance);
        }
       
+        if (tokenAmountAccrued[token_] > 0) {
+          tokenAmountAccrued[token_] = tokenAmountAccrued[token_] - amount_;
+        }
        
        emit SweepToken(address(token_), to_, amount_);

        token_.safeTransfer(to_, amount_);
    }