Venus Prime - deadrxsezzz's results

Lines of code

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

Vulnerability details

Impact

User will break internal accounting and will lead to key function revert

Proof of Concept

Whenever, _startScoreUpdateRound is invoked pendingScoreUpdates is set to the current totalIrrevocable + totalRevocable

    function _startScoreUpdateRound() internal {
        nextScoreUpdateRoundId++;
        totalScoreUpdatesRequired = totalIrrevocable + totalRevocable;
        pendingScoreUpdates = totalScoreUpdatesRequired;
    }

However, this accounting can easily be broken in the following scenario: After _startScoreUpdateRound is invoked, a user is minted their prime token

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

Shortly after the user unstakes from the XVS vault and falls bellow the minimum stake threshold, therefore getting his Prime token burned. _burn at the end calls _updateRoundAfterTokenBurned

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

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

As it can be seen, it decreases pendingScoreUpdates. This would break internal accounting. This is especially a problem as then when the function updateScores is called with all users pending score updates, it will revert due to underflow:

    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--;  // @audit - will revert here due to underflow 
            isScoreUpdated[nextScoreUpdateRoundId][user] = true;

            unchecked {
                i++;
            }

            emit UserScoreUpdated(user);
        }
    }

Since pendingScoreUpdates initial value will be lower than the number of accounts updated, it will unexpectedly revert to underflow. Note that this issue could both happen intentionally and unintentionally, hence the medium severity of it.

Tools Used

Manual review

Recommended Mitigation Steps

Add the following change to _mint:

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

        tokens[user].exists = true;
        tokens[user].isIrrevocable = isIrrevocable;
+       isScoreUpdated[nextScoreUpdateRoundId][user] = true;

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

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

        emit Mint(user, isIrrevocable);
    }

Assessed type

Context