sherlock-admin3 commented 2 months ago

Bigsam

Medium

Liquidation fails to update the interest Rate when liquidation funds are sent to the treasury thus the next user uses an inflated index

Summary

A bug exists in the Zerolend liquidation process where the interest rate is not updated before transferring liquidation funds to the treasury. This omission leads to an inflated index being used by the next user when performing subsequent actions such as deposits, withdrawals, or borrowing, similar to the previously reported bug in the withdrawal function. As a result, the next user may receive fewer shares or incur an incorrect debt due to the artificially high liquidity rate.

Root Cause

Examples of update rate before transferring everywhere in the protocol to maintain Rate

https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/pool/logic/SupplyLogic.sol#L69-L81

https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/pool/logic/SupplyLogic.sol#L125-L146

https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/pool/logic/BorrowLogic.sol#L88-L99

https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/pool/logic/BorrowLogic.sol#L139-L158

The same process can be observed in Aave v 3.

Looking at the effect of updating rate

https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/pool/logic/ReserveLogic.sol#L134-L182

https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/periphery/ir/DefaultReserveInterestRateStrategy.sol#L98-L131

This rates are used to get the new index

https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/pool/logic/ReserveLogic.sol#L225-L227

https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/pool/logic/ReserveLogic.sol#L235-L237

-

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

During the liquidation process in Zerolend, when funds are transferred to the treasury as a liquidation protocol fee, the interest rate in the pool is not updated before the transfer. This failure results in the next user's interaction with the protocol (such as a deposit, withdrawal, or loan) being calculated based on an inflated liquidity rate. The inflated rate causes the user to receive fewer shares than they should or be charged an incorrect interest rate.

In contrast, Aave’s approach ensures that the interest rate is always updated when necessary and adjusted when funds are moved outside the system. Aave achieves this by transferring the funds inside the contract in the form of aTokens, which track liquidity changes, and since atokens are not burnt there is no need to update the interest rate accordingly in this case.

Zerolend, however, directly transfers funds out of the pool without recalculating the interest rate, which leads to inconsistencies in the index used by the next user.

Code Context:

In Zerolend's liquidation process, when a user is liquidated and the liquidation fee is sent to the treasury, the protocol transfers the funds directly without updating the interest rate.

// Transfer fee to treasury if it is non-zero
if (vars.liquidationProtocolFeeAmount != 0) {
    uint256 liquidityIndex = collateralReserve.getNormalizedIncome();
    uint256 scaledDownLiquidationProtocolFee = vars.liquidationProtocolFeeAmount.rayDiv(liquidityIndex);
    uint256 scaledDownUserBalance = balances[params.collateralAsset][params.position].supplyShares;

    if (scaledDownLiquidationProtocolFee > scaledDownUserBalance) {
        vars.liquidationProtocolFeeAmount = scaledDownUserBalance.rayMul(liquidityIndex);
    }
@audit >> transferring underlying asset out without updating interest rate first>>>>

    IERC20(params.collateralAsset).safeTransfer(IPool(params.pool).factory().treasury(), vars.liquidationProtocolFeeAmount);
}

As can be seen in the code, the liquidation protocol fee is transferred to the treasury, but no interest rate update takes place before the transfer. This results in an incorrect liquidity rate for the next user interaction.

Comparison with Aave:

Aave uses aTokens for transfers within the protocol, and the interest rate is updated accordingly when funds are moved, ensuring that the liquidity rate and index are always accurate. In Aave’s liquidation process, the aTokens are transferred to the treasury rather than removing liquidity directly from the pool.

vars.collateralAToken.transferOnLiquidation(
    params.user,
    vars.collateralAToken.RESERVE_TREASURY_ADDRESS(),
    vars.liquidationProtocolFeeAmount
);

In Aave’s implementation, the aToken system ensures that the liquidity and interest rates are intact based on the movement of funds and not transferring underlying assets.

Impact

Incorrect Share Calculation: Deposits, withdrawals, and loans after a liquidation may use an inflated liquidity rate, resulting in fewer shares minted for depositors or incorrect debt calculations for borrowers.
Protocol Inconsistency: The protocol operates with an inaccurate interest rate after each liquidation, leading to potential financial discrepancies across user interactions.

PoC

No response

Mitigation

