code-423n4 / 2024-07-benddao-findings

9 stars 6 forks source link

User can hold several collateral assets to decrease liquidation penalties #21

Open c4-bot-6 opened 3 months ago

c4-bot-6 commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-07-benddao/blob/main/src/libraries/logic/LiquidationLogic.sol#L99 https://github.com/code-423n4/2024-07-benddao/blob/main/src/libraries/logic/LiquidationLogic.sol#L416-L417

Vulnerability details

Impact

Users can abuse holding several collaterals to reduce liquidation penalties. If they hold 1 collateral they would be liquidated from 50 to 100%. However, by holding several collateral assets they can reduce this amount significantly as their position would likely be healthy after liquidating a single collateral. This happens because the collateral is sold to the liquidator at a discount, so the less the borrower is liquidated, the better for them.

Proof of Concept

LiquidateLogic::executeCrossLiquidateERC20() calculates the actual collateral to liquidated capped to the available collateral balance of the user in the collateral picked by the liquidator. In LiquidationLogic::_calculateAvailableERC20CollateralToLiquidate(), the collateral is capped to userCollateralBalance, the collateral balance of the user in the picked asset by the liquidator.

Thus, by abusing having multiple collaterals the user can reduce how much it is liquidated, as the position becomes healthy before liquidating all collaterals.

For example, suppose the same collateral/debt asset price and threshold to liquidate is 0.9, the user has 1000 collateral and 901 debt. The health factor is 1000*0.9/901 == 0.99889012208, below 1, which means it may be liquidated.

If the user has 4 collateral assets with 250 value each, the liquidator may pick one of them to liquidate. Assuming the close factor would be 50%, the debt to repay should be 901*0.5 == 450.5.

However, this would lead to a collateral value bigger than any collateral amount he user has, so the max collateral to liquidate is capped at 250.

Assuming a liquidation bonus of 10%, the debt to repay based on this collateral would be 250/1.1 == 227.272727273.

Thus, the resulting health factor would be (1000 - 250) * 0.9 / (901 - 227.272727273) == 1.00188908379, so the position becomes healthy and only 25% of the total collateral was liquidated.

If we do the same math assuming 1 collateral, the debt to cover is 450.5 and the collateral 450.5 * 1.1 == 495.55. The resulting health factor is (1000 - 495.55) * 0.9 / (901 - 450.5) == 1.00778024417, bigger than the with multiple assets, but the liquidatee lost more collateral at a discount to the liquidator.

Tools Used

Vscode

Foundry

Recommended Mitigation Steps

The calculated debt to repay on liquidation should be enforced regardless of the number of collateral assets. If the liquidation calculates that 100% of the debt should be repaid, then it should go through all collateral assets and liquidate them until the calculated target repaid debt is reached. It should get the debt to liquidate in LiquidationLogic::_calculateUserERC20Debt() and then in_calculateAvailableERC20CollateralToLiquidate()` cap the debt to repay to the total collateral balance of the borrower, not just of the selected collateral asset. This way users can not abuse the mechanism of having several collateral assets to reduce liquidation penalties.

Assessed type

Other

MarioPoneder commented 3 months ago

Seems intended and desirable for the user, falling below Questioning the protocol’s business model is unreasonable.
Invalidating for now. Further input from @thorseldon and Warden is welcome.

c4-judge commented 3 months ago

MarioPoneder marked the issue as unsatisfactory: Invalid

thorseldon commented 3 months ago

Let's user can holding multiple collateral assets is our cross margin service design goal. The health factor calculation are based on the whole account in cross margin mode which help user reduce the liquidation risk.

Simao123133 commented 3 months ago

@MarioPoneder @thorseldon Thank you for your replies. I am not questioning the fact that a user can hold several collaterals and they affect the health factor. But a user benefits from holding several collateral because he will be less liquidated, at the expense of other users and liquidators (see the example for details, if holding 1 collateral, 50% is liquidated, if holding several, only 25% is liquidated). Usually protocols allow liquidating more than 1 collateral at a time to tackle this.

MarioPoneder commented 3 months ago

Thanks for all the comments, after reviewing the report again I can see the following situations for unhealthy positions:

It seems intended by design and desirable that a user's position must not be further liquidated if already health again after liquidating 1 of multiple collateral assets.
I rather see a problem the other way round that users with just 1 collateral asset can be fully liquidated.

Anyways, pointing out this discrepancy in the present liquidation design is a valid QA suggestion.

c4-judge commented 3 months ago

MarioPoneder changed the severity to QA (Quality Assurance)

c4-judge commented 3 months ago

MarioPoneder marked the issue as grade-b

Simao123133 commented 3 months ago

@MarioPoneder thank you for your rereview. I am not saying the user should be liquidated more once it is healthy, but the final health should be similar regardless of holding 1 collateral asset or several. Users should be liquidated the same USD value regardless of holding 1 or several assets. If a user holds 10 assets, and the total collateral value is 10% of each asset, there would be cases where it would only get liquidated on 10% of the collateral, while if holding a single collateral asset, the user would get liquidated 50%. This also matters to make liquidations profitable for liquidators, or a much higher number of liquidation calls are required, where it may not become profitable due to gas costs.

My point is in agreement with your statement, it's extremely beneficial to hold several assets.

thorseldon commented 3 months ago

@Simao123133 @MarioPoneder In our sevice model, we support multiple pools at the same. Some pools can have mutiple collateras and other pools have only one collaterals. Using which type pool is a dynamic operations according the market situations.

E.g. Pool A: WETH, USDT, USDC, CryptoPunks, all those asset can be collaterals; Pool B: WETH (LTV is 60 & Disable Borrow), USDT (LTV is 0 & Enable Borrow), only 1 collatera is WETH;