Venus Protocol Isolated Pools - Team_Rocket's results

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/WhitePaperInterestRateModel.sol#L17

Vulnerability details

[M-02] Incorrect blocksPerYear constant in WhitepaperInterestRateModel

Location

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/WhitePaperInterestRateModel.sol#L17

Impact

The interest rate per block is 5x greater than it's intended to be for markets that use the Whitepaper interest rate model.

Proof of Concept

The WhitePaperInterestRateModel contract is forked from Compound Finance, which was designed to be deployed on Ethereum Mainnet. The blocksPerYear constant inside the contract is used to calculate the interest rate of the market on a per-block basis and is set to 2102400, which assumes that there are 365 days a year and that the block-time is 15 seconds.

However, Venus Protocol is deployed on BNB chain, which has a block-time of only 3 seconds. This results in the interest rate per block on the BNB chain to be 5x greater than intended.

Both baseRatePerBlock and multiplierPerBlock are affected and are 5x the value they should be. This also implies that the pool's interest rate is also 5 times more sensitive to utilization rate changes than intended. It is impossible for the market to arbitrage and adjust the interest rate back to the intended rate as seen in the PoC graph below. It's likely that arbitrageurs will deposit as much collateral as possible to take advantage of the high supply rate, leading to a utilization ratio close to 0.

The following Python script plots the WhitePaperInterestRateModel curves for a 15 second and a 3 second block time.

import matplotlib.pyplot as plt

# Constants
BASE = 1e18

# Solidity functions converted to Python functions
def utilization_rate(cash, borrows, reserves):
    if borrows == 0:
        return 0
    return (borrows * BASE) / (cash + borrows - reserves)

def get_borrow_rate(ur, base_rate_per_block, multiplier_per_block):
    return ((ur * multiplier_per_block) / BASE) + base_rate_per_block

def generate_data_points(base_rate_per_year, multiplier_per_year, blocks_per_year, cash, borrows, reserves):
    base_rate_per_block = base_rate_per_year / blocks_per_year
    multiplier_per_block = multiplier_per_year / blocks_per_year

    utilization_rates = [i / 100 for i in range(101)]
    borrow_rates = [get_borrow_rate(ur * BASE, base_rate_per_block, multiplier_per_block) for ur in utilization_rates]

    return utilization_rates, borrow_rates

# User inputs
base_rate_per_year = 5e16 # 5%
multiplier_per_year = 1e16 # 1%
blocks_per_year1 = 2102400 # 15 second block-time
blocks_per_year2 = 10512000 # 3 second block-time

# Example values for cash, borrows, and reserves
cash = 1e18
borrows = 5e18
reserves = 0.1e18

# Generate data points for both curves
utilization_rates1, borrow_rates1 = generate_data_points(base_rate_per_year, multiplier_per_year, blocks_per_year1, cash, borrows, reserves)
utilization_rates2, borrow_rates2 = generate_data_points(base_rate_per_year, multiplier_per_year, blocks_per_year2, cash, borrows, reserves)

# Plot both curves on the same plot with a key
plt.plot(utilization_rates1, borrow_rates1, label=f"Blocks per year: {blocks_per_year1}")
plt.plot(utilization_rates2, borrow_rates2, label=f"Blocks per year: {blocks_per_year2}")
plt.xlabel("Utilization Rate")
plt.ylabel("Borrow Rate")
plt.title("Interest Rate Curves")
plt.legend()
plt.show()

Result:

As seen above, the borrow rate curves are different and do not intersect. Hence, it's impossible via arbitrage for market participants to adjust the rate back to its intended value.

Tools Used

Manual review

Recommended Mitigation Steps

Fix the blocksPerYear constant so that it accurately describes the number of blocks a year on BNB chain, which has a block-time of 15 seconds. The correct value is 10512000.

\begin{aligned}
\text{blocksPerYear} &= \frac{\text{secondsInAYear}}{\text{blockTime}} \\
&= \frac{365 \times 24 \times 60 \times 60}{3} \\
&= 10{,}512{,}000
\end{aligned}

