Venus Protocol Isolated Pools - rvierdiiev's results

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L1296-L1361

Vulnerability details

Impact

Oracle price is not updated for each asset inside _getHypotheticalLiquiditySnapshot. This can cause wrong calculations and can allow/disallow user to make some actions when he should/not be able to do them.

Proof of Concept

Venus protocol has their oracle system, which provides assets price for them. This price is fetched using _safeGetUnderlyingPrice function. This function queries oracle for the price. It's noted in the oracle repository, that oracle.getUnderlyingPrice function should only be called after oracle.updatePrice in order to get fresh price.

So the pattern of Venus team is to update oracle price, before any interactions with oracle. Let's check how it's done in the preBorrowHook function. The price for the vToken that user wants to borrow is updated.

Then later, _getHypotheticalLiquiditySnapshot function is called, which should calculate if this user has shortfall or no.

_getHypotheticalLiquiditySnapshot function is going to loop through all assets of user in order to calculate how much collateral he has in that market and how much debt he has. For each asset, the price is fetched using _safeGetUnderlyingPrice function. As i have already mentioned in the beginning, updatePrice should be called before getting the price.

The problem is that we have updated price for only 1 vToken that user is going to borrow, but we haven't updated it for another assets that are used by user. Because the price is not updated, calculations can be outdated, which will break all protection calculations of protocol.

Tools Used

VsCode

Recommended Mitigation Steps

You need to update oracle price for each asset that user has.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L187-L235

Vulnerability details

Impact

Oracle price is not updated before exiting from market. This allows user to exit market, when he has not enough collateral to cover his debts.

Proof of Concept

Venus protocol has their oracle system, which provides assets price for them. This price is fetched using _safeGetUnderlyingPrice function. This function queries oracle for the price. It's noted in the oracle repository, that oracle.getUnderlyingPrice function should only be called after oracle.updatePrice in order to get fresh price.

So the pattern of Venus team is to update oracle price, before any interactions with oracle. But this is not done inside Comptroller.exitMarket function. This function allows user to exit market and it calls _checkRedeemAllowed function which will be using _safeGetUnderlyingPrice function to calculate collateral and debt.

Because the price is not updated, calculations can be outdated, which will help user to exit market on better conditions.

Tools Used

VsCode

Recommended Mitigation Steps

You need to update oracle price for vtoken.underlying inside exitMarket function.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

VToken.transfer doesn't accrue interests, so user's debt amount is not up to date when is checked with Comptroller.preTransferHook. As result user can leave his account in debt state.

Proof of Concept

When someone wants transfer his vToken, he can use transfer and transferFrom methods. Both of them just call _transferTokens function. You can check that this function doesn't accrue interests. It calls comptroller.preTransferHook function the purpose of which is to check if user has enough collateral through all his markets to be able to transfer those funds. Comptroller also doesn't accrue interests on vToken.

The problem here is that _transferTokens function doesn't call accrueInterest function, that should update borrow balance of transfer initiator. Because of that it's possible that user has accrued additional borrow amount between last interest accruing, which will not be taken into consideration when comptroller will check allowance to transfer funds. As result user can transfer tokens and leave his position in bad state.

Tools Used

VsCode

Recommended Mitigation Steps

Make _transferTokens function call accrueInterest to make borrow amount up to date.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L1296-L1361

Vulnerability details

Impact

Interest are not accrued for each asset inside _getHypotheticalLiquiditySnapshot. This can cause wrong calculations of borrowed amount and can allow user to redeem and leave his account in bad state.

Proof of Concept

