Lines of code
<https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/LiquidationLogic.sol#L516-L556&gt;
<https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/LiquidationLogic.sol#L860-L881&gt;
<https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/GenericLogic.sol#L188-L195&gt;

Vulnerability details

Impact

Function executeLiquidateERC20() is for liquidating a position if its Health Factor drops below 1. The caller (liquidator) covers liquidationAmount amount of debt of the user getting liquidated, and receives a proportional amount of the collateralAsset plus a bonus to cover market risk.
by this issue, attacker can liquidate his/her account(with another address) and perform reentrancy and transfer his collaterals while attacker’s account has bad health factor and by doing so attacker would steal other suppliers tokens. the problem is that code don’t follow check-effect-interact pattern in executeLiquidateERC20() logics. before transferring debt tokens or collateral asset, code set values for setBorrowing() and setUsingAsCollateral() and then code makes external call which gives attacker this opportunity to perform reentrancy attack when protocol contracts storage is in wrong state(borrowing set as false for borrower in liquidation token while borrower still has debt). this wrong state would increase health factor of the account which would be possible to transfer other collaterals of the user while in reality user has bad health factor. (if you have question about the issue please contact me)

There are other impacts too, attacker can manipulate setBorrowing() and setUsingAsCollateral() for user or his/her account and PTokens and make those values to be in wrong states(for example user has collateral but usingAsCollateral would be false or the opposite)

Proof of Concept

if user had bad health factor then any other user can call liquidation function. during the call to executeLiquidateERC20(), if all user’s debt in that liquidationToken is going to get paid contract sets userConfig.setBorrowing(liquidationAssetReserve.id, false) and then it makes an external call if liquidator sends ETH, contract swap required amount of ETH for wETH and sends extra amount back to liquidator(function _depositETH()) which gives attacker to perform reentrancy attack. as borrowing has been set to false for that liquidationAssetReserve.id so health factor of user would increase, and it would be possible to perform some action without failing because of bad health factor. there are reentrancy protection in pool contract so it won’t be possible to call them but it’s possible transfer other collaterals of the user because contract would calculate less debt for user and health factor would be higher than 1 so transfer of collaterals would be possible and transfer method is in PTokens and Ntokens contracts other than Pool contract and it would be possible to call PToken.transfer() during reentrancy.

This is part of executeLiquidateERC20() code which issue is happing:

    function executeLiquidateERC20(
        mapping(address => DataTypes.ReserveData) storage reservesData,
        mapping(uint256 => address) storage reservesList,
        mapping(address => DataTypes.UserConfigurationMap) storage usersConfig,
        DataTypes.ExecuteLiquidateParams memory params
    ) external returns (uint256) {
        ExecuteLiquidateLocalVars memory vars;

   .......
   .......
   ......

        if (vars.userDebt == vars.actualLiquidationAmount) {
            userConfig.setBorrowing(liquidationAssetReserve.id, false);
        }

        // If the collateral being liquidated is equal to the user balance,
        // we set the currency as not being used as collateral anymore
        if (
            vars.actualCollateralToLiquidate + vars.liquidationProtocolFee ==
            vars.userCollateral
        ) {
            userConfig.setUsingAsCollateral(collateralReserve.id, false);
            emit ReserveUsedAsCollateralDisabled(
                params.collateralAsset,
                params.borrower
            );
        }
   .......
   .......
   ......
        _burnDebtTokens(liquidationAssetReserve, params, vars);

        if (params.receiveXToken) {
            _liquidatePTokens(usersConfig, collateralReserve, params, vars);
        } else {
            _burnCollateralPTokens(collateralReserve, params, vars);
        }
   .......
   .......
   ......
        return vars.actualLiquidationAmount;
    }

As you can see code first set the values for isBorrowing status of user in the liquidationAsset’s reserve and isUsingAsCollateral status of user in the collateralAsset’s reserve. if liquidator is going to pay all the debt of borrower in the liqudationAsset the code would set userConfig.setBorrowing(liquidationAssetReserve.id, false) and if liquidator is going to receive all the collaterals of the borrower the code would set userConfig.setUsingAsCollateral(collateralReserve.id, false). then it calls _burnDebtTokens() which repays borrowers debt by transferring liquidationAsset from liquidator. and then it transfer collateral to liquidator. during _burnDebtTokens() external call happens so code didn’t follows check-effect-interact pattern and when the external call happens the contract state is wrong.
This is _burnDebtTokens() code:

    function _burnDebtTokens(
        DataTypes.ReserveData storage liquidationAssetReserve,
        DataTypes.ExecuteLiquidateParams memory params,
        ExecuteLiquidateLocalVars memory vars
    ) internal {
        _depositETH(params, vars);

        // Transfers the debt asset being repaid to the xToken, where the liquidity is kept
        IERC20(params.liquidationAsset).safeTransferFrom(
            vars.payer,
            vars.liquidationAssetReserveCache.xTokenAddress,
            vars.actualLiquidationAmount
        );
        // Handle payment
        IPToken(vars.liquidationAssetReserveCache.xTokenAddress)
            .handleRepayment(params.liquidator, vars.actualLiquidationAmount);
        // Burn borrower's debt token
        vars
            .liquidationAssetReserveCache
            .nextScaledVariableDebt = IVariableDebtToken(
            vars.liquidationAssetReserveCache.variableDebtTokenAddress
        ).burn(
                params.borrower,
                vars.actualLiquidationAmount,
                vars.liquidationAssetReserveCache.nextVariableBorrowIndex
            );
        // Update borrow & supply rate
        liquidationAssetReserve.updateInterestRates(
            vars.liquidationAssetReserveCache,
            params.liquidationAsset,
            vars.actualLiquidationAmount,
            0
        );
    }

