code-423n4 / 2023-09-venus-findings

4 stars 4 forks source link

A user can frontrun and receive a revocable token when the `issue` function is executed, even though they’re no longer qualified. #485

Open c4-submissions opened 11 months ago

c4-submissions commented 11 months ago

Lines of code

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

Vulnerability details

Impact

Stated in the discord channel:

“What is the primary use of the issue function?

Issue the initial set of Prime tokens, to the users that were staking more than 1,000 XVS tokens for the last 90 days. Moreover, it will be used to issue the irrevocable tokens (the business criteria for issuing these irrevocable tokens are not defined yet)”.

A user can bypass these criteria by first becoming eligible to be issued a revocable token, then frontrunning the issue function and unstaking their XVS tokens (resulting in their staked XVS balance dropping below 1,000 XVS).

Since the issue function lacks checks when minting a revocable token, the user would still receive a revocable token when the issue function is executed, even though they no longer meet the qualification criteria (they no longer stake >= 1000 XVS tokens).

Proof of Concept

Here is the coded scenario to illustrate the vulnerability

  describe("mint and burn", () => {
    let prime: PrimeScenario;
    let xvsVault: XVSVault;
    let xvs: XVS;

    beforeEach(async () => {
      ({ prime, xvsVault, xvs } = await loadFixture(deployProtocol));
    });

    it.only("POC - user still receives a revocable token when the `issue` function is executed, even though they’re no longer qualified.", async () => {
      // user stakes 10000 XVS for the last 90 days - make them eligible for being issued a revocable token by the protocol
      await xvs.connect(user1).approve(xvsVault.address, bigNumber18.mul(10000));
      await xvsVault.connect(user1).deposit(xvs.address, 0, bigNumber18.mul(10000));
      await mine(90 * 24 * 60 * 60);

      // user frontruns `issue` function to unstake XVS tokens
      await xvsVault.connect(user1).requestWithdrawal(xvs.address, 0, bigNumber18.mul(9700));

      // user gets issued revocable token, despite not staking more than 1000 XVS tokens
      await expect(prime.issue(false, [user1.getAddress()])).to.be.not.reverted;
      let token = await prime.tokens(user1.getAddress());
      expect(token.isIrrevocable).to.be.equal(false);
      expect(token.exists).to.be.equal(true);
    });
  });

Tools Used

VsCode

Recommended Mitigation Steps

In issue function, add a requirement to check if a user has staked a sufficient amount of XVS tokens before minting to them a revocable token

    /**
     * @notice Directly issue prime tokens to users
     * @param isIrrevocable are the tokens being issued
     * @param users list of address to issue tokens to
     */
    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; ) {
                uint256 totalStaked = _xvsBalanceOfUser(users[i]);
                bool isAccountEligible = isEligible(totalStaked);
                if (!isAccountEligible) {
                    revert IneligibleToClaim();
                }
                _mint(false, users[i]);
                _initializeMarkets(users[i]);
                delete stakedAt[users[i]];

                unchecked {
                    i++;
                }
            }
        }
    }

Assessed type

MEV

c4-pre-sort commented 11 months ago

0xRobocop marked the issue as low quality report

0xRobocop commented 11 months ago

If issue should validate if a user has enough staking or not seems like a design decision.

Leaving for sponsor validation.

c4-pre-sort commented 11 months ago

0xRobocop marked the issue as primary issue

c4-pre-sort commented 11 months ago

0xRobocop marked the issue as high quality report

c4-sponsor commented 11 months ago

chechu marked the issue as disagree with severity

chechu commented 11 months ago

Consider QA

1) the motivation of the user to do this is low because the rewards are proportional to the XVS staked, so less XVS implies less rewards 2) there is a mechanism to fix this (unlikely) situation: execute a Critical VIP (only 7 hours of delay) to burn the token

If the user frontrun the issue function:

The issue function is only executed by Governance, and we tried to reduce the gas consumed to emit as many Prime tokens as possible in one transaction

c4-sponsor commented 11 months ago

chechu (sponsor) acknowledged

c4-judge commented 10 months ago

fatherGoose1 changed the severity to QA (Quality Assurance)

fatherGoose1 commented 10 months ago

Agree with QA given the lack of incentives. The prime token will lack utility with lower XVS staked as the user won't accrue much rewards.

c4-judge commented 10 months ago

fatherGoose1 marked the issue as grade-b