Venus Prime - bin2chen's results

Lines of code

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

Vulnerability details

Vulnerability details

Prime.pendingScoreUpdates is used to record the number of users whose score needs to be recalculated when addMarket() , updateAlpha() , updateMultipliers() occurs. Record pendingScoreUpdates=totalIrrevocable + totalRevocable when the above methods occur If there is a partial user update via updateScores(), the corresponding number is reduced, pendingScoreUpdates -- and records whether the current round of users has been modified : isScoreUpdated[nextScoreUpdateRoundId][user] = true

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

When the user burns it also determines if isScoreUpdated[nextScoreUpdateRoundId][user] = false, and performs pendingScoreUpdates--;

    function _burn(address user) internal {
...

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

@>      _updateRoundAfterTokenBurned(user);

        emit Burn(user);
    }

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

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

But the new mint() user does not change isScoreUpdated[nextScoreUpdateRoundId][user] to true. So that in the current round, the new mint user immediately burn, or someone malicious can execute updateScores(new user) which will take up pendingScoreUpdates, causing pendingScoreUpdates to be miscalculated.

Impact

burn() immediately after mint() for the current round will result in inaccurate counting of pendingScoreUpdates. cause updateScores() to judge pendingScoreUpdates==0 revert, can't be executed

Proof of Concept

Given: totalIrrevocable = 5 totalRevocable = 5

When:

1. Administrator execution execute addMarket() : pendingScoreUpdates = 5 + 5 =10
2. Alice execute claim() to mint new Token , isScoreUpdated[1][alice] = false
3. Alice down to 1000, passive execution burn(Alice) : pendingScoreUpdates = 10 - 1 = 9
4. or someone execute updateScores(alice] : pendingScoreUpdates = 10 - 1 = 9

pendingScoreUpdates correct should be 10

Recommended Mitigation

when mint set isScoreUpdated[nextScoreUpdateRoundId][user] = true

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

+       isScoreUpdated[nextScoreUpdateRoundId][user] = true
        emit Mint(user, isIrrevocable);
    }

Assessed type

Context

Findings Summary

[L-01] addMarket() needs to be forced not to have the same underlying

If different vTokens have the same underlying, there are multiple problems

1. vTokenForAsset[] will conflict
2. rewards are recorded as underlying, and different vTokens will compete with each other for rewards

It is recommended to revert if there is already an underlying.

    function addMarket(address vToken, uint256 supplyMultiplier, uint256 borrowMultiplier) external {
        _checkAccessAllowed("addMarket(address,uint256,uint256)");
        if (markets[vToken].exists) revert MarketAlreadyExists();
+       if (vTokenForAsset[_getUnderlying(vToken)] !=address(0) revert InvalidVToken();         

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

...
    }

[L-02] BLOCKS_PER_YEAR The block out time in L2 may be modified

BLOCK_TIME actually changes every now and then, especially in L2s.For example, the recent Bedrock upgrade in Optimism completely changed the block time generation. Since BLOCKS_PER_YEAR is currently immutable, if BLOCK_TIME is modified, it will not be able to be adjusted, affecting the calculation of APR.

Suggest to change it to be changeable

[L-03] sweepToken() Need to limit remaining tokens to no less than tokenAmountAccrued[token_]

sweepToken() can be used to move tokens But there is no restriction if the token moved is a rewarded token If the remaining token after removal is less than tokenAmountAccrued[token_], it will cause the following 2 methods to underflow

1. accrueTokens()
2. getEffectiveDistributionSpeed()

suggestion:

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

        emit SweepToken(address(token_), to_, amount_);

        token_.safeTransfer(to_, amount_);
    }