Venus Prime - xAriextz's results

Lines of code

_startScoreUpdateRound: https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L818-L822 updateScores: https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L200-L230 _updateRoundAfterTokenBurned: https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L827-L833

Vulnerability details

Impact

Documentation that explains how updateScores is going to be called: https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/README.md#update-cap-multipliers-and-alpha

When a market is added, alpha is updated or multipliers are updated, Venus will use a script to update the scores of all users using updateScores(). The inconsistencies that this will arise will be minor as stated in the docs. The problem is that if a user decides to claim() his Prime token and call updateScores(), the pendingScoreUpdates will be mistakenly reduced, which leads to the Venus script reverting in the last call. This will make the inconsistencies bigger, leading to unfair interest amounts claimed.

Proof of Concept

Example: Venus needs to run the script for updating the scores of a 100 users. This means that pendingScoreUpdates == 100 at the moment. If a user claims his Prime token and calls updateScores() with his wallet, pendingScoreUpdates will fall to 99. Now, if Venus tries to update the 100 users score by making 4 calls to updateScores() (in order to avoid gas limits), the last call will revert with the custom error NoScoreUpdatesRequired since nextScoreUpdateRoundId == 0 when trying to update the last user, letting 25 users scores without being updated. This will lead to unfair interest amounts claimed.

This kind of problem was taken into account in the burn() function, but not when using claim(). After realizing why the script is failing, Venus would still update the necessary users' scores manually or with another script, but there would be a pretty big timeframe where the inconsistencies would be bigger than expected. The reason why I think this is a medium issue is because there would be a big timeframe where user claims would be wrongly calculated, making it unfair. Even if minor inconsistencies are contemplated in the docs, as I have explained here, this inconsistencies would be a lot bigger and with a bigger timeframe.

Tools Used

Manual review and Hardhat

Recommended Mitigation Steps

When minting a new token, add the following line to prevent users calling updateScores(): isScoreUpdated[nextScoreUpdateRoundId][user] = true; I recommend doing it in the mint(), since Prime tokens can be minted in claim() and in issue().

Assessed type

Other

Lines of code

Accrue interest function: https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L554-L589 Interest accrued function: https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L918-L923

Vulnerability details

Impact

If tokens that implement a fee on transfer are used, some users will not receive their interests when claiming.

Proof of Concept

When a user claims interest, accrueInterest() function is called. In this function funds are sent from PLP and PSR to the Prime contract, in order for users for being able to claim their interests. The problem is that it is not taken into consideration the possibility of this tokens having a fee on transfer. Tokens such as USDT have this functionality, even if the fee is set to 0 at the moment.

As we can see in accrueInterest(), totalIncomeUnreleased and totalAccruedInPLP don't take into account fees on transfer. This leads to the following variables being wrongly calculated:

• distributionIncome
• unreleasedPSRIncome[]
• unreleasedPLPIncome[]
• delta
• markets[].rewardIndex

The last one is the most important one, since it is used for calculating the amount of interests a user gets in _interestAccrued() .

Here is a more detailed explanation. I tried to simplify how the project works in order to focus in the issue:

We are going to distribute 100 USDT tokens in interests, among 10 different users. USDT has a fee on transfer of a 10%. This users, will receive 10 tokens each, since they have the same score and rewardIndex. First 9 users receive their interests correctly when using accrueInterest(), receiving 10 tokens each. The calculations in accrueInterest() didn't take into account the fees on transfer, leading to market[vusdt].rewardIndex being wrongly calculated, which leads to the amount of interest claimed by the users being wrong. The calculated interest amount is too high since market[vusdt].rewardIndex is also too high.

The problem is that now, when the last user tries to claim his interest, there won't be any funds in the contract left. This is because when funds were sent from the PLP and PSR to the Prime contract, it wasn't taken into account the fact that the 10% of the tokens transferred wouldn't reach the contract, since they were paid as fees. Only 90 USDT were deposited into Prime, but because of the wrong calculations done when not taking into account the fees on transfer, the first 9 users were able to claim 10 USDT of interests, letting the last user without any funds left to be claimed.

Tools Used

Manual review

Recommended Mitigation Steps

Take into consideration that same tokens have a fee on transfer when making calculations in accrueInterest().

Assessed type

ERC20