_getHypotheticalLiquiditySnapshot function is called to calculate if this user has shortfall or no when he will do specific action like redeem or borrow. https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L1296-L1361

    function _getHypotheticalLiquiditySnapshot(
        address account,
        VToken vTokenModify,
        uint256 redeemTokens,
        uint256 borrowAmount,
        function(VToken) internal view returns (Exp memory) weight
    ) internal view returns (AccountLiquiditySnapshot memory snapshot) {
        // For each asset the account is in
        VToken[] memory assets = accountAssets[account];
        uint256 assetsCount = assets.length;

        for (uint256 i; i < assetsCount; ++i) {
            VToken asset = assets[i];

            // Read the balances and exchange rate from the vToken
            (uint256 vTokenBalance, uint256 borrowBalance, uint256 exchangeRateMantissa) = _safeGetAccountSnapshot(
                asset,
                account
            );

            // Get the normalized price of the asset
            Exp memory oraclePrice = Exp({ mantissa: _safeGetUnderlyingPrice(asset) });

            // Pre-compute conversion factors from vTokens -> usd
            Exp memory vTokenPrice = mul_(Exp({ mantissa: exchangeRateMantissa }), oraclePrice);
            Exp memory weightedVTokenPrice = mul_(weight(asset), vTokenPrice);

            // weightedCollateral += weightedVTokenPrice * vTokenBalance
            snapshot.weightedCollateral = mul_ScalarTruncateAddUInt(
                weightedVTokenPrice,
                vTokenBalance,
                snapshot.weightedCollateral
            );

            // totalCollateral += vTokenPrice * vTokenBalance
            snapshot.totalCollateral = mul_ScalarTruncateAddUInt(vTokenPrice, vTokenBalance, snapshot.totalCollateral);

            // borrows += oraclePrice * borrowBalance
            snapshot.borrows = mul_ScalarTruncateAddUInt(oraclePrice, borrowBalance, snapshot.borrows);

            // Calculate effects of interacting with vTokenModify
            if (asset == vTokenModify) {
                // redeem effect
                // effects += tokensToDenom * redeemTokens
                snapshot.effects = mul_ScalarTruncateAddUInt(weightedVTokenPrice, redeemTokens, snapshot.effects);

                // borrow effect
                // effects += oraclePrice * borrowAmount
                snapshot.effects = mul_ScalarTruncateAddUInt(oraclePrice, borrowAmount, snapshot.effects);
            }
        }

        uint256 borrowPlusEffects = snapshot.borrows + snapshot.effects;
        // These are safe, as the underflow condition is checked first
        unchecked {
            if (snapshot.weightedCollateral > borrowPlusEffects) {
                snapshot.liquidity = snapshot.weightedCollateral - borrowPlusEffects;
                snapshot.shortfall = 0;
            } else {
                snapshot.liquidity = 0;
                snapshot.shortfall = borrowPlusEffects - snapshot.weightedCollateral;
            }
        }

        return snapshot;
    }

This function is going to loop through all assets of user in order to calculate how much collateral he has in that market and how much debt he has. For each asset, the balance and borrow amount is fetched using _safeGetAccountSnapshot function. This function just queries vToken.getAccountSnapshot. And vToken returns borrow balance that was stored for user. https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L1439-L1456

        function _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;
        }

        /* Calculate new borrow balance using the interest index:
         *  recentBorrowBalance = borrower.borrowBalance * market.borrowIndex / borrower.borrowIndex
         */
        uint256 principalTimesIndex = borrowSnapshot.principal * borrowIndex;

        return principalTimesIndex / borrowSnapshot.interestIndex;
    }

To calculate balance they use borrowIndex and this index is increasing with time. In order to receive correct borrow amount, accrueInterests function should be called for market, which will update borrowIndex. But protocol doesn't accrue interests for each market of borrower and as result, _getHypotheticalLiquiditySnapshot can allow user to redeem, when he should not be allowed.

Tools Used

VsCode

Recommended Mitigation Steps

You need to accrue interest for each market of user.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L641-L694

Vulnerability details

Impact

Because Comptroller.liquidateAccount doesn't update prices and interests it can allow liquidation that should not be allowed.

Proof of Concept

Comptroller.liquidateAccount function should be used only if snapshot.totalCollateral <= minLiquidatableCollateral. Otherwise usual vToken.liquidateBorrow should be used. In order to calculate snapshot.totalCollateral, function calls _getCurrentLiquiditySnapshot. You can see the comment here: We will accrue interest and update the oracle prices later during the liquidation

It's important to accrue interest and update oracle price before the _getCurrentLiquiditySnapshot call, because this can have impact on snapshot.totalCollateral. For example price of asset has increased and snapshot.totalCollateral has also increased, so snapshot.totalCollateral > minLiquidatableCollateral and Comptroller.liquidateAccount function should not be used for such liquidations.

But because protocol doesn't update interest and oracle price here, such situation is possible.

