Venus Protocol Isolated Pools - Co0nan's results

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L1463-L1480

Vulnerability details

Description

The vToken is minted when any user deposits some units of underlying tokens. The amount of vToken minted to a user is calculated based upon the amount of underlying tokens user is depositing.

As per the implementation of vToken contract, there exist two cases for vToken amount calculation:

1 - First deposit - when vToken.totalSupply() is 0. 2 - All subsequent deposits.

Here is the actual vToken code:

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L743-L791

...
...
...

Exp memory exchangeRate = Exp({ mantissa: _exchangeRateStored() });

uint256 actualMintAmount = _doTransferIn(payer, mintAmount);


uint256 mintTokens = div_(actualMintAmount, exchangeRate);
...
...
...

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L1463-L1480

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();
            uint256 cashPlusBorrowsMinusReserves = totalCash + totalBorrows + badDebt - totalReserves;
            uint256 exchangeRate = (cashPlusBorrowsMinusReserves * expScale) / _totalSupply;

            return exchangeRate;
        }
    }
}

Proof of Concept

The above implementation contains a critical bug which can be exploited to steal funds of initial depositors of a freshly deployed vToken contract.

As the exchange rate is dependent upon the ratio of vToken's totalSupply and underlying token balance of vToken contract, the attacker can craft transactions to manipulate the exchange rate.

• Steps to attack:

1. Once the vToken has been deployed and added to the lending protocol, the attacker mints the smallest possible amount of vTokens.

2. Then the attacker does a plain underlying token transfer to the vToken contract, artificially inflating the underlying.balanceOf(vToken) value.

3. Due to the above steps, during the next legitimate user deposit, the mintTokens value for the user will become less than 1 and essentially be rounded down to 0 by Solidity. Hence the user gets 0 vTokens against his deposit and the vToken's entire supply is held by the Attacker.

4. The Attacker can then simply redeem his vToken balance for the entire underlying token balance of the vToken contract.

It should be noted that the attack can happen in two ways:

1. The attacker can simply execute the Step 1 and 2 as soon as the vToken gets added to the lending protocol.
2. The attacker watches the pending transactions of the network and front-runs the user's deposit transaction by executing Step 1 and 2 and then back-runs it with Step 3.

Impact

Since this attack is a replicable attack it can be performed continuously to steal the deposits of all depositors that try to deposit into the vToken contract.

Tools Used

Manual

Recommended Mitigation Steps

Enforce a minimum deposit that cannot be withdrawn.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/BaseJumpRateModelV2.sol#L151-L163

Vulnerability details

Impact

Borrowers can reach a state where they can't repay their loan due to higher interest rate set by BaseJumpRateModelV2._updateJumpRateModel()

This issue could also be used in an adversarial fashion by the owner if he/she would disable the repay functionality for some time and enables it at a later point again with the demand of a higher interest to be paid by the borrower.

Proof of Concept

User call repayBorrowFresh() to repay their loan:

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L255-L260

 function repayBorrow(uint256 repayAmount) external override nonReentrant returns (uint256) {
        accrueInterest();
        // _repayBorrowFresh emits repay-borrow-specific logs on errors, so we don't need to
        _repayBorrowFresh(msg.sender, msg.sender, repayAmount);
        return NO_ERROR;
    }

