Venus Protocol Isolated Pools - bin2chen's results

Lines of code

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

Vulnerability details

Impact

borrower's vTokens that do not join the market, but it still be Liquidation as collateral

Proof of Concept

If you want to use it as collateral and need to enter the market, you can do so by using the enterMarkets() method with the following code and comments:

    /**
     * @notice Add assets to be included in account liquidity calculation; enabling them to be used as collateral
...     
    function enterMarkets(address[] memory vTokens) external override returns (uint256[] memory) {     

Those who have joined the market can exit with the following code and comments:

    /**
     * @notice Removes asset from sender's account liquidity calculation; disabling them as collateral
    function exitMarket(address vTokenAddress) external override returns (uint256) {     

So if it is not in the market, it should not be used as collateral, and this agreement should also use to Liquidation But the current Liquidation method does not determine whether vTokenCollateral is in the market or not

    function _liquidateBorrowFresh(
        address liquidator,
        address borrower,
        uint256 repayAmount,
        VTokenInterface vTokenCollateral,
        bool skipLiquidityCheck
    ) internal {
        /* Fail if liquidate not allowed */
        comptroller.preLiquidateHook(
            address(this),
            address(vTokenCollateral),
            borrower,
            repayAmount,
            skipLiquidityCheck
        );


        /* Verify market's block number equals current block number */
        if (accrualBlockNumber != _getBlockNumber()) {
            revert LiquidateFreshnessCheck();
        }

        /* Verify vTokenCollateral market's block number equals current block number */
        if (vTokenCollateral.accrualBlockNumber() != _getBlockNumber()) {
            revert LiquidateCollateralFreshnessCheck();
        }

        /* Fail if borrower = liquidator */
        if (borrower == liquidator) {
            revert LiquidateLiquidatorIsBorrower();
        }

        /* Fail if repayAmount = 0 */
        if (repayAmount == 0) {
            revert LiquidateCloseAmountIsZero();
        }

        /* Fail if repayAmount = -1 */
        if (repayAmount == type(uint256).max) {
            revert LiquidateCloseAmountIsUintMax();
        }

        /* Fail if repayBorrow fails */
        uint256 actualRepayAmount = _repayBorrowFresh(liquidator, borrower, repayAmount);

    function preLiquidateHook(
        address vTokenBorrowed,
        address vTokenCollateral,
        address borrower,
        uint256 repayAmount,
        bool skipLiquidityCheck
    ) external override {
        // Pause Action.LIQUIDATE on BORROWED TOKEN to prevent liquidating it.
        // If we want to pause liquidating to vTokenCollateral, we should pause
        // Action.SEIZE on it
        _checkActionPauseState(vTokenBorrowed, Action.LIQUIDATE);

        oracle.updatePrice(vTokenBorrowed);
        oracle.updatePrice(vTokenCollateral);

        if (!markets[vTokenBorrowed].isListed) {
            revert MarketNotListed(address(vTokenBorrowed));
        }
        if (!markets[vTokenCollateral].isListed) {
            revert MarketNotListed(address(vTokenCollateral));
        }

        uint256 borrowBalance = VToken(vTokenBorrowed).borrowBalanceStored(borrower);

        /* Allow accounts to be liquidated if the market is deprecated or it is a forced liquidation */
        if (skipLiquidityCheck || isDeprecated(VToken(vTokenBorrowed))) {
            if (repayAmount > borrowBalance) {
                revert TooMuchRepay();
            }
            return;
        }

As we can see by the above code, it only determines whether vTokenCollateral is a legitimate market, and does not determine whether borrower has added it to the market market.accountMembership[borrower] == true

So there should be a restriction that the liquidator needs to choose one of the collaterals that this borrower has joined the market, not one that has not joined the market

Tools Used

Recommended Mitigation Steps

    function preLiquidateHook(
        address vTokenBorrowed,
        address vTokenCollateral,
        address borrower,
        uint256 repayAmount,
        bool skipLiquidityCheck
    ) external override {
....


+       if (vTokenBorrowed != vTokenCollateral && vTokenCollateral != address(this)) {
+           require(markets[vTokenCollateral].accountMembership[borrower],"not in market");
+        }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/Rewards/RewardsDistributor.sol#L291

Vulnerability details

Impact

If RewardToken is ERC777, the balance can be drained

Proof of Concept

claimRewardToken() does not prevent reentry and does not follow the checks effects interactions rule, resulting in the possibility of reentry to steal the balance

    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);
        }
        //<-------tranfer first then modify rewardTokenAccrued[holder]
        rewardTokenAccrued[holder] = _grantRewardToken(holder, rewardTokenAccrued[holder]);
    }

From the above code we can see that the transfer is executed first, and then the rewardTokenAccrued[holder] is modified.

This leads to if rewardToken is ERC777, the malicious user can reenter, and when reentering, the rewardTokenAccrued[holder] does not change, so the transfer can be repeated.