Tools Used

VsCode

Recommended Mitigation Steps

You need to update interest and oracle prices.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L470-L472

Vulnerability details

Impact

Because of check inside Comptroller.preLiquidateHook, attacker can prevent liquidator from liquidation of account in order to wait better conditions for himself.

Proof of Concept

When someone wants to liquidate account he can call VToken.liquidateBorrow function. Liquidator should provide repayAmount, which is the amount that he would like to repay on behalf of liquidated account.

Next _liquidateBorrowFresh function will be called to handle liquidation. It will then call comptroller.preLiquidateHook function to check if liquidation is allowed. https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L424-L473

    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;
        }

        /* The borrower must have shortfall and collateral > threshold in order to be liquidatable */
        AccountLiquiditySnapshot memory snapshot = _getCurrentLiquiditySnapshot(borrower, _getLiquidationThreshold);

        if (snapshot.totalCollateral <= minLiquidatableCollateral) {
            /* The liquidator should use either liquidateAccount or healAccount */
            revert MinimalCollateralViolated(minLiquidatableCollateral, snapshot.totalCollateral);
        }

        if (snapshot.shortfall == 0) {
            revert InsufficientShortfall();
        }

        /* The liquidator may not repay more than what is allowed by the closeFactor */
        uint256 maxClose = mul_ScalarTruncate(Exp({ mantissa: closeFactorMantissa }), borrowBalance);
        if (repayAmount > maxClose) {
            revert TooMuchRepay();
        }
    }

Inside comptroller.preLiquidateHook function there is strict check that repayAmount <= maxClose, where maxClose depends on current borrowBalance of account. So in case if liquidator has calculated amount that he can liquidate before and provide value repayAmount == maxClose, then any malicious party can frontrun this call and repay small amount of debt on behalf of liquidated account, to make maxClose value to be smaller than repayAmount, so function will revert.

Attacker can be interested in such thing in case if he expects, that he can gain more value from liquidation by himself if he can wait some more time(for example when collateral asset's price is decreasing).

Tools Used

VsCode

Recommended Mitigation Steps

You can allow users to repay maxClose, when they provide bigger value. But make sure to return that value back to VToken.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

RiskFund.swapPoolsAssets has minAmountToConvert value that is scaled in convertibleBaseAsset tokens decimals. Protocol supposes that this convertibleBaseAsset can never depeg from usd. But in case if this will happen then it will work incorrectly.

Proof of Concept

RiskFund contract accumulates reserves that are sent to it from ProtocolShareReserve contract. Using swapPoolsAssets function, these reserves can be swapped through PancakeSwap into the convertibleBaseAsset. The swap is done one by one for each market.

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

        function _swapAsset(
        VToken vToken,
        address comptroller,
        uint256 amountOutMin,
        address[] calldata path
    ) internal returns (uint256) {
        require(amountOutMin != 0, "RiskFund: amountOutMin must be greater than 0 to swap vToken");
        require(amountOutMin >= minAmountToConvert, "RiskFund: amountOutMin should be greater than minAmountToConvert");
        uint256 totalAmount;

        address underlyingAsset = VTokenInterface(address(vToken)).underlying();
        uint256 balanceOfUnderlyingAsset = poolsAssetsReserves[comptroller][underlyingAsset];

        ComptrollerViewInterface(comptroller).oracle().updatePrice(address(vToken));

        uint256 underlyingAssetPrice = ComptrollerViewInterface(comptroller).oracle().getUnderlyingPrice(
            address(vToken)
        );

        if (balanceOfUnderlyingAsset > 0) {
            Exp memory oraclePrice = Exp({ mantissa: underlyingAssetPrice });
            uint256 amountInUsd = mul_ScalarTruncate(oraclePrice, balanceOfUnderlyingAsset);

            if (amountInUsd >= minAmountToConvert) {
                assetsReserves[underlyingAsset] -= balanceOfUnderlyingAsset;
                poolsAssetsReserves[comptroller][underlyingAsset] -= balanceOfUnderlyingAsset;

                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),
                        block.timestamp
                    );
                    totalAmount = amounts[path.length - 1];
                } else {
                    totalAmount = balanceOfUnderlyingAsset;
                }
            }
        }

        return totalAmount;
    }

