Venus Protocol Isolated Pools - 0xnev's results

Lines of code

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

Vulnerability details

Impact

Shortfall._startAuction() assumes underlying assets always has 18 decimals which can skew calculation of usdValue and pool bad debt, resulting in either wrongful start of auction or preventing starting of auction based on minimumPoolBadDebt

Proof of Concept

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

function _startAuction(address comptroller) internal
    ...
    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");
    ...

Users can call Shortfall.startAuction to start an auction to auction off accumulated bad debt in a pool as long as minimumPoolBadDebt is met.

When calculating usd value of bad debt in pool, it calls function getUnderlyingPrice(address cToken) external view returns (uint) that returns the price of the asset in USD as an unsigned integer scaled up by 10 ^ (36 - underlying asset decimals). The function uses divisor of 1e18, which assumes all underlying assets of vTokens has decimals of 18 which could be incorrect. If underlying assets of vTokens does not have 18 decimals (e.g. SAFEMOON, 9 decimals) then usdValue can be skewed.

If underlying asset has decimals more than 18, usd value will be less than expected If underlying asset has decimals less than 18, usd value will be more than expected

This will either allow auction to be started before hitting minimum bad debt require (minimumPoolBadDebt) or unfairly prevent auction from starting due to underestimation of poolBadDebt.

In addition, in the PoolRegistry.sol contract, the fact that rate is scaled with input.decimals indicates that there could potentially be vTokens added with underlying assets not having 18 decimals on BNB chain (for example, SAFEMOON). Furthermore, it also makes ShortFall contract not compatible with ethereum mainnet/L2 since decimals of underlying tokens are different in those chains, which can force redeployment if there are plans to integrate protocol in these chains.

Tools Used

Manual Analysis

Recommendation

function _startAuction(address comptroller) internal
    ...
    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;
++      uint256 usdValue = (priceOracle.getUnderlyingPrice(address(vTokens[i])) * marketBadDebt) / (10 ^ (36 - IERC20Metadata(vTokens[i].underlying()).decimals()));

        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");
    ...

Assessed type

Decimal

Lines of code

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

Vulnerability details

Impact

Not allowing users to supply their own deadline could potentially expose them to sandwich attacks

Proof of Concept

RiskFund.sol#L174

function swapPoolsAssets(
    address[] calldata markets,
    uint256[] calldata amountsOutMin,
    address[][] calldata paths
) external override returns (uint256) {
    _checkAccessAllowed("swapPoolsAssets(address[],uint256[],address[][])");
    require(poolRegistry != address(0), "Risk fund: Invalid pool registry.");
    require(markets.length == amountsOutMin.length, "Risk fund: markets and amountsOutMin are unequal lengths");
    require(markets.length == paths.length, "Risk fund: markets and paths are unequal lengths");

    uint256 totalAmount;
    uint256 marketsCount = markets.length;

    _ensureMaxLoops(marketsCount);

    for (uint256 i; i < marketsCount; ++i) {
        VToken vToken = VToken(markets[i]);
        address comptroller = address(vToken.comptroller());

        PoolRegistry.VenusPool memory pool = PoolRegistry(poolRegistry).getPoolByComptroller(comptroller);
        require(pool.comptroller == comptroller, "comptroller doesn't exist pool registry");
        require(Comptroller(comptroller).isMarketListed(vToken), "market is not listed");

        uint256 swappedTokens = _swapAsset(vToken, comptroller, amountsOutMin[i], paths[i]);
        poolReserves[comptroller] = poolReserves[comptroller] + swappedTokens;
        totalAmount = totalAmount + swappedTokens;
    }

    emit SwappedPoolsAssets(markets, amountsOutMin, totalAmount);

    return totalAmount;
}

In RiskFund.swapPoolsAsset, there is a parameter to allow users to supply slippage through amountOutMin but does not allow user to include a deadline check when swapping pool assets into base assets, in the event that pool assets is not equal to convertibleBaseAsset.

uint256 swappedTokens = _swapAsset(vToken, comptroller, amountsOutMin[i], paths[i]);

In RiskFund._swapAsset, there is a call to IPancakeswapV2Router(pancakeSwapRouter).swapExactTokensForTokens() but the deadline parameter is simply passed in as current block.timestamp in which transaction occurs. This effectively means that transaction has no deadline, which means that swap transaction may be included anytime by validators and remain pending in mempool, potentially exposing users to sandwich attacks by attackers or MEV bots.

RiskFund.sol#L265