1.Alice call claimRewardToken() 2.rewardTokenAccrued[alice] = 100, then transfer 100 to alice 3.if alice is erc777 then reenter claimRewardToken() 4.in reenter claimRewardToken() , rewardTokenAccrued[alice] still ==100 ,then do transfer 5.repeat step 3,4 6.when RewardsDistributor's balance ==0 ,return to first call claimRewardToken() 7.set rewardTokenAccrued[holder] = 0

Suggestion: Add reentrant restrictions and modify rewardTokenAccrued[holder] before executing transfer

Tools Used

Recommended Mitigation Steps

Add reentrant restrictions and modify rewardTokenAccrued[holder] before executing transfer

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/VToken.sol#L1476-L1478

Vulnerability details

Impact

VToken() May be Inflation Attack like ERC4626 Inflation Attack

Proof of Concept

Currently vToken still has the common ERC4626 'Inflation Attack' The number of assets can be increased by donating

    function _exchangeRateStored() internal view virtual returns (uint256) {
        uint256 _totalSupply = totalSupply;
        if (_totalSupply == 0) {
            /*
             * If there are no tokens minted:
             *  exchangeRate = initialExchangeRate
             */
            return initialExchangeRateMantissa;
        } else {
            /*
             * Otherwise:
             *  exchangeRate = (totalCash + totalBorrows + badDebt - totalReserves) / totalSupply
             */
            uint256 totalCash = _getCashPrior();  //<---------------asset from _getCashPrior
            uint256 cashPlusBorrowsMinusReserves = totalCash + totalBorrows + badDebt - totalReserves;
            uint256 exchangeRate = (cashPlusBorrowsMinusReserves * expScale) / _totalSupply;

            return exchangeRate;
        }
    }

    function _getCashPrior() internal view virtual returns (uint256) {
        IERC20Upgradeable token = IERC20Upgradeable(underlying);
        return token.balanceOf(address(this)); //<---------------from balaceOf()
    }

So it is still possible to mint first and then redeem, and finally 1 shares left, and then donate by direct transfer assert,Amplify exchangeRate, thus carrying out 'Inflation Attack'

Note: Although the initial mint initialSupply can be executed when creating a VToken, there is no restriction on initialSupply==0

It is recommended to refer to the new 4.9.0 release of OpenZeppelin, which has a special version of ERC4626 'Inflation Attack' for this

https://twitter.com/OpenZeppelin/status/1656066698410328064?s=20

Tools Used

Recommended Mitigation Steps

It is recommended to refer to the new 4.9.0 release of OpenZeppelin, which has a special version of ERC4626 'Inflation Attack' for this

https://twitter.com/OpenZeppelin/status/1656066698410328064?s=20

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

placeBid() Lack of checking if auction are restarted and participating in auction that are not expected by the user may result in the user losing funds

Proof of Concept

When the user makes a bid, simply pass in comptroller and bidBps with the following code:

    function placeBid(address comptroller, uint256 bidBps) external nonReentrant {
        Auction storage auction = auctions[comptroller];

        require(_isStarted(auction), "no on-going auction");
        require(!_isStale(auction), "auction is stale, restart it");
        require(bidBps <= MAX_BPS, "basis points cannot be more than 10000");

Because comptroller corresponds to the Auction there are two cases that will generate new Auction (the new one and the old one may have completely different types and amounts)

1. If the first auction takes too long and no one bids _isStale() you can restart the auction
2. If the auction ends and the last bid time > nextBidderBlockLimit, then you can restart the auction after it ends

Since bidding can be restarted, so placeBid() is only based on comptroller, it may be possible to participate in the old Auction but end up participating in the new Auction, as the old and new Auction may be very different, resulting in the user losing money.

For example, the following scenario

1. Alice learns about the auction in the UI, auctions = {type=LARGE_RISK_FUND,debt=100,seizedRiskFund=100} and submits it to participate in the auction
2. When alice commits, her transaction will exist in memorypool

Note: Because alice needs to stay in the UI interface for some time, or because of the GAS price and block size, alice transaction is delayed, resulting in a much higher possibility of preemptive execution in step 3

3. The auction is restarted due to any of the following situations (before alice's task is executed)

a) auction >waitForFirstBidder leads to _isStale(), bob executes restarted auction, restarted auction debt increases e.g.: auction = {type=LARGE_POOL_DEBT,debt=100000,seizedRiskFund=100}

b) The auction ends > nextBidderBlockLimit, bob restarts the auction, the restarted auction seizedRiskFund becomes small, like 0, the debt is already very high as auction = {type=LARGE_POOL_DEBT,debt=100,seizedRiskFund=0}, there may also be still LARGE_RISK_FUND

4. alice's turn to execute the transaction, this time the new auction and alice expected has been much worse, but the transaction will still be executed. The result is that alice may pay a lot of debt, but get very little seizedRiskFund.

So, we need to add a restriction to placeBid() to ensure that the auction has not been restarted when the transaction is executed.

A simple way to do this is to add the parameter : auctionStartBlock, and compare it to the auction.startBlock of the current transaction, if the two are different, then the auction has been restarted and the transaction revert

Tools Used

Recommended Mitigation Steps