To address this issue, the interest rate must be updated before transferring any liquidation protocol fees to the treasury. This ensures that the system correctly accounts for the reduction in liquidity due to the transfer. This will be my last report here before transferring funds to the treasury also a bug was discovered before transferring. kind fix also. thank you for the great opportunity to audit your code i wish zerolend the very best in the future.

Suggested Fix:

In the liquidation logic, invoke the updateInterestRates function on the collateral reserve before transferring the funds to the treasury. This will ensure that the correct liquidity rate is applied to the pool before the funds are removed.

Modified Code Example:


if (vars.liquidationProtocolFeeAmount != 0) {
    uint256 liquidityIndex = collateralReserve.getNormalizedIncome();
    uint256 scaledDownLiquidationProtocolFee = vars.liquidationProtocolFeeAmount.rayDiv(liquidityIndex);
    uint256 scaledDownUserBalance = balances[params.collateralAsset][params.position].supplyShares;

    if (scaledDownLiquidationProtocolFee > scaledDownUserBalance) {
        vars.liquidationProtocolFeeAmount = scaledDownUserBalance.rayMul(liquidityIndex);
    }

++   // Before transferring liquidation protocol fee to treasury, update the interest rates
++   collateralReserve.updateInterestRates(
++  totalSupplies,
++  collateralReserveCache,
++  params.collateralAsset,
++  IPool(params.pool).getReserveFactor(),
++  0, // No liquidity added
++   vars.liquidationProtocolFeeAmount, // Liquidity taken during liquidation
++  params.position,
++ params.data.interestRateData
++ );

++ // Now, transfer fee to treasury if it is non-zero

    IERC20(params.collateralAsset).safeTransfer(IPool(params.pool).factory().treasury(), vars.liquidationProtocolFeeAmount);
}

In this updated version, the interest rates are recalculated before the liquidation protocol fee is transferred to the treasury. This ensures that subsequent deposits, withdrawals, and loans use the correct liquidity rate and avoid discrepancies caused by an inflated index.

nevillehuang commented 2 months ago

request poc

Seems related to #387 in terms of root cause

sherlock-admin4 commented 2 months ago

PoC requested from @Tomiwasa0

Requests remaining: 14

Tomiwasa0 commented 2 months ago

After setting liquidationProtocolFeePercentage to 20%, 20-10% using aave's examples
add to addresses

  ++   address sam = address(3);
  ++   address dav = address(4);

PASTE AND RUN THE POC

  function _generateLiquidationCondition() internal {
   _mintAndApprove(alice, tokenA, mintAmountA, address(pool)); // alice 1000 tokenA
   _mintAndApprove(sam, tokenA, mintAmountA, address(pool)); // alice 1000 tokenA
    _mintAndApprove(bob, tokenB, mintAmountB, address(pool)); // bob 2000 tokenB
     _mintAndApprove(dav, tokenA, mintAmountA, address(pool)); // bob 2000 tokenB

    vm.startPrank(alice);
    pool.supplySimple(address(tokenA), alice, supplyAmountA, 0); // 550 tokenA alice supply
    vm.stopPrank();

    vm.startPrank(sam);
    pool.supplySimple(address(tokenA), sam, supplyAmountA, 0); // 550 tokenA alice supply
    vm.stopPrank();

    vm.startPrank(bob);
    pool.supplySimple(address(tokenB), bob, supplyAmountB, 0); // 750 tokenB bob supply
    vm.stopPrank();

    vm.startPrank(alice);
    pool.borrowSimple(address(tokenB), alice, borrowAmountB, 0); // 100 tokenB alice borrow
    vm.stopPrank();

     vm.startPrank(sam);
    pool.borrowSimple(address(tokenB), sam, borrowAmountB, 0); // 100 tokenB alice borrow
    vm.stopPrank();

    vm.startPrank(bob);
    pool.borrowSimple(address(tokenA), bob , 500e18, 0); // 100 tokenB alice borrow
    vm.stopPrank();
     // Get the current block timestamp
        uint256 currentTime = block.timestamp;

    // Set the block.timestamp to current time plus 100 seconds
        vm.warp(currentTime + 1000);

    assertEq(tokenB.balanceOf(alice), borrowAmountB);

    oracleA.updateAnswer(0.45e8);
  }