@@ -14,7 +14,7 @@ contract WhitePaperInterestRateModel is InterestRateModel {
     /**
      * @notice The approximate number of blocks per year that is assumed by the interest rate model
      */
-    uint256 public constant blocksPerYear = 2102400;
+    uint256 public constant blocksPerYear = 10512000;

     /**
      * @notice The multiplier of utilization rate that gives the slope of the interest rate

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/Shortfall/Shortfall.sol#L183 https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/Shortfall/Shortfall.sol#L190 https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/Shortfall/Shortfall.sol#L248

Vulnerability details

Impact

A malicious attacker can be the auction.highestBidder and prevent anyone from outbidding them by a DoS of the placeBid function.

Proof of Concept

When a user places a new highest bid in a Shortfall auction by calling placeBid, the contract refunds the previous auction.highestBidder all of the tokens that they deposited into the Shortfall contract.

If any of the tokens to be transferred are ERC-777 tokens, it is possible for a malicious attacker to use the tokensReceived hook to cause a denial-of-service attack. This prevents new bids from replacing auction.highestBidder, causing the auction.highestBidder to win the auction without any competition.

The DoS attack can be performed as so:

1. Deploy a smart contract that can place bids and reverts when the tokenReceived hook is called. An example of one is below:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "@openzeppelin/contracts/interfaces/IERC777Recipient.sol";

interface IShortfall {
    function placeBid(address comptroller, uint256 bidBps) external;
}

contract MaliciousContract is IERC777Recipient {
    IShortfall private immutable shortfall;

    constructor(IShortfall _shortfall) {
        shortfall = _shortfall;
    }

    function placeBid(address comptroller, uint256 bidBps) external {
        shortfall.placeBid(comptroller, bidBps);
    }

    function tokensReceived(
        address operator,
        address from,
        address to,
        uint256 amount,
        bytes calldata userData,
        bytes calldata operatorData
    ) external {
        revert();
    }
}

2. Become auction.highestBidder by calling MaliciousContract.placeBid.

Once MaliciousContract becomes auction.highestBidder, other bidders cannot call placeBid, as it would just revert when transferring tokens back to MaliciousContract.

Tools Used

Manual review

Recommended Mitigation Steps

The current implementation uses a push configuration, meaning that it's the contract that 'initiates' the transfer of tokens back to the previous auction.highestBidder.

The contract should use a pull configuration instead, where it keeps track of bidders' balances and allows outbid bidders to withdraw their own funds from the contract through a withdraw function. The placeBid function should no longer transfer tokens back to the current auction.highestBidder and only change the value of auction.highestBidder.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/Shortfall/Shortfall.sol#L384-L406 https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/Shortfall/Shortfall.sol#L227-L230 https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/Shortfall/Shortfall.sol#L248

Vulnerability details

Impact

If convertibleBaseAsset is not a stablecoin, bidders can receive a lot more value than the incentiveBps bonus they should.

Proof of Concept

        uint256 poolBadDebt;

        uint256[] memory marketsDebt = new uint256[](marketsCount);
        auction.markets = new VToken[](marketsCount);

        for (uint256 i; i < marketsCount; ++i) {
            uint256 marketBadDebt = vTokens[i].badDebt();

            priceOracle.updatePrice(address(vTokens[i]));
            uint256 usdValue = (priceOracle.getUnderlyingPrice(address(vTokens[i])) * marketBadDebt) / 1e18;

            poolBadDebt = poolBadDebt + usdValue;
            auction.markets[i] = vTokens[i];
            auction.marketDebt[vTokens[i]] = marketBadDebt;
            marketsDebt[i] = marketBadDebt;
        }

        require(poolBadDebt >= minimumPoolBadDebt, "pool bad debt is too low");

        uint256 riskFundBalance = riskFund.poolReserves(comptroller);
        uint256 remainingRiskFundBalance = riskFundBalance;
        uint256 incentivizedRiskFundBalance = poolBadDebt + ((poolBadDebt * incentiveBps) / MAX_BPS);
        if (incentivizedRiskFundBalance >= riskFundBalance) {

poolBadDebt represents the USD value for entire pool bad debt.

For incentiveBps = 10%, incentivizedRiskFundBalance = 1.1 * poolBadDebt

On the other hand riskFundBalance gets the poolReserves(comptroller) which is a convertibleBaseAsset. On line 406 the amount of two different currencies is compared :

        if (incentivizedRiskFundBalance >= riskFundBalance) {

Next auction.seizedRiskFund is either riskFundBalance (if AuctionType.LARGE_POOL_DEBT) or incentivizedRiskFundBalance ( if AuctionType.LARGE_RISK_FUND).

In closeAuction() highestBidder:

• pays marketBadDebt * highestBidBps (underlying amount) and receive riskFundBalance (convertibleBaseAsset amount) ( in a LARGE_POOL_DEBT auction type).

============================================

Let's have a pool with one market:

• price underlyingToken = 10 USD,
• incentiveBps = 10%,
• badDebt = 100 underlyingToken
• convertibleBaseAsset is WBSC
• riskFundBalance = 50 WBSC (@300USD/WBSC) The following is true: if(incentivizedRiskFundBalance >= riskFundBalance) (1.1 * 100 * 10 >= 50)

StartBidPbs = MAX * riskFundBalance / 1.1 * badDebt
StartBidPbs = 10000 * 50 / 1.1 * 100 = 4545
auction.seizedRiskFund = riskFundBalance
auction.marketDebt = badDebt

Let's suppose we have only one bidder who bids highestBidBps = 4545.

He pays: auction.marketDebt * 4545 / 10000 = 100 * 0.4545 = 45.45 underlyingToken

(total value payed: 45.45 * 10 = 454.5 USD)

He receive seizedRiskFund = riskFundBalance = 50 WBSC

(total value received: 50 * 300 = 1500 USD).

Tools Used

Manual review

Recommended Mitigation Steps

Get the riskFundBalance USD value( eg. usdRiskFundValue), compare it with incentivizedRiskFundBalance;

• use usdRiskFundValue instead of remainingRiskFundBalance in the startBidBps calculation.
• in the else branch, update remainingRiskFundBalance to use convertibleBaseAsset amounts only

- if (incentivizedRiskFundBalance >= riskFundBalance) {
+ if (incentivizedRiskFundBalance >= usdRiskFundValue) {
            auction.startBidBps =
-                (MAX_BPS * MAX_BPS * remainingRiskFundBalance) /
+               (MAX_BPS * MAX_BPS * usdRiskFundValue) /
                (poolBadDebt * (MAX_BPS + incentiveBps));
            remainingRiskFundBalance = 0;
            auction.auctionType = AuctionType.LARGE_POOL_DEBT;
        } else {
            uint256 maxSeizeableRiskFundBalance = incentivizedRiskFundBalance;

-            remainingRiskFundBalance = remainingRiskFundBalance - maxSeizeableRiskFundBalance;
+            remainingUsdRiskFundValue = usdRiskFundValue - maxSeizeableRiskFundBalance;
+            remainingRiskFundBalance = remainingRiskFundBalance - (remainingUsdRiskFundValue * convertibleBaseAssetPrice);
            auction.auctionType = AuctionType.LARGE_RISK_FUND;
            auction.startBidBps = MAX_BPS;
        }

Assessed type

Other

Code4rena Venus Protocol May 2023 - Team_Rocket QA Report

Summary

The codebase is well coded, but could improve in adding sanity checks to make sure that user/admin inputs are intended. The codebase can also provide more informational comments about the purpose of some state variables to improve readability.

Low Severity Findings

[L-01] Add a sanity check in addMarket to make sure that the correct interest rate model is being used

Location

PoolRegistry.sol#L256-L281

Description

The addMarket function allows the Comptroller contract to add a new market that follows either the Jump Rate or the Whitepaper interest rate model. These two models have different parameters that are given to the function through the AddMarketInput struct.

The wrong interest rate model could be used to create a new market due to incorrect input. For example, a market was intended to use the Jump Rate model but through incorrect input, is set to use the Whitepaper model.

Impact

The wrong interest rate model could be used for a market.

Tools Used

Manual review

Recommended Mitigation Steps

Include a check for kink and jumpMultiplier to be equal to zero if the market is intended to be based on the Whitepaper model.

@@ -277,6 +277,7 @@ contract PoolRegistry is Ownable2StepUpgradeable, AccessControlledV8, PoolRegist
                 )
             );
         } else {
+            require(input.kink_ == 0 && input.jumpMultiplierPerYear == 0, "PoolRegistry: Invalid Whitepaper parameters");
             rate = InterestRateModel(whitePaperFactory.deploy(input.baseRatePerYear, input.multiplierPerYear));
         }

[L-02] RewardsDistributor._grantRewardToken return values are confusing

Location

RewardsDistributor.sol#L416-L423

Description

The _grantRewardToken function returns a uint256 value that is either 0 for a successful transfer of reward tokens or amount for an unsuccessful transfer. This is confusing, and can lead to vulnerabilities in the future if the contract was upgraded.

Tools Used

Manual review

Recommended Mitigation Steps

Use a bool value indicating whether the transfer of reward tokens for successful. Refactor claimRewardToken(address,VToken[]) and grantRewardToken to account for the new bool return value.

[L-03] PoolsLens.getPoolBadDebt() returns non-truncated USD value.

Location

PoolLens.sol#L262-L271

Description

        // // Calculate the bad debt is USD per market
        for (uint256 i; i < markets.length; ++i) {
            BadDebt memory badDebt;
            badDebt.vTokenAddress = address(markets[i]);
            //@audit divide badDebtUsd by 1e18
            badDebt.badDebtUsd =
                VToken(address(markets[i])).badDebt() *
                priceOracle.getUnderlyingPrice(address(markets[i]));
            badDebtSummary.badDebts[i] = badDebt;
            totalBadDebtUsd = totalBadDebtUsd + badDebt.badDebtUsd;
        }

Both badDebt() and getUnderlyingPrice() are 1e18 decimals based.

Tools Used

Manual review

Recommended Mitigation Steps

Divide the results with 1e18 as it is done in other places (ex1, ex2).

Non-Critical Findings

[NC-01] Incorrect comment to describe protocolShareReserve state variable

Location

PoolRegistry.sol#L75-L77

Description

The comment that describes the protocolShareReserve state variable is incorrect and actually describes the shortfall state variable.

Tools Used

Manual review

Recommended Mitigation Steps

Fix the comment to correctly describe protocolShareReserve.

@@ -73,7 +73,7 @@ contract PoolRegistry is Ownable2StepUpgradeable, AccessControlledV8, PoolRegist
     Shortfall public shortfall;

     /**
-     * @notice Shortfall contract address
+     * @notice ProtocolShareReserve contract address
      */
     address payable public protocolShareReserve;

[NC-02] Incorrect comment to describe values in newSupplyCaps and newBorrowCaps

Location

Comptroller.sol#L826-L835

Comptroller.sol#L852-L861

Description

The comment that describes newSupplyCaps and newBorrowCaps states that a value of -1 corresponds to an unlimited supply/borrow. This is impossible since the newSupplyCaps and newBorrowCaps are both of type uint256[] calldata and cannot store negative integers.

Furthermore, the code in preMintHook and preBorrowHook check for type(uint256).max.

Tools Used

Manual review

Recommended Mitigation Steps

Fix the comments to correctly describe that an unlimited supply/borrow is represented by type(uint256).max.

Code4rena Venus Protocol May 2023 - Team_Rocket Gas Optimizations Report

[G-01] Calling safeApprove twice is unneccessary

Location

PoolRegistry.sol#L322-L323

RiskFund.sol#L258-L259

Description

safeApprove is called twice each time to safely approve the VToken contract to spend the underlying asset, and the PancakeSwap Router to swap assets. Using safeApprove is unnecessary as the allowance is spent in the same transaction that the approval occurs, meaning that there will never be leftover allowance after a transaction that can be exploited.

If a scenario where there is actually leftover allowance were to occur, safeApprove will not prevent a front-running attack from occurring since the call with amount = 0 is called in the same transaction as the call with amount > 0.

Recommended Steps

Just call approve once with the required amount.

@@ -319,8 +319,7 @@ contract PoolRegistry is Ownable2StepUpgradeable, AccessControlledV8, PoolRegist

         IERC20Upgradeable token = IERC20Upgradeable(input.asset);
         uint256 amountToSupply = _transferIn(token, msg.sender, input.initialSupply);
-        token.safeApprove(address(vToken), 0);
-        token.safeApprove(address(vToken), amountToSupply);
+        token.approve(address(vToken), amountToSupply);
         vToken.mintBehalf(input.vTokenReceiver, amountToSupply);

         emit MarketAdded(address(comptroller), address(vToken));

@@ -255,8 +255,7 @@ contract RiskFund is
                         path[path.length - 1] == convertibleBaseAsset,
                         "RiskFund: finally path must be convertible base asset"
                     );