-   function placeBid(address comptroller, uint256 bidBps) external nonReentrant {
+   function placeBid(address comptroller, uint256 bidBps , uint256 auctionStartBlock) external nonReentrant {    
        Auction storage auction = auctions[comptroller];

+       require(auction.startBlock == auctionStartBlock,"auction has been restarted");
        require(_isStarted(auction), "no on-going auction");
        require(!_isStale(auction), "auction is stale, restart it");
        require(bidBps <= MAX_BPS, "basis points cannot be more than 10000");

Assessed type

Context

L-01:setShortfallContract() May cause old shortfall to be in bidding and cannot be closed forever

The current protocol VToken can set a new shortfall with setShortfallContract(). So that if the old shortfall has a bid in progress vToken.badDebtRecovered() on vToken will fail, because there is no permission and shortfall has been changed The auction cannot be closed, closeAuction() will revert The user's money may be lost Suggest adding a mechanism to determine that the old shortfall does not have the vToken in the auction

L-02:preLiquidateHook() Missing trigger rewardsDistributors for borrower index update

Liquidate It is recommended to update the index of borrower even if it is liquidated.

    function preLiquidateHook(
        address vTokenBorrowed,
        address vTokenCollateral,
        address borrower,
        uint256 repayAmount,
        bool skipLiquidityCheck
    ) external override {
....


+        uint256 rewardDistributorsCount = rewardsDistributors.length;
+        for (uint256 i; i < rewardDistributorsCount; ++i) {
+            Exp memory borrowIndex = Exp({ mantissa: VToken(vToken).borrowIndex() });
+            rewardsDistributors[i].updateRewardTokenBorrowIndex(vTokenBorrowed, borrowIndex);
+            rewardsDistributors[i].distributeBorrowerRewardToken(vTokenBorrowed, borrower, borrowIndex);
+        }        
    }    

L-03:transfer() missing accrueInterest() first

When the user performs transfer(), it will check if there is enough collateral. However, the current transfer is not executed first accrueInterest(), so the amount owed by the user may be less than the actual amount owed, resulting in a transfer that would otherwise be impossible.

It is recommended to execute accrueInterest() first when transferring

    function transfer(address dst, uint256 amount) external override nonReentrant returns (bool) {
+       accrueInterest();
        _transferTokens(msg.sender, msg.sender, dst, amount);
        return true;
    }

    function transferFrom(
        address src,
        address dst,
        uint256 amount
    ) external override nonReentrant returns (bool) {
+       accrueInterest();    
        _transferTokens(msg.sender, src, dst, amount);
        return true;
    }

L-04: healBorrow() lacks accrueInterest() first

healBorrow() will process the amount owed by the user first, currently there is no accrueInterest(), the actual operation of the amount owed may become less

    function healBorrow(
        address payer,
        address borrower,
        uint256 repayAmount
    ) external override nonReentrant {
        if (msg.sender != address(comptroller)) {
            revert HealBorrowUnauthorized();
        }
+       accrueInterest();

L-05:sweepToken() The underlying address is invalid for a token with a double address

sweepToken() will restrict the transfer of underlying. But currently it is a comparison address, this restriction will be invalid for some tokens with proxy address, we suggest to check wether the balance of underlying has changed.

https://github.com/d-xo/weird-erc20/#multiple-token-addresses

    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));
+       uint256 underlyingBalance = underlying.balanceOf(address(this)); 
        token.safeTransfer(owner(), balance);
+       require(underlying.balanceOf(address(this)) == underlyingBalance,"can not sweep underlying token");
        emit SweepToken(address(token));
    }

L-06: The redeemTokens calculation for _redeemFresh() is recommended to use round up

According to the ERC4626 standard, for protocol security, the tokens fetched by the asset calculation should be round up when redeeming

    function _redeemFresh(
        address redeemer,
        uint256 redeemTokensIn,
        uint256 redeemAmountIn
    ) internal {
...

            redeemTokens = div_(redeemAmountIn, exchangeRate);
+           if(redeemTokens * exchangeRate !=  redeemAmountIn) redeemTokens++;// round up            
            redeemAmount = redeemAmountIn;
        }    

L-07:updateNextBidderBlockLimit() will break the end mechanism already in the auction

updateNextBidderBlockLimit() will modify the protocol nextBidderBlockLimit. This value is very important for bidding, to indicate whether the current bidder has already bid successfully However, the current protocol does not record the nextBidderBlockLimit for bids that have already started. This leads to the fact that if the old auction has already started, the bidder will think that the calculation time is: nextBidderBlockLimit==100000.

But then the protocol is changed to nextBidderBlockLimit==10 So the old bidder also becomes 10, not at all the 100000 that the bidder saw at that time This corresponds to the user is not fair So it is recommended that each bid add a record of the nextBidderBlockLimit time at the time, the new modification, and will not affect the old

    struct Auction {
        uint256 startBlock;
        AuctionType auctionType;
        AuctionStatus status;
        VToken[] markets;
        uint256 seizedRiskFund;
        address highestBidder;
        uint256 highestBidBps;
        uint256 highestBidBlock;
        uint256 startBidBps;
        mapping(VToken => uint256) marketDebt;
+       uint256 nextBidderBlockLimit;
    }