search

code-423n4 / 2024-02-wise-lending-findings

11 stars 8 forks source link

Bad accounting bug on `WiseSecurity::checkBadDebtLiquidation()` leads to permanently unclaimable incentives and fees #243

Closed c4-bot-3 closed 4 months ago

c4-bot-3 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseSecurity/WiseSecurity.sol#L427-L429 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/FeeManager/FeeManagerHelper.sol#L94 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseSecurity/WiseSecurity.sol#L431-L434 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/FeeManager/FeeManagerHelper.sol#L83 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/FeeManager/FeeManagerHelper.sol#L147-L149 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/FeeManager/FeeManagerHelper.sol#L157-L159 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/FeeManager/FeeManagerHelper.sol#L179-L181 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/FeeManager/FeeManager.sol#L663-L670 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/FeeManager/FeeManager.sol#L697-L699

Vulnerability details

Impact

The WiseSecurity::checkBadDebtLiquidation() got a bad accounting bug while updating the global bad debt (i.e., totalBadDebtETH variable).

This bug can be triggered by normal liquidation events or even by an attacker who forces the liquidable position (with bad debt) to be liquidated twice (i.e., executing the WiseLending::liquidatePartiallyFromTokens() upon the target position twice).

In addition to the attack cost, an attacker can even pay 1 wei (in terms of the payback token) for each liquidation by specifying the _shareAmountToPay parameter == 0 when invoking the liquidatePartiallyFromTokens(). Thus, the attack cost is very cheap.

Subsequently, this bug will permanently increment the totalBadDebtETH variable (global bad debt). In other words, the totalBadDebtETH variable will not be able to decrease to 0 anymore, even if all bad debt of that associated position is paid back in full.

As a result, the incentiveOwnerA and incentiveOwnerB will no longer receive their incentives via the FeeManager::claimWiseFees() since the _distributeIncentives() will no longer be executed. Furthermore, all beneficiaries will no longer claim gathered fees via the FeeManager::claimFeesBeneficial() since the transaction will always be reverted.

For this reason, this issue deserves a high severity rating:

Please refer to the Proof of Concept for more details.

Proof of Concept

This PoC section can be categorized into two subsections, as follows.

  1. Code Walkthrough
  2. Attack Illustration With Simple Math

1. Code Walkthrough

The WiseSecurity::checkBadDebtLiquidation() got a bad accounting bug while updating the global bad debt (i.e., totalBadDebtETH variable).

Specifically, if the liquidating position has bad debt, the checkBadDebtLiquidation() will perform two primary steps:

  1. Execute the FeeManager::increaseTotalBadDebtLiquidation() to increase the totalBadDebtETH variable by the position's bad debt (i.e., debt variable).

  2. Execute the FeeManager::setBadDebtUserLiquidation() to set the position's bad debt (badDebtPosition[_nftId]).

The bad accounting bug in question is the 2nd step. The setBadDebtUserLiquidation() will execute the FeeManagerHelper::_setBadDebtPosition() to replace the position's previous bad debt with the new bad debt.

To elaborate, assume that the position (with bad debt) is liquidated twice. The checkBadDebtLiquidation() will increase the totalBadDebtETH variable (global bad debt) twice, but the function will account for the position's bad debt only once.

    // FILE: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseSecurity/WiseSecurity.sol
    function checkBadDebtLiquidation(
        uint256 _nftId
    )
        external
        onlyWiseLending
    {
        uint256 bareCollateral = overallETHCollateralsBare(
            _nftId
        );

        uint256 totalBorrow = overallETHBorrowBare(
            _nftId
        );

        if (totalBorrow < bareCollateral) {
            return;
        }

        unchecked {
            uint256 diff = totalBorrow
                - bareCollateral;

@1.1        FEE_MANAGER.increaseTotalBadDebtLiquidation(
@1.1            diff
@1.1        ); //@audit -- The increaseTotalBadDebtLiquidation() will eventually execute the _increaseTotalBadDebt() to increase the totalBadDebtETH (global debt) -- see @1.2

@2.1        FEE_MANAGER.setBadDebtUserLiquidation(
@2.1            _nftId,
@2.1            diff
@2.1        ); //@audit -- The setBadDebtUserLiquidation() will eventually execute the _setBadDebtPosition(). This step will replace the position's previous bad debt with the new one. -- see @2.2
        }
    }

    ...

    // FILE: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/FeeManager/FeeManagerHelper.sol
    function _increaseTotalBadDebt(
        uint256 _amount
    )
        internal
    {
@1.2    totalBadDebtETH += _amount; //@audit -- Increase the totalBadDebtETH (global debt)

        emit TotalBadDebtIncreased(
            _amount,
            block.timestamp
        );
    }

    ...

    // FILE: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/FeeManager/FeeManagerHelper.sol
    function _setBadDebtPosition(
        uint256 _nftId,
        uint256 _amount
    )
        internal
    {
@2.2    badDebtPosition[_nftId] = _amount; //@audit -- The position's previous bad debt will be replaced with the new bad debt
    }

