Venus Prime - dethera's results

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L196-L230 https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L204-L229 https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L208 https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L224-L226

Vulnerability details

Impact

If for some user from the input array users, the condition isScoreUpdated[nextScoreUpdateRoundId][user] in:

if (isScoreUpdated[nextScoreUpdateRoundId][user]) continue;

is true, continue will be executed and all of the gas will be consumed by the infinite loop leading to an out of gas exception and the transaction will revert. This means the current implementation, vulnerable to an infinite loop, can cause a DoS for the whole updateScores function and thus none of the users from the users input array will have their scores updated, but all of the gas will be consumed.

Proof of Concept

In the file Prime.sol there is a function called updateScores. The following for loop in updateScores is problematic:

        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 issue in the outer for loop is that the loop variable i update:

            unchecked {
                i++;
            }

comes after the line:

if (isScoreUpdated[nextScoreUpdateRoundId][user]) continue;

which means if isScoreUpdated[nextScoreUpdateRoundId][user] ever becomes true, an infinite loop happens (because the for loop will be executed with the i from the previous iteration again and again and again since unchecked { i++; } is never executed because of the continue statement).

Tools Used

Manual review.

Recommended Mitigation Steps

There are many ways to fix this issue:

1. It is easiest and cleanest to put the update of the i variable to its canonical place in the for loop:

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

        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;

            emit UserScoreUpdated(user);
        }
    }

This fix will make the code more readable and automatically fixes the issue.

N.B. As a general remark (when gas optimization is not of utmost importance), I would advise to always use the for loop written like:

for (uint256 i = 0; i < users.length; ++i)

over the one with the trailing update (unchecked { i++; }) that currently causes the infinite loop - it makes the code more understadable, easier to read and prevents issues like infinity loops.

2. Another option would be to put the trailing update logic of the i variable (unchecked { i++; }) right before continue. Using this approach, the code would look like:

    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]) {

                unchecked {
                    i++;
                }

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

This implementation makes sure the update of the i variable is performed before continue is executed and the infinity loop is avoided.

Assessed type

Loop