The function accrueInterest(); got invoked:

 function accrueInterest() public virtual override returns (uint256) {
        /* Remember the initial block number */
        uint256 currentBlockNumber = _getBlockNumber();
        uint256 accrualBlockNumberPrior = accrualBlockNumber;

        /* Short-circuit accumulating 0 interest */
        if (accrualBlockNumberPrior == currentBlockNumber) {
            return NO_ERROR;
        }

        /* Read the previous values out of storage */
        uint256 cashPrior = _getCashPrior();
        uint256 borrowsPrior = totalBorrows;
        uint256 reservesPrior = totalReserves;
        uint256 borrowIndexPrior = borrowIndex;

        /* Calculate the current borrow interest rate */
        uint256 borrowRateMantissa = interestRateModel.getBorrowRate(cashPrior, borrowsPrior, reservesPrior);
        require(borrowRateMantissa <= borrowRateMaxMantissa, "borrow rate is absurdly high");

        /* Calculate the number of blocks elapsed since the last accrual */
        uint256 blockDelta = currentBlockNumber - accrualBlockNumberPrior;

        /*
         * Calculate the interest accumulated into borrows and reserves and the new index:
         *  simpleInterestFactor = borrowRate * blockDelta
         *  interestAccumulated = simpleInterestFactor * totalBorrows
         *  totalBorrowsNew = interestAccumulated + totalBorrows
         *  totalReservesNew = interestAccumulated * reserveFactor + totalReserves
         *  borrowIndexNew = simpleInterestFactor * borrowIndex + borrowIndex
         */

        Exp memory simpleInterestFactor = mul_(Exp({ mantissa: borrowRateMantissa }), blockDelta);
        uint256 interestAccumulated = mul_ScalarTruncate(simpleInterestFactor, borrowsPrior);
        uint256 totalBorrowsNew = interestAccumulated + borrowsPrior;
        uint256 totalReservesNew = mul_ScalarTruncateAddUInt(
            Exp({ mantissa: reserveFactorMantissa }),
            interestAccumulated,
            reservesPrior
        );
        uint256 borrowIndexNew = mul_ScalarTruncateAddUInt(simpleInterestFactor, borrowIndexPrior, borrowIndexPrior);

        /////////////////////////
        // EFFECTS & INTERACTIONS
        // (No safe failures beyond this point)

        /* We write the previously calculated values into storage */
        accrualBlockNumber = currentBlockNumber;
        borrowIndex = borrowIndexNew;
        totalBorrows = totalBorrowsNew;
        totalReserves = totalReservesNew;

        /* We emit an AccrueInterest event */
        emit AccrueInterest(cashPrior, interestAccumulated, borrowIndexNew, totalBorrowsNew);

        return NO_ERROR;
    }

On line 695 we see the function fetch the borrowRateMantissa by calling interestRateModel.getBorrowRate.

Then a require statement check if the returnedValue < borrowRateMaxMantissa is so, the function revert.

require(borrowRateMantissa <= borrowRateMaxMantissa, "borrow rate is absurdly high");

Both rate models in the system relay on BaseJumpRateModelV2._getBorrowRate()

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

function _getBorrowRate(
        uint256 cash,
        uint256 borrows,
        uint256 reserves
    ) internal view returns (uint256) {
        uint256 util = utilizationRate(cash, borrows, reserves);

        if (util <= kink) {
            return ((util * multiplierPerBlock) / BASE) + baseRatePerBlock;
        } else {
            uint256 normalRate = ((kink * multiplierPerBlock) / BASE) + baseRatePerBlock;
            uint256 excessUtil;
            unchecked {
                excessUtil = util - kink;
            }
            return ((excessUtil * jumpMultiplierPerBlock) / BASE) + normalRate;
        }
    }
}

The value of baseRatePerBlock determines the final borrowRateMantissa, there is no check or indicator in _updateJumpRateModel() to prevent the owner to set such a high rate that effectively disables repay of borrowed funds:

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

function _updateJumpRateModel(
        uint256 baseRatePerYear,
        uint256 multiplierPerYear,
        uint256 jumpMultiplierPerYear,
        uint256 kink_
    ) internal {
        baseRatePerBlock = baseRatePerYear / blocksPerYear;
        multiplierPerBlock = (multiplierPerYear * BASE) / (blocksPerYear * kink_);
        jumpMultiplierPerBlock = jumpMultiplierPerYear / blocksPerYear;
        kink = kink_;

        emit NewInterestParams(baseRatePerBlock, multiplierPerBlock, jumpMultiplierPerBlock, kink);
    }

Tools Used

Manual

Recommended Mitigation Steps

Disallow setting the interest rate too high with a check in BaseJumpRateModelV2::_updateJumpRateModel().

Assessed type

Context