Venus Prime - sces60107's results

Lines of code

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

Vulnerability details

Impact

In Prime._calculateScore, it would calculate the score. One of the steps is normalizing capital. It normalizes the decimals to 1e18. However, the original decimals of the capital should be the decimals of the underlying token instead of the decimals of vToken. The result after normalizing is wrong.

Proof of Concept

In _calulateScore, capital is calculated based on borrow and supply. And it would be normalized by capital = capital * (10 ** (18 - vToken.decimals()));. It considers the decimals of vToken to be the original decimals. https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L661

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

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

However, the decimals of capital should be the same as the decimals of supply and borrow. vToken.borrowBalanceStored returns the borrow balance in units of the underlying asset. vToken.exchangeRateStored returns exchange rate, scaled by 1 * 10^(18 - vToken underlying token decimals + underlying token decimals). Briefly speaking, supply and borrow have the same decimals as the underlying asset.

Here are some real examples. Venus Prime has deployed vUSDT and vHAY on testnet.

vUSDT: https://testnet.bscscan.com/address/0x3338988d0beb4419Acb8fE624218754053362D06#readProxyContract USDT: https://testnet.bscscan.com/address/0xA11c8D9DC9b66E209Ef60F0C8D969D3CD988782c#readContract

vUSDT decimals = 8
vUSDT exchangeRateStored = 10000073138314892 = 1.0000073138314892 * 10^16
USDT decimals = 6

vHAY: https://testnet.bscscan.com/address/0x170d3b2da05cc2124334240fB34ad1359e34C562#readProxyContract HAY: https://testnet.bscscan.com/address/0xe73774DfCD551BF75650772dC2cC56a2B6323453#readContract

vHAY decimals = 8
vUSDT exchangeRateStored = 10056552678316331430310897002 = 1.0056552678316331430310897002 * 10^28
USDT decimals = 18

We can find out that the decimals of vToken and underlying assets are different. Prime._calculateScore should not use the decimals of vToken.

Tools Used

Manual Review

Recommended Mitigation Steps

Use the decimals of the underlying token instead of the decimals of vToken

    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()));
+       capital = capital * (10 ** (18 - _getUnderlying(vToken).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#L208

Vulnerability details

Impact

Prime.updateScores can update the total score of multiple users and the market. And if the score of one of the users is already updated, it should skip the update. However, there is an implementation error, leading to an infinite loop.

Proof of Concept

In the for loop of Prime.updateScores, if isScoreUpdated[nextScoreUpdateRoundId][user] is true, it will continue. But it doesn’t increase the value of i, leading to an infinite loop https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L208

    function updateScores(address[] memory users) external {
        if (pendingScoreUpdates == 0) revert NoScoreUpdatesRequired();
        if (nextScoreUpdateRoundId == 0) revert NoScoreUpdatesRequired();

        for (uint256 i = 0; i < users.length; ) {
            address user = users[i];

            if (!tokens[user].exists) revert UserHasNoPrimeToken();
            if (isScoreUpdated[nextScoreUpdateRoundId][user]) continue;

            …

            unchecked {
                i++;
            }

            emit UserScoreUpdated(user);
        }
    }

Here is the PoC that can demonstrate this issue.

      it("Infinite loop", async () => {
        await xvs.connect(user3).approve(xvsVault.address, bigNumber18.mul(2000));
        await xvsVault.connect(user3).deposit(xvs.address, 0, bigNumber18.mul(2000));
        await prime.issue(false, [user3.getAddress()]);
        await prime.addMarket(vbnb.address, bigNumber18.mul(1), bigNumber18.mul(1));

        await prime.updateScores([user1.getAddress()]);
        await prime.updateScores([user1.getAddress(), user3.getAddress()]); // infinite loop happens
      }); 

Tools Used

Manual Review

Recommended Mitigation Steps

Increase i when isScoreUpdated[nextScoreUpdateRoundId][user] is true.

    function updateScores(address[] memory users) external {
        if (pendingScoreUpdates == 0) revert NoScoreUpdatesRequired();
        if (nextScoreUpdateRoundId == 0) revert NoScoreUpdatesRequired();

        for (uint256 i = 0; i < users.length; ) {
            address user = users[i];

            if (!tokens[user].exists) revert UserHasNoPrimeToken();
-           if (isScoreUpdated[nextScoreUpdateRoundId][user]) continue;
+           if (isScoreUpdated[nextScoreUpdateRoundId][user]) {
+               continue;
+               i++;
+           }
            …

            unchecked {
                i++;
            }

            emit UserScoreUpdated(user);
        }
    }

Assessed type

Loop