Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bad debt can occur if the collateral token blacklists a borrower leading to total loss of stake for all lenders on that term #691

Open
c4-bot-1 opened this issue Dec 27, 2023 · 7 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-1245 grade-a high quality report This report is of especially high quality QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@c4-bot-1
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L803-L809
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L617-L621

Vulnerability details

Summary

When a borrower gets blacklisted by a collateral token with a blacklist (like for example USDC), the borrower is not able to repay the loan as the repay function tries to transfer the collateral back to the borrower which will revert. Therefore, sooner or later someone will call the loan for liquidation and an auction start. But nobody is able to bid on the auction as the bid function call will revert too when trying to transfer funds to a blacklisted address. As nobody is able to bid on the auction, the auction will continue till the midPoint is reached. The mechanism of this midPoint is as follows:

During the first half of the auction (before the midPoint is reached), an increasing amount of the collateral is offered, for the full CREDIT amount.
During the second half of the action (after the midPoint), all collateral is offered, for a decreasing CREDIT amount. Bad debt can occur.

Therefore, the moment the auction can be called again is when the system is in a dangerous situation where bad debt can occur. Which is handled by the ECG in a way that all lenders of the given term lose their stake.

Vulnerability Details

The flow of the issue is as follows:

  • The borrower takes a loan and deposits collateral (with a token that has a blacklist like for example USDC)
  • The borrower gets blacklisted
  • The borrower is not able to repay the loan even if willing to return the credit tokens, as the repay function will revert when trying to transfer funds to a blacklisted address:
function _repay(address repayer, bytes32 loanId) internal {
    ...
    // return the collateral to the borrower
    IERC20(params.collateralToken).safeTransfer(loan.borrower, loan.collateralAmount);
    ...
}
  • As the borrower is not able to repay the loan, sooner or later someone will call the loan for liquidation and an auction will start
  • But nobody is able to bid on the auction, as the bid function call will revert when trying to transfer funds to a blacklisted address:
function onBid(
    bytes32 loanId,
    address bidder,
    uint256 collateralToBorrower,
    uint256 collateralToBidder,
    uint256 creditFromBidder
) external {
    ...
    // send collateral to borrower
    if (collateralToBorrower != 0) {
        IERC20(params.collateralToken).safeTransfer(
            loans[loanId].borrower,
            collateralToBorrower
        );
    }
    ...
}
  • As nobody is able to bid on the auction, the auction will continue till the midPoint is reached
  • At this time, the auction can be called again as the collateralToBorrower is 0
  • But the whole situation becomes very dangerous at this moment as bad debt / loss for the system can occur after the midPoint is reached and ECG handles bad debt by slashing all lenders of the given term which means that all lenders will lose their stake on the given term

Even if the lenders are trying to instantly buy the auction at the midPoint and therefore prevent bad debt, depending on the current on chain activity the transaction could not get mined in the given block and therefore bad debt occurs anyway.

The following POC can be implemented in the AuctionHouse.t.sol test file:

// import OZ contracts
import '@openzeppelin/contracts/token/ERC20/extensions/ERC20Burnable.sol';
import '@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol';
import '@openzeppelin/contracts/token/ERC20/ERC20.sol';

// import ERC20 contract avove the AuctionHouseUnitTest
contract MockCollateralERC20 is ERC20, ERC20Permit, ERC20Burnable {
  constructor() ERC20('MockToken', 'MCT') ERC20Permit('MockToken') {}

  uint8 internal _decimals = 18;

  function decimals() public view virtual override returns (uint8) {
    return _decimals;
  }

  function setDecimals(uint8 dec) public {
    _decimals = dec;
  }

  function mint(address account, uint256 amount) public returns (bool) {
    _mint(account, amount);
    return true;
  }

  function mockBurn(address account, uint256 amount) public returns (bool) {
    _burn(account, amount);
    return true;
  }

  function approveOverride(address owner, address spender, uint256 amount) public {
    _approve(owner, spender, amount);
  }

  // mock governance features of ERC20Votes
  mapping(address => uint256) public _votes;

  function getPastVotes(address account, uint256 /* blockNumber*/) external view virtual returns (uint256) {
    return _votes[account];
  }

  function mockSetVotes(address account, uint256 votes) external {
    _votes[account] = votes;
  }

  // blacklist logic
  mapping(address => bool) public _blacklist;

  function blackList(address account) external {
    _blacklist[account] = true;
  }

  function _beforeTokenTransfer(address from, address to, uint256) internal virtual override(ERC20) {
    require(!_blacklist[from], 'ERC20: transfer from blacklisted address');
    require(!_blacklist[to], 'ERC20: transfer to blacklisted address');
  }
}

// update the collateral token inside the AuctionHouseUnitTest and setUp function
contract AuctionHouseUnitTest is Test {
    ...
    MockCollateralERC20 collateral;
    ...

    function setUp() public {
        ...
        collateral = new MockCollateralERC20();
        ...
    }

    // implement the POC
    function testCollateralTokenBlacklist() public {
        bytes32 loanId = _setupAndCallLoan();
        vm.roll(block.number + 1);

        // borrower gets blacklisted
        collateral.blackList(address(borrower));

        // At this time, get full collateral, repay half debt
        (uint256 collateralReceived, uint256 creditAsked) = auctionHouse.getBidDetail(loanId);

        credit.mint(bidder, creditAsked);
        vm.startPrank(bidder);
        credit.approve(address(term), creditAsked);
        // it is not possible to bid till the midPoint is reached
        vm.expectRevert('ERC20: transfer to blacklisted address');
        auctionHouse.bid(loanId);
        vm.stopPrank();
    }
}

Impact

If any borrower gets blacklisted by a collateral token with a blacklist (like for example USDC), the system is forced into a dangerous situation where bad debt can occur and all lenders of the given term lose their stake.

Recommendations

Implement a two-step approach for the collateral token so that the borrower is able to withdraw the collateral instead of transferring it directly to the borrower in the repay and onBid function.

Assessed type

ERC20

@c4-bot-1 c4-bot-1 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Dec 27, 2023
c4-bot-1 added a commit that referenced this issue Dec 27, 2023
@0xSorryNotSorry
Copy link

The issue is well demonstrated, properly formatted, contains a coded POC.
Marking as HQ.

@c4-pre-sort c4-pre-sort added the high quality report This report is of especially high quality label Jan 2, 2024
@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as high quality report

@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as primary issue

@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label Jan 2, 2024
@c4-pre-sort c4-pre-sort added duplicate-1245 and removed primary issue Highest quality submission among a set of duplicates labels Jan 3, 2024
@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as duplicate of #1245

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jan 27, 2024
@c4-judge
Copy link
Contributor

Trumpero changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

Trumpero marked the issue as grade-b

@Trumpero Trumpero mentioned this issue Jan 31, 2024
@c4-judge c4-judge added grade-a and removed grade-b labels Jan 31, 2024
@c4-judge
Copy link
Contributor

Trumpero marked the issue as grade-a

@C4-Staff C4-Staff reopened this Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-1245 grade-a high quality report This report is of especially high quality QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

5 participants