c4-bot-3 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/WiseSecurity/WiseSecurity.sol#L427-L429 https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/FeeManager/FeeManager.sol#L663-L675 https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/FeeManager/FeeManager.sol#L689-L699

Vulnerability details

Impact

All the fees collected from the FEE_MANAGER_NFT will be unclaimable on the FeeManager contract because the totalBadDebtETH will never be able to go back to 0.

Proof of Concept

Liquidations in the WiseLending market are designed to allow the liquidators to liquidate debt from a specific pool and receive the liquidation payment on a different pool (if they want it, they can also receive the liquidation payment in the same type of poolToken if the position has collateralized deposits on such a pool).

For example, if a position has collateralized deposits on PoolA and PoolB, and has loans on PoolA and PoolC.
- The liquidator can liquidate the debt of the PoolA and receive tokens from the PoolB as the liquidation payment for liquidating the debt of the poolA.

When a liquidation is done, after all the lending and borrowing storage variables have been updated according to the amount of debt repaid by the liquidator, the liquidation logic checks if there is any bad debt in the position by calling the WISE_SECURITY.checkBadDebtLiquidation() function, any bad debt (the difference between the overallETHBorrowBare - overallETHCollateralsBare) is reported to the FeeManager, to do this, two functions are called, the FEE_MANAGER.increaseTotalBadDebtLiquidation() function and the FEE_MANAGER.setBadDebtUserLiquidation() function.


function checkBadDebtLiquidation(
uint256 _nftId
)
external
onlyWiseLending
{

uint256 bareCollateral = overallETHCollateralsBare(
    _nftId
);

uint256 totalBorrow = overallETHBorrowBare(
    _nftId
);

//@audit-info => If the total leftover debt is lower than the total leftover collateral, no bad debt, returns!
if (totalBorrow < bareCollateral) {
    return;
}

//@audit-info => If the total leftover debt is greater than the total leftover collateral, bad debt has been generated!
unchecked {
    uint256 diff = totalBorrow
        - bareCollateral;

    FEE_MANAGER.increaseTotalBadDebtLiquidation(
        diff
    );

    FEE_MANAGER.setBadDebtUserLiquidation(
        _nftId,
        diff
    );
}

}