-                    IERC20Upgradeable(underlyingAsset).safeApprove(pancakeSwapRouter, 0);
-                    IERC20Upgradeable(underlyingAsset).safeApprove(pancakeSwapRouter, balanceOfUnderlyingAsset);
+                    IERC20Upgradeable(underlyingAsset).approve(pancakeSwapRouter, balanceOfUnderlyingAsset);
                     uint256[] memory amounts = IPancakeswapV2Router(pancakeSwapRouter).swapExactTokensForTokens(
                         balanceOfUnderlyingAsset,
                         amountOutMin,

[G-02] Use calldata arrays instead of memory arrays as function parameters wherever possible

Location

Comptroller.sol#L154

Description

Using a memory array incurs extra gas costs as there is extra overhead relating to copying the calldata onto memory before adding it onto the EVM stack to access its elements. This is not the case with calldata arrays, as the array is loaded from calldata directly onto the stack using the CALLDATALOAD opcode. In situations where the array does not need to be modified, it is more gas efficient to use a calldata array.

Recommended Steps

Change the vTokens type from address[] memory to address[] calldata.

@@ -151,7 +151,7 @@ contract Comptroller is
      * @custom:error MarketNotListed error is thrown if any of the markets is not listed
      * @custom:access Not restricted
      */
-    function enterMarkets(address[] memory vTokens) external override returns (uint256[] memory) {
+    function enterMarkets(address[] calldata vTokens) external override returns (uint256[] memory) {
         uint256 len = vTokens.length;

         uint256 accountAssetsLen = accountAssets[msg.sender].length;

[G-03] Cheaper checks should go before more expensive checks

Location

Comptroller.sol#L190-L206

Description

The exitMarket function performs a series of checks to make sure that the user is allowed to exit the market. It should check if the sender is not already in the market first, to avoid unnecessary expensive calls to _safeGetAccountSnapshot and _checkRedeemAllowed in the case that it is true.

Recommended Steps

Check if the sender is not already in the market before getting a snapshot of their account.

@@ -187,6 +187,12 @@ contract Comptroller is
     function exitMarket(address vTokenAddress) external override returns (uint256) {
         _checkActionPauseState(vTokenAddress, Action.EXIT_MARKET);
         VToken vToken = VToken(vTokenAddress);
+
+        /* Return true if the sender is not already ‘in’ the market */
+        if (!marketToExit.accountMembership[msg.sender]) {
+            return NO_ERROR;
+        }
+
         /* Get sender tokensHeld and amountOwed underlying from the vToken */
         (uint256 tokensHeld, uint256 amountOwed, ) = _safeGetAccountSnapshot(vToken, msg.sender);

@@ -200,11 +206,6 @@ contract Comptroller is

         Market storage marketToExit = markets[address(vToken)];

-        /* Return true if the sender is not already ‘in’ the market */
-        if (!marketToExit.accountMembership[msg.sender]) {
-            return NO_ERROR;
-        }
-
         /* Set vToken account membership to false */
         delete marketToExit.accountMembership[msg.sender];

[G-04] Assign values to storage directly instead of memory first

Location

VToken.sol#L1319-L1328

Description

Math is performed and assigned to a memory variable before it's assigned to a storage variable to check for under/overflows. This is unnecessary, as these checks are also done if the storage variable was directly performed on. You can avoid the gas costs of storing data into memory by just performing the math directly on the storage variable.

Recommended Steps

Perform math directly on storage variables.

@@ -1316,20 +1316,15 @@ contract VToken is
             startingAllowance = transferAllowances[src][spender];
         }

-        /* Do the calculations, checking for {under,over}flow */
-        uint256 allowanceNew = startingAllowance - tokens;
-        uint256 srcTokensNew = accountTokens[src] - tokens;
-        uint256 dstTokensNew = accountTokens[dst] + tokens;
-
         /////////////////////////
         // EFFECTS & INTERACTIONS

-        accountTokens[src] = srcTokensNew;
-        accountTokens[dst] = dstTokensNew;
+        accountTokens[src] = accountTokens[src] - tokens;
+        accountTokens[dst] = accountTokens[dst] + tokens;

         /* Eat some of the allowance (if necessary) */
         if (startingAllowance != type(uint256).max) {
-            transferAllowances[src][spender] = allowanceNew;
+            transferAllowances[src][spender] = startingAllowance - tokens;
         }

         /* We emit a Transfer event */