Venus Prime - rokinot's results

Judge has assessed an item in Issue #593 as 3 risk. The relevant finding follows:

Prime.sol: Users with issued tokens can instantly re-enter the protocol if they were in the 90 days waiting period If an user is issued a prime token while waiting for the claim period, his stakedAt is not zeroed. If the user burns this token in the future by withdrawing from the XVS vault, he can earn it back by calling the claim() function, bypassing the 90 day waiting period.

In order to fix this, consider setting the stakedAt[user] to zero in the token issue, as shown below:

            } else {
                _mint(true, users[i]);
                _initializeMarkets(users[i]);
                stakedAt[user] = 0;
            }

Lines of code

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

Vulnerability details

Impact

Previous users who need to have their scores updated after key parameters are updated are unable to do so.

Proof of Concept

Let's imagine a market with two users with valid scores. If a new market is added, the multipliers are updated, or the alpha is updated, _updateRoundAfterTokenBurned() is called and sets the pending score updates as 2. Now a third user joins the market. Since the key parameters to calculate the score are already updated for him, a new update isn't required.

This new user decides to quit the market by requesting a withdrawal from the xvsVault. This call will trigger a xvsUpdate() call, which will burn the user's tokens and remove his score. The issue lies in the fact that this burn reduces the pendingScoreUpdates and totalScoreUpdatesRequired variable, regardless of whether the user needs an update or not, since isScoreUpdated for this user is false. With the burn, this variable will now be stored as 1, and only one of the first two users will be able to update their score.

The code below simulates this event, it can be added under the "update score" description in Prime.ts

      it("new users quitting will prevent the score update of old users", async () => {
        await prime.issue(true, [user2.getAddress()]);

        await usdt.transfer(user2.address, bigNumber18.mul(10000));
        await usdt.connect(user2).approve(vusdt.address, bigNumber18.mul(100));
        await vusdt.connect(user2).mint(bigNumber18.mul(100)); //introduce user2 to the market

        await prime.updateAlpha(4, 5); //0.80 //update the alpha
        expect(await prime.pendingScoreUpdates()).to.be.equal(2); //now user 1 and 2 needs an update

        await xvs.connect(user3).approve(xvsVault.address, bigNumber18.mul(100));
        await xvsVault.connect(user3).deposit(xvs.address, 0, bigNumber18.mul(100));
        await prime.issue(false, [user3.getAddress()]);

        await usdt.transfer(user3.address, bigNumber18.mul(10000));
        await usdt.connect(user3).approve(vusdt.address, bigNumber18.mul(100));
        await vusdt.connect(user3).mint(bigNumber18.mul(100)); //user 3 enters the market

        expect(await prime.pendingScoreUpdates()).to.be.equal(2); //user 1 and 2 still needs an update

        await xvsVault.connect(user3).requestWithdrawal(xvs.address, 0, bigNumber18.mul(100)); //user 3 exits, triggers his token burn and zeroes his scores
        //but user 3 burning his tokens decreases the pending score updates as well

        expect(await prime.pendingScoreUpdates()).to.be.equal(1); //pending score update is now 1, even though 2 users need it

        await prime.updateScores([user1.getAddress()]); //user 1 sucessfully updates his score
        await expect(prime.updateScores([user2.getAddress()])).to.be.revertedWithCustomError(prime, "NoScoreUpdatesRequired"); //user 2 is stuck with his old score
      });

Tools Used

Hardhat, manual code review

Recommended Mitigation Steps

Modify the function as the following:

    function _updateRoundAfterTokenBurned(address user) internal {
        if (totalScoreUpdatesRequired > 0 && !isScoreUpdated[nextScoreUpdateRoundId][user]) totalScoreUpdatesRequired--;

        if (pendingScoreUpdates > 0 && !isScoreUpdated[nextScoreUpdateRoundId][user]) {
            pendingScoreUpdates--;
        }
    }

And also, when creating a new user, set his score update for the current round as true:

    function _mint(bool isIrrevocable, address user) internal {
        if (tokens[user].exists) revert IneligibleToClaim();

        tokens[user].exists = true;
        tokens[user].isIrrevocable = isIrrevocable;

        isScoreUpdated[nextScoreUpdateRoundId][user] = true;

        if (isIrrevocable) {
            totalIrrevocable++;
        } else {
            totalRevocable++;
        }

        if (totalIrrevocable > irrevocableLimit || totalRevocable > revocableLimit) revert InvalidLimit();

        emit Mint(user, isIrrevocable);
    }

Assessed type

Context

Low

Prime.sol: Users with issued tokens can instantly re-enter the protocol if they were in the 90 days waiting period

If an user is issued a prime token while waiting for the claim period, his stakedAt is not zeroed. If the user burns this token in the future by withdrawing from the XVS vault, he can earn it back by calling the claim() function, bypassing the 90 day waiting period.

In order to fix this, consider setting the stakedAt[user] to zero in the token issue, as shown below:

                } else {
                    _mint(true, users[i]);
                    _initializeMarkets(users[i]);
                    stakedAt[user] = 0;
                }

Prime.sol: In case of a prime contract migration, earned yield cannot be claimed if the previous contract's balance + the PSR funds are not enough to cover them

In _claimInterest(), the PLP unreleased funds are transfered in case the prime contract and the PSR unreleased funds are not enough to cover the interest claim. If there's a prime contract migration, this call will revert as the previous prime contract will not be authorized to claim the PLP tokens. This migration will cause users who have earned interest to only be able to claim what's in the contract and the PSR, if such PSR has not migrated to a new prime token yet. Worst case scenario, the PSR has migrated as well and only the previous prime contract balance can cover the users claims.

Submitting this as low as it's possible for the developers to remigrate to the previous contract in order to fix this. Developers can also claim the interest in the users stead before migrating.

PrimeLiquidityProvider.sol: Governance shouldn't sweep XVS tokens

If this happens, getEffectiveDistributionSpeed() could potentially have a balance lower than accrued value, which leads to this function always reverting whenever it's called.

Non-Critical

Prime.sol: Remove comptroller check

updateAssetsState() requires a redudant comptroller argument, given there's only one comptroller and the check serves no purpose in the function.

PrimeLiquidityProvider.sol: Max distribution speed could be too low in case of future tokens with very high total supply

Tokens, like SHIBA for example, have an extremely high total supply which leads to a very low USD value per token. Since the maximum distribution speed is 1e18 per block, if the protocol includes tokens like these in the future, the reward speed could be too insignificant.

Scores.sol: Improved math

In line 58, when calculating the exponentiation, consider changing e ^ ( ln(ratio) * 𝝰 ) to ratio^alpha instead, since they're mathematically equivalent.