This bug will lead to a permanent increment of the totalBadDebtETH variable (global bad debt). When a user executes the FeeManager::paybackBadDebtForToken() or FeeManager::paybackBadDebtNoReward() to pay back the position's bad debt, the FeeManagerHelper::_updateUserBadDebt() will be triggered to update the position's bad debt.

If there is no more bad debt, the _updateUserBadDebt() will remove the position's tracked bad debt (currentBadDebt) from the global bad debt (totalBadDebtETH). But if there exists bad debt, the _updateUserBadDebt() will update the global bad debt (totalBadDebtETH) by accordingly adjusting (increasing/decreasing) the debt value from the newBadDebt and currentBadDebt (previously tracked bad debt) variables.

However, the previously tracked bad debt (currentBadDebt) will not include the replaced debt value previously mentioned. In other words, the totalBadDebtETH variable (global bad debt) will not be able to decrease to 0 anymore, even if all bad debt of that position is paid back in full.

    // FILE: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/FeeManager/FeeManagerHelper.sol
    function _updateUserBadDebt(
        uint256 _nftId
    )
        internal
    {
        uint256 currentBorrowETH = WISE_SECURITY.overallETHBorrowHeartbeat(
            _nftId
        );

        uint256 currentCollateralBareETH = WISE_SECURITY.overallETHCollateralsBare(
            _nftId
        );

@3      uint256 currentBadDebt = badDebtPosition[
@3          _nftId
@3      ]; //@audit -- Here, the _updateUserBadDebt() will load the position's previous bad debt (currentBadDebt)

        if (currentBorrowETH < currentCollateralBareETH) {

            _eraseBadDebtUser(
                _nftId
            );

@4          _decreaseTotalBadDebt(
@4              currentBadDebt
@4          ); //@audit -- If there is no more bad debt, the function will remove the position's previous bad debt (currentBadDebt) from the global bad debt (totalBadDebtETH)

            ...

            return;
        }

        unchecked {
            uint256 newBadDebt = currentBorrowETH
                - currentCollateralBareETH;

            _setBadDebtPosition(
                _nftId,
                newBadDebt
            );

@5          newBadDebt > currentBadDebt //@audit -- If there exists bad debt, the function will update the global bad debt (totalBadDebtETH) by accordingly adjusting (increasing/decreasing) the debt value from the newBadDebt and currentBadDebt variables
@5              ? _increaseTotalBadDebt(newBadDebt - currentBadDebt)
@5              : _decreaseTotalBadDebt(currentBadDebt - newBadDebt);

            ...
        }
    }

This bug can be triggered by normal liquidation events or even by an attacker who forces the liquidable position (with bad debt) to be liquidated twice (i.e., executing the WiseLending::liquidatePartiallyFromTokens() upon the target position twice).

