Venus Prime - neumo's results

Lines of code

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

Vulnerability details

Impact

If a user token is minted in the middle of a round (via a call to claim or issue), pendingScoreUpdates is not updated and there would be more users with pending scoreUpdates than the value stored in the pendingScoreUpdates variable. This could cause a DoS in calls to function updateScores. Now say the system has 10 users pending score update. Imagine a user claims a Prime token, calling claim function. After some time, calls updateScores to accrue interest and update the score. So pendingScoreUpdates is decremented (to 9) in Line 221. We still have 10 users pending score update, but pendingScoreUpdates value is 9. So the last call to updateScores, trying to update the scores of the 10 users, would revert due to an underflow in Line 221.

All this could happen accidentally or a malicious user could frontrun calls to updateScores and make them revert, provided they have one or more Prime tokens to claim.

Proof of Concept

The following test can be added in file Prime.ts after line 801:

    it("Cannot update all pending scores if a user is minted while in a round", async () => {

      // We issue 2 tokens
      await prime.issue(true, [user1.getAddress(), user2.getAddress()]);
      // Update alpha to force a new round of score updates
      await prime.updateAlpha(4, 5); //0.80
      // We issue 1 more token (but the round has already started)
      await prime.issue(true, [user3.getAddress()]);

      // Update the score of the last added
      await prime.updateScores([user3.getAddress()]);
      // Update the score of both initial users. There is an underflow when trying to decrement pendingScoreUpdates
      await expect(prime.updateScores([user1.getAddress(), user2.getAddress()])).to.be.revertedWithPanic(0x11);
      
    });

Tools Used

Manual review and Hardhat tests.

Recommended Mitigation Steps

Function _mint should increment the value of pendingScoreUpdates.

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Impact

There is a gas optimization in function updateScores in Prime contract when incrementing the variables of a loop.

200	    function updateScores(address[] memory users) external {
201	        if (pendingScoreUpdates == 0) revert NoScoreUpdatesRequired();
202	        if (nextScoreUpdateRoundId == 0) revert NoScoreUpdatesRequired();
203	
204	        for (uint256 i = 0; i < users.length; ) {
205	            address user = users[i];
206	
207	            if (!tokens[user].exists) revert UserHasNoPrimeToken();
208	            if (isScoreUpdated[nextScoreUpdateRoundId][user]) continue;
209	
210	            address[] storage _allMarkets = allMarkets;
211	            for (uint256 j = 0; j < _allMarkets.length; ) {
212	                address market = _allMarkets[j];
213	                _executeBoost(user, market);
214	                _updateScore(user, market);
215	
216	                unchecked {
217	                    j++;
218	                }
219	            }
220	
221	            pendingScoreUpdates--;
222	            isScoreUpdated[nextScoreUpdateRoundId][user] = true;
223	
224	            unchecked {
225	                i++;
226	            }
227	
228	            emit UserScoreUpdated(user);
229	        }
230	``    }

As we can see, there is an unchecked block in lines 224 - 226 which increments the i variable of the outer loop. But in line 208 we have a condition that would skip the iteration and enter the loop again. The issue lies in that variable i is only incremented at the end of the loop, and if condition in line 208 is true, the loop will be infinite until all gas of the transaction is consumed. So if the array of users passed to the function contains at least one for which the score has already been updated for the nextScoreUpdateRoundId, the call will run out of gas. The call will presumably be called with a big array of users, so the gas sent will presumably be a considerable amount, which will be lost in case this situation happens.

Proof of Concept

The following test can be added in file Prime.ts after line 801:

    it("infinite loop OOG revert", async () => {

      // We issue 2 tokens
      await prime.issue(true, [user1.getAddress(), user2.getAddress()]);
      // Update alpha to force a new round of score updates
      await prime.updateAlpha(4, 5); //0.80

      // Update the score of one of them
      await prime.updateScores([user1.getAddress()]);
      // Update the score of both users. As the first one is already updated, we enter the infinite loop
      await expect(prime.updateScores([user1.getAddress(), user2.getAddress()])).to.be.revertedWithoutReason();
      
    });

Tools Used

Manual review and hardhat tests.

Recommended Mitigation Steps

This line of function ``:

...
208	            if (isScoreUpdated[nextScoreUpdateRoundId][user]) continue;
...

Could be changed by:

...
208		if (isScoreUpdated[nextScoreUpdateRoundId][user]){
209			unchecked {i++;}
210			continue;
211		}
...

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

In contract PrimeLiquidityProvider, constant MAX_DISTRIBUTION_SPEED is set to 1e18. This constant serves the purpose of limiting the distribution speed of tokens, this is the number of tokens that can be accrued per block.

    function _setTokenDistributionSpeed(address token_, uint256 distributionSpeed_) internal {
        if (distributionSpeed_ > MAX_DISTRIBUTION_SPEED) {
            revert InvalidDistributionSpeed(distributionSpeed_, MAX_DISTRIBUTION_SPEED);
        }

        if (tokenDistributionSpeeds[token_] != distributionSpeed_) {
            // Distribution speed updated so let's update distribution state to ensure that
            //  1. Token accrued properly for the old speed, and
            //  2. Token accrued at the new speed starts after this block.
            accrueTokens(token_);

            // Update speed
            tokenDistributionSpeeds[token_] = distributionSpeed_;

            emit TokenDistributionSpeedUpdated(token_, distributionSpeed_);
        }
    }

This makes impossible the use of low valued tokens such as SHIB.

Proof of Concept

SHIB is a populat ERC20 token that has 18 decimals and a current value of 0.00000723 USD. This means that, using a distribution speed equal to MAX_DISTRIBUTION_SPEED, the total value accrued after 1,000,000 blocks would be of 7.23 USD, which makes impractical the use of such a token.

Tools Used

Manual review.

Recommended Mitigation Steps

Don't make the distribution speed cap a constant value, make it editable by governance.

Another option would be to create a mapping to store the distribution speed per initialized token. This would be more flexible but also more gas intensive.

Assessed type

Other