the code first calls _depositETH() to convert msg.value to WETH (if msg.value > 0) and then it would tries to transfer liquidationAsset and burn debt token of borrower.
This is _depositETH() code:

    function _depositETH(
        DataTypes.ExecuteLiquidateParams memory params,
        ExecuteLiquidateLocalVars memory vars
    ) internal {
        if (msg.value == 0) {
            vars.payer = msg.sender;
        } else {
            vars.payer = address(this);
            IWETH(params.weth).deposit{value: vars.actualLiquidationAmount}();
            if (msg.value > vars.actualLiquidationAmount) {
                Address.sendValue(
                    payable(msg.sender),
                    msg.value - vars.actualLiquidationAmount
                );
            }
        }
    }

As you can see when liquidator sends ETH code would try to convert it to wETH and send the extra amount of ETH back to msg.sender and it would make an external call when liquidator tries to pay wETH loan of borrower.
This is part of calculateUserAccountData() code in GenericLogic which Calculates the user data across the reserves.

    function calculateUserAccountData(
        mapping(address => DataTypes.ReserveData) storage reservesData,
        mapping(uint256 => address) storage reservesList,
        DataTypes.CalculateUserAccountDataParams memory params
    )
        internal
        view
    {


        CalculateUserAccountDataVars memory vars;

        while (vars.i < params.reservesCount) {
            if (!params.userConfig.isUsingAsCollateralOrBorrowing(vars.i)) {
                unchecked {
                    ++vars.i;
                }
                continue;
            }
....
....
            if (
                vars.reserveConfiguration.getAssetType() ==
                DataTypes.AssetType.ERC20
            ) {
....
....
                if (params.userConfig.isBorrowing(vars.i)) {
                    vars.totalDebtInBaseCurrency += _getUserDebtInBaseCurrency(
                        params.user,
                        currentReserve,
                        vars.assetPrice,
                        vars.assetUnit
                    );
                }
            } else {
....
....
            }

            unchecked {
                ++vars.i;
            }
        }
....
....
        vars.healthFactor = (vars.totalDebtInBaseCurrency == 0)
            ? type(uint256).max
            : (
                vars.totalCollateralInBaseCurrency.percentMul(
                    vars.avgLiquidationThreshold
                )
            ).wadDiv(vars.totalDebtInBaseCurrency);
....
....
    }

As you can see when a token is not used as borrowing it won’t get calculated in totalDebt and it won’t get considered in health factor even if user has debt in that token.
so to perform this attack, attacker need to have this conditions:

1. attackerAddress1 should have health factor below 1.
2. attackerAddress1 should have take wETH as loan.
3. attacker would liquidate attackerAddress1 with his attackerAddress2 and pays all wETH debt of attackerAddress1 in doin so.
   to perform this attack, attacker can perform this scenario:
4. let’s assume loan to collateral ratio is 0.7 for USDC and USDT and platform support this tokens.
5. attacker would put 110 USDC and 90 USDT as collateral in the platform which USDC worth 110$ and USDT worth 90$ with his attackerAddress1 so attackerAddress1 balance in PTokenUSDC would be 110 and attackerAddress1 balance in PTokenUSDT would be 90.
6. attacker would take wETH loan worth of 70$ and wBTC loan worth of 70$ with hsi attackerAddress1 and attacker health factor would be below 1.
7. attackerAddress1 would give spending address in all of his xTokens(collaterals) for attackerAddress2 so attackerAddress2 can call transfer of attackerAddress1 PTokens.
8. attacker would call executeLiquidateERC20(borrower=attackerAddress1, liquidationAsset=wETH, ETHSent=worth of 77$, collateralAsset=USDC) with his attackerAddress2(which is a contract) to liquidate his other address.
9. code would check and see that attackerAddress2 is going to pay all the wETH debt of attackerAddress1 an so it would set: attackerAddress1Config.setBorrowing(wETHReserve.id, false). this would make health factor of attackerAddress1 to be about 2 (110+100 collateral with 0.7 loan to collateral ratio and 70$ wBTC debt) because in that moment the health factor calculating code would skip wETH and don’t consider wETH debt of attackerAddress1.
10. then code would convert ETH worth of 70$ to wETH and send back ETH worth of 7$ to msg.sender which is attackerAddress2.
11. attackerAddress2 would take the execution and would call PTokenUSDT.transferFrom(attackerAddress1, attackerAddress2, 90). protocol would calculate attackerAddress1 health factor and because wETH won’t be considered as debt the health factor would be ok and attackerAddress2 has spending allowance so protocol would transfer the collateral tokens.(the reentrancy guard won’t work because Pool and PTokenUSDT has different address)
12. attackerAddress2 would return the execution to the executeLiquidateERC20() and the code would transfer ETH worth of 70$ and would send about 70$ worth of USDC collaterals of attackerAddress1 to attackerAddress2.
13. right now attacker would have 70$ worth of USDC(the collateral received when liquidating as reward for debt payment) and 90$ worth of USDT(the collateral received by reentrancy and transferring) and wETH worth of 70$ and wBTC worth of 70$(borroweds) so attacker has tokens worth of 300$ and attacker only deposited tokens worth of 200$ to the protocol as collateral.

this is critical issue because anyone can steal contracts funds whenever an liquidation happens(by liquidating himself). attacker can create a contract which gives this steal-op-liquidation feature to others and users would give spending permission for attacker contract and call it whenever liquidation is possible.

There are other scenarios that attacker can make contracts to have wrong states for example isUsingAsCollateral would be false when user has collateral in that token or the reverse.

Tools Used

VIM

Recommended Mitigation Steps

follow the check-effect-interaction pattern and don’t set isUsingAsCollateral and isBorrowing before transferring tokens.

---

The text was updated successfully, but these errors were encountered:

All reactions