We need to look into this check: if (amountInUsd >= minAmountToConvert). In case if this condition is true then swap will be performed, otherwise, it will not.

amountInUsd is the amount of underlying token that is available for comptroller, converted to usd(not into convertibleBaseAsset token). convertibleBaseAsset token is going to be dollar peged stable coin which can depeg and is not guaranteed to be same as usd. In case if convertibleBaseAsset depegs then it will be incorrect to compare these values. As result swap can be allowed earlier or not allowed when it should be, because of depeg and price changing.

Tools Used

VsCode

Recommended Mitigation Steps

You need to convert underlying amount into convertibleBaseAsset token amount.

Assessed type

Decimal

Lines of code

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

Vulnerability details

Impact

Shortfall contract can work incorrectly in case when convertibleBaseAsset will depeg.

Proof of Concept

When new auction is started for the comptroller, then debt for all its markets is calculated and converted to usd. poolBadDebt variable contains all debt in usd.

Later such code is used to create auction. https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Shortfall/Shortfall.sol#L401C17-L418

        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) {
            auction.startBidBps =
                (MAX_BPS * MAX_BPS * remainingRiskFundBalance) /
                (poolBadDebt * (MAX_BPS + incentiveBps));
            remainingRiskFundBalance = 0;
            auction.auctionType = AuctionType.LARGE_POOL_DEBT;
        } else {
            uint256 maxSeizeableRiskFundBalance = incentivizedRiskFundBalance;

            remainingRiskFundBalance = remainingRiskFundBalance - maxSeizeableRiskFundBalance;
            auction.auctionType = AuctionType.LARGE_RISK_FUND;
            auction.startBidBps = MAX_BPS;
        }

The main point here is how auction will be created: as LARGE_POOL_DEBT or LARGE_RISK_FUND. To determine that incentivizedRiskFundBalance >= riskFundBalance condition is checked. The problem is that incentivizedRiskFundBalance variable is in usd, when riskFundBalance is in convertibleBaseAsset. And it's possible that convertibleBaseAsset will depeg from usd, so this check will not be correct and may create another type of auction.

Tools Used

VsCode

Recommended Mitigation Steps

You need to convert incentivizedRiskFundBalance to convertibleBaseAsset token.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Shortfall contract will work incorrectly when convertibleBaseAsset doesn't have 18 decimal.

Proof of Concept

When new auction is started for the comptroller, then debt for all its markets is calculated and converted to usd. poolBadDebt variable contains all debt in usd. And this variable is scaled in e18, because oracle of Venus protocol returns price in 10 ^ (36 - underlying asset decimals).

Later such code is used to create auction. https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Shortfall/Shortfall.sol#L401C17-L418

        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) {
            auction.startBidBps =
                (MAX_BPS * MAX_BPS * remainingRiskFundBalance) /
                (poolBadDebt * (MAX_BPS + incentiveBps));
            remainingRiskFundBalance = 0;
            auction.auctionType = AuctionType.LARGE_POOL_DEBT;
        } else {
            uint256 maxSeizeableRiskFundBalance = incentivizedRiskFundBalance;

            remainingRiskFundBalance = remainingRiskFundBalance - maxSeizeableRiskFundBalance;
            auction.auctionType = AuctionType.LARGE_RISK_FUND;
            auction.startBidBps = MAX_BPS;
        }

The main point here is how auction will be created: as LARGE_POOL_DEBT or LARGE_RISK_FUND. To determine that incentivizedRiskFundBalance >= riskFundBalance condition is checked. The problem is that incentivizedRiskFundBalance variable is in usd, when riskFundBalance is in convertibleBaseAsset. AThe only way when this check will be valid is when convertibleBaseAsset.decimals == 18. But it's not guaranteed, as sponsor said that any stable token can be used(for example usdt).

So when such convertibleBaseAsset token will be used, where decimals amount is not 18, then contract will work not properly.

Tools Used

VsCode

Recommended Mitigation Steps

You need to convert incentivizedRiskFundBalance to convertibleBaseAsset token or make sure that convertibleBaseAsset has 18 decimals.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