- The [`FEE_MANAGER.increaseTotalBadDebtLiquidation() function`](https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/FeeManager/FeeManager.sol#L494-L508) will increase the `totalBadDebtETH` by the `amount` of baddebt calculated in the [`WISE_SECURITY.checkBadDebtLiquidation() function`](https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/WiseSecurity/WiseSecurity.sol#L405-L436).
>

function increaseTotalBadDebtLiquidation( uint256 _amount ) external onlyWiseSecurity { _increaseTotalBadDebt( _amount );

...

}

function _increaseTotalBadDebt( uint256 _amount ) internal { //@audit-info => totalBadDebtETH is increased by the baddebt computed in the current liquidation totalBadDebtETH += _amount; ... }

- The [`FEE_MANAGER.setBadDebtUserLiquidation() function`](https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/FeeManager/FeeManager.sol#L514-L531) will simply override the user's baddebt to the amount that was computed during this liquidation.
>

function setBadDebtUserLiquidation( uint256 _nftId, uint256 _amount ) external onlyWiseSecurity { _setBadDebtPosition( _nftId, _amount ); ... }

function _setBadDebtPosition( uint256 _nftId, uint256 _amount ) internal { //@audit-info => badDebtPosition is overridden by the baddebt computed in the current liquidation badDebtPosition[_nftId] = _amount; }


**The problem with the existing logic** to report bad debt to the FeeManager is that the bad debt of the same position will be reported multiple times. Let's do an example where a position has collateralized deposits on 2 pools, and has taken loans on 3 different pools. (For this example we will simulate the state of the position as if bad debt would've been generated in the position.)
- Collateralized Deposits - `overallETHCollateralsBare`: 2.3_e18 ETH
  - PoolA: 0.8_e18 ETH
  - PoolB: 1.5_e18 ETH
- Loans - `overallETHBorrowBare`: 2.4_e18 ETH
  - PoolA: 0.4_e18 ETH
  - PoolB: 0.8_e18 ETH
  - PoolC: 1.2_e18 ETH

This position is in a liquidable state, thus, a liquidator will proceed to liquidate debt from the pools where the position has borrowed.

1. A liquidator will liquidate the debt of the poolA and ask to receive the same tokens of poolA as the liquidation payment.
- When the liquidation logic gets to the [`WISE_SECURITY.checkBadDebtLiquidation() function`](https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/WiseSecurity/WiseSecurity.sol#L405-L436), the state of the position will be as follow:
  - Collateralized Deposits - `overallETHCollateralsBare`: 1.9_e18 ETH
    - PoolA: ~0.4_e18 ETH (leftover collateral on PoolA after covering the liquidator's payment)
    - PoolB: 1.5_e18 ETH
  - Loans - `overallETHBorrowBare`: 2_e18 ETH
    - PoolA: 0 ETH (The liquidator repaid the debt of this pool during this liquidation)
    - PoolB: 0.8_e18 ETH
    - PoolC: 1.2_e18 ETH

  - **diff / totalBadDebt** = (overallETHBorrowBare - overallETHCollateralsBare) === (2_e18 - 1.9_e18) == 0.1_e18 ETH
    - This means, **this liquidation will report to the FeeManager that the position being liquidated has a total of `0.1_e18 ETH` worth of bad debt**
      - `totalBadDebtETH` will be increased by the current's position bad debt (0.1_e18 ETH)
      - User's bad debt (`badDebtPosition`) will be set to the current's position bad debt (0.1_e18 ETH)

2. Now, the position has debt only on PoolB and PoolC (PoolA's debt has been liquidated), and the position has still collateralized deposits on PoolA and PoolB. For this liquidation, the liquidator decides to liquidate the debt of the PoolC and asks to receive poolTokens from the PoolB as the liquidation payment.
- When the liquidation logic gets to the [`WISE_SECURITY.checkBadDebtLiquidation() function`](https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/WiseSecurity/WiseSecurity.sol#L405-L436), the state of the position will be as follow:
  - Collateralized Deposits - `overallETHCollateralsBare`: ~0.7_e18 ETH
    - PoolA: ~0.4_e18 ETH 
    - PoolB: ~0.3_e18 ETH (leftover collateral on PoolB after covering the liquidator's payment)
  - Loans - `overallETHBorrowBare`: 0.8_e18 ETH
    - PoolA: 0 ETH
    - PoolB: 0.8_e18 ETH
    - PoolC: 0 ETH (The liquidator repaid the debt of this pool during this liquidation)

  - **diff / totalBadDebt** = (overallETHBorrowBare - overallETHCollateralsBare) === (0.7_e18 - 0.8_e18) == 0.1_e18 ETH
    - This means, **the second liquidation will also report to the FeeManager that the position has a total of `0.1_e18 ETH` worth of bad debt**
      - `totalBadDebtETH` will be increased by the current's position bad debt (0.1_e18 ETH)
        - **Here, the `totalBadDebtETH` has been increased two times by the bad debt of the same position**
      - User's bad debt (`badDebtPosition`) will be set to the current's position bad debt (0.1_e18 ETH)
        - The userBadDebt is registered only as the current bad debt.

3. The same will happen for the liquidation of the debt on the PoolB, the leftover bad debt will be reported again to the FeeManager, which will cause the `totalBadDebtEth` to be increased again.

4. The result of liquidating the debt across all the pools from this position is that the FeeManager has registered multiple times the same bad debt from the same position as if it would have been bad debt from different positions. **In the end, the `totalBadDebtEth` was increased by `0.4_e18 ETH`, and the userBadDebt was set as `0.1_e18 ETH`**

- When the bad debt of this position is repaid using the [`FeeManager.paybackBadDebtForToken() function`](https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/FeeManager/FeeManager.sol#L730-L809), the repaid amount would be the current bad debt, which would be the `0.1_e18 ETH` of bad debt registered in the last liquidation of the position. The `totalBadDebtEth` will only be decreased by the current userBadDebt. 
  - This means, even though the `totalBadDebtEth` was updated multiple times with the bad debt of the same position, the maximum amount of bad debt that can be decreased when repaying the position's baddebt is the current position's bad debt (Which at most can represent the bad debt that was registered in the last liquidation)
  - For this example, after the bad debt has been repaid, **the `totalBadDebtEth` would end up with an extra `0.3_e18 ETH` of bad debt, while the userBadDebt is deleted. This will make the `totalBadDebtEth` impossible to be taken down to 0 again because the userBadDebt and the `totalBadDebtEth` were incorrectly updated by the liquidation process**

This is a problem that will occur for any position that generates bad debt and has loans in more than 2 pools. The liquidation of the loans on each pool will report to the FeeManager the leftover bad debt in the position. As the liquidators liquidate the loans on the different pools, each liquidation will report the same bad debt to the FeeManager causing the `totalBadDebtEth` to grow more than what it really should.

## Tools Used
Manual Audit

## Recommended Mitigation Steps
The recommendation would be to use a similar approach to the [`FeeManagerHelper._updateUserBadDebt() function`](https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/FeeManager/FeeManagerHelper.sol#L134-L189)
- Instead of straight updating the `totalBadDebtETH`, first update the `userBadDebt`, and **update the `totalBadDebtETH` depending on the difference between the newBadDebt and the currentBadDebt**

1. The easiest way to implement this recommendation would be to add the below function on the `FeeManager` contract
>

//@audit-info => _newBadDebt would be the current bad debt on the position that is been liquidated on the WiseLending contract! function registerBadDebt(_nftId, _newBadDebt) external onlyWiseSecurity {

//@audit-info => Load the current bad debt of the position registered in the FeeManager
uint256 currentBadDebt = badDebtPosition[
    _nftId
];

unchecked {
    _setBadDebtPosition(
        _nftId,
        _newBadDebt
    );

    //@audit-info => `totalBadDebtEth` will grow or shrink according to the change of the userBadDebtPosition.
    _newBadDebt > currentBadDebt
        ? _increaseTotalBadDebt(_newBadDebt - currentBadDebt)
        : _decreaseTotalBadDebt(currentBadDebt - _newBadDebt);
}

}


2. On the [`WISE_SECURITY.checkBadDebtLiquidation() function`](https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/WiseSecurity/WiseSecurity.sol#L405-L436), do the next changes
>

function checkBadDebtLiquidation( uint256 _nftId ) external onlyWiseLending {

...

//@audit-info => If the total leftover debt is greater than the total leftover collateral, bad debt has been generated!
unchecked {
    uint256 diff = totalBorrow
        - bareCollateral;

FEE_MANAGER.registerBadDebt(_nftId,diff);
FEE_MANAGER.increaseTotalBadDebtLiquidation(
diff
);
FEE_MANAGER.setBadDebtUserLiquidation(
_nftId,
diff
); } }

The FEE_MANAGER.increaseTotalBadDebtLiquidation() function && the FEE_MANAGER.setBadDebtUserLiquidation() function can now be deleted from the FeeManager contract. They will not be used anymore. The new function will be in charge of tracking and updating the total bad debt as well as the individual user's bad debt.