code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

TotalBorrowed in VToken is not accounted properly when healAccount() is called.

Proof of Concept

Comptroller#healAccount() intends to forgive the debt of the account if the debt goes far too underwater. When that happens, the collateral is seized, some of the debt is paid off and the rest of the debt is left to become bad debt in the protocol.

VToken.sol/Line 377

     * @notice Repays a certain amount of debt, treats the rest of the borrow as bad debt, essentially
     *   "forgiving" the borrower. Healing is a situation that should rarely happen. However, some pools

Here is the function of healBorrow() in VToken, where all the accounting happens.

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

        uint256 accountBorrowsPrev = _borrowBalanceStored(borrower);
        uint256 totalBorrowsNew = totalBorrows;

        uint256 actualRepayAmount;
        if (repayAmount != 0) {
            // _doTransferIn reverts if anything goes wrong, since we can't be sure if side effects occurred.
            // We violate checks-effects-interactions here to account for tokens that take transfer fees
            actualRepayAmount = _doTransferIn(payer, repayAmount);
            totalBorrowsNew = totalBorrowsNew - actualRepayAmount;
            emit RepayBorrow(payer, borrower, actualRepayAmount, 0, totalBorrowsNew);
        }

        // The transaction will fail if trying to repay too much
        uint256 badDebtDelta = accountBorrowsPrev - actualRepayAmount;
        if (badDebtDelta != 0) {
            uint256 badDebtOld = badDebt;
            uint256 badDebtNew = badDebtOld + badDebtDelta;
            totalBorrowsNew = totalBorrowsNew - badDebtDelta;
            badDebt = badDebtNew;

            // We treat healing as "repayment", where vToken is the payer
            emit RepayBorrow(address(this), borrower, badDebtDelta, accountBorrowsPrev - badDebtDelta, totalBorrowsNew);
            emit BadDebtIncreased(borrower, badDebtDelta, badDebtOld, badDebtNew);
        }

        accountBorrows[borrower].principal = 0;
        accountBorrows[borrower].interestIndex = borrowIndex;
//@--audit issue here with totalBorrows calculation 
        totalBorrows = totalBorrowsNew;

When an account is healed, the borrowedAmount from that account should be taken away because it is translated into bad debt. However, the borrowedAmount is not reduced appropriately.

Let's use an example to illustrate the problem clearly.

Let's say user deposits 1000 ABC tokens worth 1000 USD with 60% LTV and 65% liquidation threshold. User borrows a maximum amount of 600 USD worth. A short while later, ABC token crashes by half instantly, and 1000 ABC tokens is now worth 500. The position is now instantly liquitable, but its too far underwater to break even. (500 < 600). Protocol decides to heal the user's account. The 500 worth of collateral of ABC token is seized, and x amount is paid to cover the debt (let's say 450 is paid, but actual calculation is collateral/(borrows*liquidationIncentive)). 50 is left to be pocketed, and now the protocol has 600 - 450 = 150 as bad debt. The user's account is remade whole and its borrow position should be 0.

In the function healBorrow(), the borrow position is not reset. accountBorrowsPrev is the borrow amount of the borrower. totalBorrowsNew is the totalBorrows in the protocol. When account is made whole, the totalBorrows should be subtracted by the accountBorrowPrev. Instead, badDebtDelta is calculated by subtracting accountBorrowsPrev with actualRepayAmount (600 - 450 = 150 in the example above). Then, totalBorrowsNew is calculated by subtracting itself from badDebtDelta. (Let's say totalBorrow in the protocol is 2000, with one person borrowing 1400 and the user borrowing 600) (2000 - 150 = 1850, but it should be 2000 - 600 = 1400)

        uint256 accountBorrowsPrev = _borrowBalanceStored(borrower);
        uint256 totalBorrowsNew = totalBorrows;

        uint256 badDebtDelta = accountBorrowsPrev - actualRepayAmount;
        totalBorrowsNew = totalBorrowsNew - badDebtDelta;
        totalBorrows = totalBorrowsNew;

The totalBorrowed should completely subtract the user's borrow because the user's borrow has been translated into bad debt already. When _borrowBalanceStored() is called for the user after healAccount(), since borrowSnapshot.principal is set to 0, _borrowBalanceStored() will return 0 but the totalBorrow balance still accounts for the user's previous borrow that has already been forgiven.

    _borrowBalanceStored(address account) internal view returns (uint256) {
        /* Get borrowBalance and borrowIndex */
        BorrowSnapshot memory borrowSnapshot = accountBorrows[account];

        /* If borrowBalance = 0 then borrowIndex is likely also 0.
         * Rather than failing the calculation with a division by 0, we immediately return 0 in this case.
         */
        if (borrowSnapshot.principal == 0) {
            return 0;
        }