Updated Liquidation Function:

function testLiquidationSimple2() external {
    _generateLiquidationCondition();
    (, uint256 totalDebtBase,,,,) = pool.getUserAccountData(alice, 0);

    vm.startPrank(bob);
    // vm.expectEmit(true, true, true, false);
    // emit PoolEventsLib.LiquidationCall(address(tokenA), address(tokenB), pos, 0, 0, bob);
    pool.liquidateSimple(address(tokenA), address(tokenB), pos, 100 ether);

    vm.stopPrank();

    (, uint256 totalDebtBaseNew,,,,) = pool.getUserAccountData(alice, 0);

    assertTrue(totalDebtBase > totalDebtBaseNew);

    // Get the current block timestamp
    uint256 currentTime3 = block.timestamp;

    // Set the block.timestamp to current time plus 100 seconds
    vm.warp(currentTime3 + 500);

    vm.startPrank(dav);
    pool.supplySimple(address(tokenA), dav, 100e18, 0); // 550 tokenA alice supply
    vm.stopPrank();

    assertEq(pool.getBalanceRaw(address(tokenA), dav, 0).supplyShares, 99999784100498438999);
}

Before Updating the index with Amount minted to tresury dav got - 99999784100498438999; After update - 99999783033155331339,

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 67.39ms (15.32ms CPU time)

Ran 1 test suite in 2.36s (67.39ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/forge/core/pool/PoolLiquidationTests.t.sol:PoolLiquidationTest
[FAIL. Reason: assertion failed: 99999783033155331339 != 99999784100498438999] testLiquidationSimple1() (gas: 1610672)

Other points are stated in issue 387

0xSpearmint commented 1 month ago

This issue is low severity. It does not satisfy the criteria for medium.

As shown by the POC, the difference in shares is 0.00000106% which is not large enough (0.01%) to be medium severity.

cvetanovv commented 1 month ago

I agree with @0xSpearmint. This issue does not meet the criteria for Medium severity:

Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The loss of the affected party must exceed 0.01% and 10 USD.

I'm planning to invalidate the issue.

Tomiwasa0 commented 1 month ago

@cvetanovv ,

To get the full impact of this kindly apply the appropriate fix to the bugs discovered in the liquidation function issue 473 and others, this creates a scenario also almost similar to issue 199 attacker get free funds,

In evaluating the current system's functionality, issue 91 identified seven significant impacts resulting from improper handling, specifically regarding the liquidity and collateral management mechanisms:

Incorrect Withdrawals: The amount withdrawn is consistently 1% of the liquidated amount, which deviates from expected behavior.
Test Validity: The test scenario I provided demonstrates the validity of the concern, although I was unable to use an appropriate timeframe due to the Chainlink timestamp check. To ensure accuracy, I strongly recommend both parties rerun the scenario with the following conditions:
- Funds are borrowed and remain unpaid after 3 to 6 months.
- The collateral value drops, and the user is subsequently liquidated.
Systematic Impact: As stated in issue 91, testing across all relevant functions reveals that this has a distinct impact on subsequent function calls, altering the expected outputs.
Minting of Free Shares: By not considering other influencing factors, the current setup inadvertently allows users to mint free shares. These shares can then be converted back to the original amount, creating an imbalance.
Collateral Decline Over Time: The decline in collateral value over time affects the Liquidity in the pool, further complicating the issue. The test case clearly illustrates how the system's behavior changes in these scenarios. creates DOS vulnerability like issue 488 and 375.
Use of New Inputs: By running the system using the new inputs provided, you will see the exact impact referenced by spearmint. This highlights the need for a comprehensive review of the mechanics involved.

cvetanovv commented 1 month ago

@Tomiwasa0 I will agree with your comment and leave this issue as is.

sherlock-audit / 2024-06-new-scope-judging

Bigsam - Liquidation fails to update the interest Rate when liquidation funds are sent to the treasury thus the next user uses an inflated index #401

Liquidation fails to update the interest Rate when liquidation funds are sent to the treasury thus the next user uses an inflated index

Summary

Root Cause

Internal pre-conditions

External pre-conditions

Attack Path

Code Context:

Comparison with Aave:

Impact

PoC

Mitigation

Suggested Fix:

Modified Code Example: