code-423n4 / 2023-07-amphora-findings

3 stars 2 forks source link

Borrower can avoid liquidation by frontrunning a liquidation to make it revert the liquidateVault call in VaultController #267

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-amphora/blob/main/core/solidity/contracts/core/VaultController.sol#L691

Vulnerability details

Impact

Borrower can avoid liquidation by frontrunning a liquidation to make it revert the liquidateVault call in VaultController

During a call to liquidateVault, at the end of it there is such check:

    // this mainly prevents reentrancy
    if (_getVaultBorrowingPower(_vault) > _vaultLiability(_id)) revert VaultController_OverLiquidation();

https://github.com/code-423n4/2023-07-amphora/blob/main/core/solidity/contracts/core/VaultController.sol#L691

If a liquidator intends to liquidate the maximal quantity, namely to the point where _getVaultBorrowingPower(_vault) == _vaultLiability(_id), but the vault owner frontruns the liquidator's transaction by repaying just 1wei USDA, or just a smaller amount plus the updatedInterest. Them, the liquidator's intended _tokensToLiquidate would become invalid since it would break the last check.

Consider:

Example

  1. There is a position of $100 worth of Convex LP to be liquidated.
  2. the liquidator sends the _tokensToLiquidate as whatever the maxClose was at that point,
  3. The borrower frontruns by repaying a tiny bit of USDA, eg 1wei.
  4. When the liquidation is executed, the _vaultLiability would be slightly smaller, and the transaction will be reverted even though the position is still be in a liquidatable state, however the "post-liquidation state" becomes too healthy since the liquidator now "over-liquidate".

Proof of Concept

Tools Used

Recommended Mitigation Steps

Consider implement a close factor so a liquidator is entitled to certain liquidatable whenever the position is unhealthy in the beginning of execution.

Assessed type

Context

c4-pre-sort commented 1 year ago

minhquanym marked the issue as primary issue

c4-sponsor commented 1 year ago

0xShaito marked the issue as disagree with severity

0xShaito commented 1 year ago

That is ok. If the user is not underwater they should not be liquidated.

Liquidators should use a protected rpc to avoid frontrunning anyways.

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Invalid