RiskFund.swapPoolsAssets has minAmountToConvert value that is scaled in 18 decimals. In case if convertibleBaseAsset doesn't have 18 decimals as well, then contract may not work properly.

Proof of Concept

RiskFund contract accumulates reserves that are sent to it from ProtocolShareReserve contract. Using swapPoolsAssets function, these reserves can be swapped through PancakeSwap into the convertibleBaseAsset. The swap is done one by one for each market.

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

        function _swapAsset(
        VToken vToken,
        address comptroller,
        uint256 amountOutMin,
        address[] calldata path
    ) internal returns (uint256) {
        require(amountOutMin != 0, "RiskFund: amountOutMin must be greater than 0 to swap vToken");
        require(amountOutMin >= minAmountToConvert, "RiskFund: amountOutMin should be greater than minAmountToConvert");
        uint256 totalAmount;

        address underlyingAsset = VTokenInterface(address(vToken)).underlying();
        uint256 balanceOfUnderlyingAsset = poolsAssetsReserves[comptroller][underlyingAsset];

        ComptrollerViewInterface(comptroller).oracle().updatePrice(address(vToken));

        uint256 underlyingAssetPrice = ComptrollerViewInterface(comptroller).oracle().getUnderlyingPrice(
            address(vToken)
        );

        if (balanceOfUnderlyingAsset > 0) {
            Exp memory oraclePrice = Exp({ mantissa: underlyingAssetPrice });
            uint256 amountInUsd = mul_ScalarTruncate(oraclePrice, balanceOfUnderlyingAsset);

            if (amountInUsd >= minAmountToConvert) {
                assetsReserves[underlyingAsset] -= balanceOfUnderlyingAsset;
                poolsAssetsReserves[comptroller][underlyingAsset] -= balanceOfUnderlyingAsset;

                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),
                        block.timestamp
                    );
                    totalAmount = amounts[path.length - 1];
                } else {
                    totalAmount = balanceOfUnderlyingAsset;
                }
            }
        }

        return totalAmount;
    }

minAmountToConvert is provided in e18 token scaling, because it should check that minimum amount of underlying is present in the reserves and venus oracle system provides prices in 10 ^ (36 - underlying asset decimals) scaling. convertibleBaseAsset token is going to be dollar peged stable coin. It can have different amount of decimals.

We need to look into this check: amountOutMin >= minAmountToConvert. In case if convertibleBaseAsset doesn't have 18 decimals, then this check will not work properly, because amountOutMin is provided in convertibleBaseAsset token scaling. So for example when minAmountToConvert is 100*10^18 and amountOutMin == 100*10^6(usdt) this check will fail, when it should work.

Tools Used

VsCode

Recommended Mitigation Steps

You need to convert amountOutMin into 18 decimals scaling. Or you can just skip this check, as you checking underlying amount later.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L578-L626

Vulnerability details

Impact

Comptroller.healAccount doesn't distribute rewards for healed borrower. As result healed account receives less rewards.

Proof of Concept

Comptroller.healAccount can be called by anyone in order to fully close account. Healer should repay part of account's debt in order to receive all account's collateral. At the end account debt will be cleared.

This is the part when collateral is seized and debt is cleared. https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L611-L625

        for (uint256 i; i < userAssetsCount; ++i) {
            VToken market = userAssets[i];

            (uint256 tokens, uint256 borrowBalance, ) = _safeGetAccountSnapshot(market, user);
            uint256 repaymentAmount = mul_ScalarTruncate(percentage, borrowBalance);

            // Seize the entire collateral
            if (tokens != 0) {
                market.seize(liquidator, user, tokens);
            }
            // Repay a certain percentage of the borrow, forgive the rest
            if (borrowBalance != 0) {
                market.healBorrow(liquidator, user, repaymentAmount);
            }
        }

In order to seize collateral, market.seize is called, which will then call comptroller.preSeizeHook. And this hook will distribute supply rewards to both accounts.

In order to clear healed account debt market.healBorrow is called. This function will not call any comptroller function. As result, debt of healed account is set to 0, but rewards that were earned by account before healing were not distributed to him. So user lost rewards for that debt amount.

Tools Used

VsCode

Recommended Mitigation Steps

Comptroller should distribute rewards to this account that were earned before and only then set debt to 0.

Assessed type

Other