Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bringing a position from unsafe to safe by liquidation paritally #571

Open
howlbot-integration bot opened this issue Oct 14, 2024 · 7 comments
Open
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working M-01 primary issue Highest quality submission among a set of duplicates 🤖_24_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/CDPVault.sol#L509

Vulnerability details

Impact

The CDPVault's liquidation mechanism allows partial liquidations to temporarily bring unsafe positions back to a safe state. This can delay necessary liquidations if collateral prices continue to fall, potentially leading to increased risk and larger losses for the protocol.

Proof of Concept

The issue arises from liquidatePosition function in the CDPVault contract. An attacker can exploit this to avoid full liquidation even when their position should be unsafe.

Break down the Scenario:

  • Alice creates a position with 100 ether collateral and 60 ether debt.
  • The collateral price drops, making Alice's position unsafe.
  • Bob performs a partial liquidation on Alice's position.
  • Alice's position becomes temporarily safe due to the partial liquidation.
  • The collateral price drops further, but a small liquidation attempt by Carol fails because the position is still considered safe.
  • Only after an even further price drop can the position be liquidated again.

Alice can do the same flow to herself (self liqudation) to protect her position instead of maintaining it by increasing the colleteral, which costs less and goes against how the protocol is intended to work.

at function liquidatePosition

function liquidatePosition(address owner, uint256 repayAmount) external whenNotPaused {
    // ... (validation and state loading)
    if (_isCollateralized(calcTotalDebt(debtData), wmul(position.collateral, spotPrice_), config.liquidationRatio))
        revert CDPVault__liquidatePosition_notUnsafe();
    // ... (liquidation calculations)
    position = _modifyPosition(owner, position, newDebt, newCumulativeIndex, -toInt256(takeCollateral), totalDebt);
    // ... (state updates and transfers)
}

The issue occurs because after a partial liquidation, the position can become temporarily safe, preventing further immediate liquidations even if the collateral value continues to decrease.

Tools Used

Manual Review

Recommended Mitigation Steps

Flag positions as unsafe when they become unsafe, and revert them to safe status upon additional collateral deposits. However, this approach is suboptimal. A comprehensive reevaluation of the liquidation mechanism is necessary.

Assessed type

Other

@howlbot-integration howlbot-integration bot added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value 🤖_24_group AI based duplicate group recommendation bug Something isn't working primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Oct 14, 2024
howlbot-integration bot added a commit that referenced this issue Oct 14, 2024
@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Oct 25, 2024
@c4-judge
Copy link
Contributor

koolexcrypto marked the issue as satisfactory

@c4-judge
Copy link
Contributor

koolexcrypto marked the issue as selected for report

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Oct 25, 2024
@c4-judge
Copy link
Contributor

koolexcrypto marked the issue as not selected for report

@c4-judge c4-judge removed selected for report This submission will be included/highlighted in the audit report satisfactory satisfies C4 submission criteria; eligible for awards labels Oct 25, 2024
@c4-judge
Copy link
Contributor

koolexcrypto removed the grade

@koolexcrypto
Copy link

koolexcrypto commented Oct 25, 2024

PoC confirmed.
See code-423n4/2024-07-loopfi-validation#745 (comment)

For the report in case needed

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.19;

import {TestBase, ERC20PresetMinterPauser} from "./TestBase.sol";
import {CDPVault} from "../../src/CDPVault.sol";
import {WAD, toInt256, wmul, wdiv} from "../../src/utils/Math.sol";
import {console} from "forge-std/console.sol";
import {StdCheats} from "forge-std/StdCheats.sol";

contract CDPVaultPartialLiquidationTest is TestBase {
    CDPVault internal vault;

    function setUp() public override {
        super.setUp();

        token = new ERC20PresetMinterPauser("Token", "TKN");
        vault = createCDPVault(
            token,
            1_000_000 ether, // debt ceiling
            0,               // debt floor
            1.25 ether,      // liquidation ratio
            1.0 ether,       // liquidationPenalty
            0.95 ether       // liquidationDiscount
        );
        createGaugeAndSetGauge(address(vault));

        // Grant minter role to this contract
        bytes32 minterRole = token.MINTER_ROLE();
        token.grantRole(minterRole, address(this));
        mockWETH.grantRole(minterRole, address(this));

        // Set initial price
        _updateSpot(1 ether);
    }


    function _updateSpot(uint256 price) internal {
        oracle.updateSpot(address(token), price);
    }

    function testUnsafePosition() public {
        // Create a position
        address alice = address(0x1);
        token.mint(alice, 100 ether);
        vm.startPrank(alice);
        token.approve(address(vault), 100 ether);
        vault.modifyCollateralAndDebt(alice, alice, alice, toInt256(100 ether), toInt256(60 ether));
        vm.stopPrank();

        // Record initial state
        (uint256 initialCollateral, uint256 initialDebt,,,,) = vault.positions(alice);
        console.log("Initial Collateral:", initialCollateral);
        console.log("Initial Debt:", initialDebt);
        console.log("Initial Price:", vault.spotPrice());

        // Drop price to make position unsafe
        _updateSpot(0.7 ether);
        console.log("New Price:", vault.spotPrice());

        // Partial liquidation
        address bob = address(0x2);
        mockWETH.mint(bob, 30 ether);
        vm.startPrank(bob);
        mockWETH.approve(address(vault), 30 ether);
        // console.log("Start liquidatePosition");
        vault.liquidatePosition(alice, 30 ether);
        vm.stopPrank();
        console.log("Partial Liquidation succeed");

        // Log state after partial liquidation
        (uint256 collateralAfterPartial, uint256 debtAfterPartial,,,,) = vault.positions(alice);
        console.log("Collateral after partial liquidation:", collateralAfterPartial);
        console.log("Debt after partial liquidation:", debtAfterPartial);

        // Try to liquidate again (should fail as position is now safe)
        address charlie = address(0x3);
        mockWETH.mint(charlie, 1 ether);
        vm.startPrank(charlie);
        mockWETH.approve(address(vault), 1 ether);
        vm.expectRevert(CDPVault.CDPVault__liquidatePosition_notUnsafe.selector);
        vault.liquidatePosition(alice, 1 ether);
        vm.stopPrank();
        console.log("Partial Liquidation failed");


        // Verify final state
        (uint256 finalCollateral, uint256 finalDebt,,,,) = vault.positions(alice);
        console.log("Final Collateral:", finalCollateral);
        console.log("Final Debt:", finalDebt);

    }



}

Output

  Initial Collateral: 100000000000000000000
  Initial Debt: 60000000000000000000
  Initial Price: 1000000000000000000
  New Price: 700000000000000000
  Partial Liquidation succeed
  Collateral after partial liquidation: 54887218045112781955
  Debt after partial liquidation: 30000000000000000000
  Partial Liquidation failed
  Final Collateral: 54887218045112781955
  Final Debt: 30000000000000000000

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Oct 25, 2024
@c4-judge
Copy link
Contributor

koolexcrypto marked the issue as satisfactory

@c4-judge
Copy link
Contributor

koolexcrypto marked the issue as selected for report

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Oct 25, 2024
@C4-Staff C4-Staff added the M-01 label Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working M-01 primary issue Highest quality submission among a set of duplicates 🤖_24_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

3 participants