Venus Prime - PwnStars's results

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L818-L822 https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L397-L405

Vulnerability details

Impact

The management of pending score updates within the Prime Program can lead to certain users not accruing their deserved interests in newly added markets. This is because their scores may not be updated as intended. Meanwhile, an attacker could obtain more rewards than they're entitled to due to these temporarily inaccurate scores.

Proof of Concept

1. Owner invokes addMarket, which internally calls _startScoreUpdateRound.
2. Assuming there are 4 minted Prime Tokens, pendingScoreUpdates is set to 4.

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

3. Bob claims new tokens using the claim function. This mints a new Prime Token, increasing the count to 5.
4. Bob then calls updateScores providing 3 previously minted token addresses plus their own one
5. The pendingScoreUpdates counter decrements in each iteration. As a result, it will be set to 0 after processing the Bob’s and the 3 other addresses.
6. There's still 1 user with a minted Prime Token that hasn't had their scores updated. Due to the check if (pendingScoreUpdates == 0) revert NoScoreUpdatesRequired(); no further score updates are allowed for this user. Bob will receive more rewards than expected, because the omitted user has score 0, so the available rewards won't be distributed to that user.

Tools Used

Manual review

Recommended Mitigation Steps

Increment the pendingScoreUpdates whenever any new Prime Token is claimed.

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

It has been confirmed by the sponsor that the updateScores function will be called for every user by the owner, e.g. if the new market will be added.

An attacker, upon observing the owner's transaction containing an array of user addresses intended for score updates, can front-run this transaction. By selecting a single address from the owner's list and invoking the updateScores function with a higher gas price, the attacker ensures that this specific user's score is updated prior to the owner's transaction being mined. When the owner's transaction processes and encounters the already updated address, the check if (isScoreUpdated[nextScoreUpdateRoundId][user]) continue; will trigger.

Because the continue statement is invoked and the loop counter doesn't increment until the end, the function can run out of gas and revert. This allows an attacker to repeatedly grief the owner, leading to financial losses from wasted gas.

Proof of Concept

1. Owner initiates the updateScores call, providing an array of all users addresses to update their scores.
2. An attacker observes this transaction, selects one address from the array and calls updateScores for this specific address, ensuring their transaction has a higher gas price.
3. The attacker's transaction updates the score for the chosen address before.
4. The owner's transaction starts processing, but when it reaches the already updated address, the check if (isScoreUpdated[nextScoreUpdateRoundId][user]) continue; will skip the address, resulting in transaction revert due to out of gas error.

Tools Used

Manual review

Recommended Mitigation Steps

Increment the i counter directly in the loop header:

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

Assessed type

Loop

1. Sweep Token Will Emit Incorrect Event With Fee-On-Transfer Tokens

Description:

The sweep token function here https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L216 when given a fee-on-trasfer token (considering there is some amount of that token in the contract) will emit the incorrect event as the amount in the argument of the event would be less than the amount given as parameter.

2. getEffectivedistributionSpeed Might Give Incorrect Result

Description:

The function getEffectivedistributionSpeed here https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L232 might give incorrect result as before calculating uint256 accrued it does not call accrueTokens , since it is a view function is would be incorrect to change state this way but it can be changed to a normal external function.

3. While Issuing a Prime Token _mint Might Revert

Description:

The appropriate role can call issue() function https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L331 to issue a Prime token to an array of users , but if one of the user already has a prime token the whole call will revert due to the checks in the _mint function that if the user already has a prime token then revert. Ensure that before minting the user does not have a prime token.

4 . _accrueInterestAndUpdateScore Should Be Used

We accrue interest and update score of the users across all markets in this code block https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L211-L217 , but instead of this a simple call to _accrueInterestAndUpdateScore would be sufficient since it does the same job and would make the code more readable and streamlined.

5 . isScoreUpdated Might Not Be Updated For A Round Id

Description:

updateAlpha , updateMultiplers , addMarket when called updates a round Id through _startScoreUpdate round at L818 and score updates for these rounds take place in the function updateScores() at L200 . If for a round before updateScores() is called for a user the next round gets started ( through updateAlpha , updateMultiplers , addMarket ) then score won’t be updated for that round for the users and isScoreUpdated will always be false for that round.

6 . Capital Calculation Might Revert In Future Upgrades

Description:

Capital is calculated here https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L661 , currently vToken will be 8 decimals but other are possible in future , if in future the decimal is more than 18 , capital calculation will always revert and no scores calculated.

7 . Incorrect Comment

Description:

The comment here https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L23 is incorrect , it should be “ the last block where tokens were accrued” instead of “The rate at which token is distributed to the Prime contract”