code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L641-L694 https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L449-L454 https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L946 https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L1026-L1032

Vulnerability details

Impact

In the Comptroller.liquidateAccount() function, the liquidation orders are executed by calling the forceLiquidateBorrow() function and setting the skipLiquidityCheck to true. Hence during the check for the preLiquidateHook(), in the VToken._liquidateBorrowFresh() function, the following check is performed inside the if block.

preLiquidateHook() function has the following if block code snippet :

   if (skipLiquidityCheck || isDeprecated(VToken(vTokenBorrowed))) {
        if (repayAmount > borrowBalance) {
            revert TooMuchRepay();
        }
        return;
    }

Here the transaction enters the if block since the skipLiquidityCheck == true. But the issue is the transaction will revert if the repayAmount > borrowBalance.

Hence a malicious borrower can front run the liquidateAccount transaction by repaying any of his borrowed asset such that repayAmount > borrowBalance will be made true, for that asset. Since forceLiquidateBorrow() is called for each borrowed asset of the borrower by iterating through a for loop, the borrower can revert the liquidateAccount transaction by only repaying a single borrowed asset of his choice.

If the malicious borrower has borrowed many assets, then he can delay the liquidateAccount transaction by repaying any of his borrows at a time, such that repayAmount > borrowBalance will be made true. So he can buy his own time to find more funds and provide more collateral to the protocol to avoid force liquidation permanently.

Proof of Concept

The PoC flow starts in the Comptroller.liquidateAccount() function by calling the forceLiquidateBorrow() function.

            LiquidationOrder calldata order = orders[i];
            order.vTokenBorrowed.forceLiquidateBorrow(
                msg.sender,
                borrower,
                order.repayAmount,
                order.vTokenCollateral,
                true
            );

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

        comptroller.preLiquidateHook(
            address(this),
            address(vTokenCollateral),
            borrower,
            repayAmount,
            skipLiquidityCheck
        );

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

        if (skipLiquidityCheck || isDeprecated(VToken(vTokenBorrowed))) {
            if (repayAmount > borrowBalance) {
                revert TooMuchRepay();
            }
            return;
        }

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

       uint256 repayAmountFinal = repayAmount > accountBorrowsPrev ? accountBorrowsPrev : repayAmount;

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

Tools Used

Manual Review and VS Code

Recommended Mitigation Steps

In the VToken._liquidateBorrowFresh() function there is an internal call to the _repayBorrowFresh() function. This function correctly handles the repayAmount if it is greater than the borrowBalance as shown below.

    uint256 repayAmountFinal = repayAmount > accountBorrowsPrev ? accountBorrowsPrev : repayAmount;

If repayAmount > borrowBalance then repayAmount = borrowBalance is assigned without reverting the transaction.

But this operation is conducted after the preLiquidateHook() is called. Hence if repayAmount > borrowBalance happens then transaction will already be reverted before transaction proceeds to the _repayBorrowFresh() function call.

Hence it is recommended to use the same logic used in the _repayBorrowFresh() mentioned above, in the Comptroller.preLiquidateHook() function as well.

So in the preLiquidateHook() function, if the repayAmount > borrowBalance occurs the repayAmount should be set to borrowBalance and transaction should proceed with out reverting. This will not affect the accounting of the protocol since the actual token transfer happens in the VToken._doTransferIn() function.

Assessed type

Other

0xean commented 1 year ago

closely related to #156 and the fix might be the same. Will leave as seperate but may choose to duplicate these together after sponsor review / comment.

c4-sponsor commented 1 year ago

chechu marked the issue as sponsor confirmed

c4-judge commented 1 year ago

0xean marked the issue as duplicate of #365