gearbox-protocol / core-v2 Goto Github PK

At the moment, credit manager provides liquidation premium = totalValue * liquidationPremium.
For some cases, if account has very valkid assets with low LT (for example 25%), such account could be liquidation and the premium would be unfairly high.

Idea is to change the formula to liquidation premium = min(totalValue; borrowedAmount) * liquidationPremium. At this case, bad accounts will have the same amount, however healthy accs which keep assets in token with low LT would get more fair amount

Issue:
CreditManager.closeCreditAccount(), which is invoked during liquidations, has the following line:

   // transfer remaining funds to the borrower [liquidations only]
        if (remainingFunds > 1) {
            _safeTokenTransfer(
                creditAccount,
                underlying,
                borrower,
                remainingFunds,
                false
            ); // F:[CM-13,18]
        }

Since this is always executed when there are remaining funds, liquidations on an account will fail if the borrower cannot be transferred to - such as when the borrower is blacklisted by USDC. While the account can be liquidated after some time due to account value dropping relative to borrowed amount (which leads to remainingFunds = 0), this can lead to a temporary freezing of LP funds and break "Expirable" Credit Manager logic used for fixed rate loans.

There are two proposed solutions:

1. Modify the USDC CreditFacade to check the borrower address being in the blacklist on liquidation. If the borrower is in the blacklist, the CF will transfer the Credit Account to the treasury / a treasury-controlled contract before liquidation - this will send the remaining funds to the treasury, so the borrower can recover them on a different address by contacting the DAO.
2. Change the CreditManager logic so that the borrower retains their Credit Account after liquidation. Note that the borrower would be able to close their account normally, since they can pass a different recipient address for their USDC. In addition to solving this issue, this has UX benefits. However, this requires updating Credit Managers, which is more disruptive than updating the Credit Facade.

Hi, I've noticed an issue in the v2.1 of the Dai credit contract. The Credit Facade in this version has not been added as a pauseable admin in the ACL contract. This can be observed in the image linked below:

Due to this omission, liquidations cannot proceed without adding the Credit Facade contract (0x5BcB06c56e8F28da0b038a373199240ca3F5a2f4) to the ACL contract.

Additionally, there seems to be an inconsistency in the developer documentation found at https://dev.gearbox.fi/. The Credit Facade address listed there is outdated. Refer to the image for clarification:

The correct address should be updated as can be verified on Etherscan. The relevant Etherscan image is included for reference:

more detail:

When the cumulative loss from liquidation exceeds the limit, the Credit Facade attempts to pause the credit manager, as detailed in the code snippet from the Credit Facade contract:

However, as mentioned earlier, since the Credit Facade does not have the required permissions, this action results in the failure of the liquidation process.

This issue is evident in the simulation: the _pauseCreditManager() function cannot be executed because the Credit Facade at 0x5BcB06c56e8F28da0b038a373199240ca3F5a2f4 lacks the necessary permissions.

_newPremium should be named _newDiscount.
https://github.com/Gearbox-protocol/core-v2/blob/main/contracts/credit/CreditConfigurator.sol#L842-L861

Once cumulative loss from liquidation breaks the limit, the credit facade tries to pause the credit manager:

The issue with this is that underlying Pausable implementation doesn't allow to pause an already paused contract: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/security/Pausable.sol#L89

This issue is fixed in V3: https://github.com/Gearbox-protocol/core-v3/blob/main/contracts/credit/CreditFacadeV3.sol#L338-L346

We plan to suggest the community to use a WBTC price oracle that compose BTC/USD and WBTC/BTC prices.
This is very much in favour to how the current stETH price is implemented.

On first look it seems that the code could be used as is, but a review is needed, and one might consider to change the variable names in order to make it more generic.

Issue:
creditFacade is currently only set once in AbstractAdapter in constructor, based on the current Credit Facade attached to the corresponding Credit Manager:

constructor(address _creditManager, address _targetContract) {
        if (_creditManager == address(0) || _targetContract == address(0))
            revert ZeroAddressException(); // F:[AA-2]

        creditManager = ICreditManagerV2(_creditManager); // F:[AA-1]
        creditFacade = ICreditManagerV2(_creditManager).creditFacade(); // F:[AA-1]
        targetContract = _targetContract; // F:[AA-1]
    }

This causes issues when the Credit Facade is updated in the CM, since the adapter no longer recognizes calls from the CF, and performs all health checks, even during a multicall.

Proposed solution:
AbstractAdapter must be updated to retrieve the CreditFacade dynamically from CreditManager each time it is needed, so the current Credit Facade is always recognized. E.g., _fastCheck() would be modified as follows:

    function _fastCheck(
        address creditAccount,
        address tokenIn,
        address tokenOut,
        uint256 balanceInBefore,
        uint256 balanceOutBefore,
        bool disableTokenIn
    ) private {
        address creditFacade = creditManager.creditFacade();
        
        if (msg.sender != creditFacade) {
            creditManager.fastCollateralCheck(
                creditAccount,
                tokenIn,
                tokenOut,
                balanceInBefore,
                balanceOutBefore
            );
        } else {
            if (disableTokenIn)
                creditManager.disableToken(creditAccount, tokenIn);
            creditManager.checkAndEnableToken(creditAccount, tokenOut);
        }
    }

Problem:
Current design requires 2 different contracts for pools: diesel token & pool itself. It's not optimal for gas consumption and do not support modern standards like 4626

Solution:
Combine pool & dieselToken as one contract and make it 4626 compatible

Current implementation doesn't take into account how much tokens were sent to pool back, which is okay for current underlying tokens. However, if someone will decide a pool with fee token (for example USDT), it could be a problem, because pool will get less money amountToPool * (1 -fee).

Suggestion: add functionality to CreditManager to be able to work with fee tokens.