function _swapAsset(
    VToken vToken,
    address comptroller,
    uint256 amountOutMin,
    address[] calldata path
) internal returns (uint256) 
            ...
            ...
            if (underlyingAsset != convertibleBaseAsset) {
                require(path[0] == underlyingAsset, "RiskFund: swap path must start with the underlying asset");
                require(
                    path[path.length - 1] == convertibleBaseAsset,
                    "RiskFund: finally path must be convertible base asset"
                );
                IERC20Upgradeable(underlyingAsset).safeApprove(pancakeSwapRouter, 0);
                IERC20Upgradeable(underlyingAsset).safeApprove(pancakeSwapRouter, balanceOfUnderlyingAsset);
                uint256[] memory amounts = IPancakeswapV2Router(pancakeSwapRouter).swapExactTokensForTokens(
                    balanceOfUnderlyingAsset,
                    amountOutMin,
                    path,
                    address(this),
                    /// @audit does not allow deadline to be passed in by user
                    block.timestamp
                );
                ...
                ...

Consider the following scenario:

1. Alice wants to swap 30 vBNB token for 1 BNB and later sell the 1 BNB for 300 DAI. She signs the transaction calling RiskFund.swapPoolsAsset() with inputAmount = 30 vBNB and amountOutmin = 0.99 BNB to allow for 1% slippage. <br/>
2. The transaction is submitted to the mempool, however, Alice chose a transaction fee that is too low for validators to be interested in including her transaction in a block. The transaction stays pending in the mempool for extended periods, which could be hours, days, weeks, or even longer. <br/>
3. When the average gas fee dropped far enough for Alice's transaction to become interesting again for miners to include it, her swap will be executed. In the meantime, the price of BNB could have drastically decreased. She will still at least get 0.99 BNB due to amountOutmin, but the DAI value of that output might be significantly lower. She has unknowingly performed a bad trade due to the pending transaction she forgot about.

An even worse way this issue can be maliciously exploited is through MEV:

1. The swap transaction is still pending in the mempool. Average fees are still too high for validators to be interested in it. The price of BNB has gone up significantly since the transaction was signed, meaning Alice would receive a lot more ETH when the swap is executed. But that also means that her minOutput value is outdated and would allow for significant slippage. <br/>
2. A MEV bot detects the pending transaction. Since the outdated minOut now allows for high slippage, the bot sandwiches Alice, resulting in significant profit for the bot and significant loss for Alice.

Tools Used

Manual Analysis

Recommendation

Allow users to supply their own deadline parameter within RiskFund.swapPoolsAsset

Assessed type

Other

Issues Template

Low Risk Template

Non-Critical Template

Refactor Issues Template

[L-01] Possible approve race conditions in Vtoken.approve()

VToken.sol#L133-L140

function approve(address spender, uint256 amount) external override returns (bool) {
    require(spender != address(0), "invalid spender address");

    address src = msg.sender;
    transferAllowances[src][spender] = amount;
    emit Approval(src, spender, amount);
    return true;
}

Due to the implementation of the approve() function in VToken.sol, it is possible for a user to over spend their allowance in certain situations.

Consider the following scenario

1. Alice approves an allowance of 1000 vUSDC to Bob.
2. Alice attempts to lower the allowance to 500 vUSDC.
3. Bob notices the transaction in the mempool and front-runs it by using up the full allowance with a transferFrom call.
4. Alice’s lowered allowance is confirmed and Bob now has an allowance of 500 VETH, which can be spent further for a total of 1500 vUSDC. Overall, Bob was supposed to be approved for at most 1000 vUSDC but got 1500 vUSDC.

Recommendation: Do not need to expose approve function as there is already decreaseAllowance and increaseAllowance functions that could be used which decreases and increases allowances for a recipient respectively. In this way, if the decreaseAllowance call is front-run, the call should revert as there is insufficient allowance to be decreased.

[L-02] Front runnable initialize functions

Due to a lack of access control for initializers, attackers could front-run intialization of core protocol contracts and force protocol to redeploy core contracts.

Recommendation: Implement valid access control on core contracts to ensure only the relevant deployer can initialize().

[L-03] User's accrued reward can be lost in RewardsDistributor.sol

RewardsDistributor.sol#L181-L186

function grantRewardToken(address recipient, uint256 amount) external onlyOwner {
    uint256 amountLeft = _grantRewardToken(recipient, amount);
    require(amountLeft == 0, "insufficient rewardToken for grant");
    emit RewardTokenGranted(recipient, amount);
}

When claiming reward, if the smart contract has insufficient reward token balance to distribute the reward, the reward is not sent to user claiming, which can cause users calling claimRewards() to not receive their deserved reward tokens.

Recommendation: Although it is user responsibility to trust owner for having sufficient reward token balance in contract, it could be better to allow partial claim of reward tokens so that users can at least receive some funds first.

[L-04] Anyone can claim rewards for other users

RewardsDistributor.sol#L277-L292

function claimRewardToken(address holder, VToken[] memory vTokens) public {
    uint256 vTokensCount = vTokens.length;

    _ensureMaxLoops(vTokensCount);

    for (uint256 i; i < vTokensCount; ++i) {
        VToken vToken = vTokens[i];
        require(comptroller.isMarketListed(vToken), "market must be listed");
        Exp memory borrowIndex = Exp({ mantissa: vToken.borrowIndex() });
        _updateRewardTokenBorrowIndex(address(vToken), borrowIndex);
        _distributeBorrowerRewardToken(address(vToken), holder, borrowIndex);
        _updateRewardTokenSupplyIndex(address(vToken));
        _distributeSupplierRewardToken(address(vToken), holder);
    }
    rewardTokenAccrued[holder] = _grantRewardToken(holder, rewardTokenAccrued[holder]);
}

Attacker cannot control WHERE the rewards are sent, but can control WHEN rewards are claimed for other users by calling unpermissioned claimRewards() function to claim rewards for other users which can have negative implications such as tax

Recommendation: Only allow holder of specific markets to claim rewards accrued

[NC-01] Consider using block.timestamp instead of block.number

Average block time might change in the future and may cause inconsistencies in hard coded 12-second block periods. Hence, consider using block.timestamp instead of block.number for contracts

In addition, if protocol decides to deploy on other L2 such as optimism, contract will be compatible and need not be changed

[NC-02] Missing events emitted in RewardDistributor.claimRewardToken

RewardsDistributor.sol#L277-L292 RewardsDistributor.sol#L81

event RewardTokenGranted(address recipient, uint256 amount);

Consider adding the missing RewardTokenGranted(recipient, amount) event in RewardDistributor.claimRewardToken. Events help non-contract tools to track changes, and events prevent users from being surprised by changes.

[NC-03] No cap on liquidation incentive

Comptroller.sol#L779-L792

function setLiquidationIncentive(uint256 newLiquidationIncentiveMantissa) external {
    require(newLiquidationIncentiveMantissa >= 1e18, "liquidation incentive should be greater than 1e18");

    _checkAccessAllowed("setLiquidationIncentive(uint256)");

    // Save current value for use in log
    uint256 oldLiquidationIncentiveMantissa = liquidationIncentiveMantissa;

    // Set liquidation incentive to new incentive
    liquidationIncentiveMantissa = newLiquidationIncentiveMantissa;

    // Emit event with old incentive, new incentive
    emit NewLiquidationIncentive(oldLiquidationIncentiveMantissa, newLiquidationIncentiveMantissa);
}

Consider adding a limit for liquidationIncentiveMantissa to ensure that liquidationIncentiveMantissa does not exceed (1e18 * 1e18) to not allow discounts greater than 100%(i.e. no liquidation incentive and could potentially even increase collateral) which can allow protocol owner to unfairly DOS user from liquidating positions which can cause bad debt of user to grow even more.

[NC-04] Add a a minimum max loop to set to prevent owner from accidentally setting max loops to be zero

MaxLoopsLimitHelper.sol#L25-L32

function _setMaxLoopsLimit(uint256 limit) internal {
++  require(limit >= 1, "Comptroller: maxLoopsLimit not at least 1")
    require(limit > maxLoopsLimit, "Comptroller: Invalid maxLoopsLimit");

    uint256 oldMaxLoopsLimit = maxLoopsLimit;
    maxLoopsLimit = limit;

    emit MaxLoopsLimitUpdated(oldMaxLoopsLimit, maxLoopsLimit);
}

Consider adding a minimum maxLoopsLimit to set of 1 so that owner will not accidentally set limit to 0 and cause DOS of many functionalities in the market or force redeployment.

[NC-05] Consider allowing user to reduce maxLoopsLimit

MaxLoopsLimitHelper.sol#L25-L32

function _setMaxLoopsLimit(uint256 limit) internal {
++  require(limit >= 1, "Comptroller: maxLoopsLimit not at least 1")
--  require(limit > maxLoopsLimit, "Comptroller: Invalid maxLoopsLimit");

    uint256 oldMaxLoopsLimit = maxLoopsLimit;
    maxLoopsLimit = limit;

    emit MaxLoopsLimitUpdated(oldMaxLoopsLimit, maxLoopsLimit);
}

In the current implementation of _setMaxLoopsLimit, user can never decrease maxLoopsLimit as there is a check to ensure limit to change must be greater than current maxLoopsLimit. maxLoopsLimit serve as a limit to prevent DOS of many market functionalities. Consider removing this check to allow users to decrease maxLoopsLimit and set their own gas restrictions.

[NC-06] Confusing supply and borrow cap definitions

Comptroller.sol#L826-L835

/**
    * @notice Set the given borrow caps for the given vToken markets. Borrowing that brings total borrows to or above borrow cap will revert.
    * @dev This function is restricted by the AccessControlManager
    * @dev A borrow cap of -1 corresponds to unlimited borrowing.
    * @dev Borrow caps smaller than the current total borrows are accepted. This way, new borrows will not be allowed
        until the total borrows amount goes below the new borrow cap
    * @param vTokens The addresses of the markets (tokens) to change the borrow caps for
    * @param newBorrowCaps The new borrow cap values in underlying to be set. A value of -1 corresponds to unlimited borrowing.
    * @custom:access Controlled by AccessControlManager
    */
function setMarketBorrowCaps(VToken[] calldata vTokens, uint256[] calldata newBorrowCaps) external {
    _checkAccessAllowed("setMarketBorrowCaps(address[],uint256[])");

    uint256 numMarkets = vTokens.length;
    uint256 numBorrowCaps = newBorrowCaps.length;

    require(numMarkets != 0 && numMarkets == numBorrowCaps, "invalid input");

    _ensureMaxLoops(numMarkets);

    for (uint256 i; i < numMarkets; ++i) {
        borrowCaps[address(vTokens[i])] = newBorrowCaps[i];
        emit NewBorrowCap(vTokens[i], newBorrowCaps[i]);
    }
}

Comptroller.sol#L852-L861

/**
    * @notice Set the given supply caps for the given vToken markets. Supply that brings total Supply to or above supply cap will revert.
    * @dev This function is restricted by the AccessControlManager
    * @dev A supply cap of -1 corresponds to unlimited supply.
    * @dev Supply caps smaller than the current total supplies are accepted. This way, new supplies will not be allowed
        until the total supplies amount goes below the new supply cap
    * @param vTokens The addresses of the markets (tokens) to change the supply caps for
    * @param newSupplyCaps The new supply cap values in underlying to be set. A value of -1 corresponds to unlimited supply.
    * @custom:access Controlled by AccessControlManager
    */
function setMarketSupplyCaps(VToken[] calldata vTokens, uint256[] calldata newSupplyCaps) external {
    _checkAccessAllowed("setMarketSupplyCaps(address[],uint256[])");
    uint256 vTokensCount = vTokens.length;

    require(vTokensCount != 0, "invalid number of markets");
    require(vTokensCount == newSupplyCaps.length, "invalid number of markets");

    _ensureMaxLoops(vTokensCount);

    for (uint256 i; i < vTokensCount; ++i) {
        supplyCaps[address(vTokens[i])] = newSupplyCaps[i];
        emit NewSupplyCap(vTokens[i], newSupplyCaps[i]);
    }
}

In the comments for Comptroller.setMarketBorrowCaps() and Comptroller.setMarketSupplyCaps(), it is indicated that a value of -1 indicates an unlimited borrowing and supply respective. However, since newBorrowCaps and newSupplyCaps can only be set as an unsigned integer (uint), accesss control manager will never be able to set borrow and supply caps to be -1. The correct definition would be type(uint256).max.

[R-01] sweepToken can directly use onlyOwner modifier instead of a separate require() statement

VToken.sol#L525

function sweepToken(IERC20Upgradeable token) external override {
    require(msg.sender == owner(), "VToken::sweepToken: only admin can sweep tokens");
    require(address(token) != underlying, "VToken::sweepToken: can not sweep underlying token");
    uint256 balance = token.balanceOf(address(this));
    token.safeTransfer(owner(), balance);

    emit SweepToken(address(token));
}

to

function sweepToken(IERC20Upgradeable token) external override onlyOwner {
    require(address(token) != underlying, "VToken::sweepToken: can not sweep underlying token");
    uint256 balance = token.balanceOf(address(this));
    token.safeTransfer(owner(), balance);

    emit SweepToken(address(token));
}

[R-02] Consider using delete instead of assigning default address or boolean variable

Shortfall.sol#L423

auction.highestBidder = address(0);

Using delete is the same as assigning default address variable and could potentially save some gas

[R-03] Use custom error instead of revert strings

VToken.sol#L837-L839

if (redeemTokens == 0 && redeemAmount > 0) {
    revert("redeemTokens zero");
}

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

[R-04] Consider using capital variables for constant variables

Throughout the codebase, there are many occasions where lower case variables are used to represent constant variables. Consider using upper case variables to represent

For example,

uint256 internal constant borrowRateMaxMantissa = 0.0005e16;

can be changed to

uint256 internal constant BORROW_RATE_MAX_MANTISSA = 0.0005e16;