Venus Prime - HChang26's results

Lines of code

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

Vulnerability details

Impact

Based on sponsors, any user whose prime token is burnt, needs to stake 1000 XVS and wait for 90 days in order to receive prime token again. In certain edge case, user can bypass 90 days lock period and receive prime token immediately by staking 1000 XVS.

Proof of Concept

VIP (Venus Improvement Proposal) can vote to issue() and burn() prime tokens. Specifically focusing on issue(), VIP may choose to issue prime tokens for various reasons, such as recognizing individuals with exceptional contributions or planning for future promotions, among other purposes.

However, a notable issue emerges when VIP issue() irrevocable prime tokens to users who have been staking at least 1000 XVS for some duration, let's say 90 days for simplicity's sake. These users, despite meeting the staking criteria, have not yet claim() their prime tokens. When VIP issue() irrevocable prime tokens to these same users, the problem lies in the fact that the stakedAt mapping is not reset to zero.

Here's the relevant code snippet:

        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++;
                }
            }
        }

This situation may not pose a problem for users who have not staked before. However, it creates a future concern when VIP decide to burn() prime tokens issued to users who previously staked. In this case, the stakedAt mapping is not reset once more in the _burn() function. Consequently, users can bypass the 90-day lock period in the claim() function and instantly receive prime tokens by staking 1000 XVS.

    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);
    }

While this issue can be solved by calling xvsUpdated() after burn() to reset stakeAt, user can simply front run burn() and stake enough XVS to make sure isAccountEligible = true, so none of the following conditions will trigger.

    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);
        }
    }

Add following PoC to Prime.ts:

  describe("User doesn't have to wait 90 days", () => {
    let prime: PrimeScenario;
    let xvsVault: XVSVault;
    let xvs: XVS;

    beforeEach(async () => {
      ({ prime, xvsVault, xvs } = await loadFixture(deployProtocol));
    });
    it("stakedAt mapping doesn't reset", async () => {
      const user = user1;

      //user stakes XVS and starts waiting for 90 days
      await xvs.connect(user).approve(xvsVault.address, bigNumber18.mul(10000));
      await xvsVault.connect(user).deposit(xvs.address, 0, bigNumber18.mul(10000));

      //Moves past 90 days
      //But user does not claim prime token
      await mine(90 * 24 * 60 * 60);
      //VIP (Venus Improvement Proposal) votes and issues user irrevocable prime status
      await prime.issue(true, [user.getAddress()]);

      //User withdraws XVS and falls below minimum threshold
      await xvsVault.connect(user).requestWithdrawal(xvs.address, 0, bigNumber18.mul(10000));

      //Check user is irrevocable
      expect((await prime.tokens(user.getAddress())).exists).to.be.equal(true);
      expect((await prime.tokens(user.getAddress())).isIrrevocable).to.be.equal(true);

      //VIP votes to burn user since no XVS staked
      await prime.burn(user.getAddress());
      //Check user prime token is burnt
      expect((await prime.tokens(user.getAddress())).exists).to.be.equal(false);
      expect((await prime.tokens(user.getAddress())).isIrrevocable).to.be.equal(false);

      //Check stakedAt mapping is not reset
      expect(await prime.stakedAt(user.getAddress())).to.be.gt(0);

      //User restakes XVS above minimum threshold and claims prime token without waiting for 90 days
      await xvs.connect(user).approve(xvsVault.address, bigNumber18.mul(10000));
      await xvsVault.connect(user).deposit(xvs.address, 0, bigNumber18.mul(10000));
      await prime.connect(user).claim();
      expect((await prime.tokens(user.getAddress())).exists).to.be.equal(true);
    });
  });

Tools Used

Manual Review and Hardhat

Recommended Mitigation Steps

    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]];
                }

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

                unchecked {
                    i++;
                }
            }
        }
    }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Malicious users can back-run addMarket() and block stuff, leading to unfair reward distribution.

Proof of Concept

When VIP votes to addMarket(), scores for all prime users must be updated before distributing rewards. It must NOT be a gradual update. Or the following scenario can happen, leading to unfair reward distribution.

After minting Prime tokens if the Venus community decides to add an existing market to the Prime token program then the score of all users has to be updated to start giving them rewards. The scores cannot be applied gradually in this case as the initial Prime users for the market will get large rewards for some time. So this script will prevent this scenario.

The script will have to call permission-less function updateScores() multiple times due to out-of-gas issue. Note that, the script will NOT pause the market or Prime. It does not solve the issue mentioned above. Since addMarket() is invoked by VIP (Venus Improvement Proposal). Users can simply back-run addMarket() by calling accrueInterestAndUpdateScore() to "initialized" this specific market for them. Subsequently, a malicious user could delay/extend this "updateScore" process(triggered in script) by performing block stuffing with dummy transactions. It's important to highlight that, according to data from https://www.cryptoneur.xyz/en/gas-fees-calculator, gas used range from 6 million to 15 million gas for an average block, costs approximately $3.87 to $9.68 on the Binance Smart Chain (BSC). Rewards are distributed in each block. If the financial gains outweigh the associated costs, malicious users may continue to exploit this vulnerability for profit.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider pausing the market or key functions that accrue interest in addMarket(). Run the script and unpause the market when all scores are updated.

Assessed type

Context