Venus Prime - debo's results

Lines of code

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

Vulnerability details

Impact

The function updateScores that updates the scores of a list of users. The function iterates over the list of users and for each user, it iterates over all markets, executing a boost and updating the score for each market. The potential issue here is that the function does not increment the index i if the condition isScoreUpdated[nextScoreUpdateRoundId][user] is true. This could lead to an infinite loop if the condition is always true for a user in the list, causing the transaction to run out of gas. Vulnerable Function

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

Proof of Concept

Here's a simple PoC for an exploit

// SPDX-License-Identifier: BSD-3-Clause
pragma solidity >=0.8.13;

import "./Prime.sol";

contract tPrime {
   
   Prime public x1;
   
   constructor(Prime _x1) {
   x1 = Prime(_x1);
}

// Assume attacker builds an attack contract that inherits from Prime.sol   

   function testDos(address _x1) external {
      address[] users = [_x1];
      isScoreUpdated[nextScoreUpdateRoundId][_x1] = true;
      // Attacker calls updateScores
      x1.updateScores(users);
}
}

In this case, the updateScores function will enter an infinite loop when it tries to update the score for the attacker, because the condition isScoreUpdated[nextScoreUpdateRoundId][attacker] is always true. This will cause the transaction to run out of gas and fail.

The impact of this vulnerability is that it could potentially be used to deny service to the contract by causing transactions to fail. This could prevent other users from updating their scores, which could have further implications depending on how the scores are used in the contract.

Tools Used

VS Code.

Recommended Mitigation Steps

To mitigate this issue, the function should ensure that it always increments the index i, even if the condition isScoreUpdated[nextScoreUpdateRoundId][user] is true. This can be achieved by moving the increment operation outside of the if statement, like so:

for (uint256 i = 0; i < users.length; i++) {
    address user = users[i];
    if (!tokens[user].exists) revert UserHasNoPrimeToken();
    if (!isScoreUpdated[nextScoreUpdateRoundId][user]) {
        address[] storage _allMarkets = allMarkets;
        for (uint256 j = 0; j < _allMarkets.length; j++) {
            address market = _allMarkets[j];
            _executeBoost(user, market);
            _updateScore(user, market);
        }
        pendingScoreUpdates--;
        isScoreUpdated[nextScoreUpdateRoundId][user] = true;
        emit UserScoreUpdated(user);
    }
}

With this change, the function will always increment i, preventing the infinite loop and the potential denial of service attack.

Assessed type

DoS