It's possible to borrow, redeem, transfer tokens and exit markets with outdated collateral prices and borrow interest

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L199 https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L299 https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L324 https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L553 https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L1240 https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L1255 https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L578

Vulnerability details

Impact

Incorrect borrowBalance and token collateral values. Could lead to many different exploits, such has:

• Users with a collateral token that fell in price substancially can borrow another underlying whose price has not been updated and earn profit.
• Users can borrow/redeem/transfer more if the interest/price was not updated.

Proof of Concept

In the Comptroller, the total collateral balance and borrow balance is calculated at _getHypotheticalLiquiditySnapshot(...). This function calculates these balances in the following loop:

for (uint256 i; i < assetsCount; ++i) {
    VToken asset = assets[i];

    // Read the balances and exchange rate from the vToken
    (uint256 vTokenBalance, uint256 borrowBalance, uint256 exchangeRateMantissa) = _safeGetAccountSnapshot(
        asset,
        account
    );

    // Get the normalized price of the asset
    Exp memory oraclePrice = Exp({ mantissa: _safeGetUnderlyingPrice(asset) });

    // Pre-compute conversion factors from vTokens -> usd
    Exp memory vTokenPrice = mul_(Exp({ mantissa: exchangeRateMantissa }), oraclePrice);
    Exp memory weightedVTokenPrice = mul_(weight(asset), vTokenPrice);

    // weightedCollateral += weightedVTokenPrice * vTokenBalance
    snapshot.weightedCollateral = mul_ScalarTruncateAddUInt(
        weightedVTokenPrice,
        vTokenBalance,
        snapshot.weightedCollateral
    );

    // totalCollateral += vTokenPrice * vTokenBalance
    snapshot.totalCollateral = mul_ScalarTruncateAddUInt(vTokenPrice, vTokenBalance, snapshot.totalCollateral);

    // borrows += oraclePrice * borrowBalance
    snapshot.borrows = mul_ScalarTruncateAddUInt(oraclePrice, borrowBalance, snapshot.borrows);

    // Calculate effects of interacting with vTokenModify
    if (asset == vTokenModify) {
        // redeem effect
        // effects += tokensToDenom * redeemTokens
        snapshot.effects = mul_ScalarTruncateAddUInt(weightedVTokenPrice, redeemTokens, snapshot.effects);

        // borrow effect
        // effects += oraclePrice * borrowAmount
        snapshot.effects = mul_ScalarTruncateAddUInt(oraclePrice, borrowAmount, snapshot.effects);
    }
}

As can be seen, the oracle price is not updated via calling updatePrice(...), nor the borrow interest is updated by calling AccrueInterest(...). Only the corresponding VToken that called the borrow(...), transfer(...)orredeem(...)` has an updated price and interest, which could lead to critical inaccuracies for accounts with several VTokens.

Tools Used

Vscode, Hardhat

Recommended Mitigation Steps

Update the price and interest of every collateral except the VToken that triggered the hook, which has already been updated. Similarly to what is being done on healAccount(...).

for (uint256 i; i < userAssetsCount; ++i) {
   userAssets[i].accrueInterest();
   oracle.updatePrice(address(userAssets[i]));
}

Assessed type

Oracle

[c4-judge]

0xean marked the issue as duplicate of #88

[c4-judge]

0xean changed the severity to 2 (Med Risk)

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report

[chechu]

To share more info related to this topic:

• As we said in #88, we assume the prices in the rest of the markets will be updated frequently because they are updated every time other users interact directly with these other markets
• Moreover, "update" in our context only affects the TWAP oracle. As you can see here, our oracles system uses Chainlink, Binance Oracle, Pyth, and TWAP sources.
  • Chainlink and Binance prices are not updated by Venus anyway. Chainlink and Binance update these price feeds following the classical rules of heartbeat and deviation (example here). In the Oracle system, we check the last time these prices were updated and discard them (reverting the TX) if they are staled.
  • TWAP needs a proactive update, executed by Venus when the mentioned oracle.updatePrice is invoked. If no one invokes this update function for an asset later used indirectly by another user, and the difference between the price offered by the TWAP oracle and the rest of the oracles (i.e. Chainlink) is greater than a threshold configure in our Oracle system, the TX will be also reverted. Could this generate a DoS? I suppose that potentially it can do it, but as soon as a user interact with the "staled" asset in Venus the block will disappear

[0xean]

@chechu - I believe I should mark #104 as a dupe of this as well. Since this issue talks about both prices updated and accruing interests.

we assume the prices in the rest of the markets will be updated frequently because they are updated every time other users interact directly with these other markets

This assumption I think has risks in which the wardens are calling out. Its hard to say how real these risks are apriori without making assumptions about usage of all the markets.

I think these issues should be batched together over concerns around accrueInterest and price updates into a single M issue.

[chechu]

This assumption I think has risks in which the wardens are calling out. Its hard to say how real these risks are apriori without making assumptions about usage of all the markets.

@0xean IMHO I think the risk is low, but I could understand the concern and the lack of a guarantee in the code.

Regarding grouping this and #104, #104 only mentions the outdated interests and this mentions the outdated interest and the outdated prices. So, up to you, you are the judge :)

[0xean]

Sorry, I meant duping #104 against this. thanks for the response, will judge accordingly!

[c4-judge]

0xean marked the issue as duplicate of #486

[0xean]

Issues mentioning both will receive 100% credit while issues mentioning one will receive 50%

[c4-judge]

0xean marked the issue as not a duplicate

[c4-judge]

0xean marked the issue as primary issue