After the bug is triggered, the incentiveOwnerA and incentiveOwnerB will no longer receive their incentives via the FeeManager::claimWiseFees() since the _distributeIncentives() will no longer be executed. Furthermore, all beneficiaries will no longer claim gathered fees via the FeeManager::claimFeesBeneficial() since the transaction will always be reverted.

    // FILE: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/FeeManager/FeeManager.sol
    function claimWiseFees(
        address _poolToken
    )
        public
    {
        ...

@6      if (totalBadDebtETH == 0) { //@audit -- After the bug is triggered, the incentiveOwnerA and incentiveOwnerB will no longer receive their incentives (the _distributeIncentives() will no longer be executed)
@6
@6          tokenAmount = _distributeIncentives(
@6              tokenAmount,
@6              _poolToken,
@6              underlyingTokenAddress
@6          );
@6      }

        _increaseFeeTokens(
            underlyingTokenAddress,
            tokenAmount
        );

        ...
    }

    ...

    // FILE: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/FeeManager/FeeManager.sol
    function claimFeesBeneficial(
        address _feeToken,
        uint256 _amount
    )
        external
    {
        address caller = msg.sender;

@7      if (totalBadDebtETH > 0) {
@7          revert ExistingBadDebt(); //@audit -- After the bug is triggered, all beneficiaries will no longer claim gathered fees (the transaction will always be reverted)
@7      }

        if (allowedTokens[caller][_feeToken] == false) {
            revert NotAllowed();
        }

        _decreaseFeeTokens(
            _feeToken,
            _amount
        );

        _safeTransfer(
            _feeToken,
            caller,
            _amount
        );

        ...
    }

2. Attack Illustration With Simple Math

This subsection will illustrate the possible attack with simple math that should be self-explanatory.

The attack consists of two liquidations on the same liquidable position with bad debt. The two liquidations can be executed in the same transaction, and the attacker can pay only 1 wei of the payback token for each liquidation to minimize the attack cost and preserve the bad debt ratio.

