Venus Prime - Norah's results

Lines of code

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

Vulnerability details

Background

Whenever a critical parameter such as alphaNumerator, alphaDenominator, supplyMultiplier or borrowMultiplier is updated by the governance or the new market is added to prime reward programme, There is a need to update the scores of all the users for all the market to ensure fair distribution of the reward.

Current Implementation achieves this by doing calling updateScores(address[] users) for batch of users multiple times, this is done to avoid running out gas. To track how many users have been updated so far protocol tracks two key variables pendingScoreUpdates and totalScoreUpdatesRequired.

Variable totalScoreUpdatesRequired is stored as the total number of token in circulation (one token per user) at the beginning of the new round of update. Also pendingScoreUpdates is also initially set to total of tokens (reflecting total users) and decremented subsequently on each score update for user.

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

Vulnerability

Problem here is that while the totalScoreUpdatesRequired and pendingScoreUpdates are updated whenever the tokens are burned.

In similar manner when new users are minted prime tokens isScoreUpdated[nextScoreUpdateRoundId][user] should be set True as they score assign to them will be with latest parameter anyway and to restrict malicious user to re-update their score and unnecessarily reduce the pendingScoreUpdates parameter.

Lets consider following scenario:

1. There are only three users as of now owning prime token.
2. Now say supplyMultiplier paramter is updated via governance, at that time totalScoreUpdatesRequired and pendingScoreUpdates will be stored as :
   • totalScoreUpdatesRequired = 3
   • pendingScoreUpdates = 3
3. Before there scores are updated via updateScores() function, three new users are minted prime token.
4. Ideally at this stage these will get already updated scores as per new parameter.
5. But, Anyone can still pass this three new users address as argument to updateScores()
6. The scores will be re-updated as the isScoreUpdated[nextScoreUpdateRoundId][user] will hold false value, making pendingScoreUpdates to be reduced to zero in the process
7. Which will now restrict the score update for the previous three users.

Now there is function accrueInterestAndUpdateScore(address user, address market), by calling repeatedly for each users and all of their markets one by one protocol or users can update the scores in time even after above exploit, but then this defeats the purpose of this whole mechanism, which is implemented to ensure no matter all users score are updated before any user can claim there reward as per newly updated parameters.

Also, protocol will most likely collect the list of users whose score to be updated by checking isScoreUpdated[nextScoreUpdateRoundId][user]. In that case totally list will also include new users whose score is already updated, and as result parameter pendingScoreUpdates will fall short to compelete all the updates via updateScores() functionality.

tldr; isScoreUpdated[nextScoreUpdateRoundId][user] and pendingScoreUpdates are not in sync whenever new users are minted in between _startScoreUpdateRound() and updateScores() function calls, which can lead to unfair reward distribution.

Impact

Unfair distribution of the rewards.

Proof of Concept

Tools Used

Manual Review

Recommended Mitigation Steps

Update the parameter isScoreUpdated[nextScoreUpdateRoundId][user] to True each time user is minted new token.

Assessed type

Other

Lines of code

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

Vulnerability details

Vulnerability

Prime.sol does not have any functionality to remove the market, once added in the allMarkets Array. There are at-least two scenarios where-in this functionality will be needed :

• if any of the market was exploited or has the bug in it, and it was redeployed, then it would be better to remove that market from this array.
• Admin ends up passing the address of the market to addMarket() function, that was not supposed to be added.

Also, as there is _ensureMaxLoop() implemented, there is possibility that governance may not be able add any new markets after certain occurrence of the above two cases.

Impact

User may end up paying excess gas and in extreme scenario governance wont be able to add new Market.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a functionality to remove the markets from Allmarkets Array.

Assessed type

Other