Venus Protocol Isolated Pools - 0xStalin's results

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Lens/PoolLens.sol#L266-L273

Vulnerability details

Impact

Incorrectly calculation of the totalBadDebtUsd for an entire pool. Depending on the usage the protol has for this information it could disrupt core mechanisms or cause problems with the internal accounting

Proof of Concept

Following the math applied on the ChainlinkOracle contract of the Venus Protocol, the price is returned in the scale of 10 ^ (36 - underlying asset decimals))

• The problem is when there are underlying assets of different decimals in the same pool, the price will be scaled up by different magnitudes depending on the number of decimals of the underlying assets

Example

    WBNB (18 decimals in BSC).  USD price: $300.    Value returned by the Oracles:  300^1e18
    TRX (6 decimals in BSC).    USD price: $0.068.  Value returned by the Oracles:  0.068^1e30

• When adding the badDebt for the two markets, the totalBadDebtUsd will be totally distorted because it will end up summing two values scaled up by different magnitudes, the real debt of those markets that is price is scaled up by a lower magnitude than others it will be totally diminished by the difference in scales.
• The resultant totalBadDebtUsd won't represent accurately the real debt of the pool which could lead to a number of different issues depending on how the protocol relies on this value

Tools Used

Manual Audit

Recommended Mitigation Steps

Make sure to divide the result of multiplying the badDebt() of the current market * the price of the underlyingasset by 10^18 to normalize the units before assigning the value of the badDebtUsd

• Take as example the correct calculation of the usdValue of a marketDebt made on the Shortfall contract

badDebt.badDebtUsd =
  (VToken(address(markets[i])).badDebt() *
  priceOracle.getUnderlyingPrice(address(markets[i]))) / 1e18;

Assessed type

Decimal

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L469-L472

Vulnerability details

Impact

Borrowers can cause DoS when liquidator attempts to liquidate a 100% of the borrower's position. The borrower just need to frontrun the liquidation tx and repay a slightly portion of the debt, paying as low as 1 wei will make the borrowBalance to be less than what it was when the liquidator sent the tx to liquidate the position.

Proof of Concept

If aliquidator intends to liquidate the entire position, but the borrower frontruns the liquidator's transaction and repays an insignificant amount of the total debt, will cause the borrowBalance to be less than it was when the liquidator sent its transaction, thus, will cause the value of the maxClose variable to be less than the repayAmount that the liquidator set to liquidate the whole position, which will end up causing the tx to be reverted because of this validation

Example

• There is a position of 100 WBNB to be liquidated, the liquidator sends the repayAmount as whatever the maxClose was at that point, the borrower realizes that the 100% of its position will be liquidated and then frontruns the liquidation transaction by repaying an insignificant amount of the total borrow.
• When the liquidation transaction is executed, the maxClose will be calculated based on the new borrowBalance, which will cause the calculation of maxClose to be less than the total repayAmount that was sent, and the transaction will be reverted even though the position is still in a liquidation state

Tools Used

Manual Audit

Recommended Mitigation Steps

Instead of reverting the tx if the repayAmount is greater than maxClose, recalculate the final repayAmount to be paid during the execution of the liquidation and return this calculated value back to the function that called the preLiquidateHook()

function preLiquidateHook(
    ...
    uint256 repayAmount,
    ...
+   ) external override returns(uint256 repayAmountFinal) {
    ...
    ...
-   if (repayAmount > maxClose) {
-      revert TooMuchRepay();
-   }
+   repayAmountFinal = repayAmount > maxClose ? maxClose : repayAmount

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/RiskFund/RiskFund.sol#L265

Vulnerability details

Impact

Selling of asset tokens misses the transaction expiration check, which may lead to reward tokens being sold at a price that's lower than the market price at the moment of a swap.

Proof of Concept

The _swapAsset() function, which is responsible for selling tokens on Pancackeswap, sets the deadline argument of the amountOutMin call to block.timestamp, which basically disables the transaction expiration check because the deadline will be set to whatever timestamp the block including the transaction is minted at.

Transaction expiration check (implemented in Pancackeswap via the deadline argument) allows users of Pancackeswap to protect from selling tokens at an outdated price that's lower than the current price. Consider this scenario:

1. The RiskFund.swapPoolsAssets() function is called.
2. Before the transaction is mined, there's a rapid increase of gas cost. The transaction remains in the mempool for some time since the gas cost paid by the transaction is lower than the current gas price.
3. While the transaction is in the mempool, the price of the baseToken increases.
4. After a while, gas cost drops and the transaction is mined. Since the value of amountOutMin was calculated based on an outdated baseToken price which is now lower than the current price, the swapping is sandwiched by a MEV bot. The bot decreases the price of the reward token in a Pancackeswap pool so than the minimum output amount check still holds and earns a profit from the swapping happing at a lower price.
5. As a result of the sandwich attack, asset tokens are swapped at an outdated price, which is now lower than the current price of the baseTokens. The protocol earn less than they could've received by selling the tokens at the current price.

Tools Used

Manual Audit

Recommended Mitigation Steps

Consider a reasonable value to the deadline argument. For example, Uniswap sets it to 30 minutes on the Ethereum mainnet and to 5 minutes on L2 networks, taking as reference those values, estimate how much time would be a reasonable value for the BSC chain. Also consider letting the caller to change the value when on-chain conditions change and may require a different value.

Assessed type

MEV