After the attack, the totalBadDebtETH = 6 (global debt), whereas the badDebtPosition[nftId] = 3 (the position's bad debt). As you can see, the previous debt (from the 1st liquidation) tracked by the badDebtPosition[nftId] was replaced with the new debt (from the 2nd liquidation).

// --- Initial ---
totalBadDebtETH = 0
badDebtPosition[nftId] = 0

// --------------------------------------------------------------------------------- //
// --- The attacker can pay only 1 wei of the payback token for each liquidation --- //
// --- to minimize the attack cost and preserve the bad debt ratio               --- //
// --------------------------------------------------------------------------------- //

// --- Liquidation #1 (on WiseSecurity::checkBadDebtLiquidation()) ---
bareCollateral = 7
totalBorrow = 10 // (totalBorrow > bareCollateral) -> Bad debt exists

diff = totalBorrow - bareCollateral
     = 10 - 7
     = 3

// Execute the FeeManager::increaseTotalBadDebtLiquidation(diff)
totalBadDebtETH += diff
                = 0 + 3
                = 3

// Execute the FeeManager::setBadDebtUserLiquidation(nftId, diff)
badDebtPosition[nftId] = diff
                       = 3

// --------------------------------------------------------------------------------- //
// --- The attacker can even execute the 2nd liquidation in the same transaction --- //
// --------------------------------------------------------------------------------- //

// --- Liquidation #2 (on WiseSecurity::checkBadDebtLiquidation()) ---
bareCollateral = 7
totalBorrow = 10 // (totalBorrow > bareCollateral) -> Bad debt exists

diff = totalBorrow - bareCollateral
     = 10 - 7
     = 3

// Execute the FeeManager::increaseTotalBadDebtLiquidation(diff)
totalBadDebtETH += diff
                = 3 + 3
                = 6

// Execute the FeeManager::setBadDebtUserLiquidation(nftId, diff)
badDebtPosition[nftId] = diff
                       = 3 // The previous bad debt was replaced

// --- Summary ---
totalBadDebtETH = 6
badDebtPosition[nftId] = 3 // The previous bad debt was replaced

Below simply simulates the execution of the FeeManager::paybackBadDebtForToken() to pay back the total position's bad debt.

As a result:

// --- Initial ---
totalBadDebtETH = 6
badDebtPosition[nftId] = 3 // The previous bad debt was replaced

// --------------------------------------------------------------------- //
// --- During the execution of FeeManager::paybackBadDebtForToken(), --- //
// --- the FeeManagerHelper::_updateUserBadDebt() will be triggered  --- //
// --- to update the position's bad debt (badDebtPosition[nftId])    --- //
// --- and global debt (totalBadDebtETH)                             --- //
// --------------------------------------------------------------------- //

// --- Payback the total position's bad debt (on FeeManager::paybackBadDebtForToken()) ---
bareCollateral = 7
totalBorrow = 6 // (totalBorrow < bareCollateral) -> No bad debt

currentBadDebt = badDebtPosition[nftId] // Previously tracked bad debt
               = 3

// FeeManagerHelper::_updateUserBadDebt() executes the _eraseBadDebtUser(nftId)
badDebtPosition[nftId] = 0

// FeeManagerHelper::_updateUserBadDebt() executes the _decreaseTotalBadDebt(currentBadDebt)
totalBadDebtETH -= currentBadDebt
                = 6 - 3
                = 3

// --- Summary ---
totalBadDebtETH = 3 // Permanent increase of the global bad debt
badDebtPosition[nftId] = 0 // No more position's bad debt

Tools Used

Manual Review

Recommended Mitigation Steps

Fixing this vulnerability depends on the developer's design decisions.

One possible solution is to rework the checkBadDebtLiquidation() to set the liquidating position's bad debt with the diff + currentBadDebt instead of only the diff, like the snippet below.

    function checkBadDebtLiquidation(
        uint256 _nftId
    )
        external
        onlyWiseLending
    {
        uint256 bareCollateral = overallETHCollateralsBare(
            _nftId
        );

        uint256 totalBorrow = overallETHBorrowBare(
            _nftId
        );

        if (totalBorrow < bareCollateral) {
            return;
        }

        unchecked {
            uint256 diff = totalBorrow
                - bareCollateral;

            FEE_MANAGER.increaseTotalBadDebtLiquidation(
                diff
            );

+           uint256 currentBadDebt = FEE_MANAGER.badDebtPosition(_nftId);
            FEE_MANAGER.setBadDebtUserLiquidation(
                _nftId,
-               diff
+               diff + currentBadDebt
            );
        }
    }

Assessed type

Other

c4-pre-sort commented 5 months ago

GalloDaSballo marked the issue as primary issue

c4-pre-sort commented 5 months ago

GalloDaSballo marked the issue as sufficient quality report

vonMangoldt commented 5 months ago

This doesnt lead to loss of user funds though. Hence it should be downgraded since one could just migrate and redeploy after discovering that. Otherwise good find 👍

Foon256 commented 5 months ago

Would agree with that! This is a good insight, but users' funds are never at risk. This is realted to the feeManager and the fees taken from the protocol. Therefore a medium issue.

vm06007 commented 5 months ago

@GalloDaSballo please mark this as disagree with severity (add label)

trust1995 commented 5 months ago

Fully deserving of High severity from the C4 rulebook, which doesn't constrain loss to user's funds for High.

c4-judge commented 5 months ago

trust1995 marked the issue as satisfactory

c4-judge commented 5 months ago

trust1995 marked the issue as selected for report

vm06007 commented 5 months ago

Fully deserving of High severity from the C4 rulebook, which doesn't constrain loss to user's funds for High.

Based on our agreement and scope we've agreed that high will be only those that lead to loss of funds. @vonMangoldt and @Foon256 can agree on that

vm06007 commented 5 months ago

@trust1995 you might want to recheck with organizers on our initial agreement for what is high, not by C4 book

trust1995 commented 5 months ago

Hi, You are referred to this decision of the the Supreme Court, which I am part of. Loss of yield / Loss of fees shall be treated as any other loss of capital. It is within my authority to decide on the severities of each issue presented. If you have additional concerns you may reach out through your own communication channels with C4 staff.

0xCiphky commented 4 months ago

Hey @trust1995, shouldn't this issue be tagged together with #74?

c4-judge commented 4 months ago

trust1995 marked the issue as not selected for report

c4-judge commented 4 months ago

trust1995 marked the issue as duplicate of #74