Venus Prime - Pessimistic's results

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#L200-L230

Vulnerability details

Description

In Prime.sol, updateScores function update user scores in case of parameter changes (for example, when score formula is changed). This function should be called for every user whose score has not been updated since the last time _startScoreUpdateRound function was invoked.

The number of users who still need to update the score is stored in the pendingScoreUpdates variable. If the user's score was updated in the current round, the isScoreUpdated[nextScoreUpdateRoundId][user] variable is set to true for that user. Such a user does not decrease the pendingScoreUpdates variable if someone tries to update their score again.

If the user minted a new token in the current round, they should not be eligible for an update. Therefore, such a user should not decrease the pendingScoreUpdates variable if their user address is provided to the updateScores function. However, in the current implementation, there is no check for this behaviour. If a new user provides their address to the updateScores function, they will decrease the pendingScoreUpdates value since isScoreUpdated[nextScoreUpdateRoundId][user] variable is equal to false for them. As a result, some other users who require the update will not be able to update their scores in batch with the updateScores function due to check at line 201:

if (pendingScoreUpdates == 0) revert NoScoreUpdatesRequired();

Note, that these users is still able to update their score by calling xvsUpdated function, however updateScores function would not work.

Impact

As a result of the aforementioned issue, some user scores would not be updated since the updateScores function will malfunction due to the incorrect pendingScoreUpdates variable. The score of each individual user can still be updated by calling the xvsUpdated function. However, this approach is inconvenient since the xvs balance was not actually changed for them.

Furthermore, it is worth noting that since pendingScoreUpdates is a public variable, its incorrectness can affect the functioning of external services that rely on accurate data from the contract. If the monitoring of the protocol relies of the pendingScoreUpdates variable and updateScores function for updates, it is easy to oversee the unchanged score for a user. As a result, someone will retain their old score:

• If the score is lower than it should be, such a prime user will receive a reduced income
• If the score is higher than it should be, other users will receive a reduced income

Recommended mitigation steps

We recommend setting the isScoreUpdated[nextScoreUpdateRoundId][user] value to true when the token is minted, as these users do not require a score update in the current round.

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

Description

In Prime.sol, in the updateScores function, there is a bug on line 208. This line contains a continue statement without incrementing the for loop iteration index (i). As a result, if someone provides a user address that is already updated, the updatedScores transaction will consume all provided gas in an endless loop.

Impact

There is one malicious scenario possible: a malicious agent can frontrun the updateScores function call with multiple addresses by updating a single user's score from that batch. As a result, batch update will fail.

This behavior does not lead to a profit for an attacker, but it disrupts all attempts to batch update scores. For a system with multiple users, it is crucial to have batch update functions in order to save gas on updates.

Recommended mitigation steps

To mitigate this issue, we recommend incrementing the loop index before the continue statement. In that case, even if the batch contains an already updated user, the transaction would not fail.

Assessed type

Loop