AuctionHouse.getBidDetail() - Malicious actor can intentionally slash any successful term GUILD holders.

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/AuctionHouse.sol#L118

Vulnerability details

Impact

In combination with USDT/USDC blacklist and block stuffing a malicious user can intentionally generate bad debt for successful term and slash all GUILD holders as a result.

Tokens like USDT and USDC have functions that allow them to blacklist an address. The consequence of this action is that a blacklisted user can no longer transfer or receive tokens, which will make the first phase of the auction always revert when trying to send the remaining collateral to the borrower, which will completely block the first phase.

After that when the midPoint passes, the malicious user can delay the second phase making use of block stuffing spaming the network with transactions for one or more blocks, to delay the debt repayment from active participants of the protocol. The result of this action would generate bad debt, which will slash all GUILD holders no matter if this is the best lending term out there and GUILD liquidity is worth a lot. This is huge loss of participants capital, without any real reason.

Proof of Concept

Coded PoC, which should be placed inside test/unit/loan/AuctionHouse.t.sol and executed with forge test --match-test testSlashGUILDTermBlacklistPlusBlockStuffing -vv

https://gist.github.com/NicolaMirchev/3a9d1cb926c6239493980c92136e5da8

Tools Used

Manual review

Recommended Mitigation Steps

• Remove the transfer to the borrower inside LendingTerm::onBid so this function won’t be dependant on external ERC20 logic(blacklists)
• You can introduce a state variable mapping(address -> uint256) collateralToBeRepayed or other name and a function ``withdrawRepayedLoanCollatelwhich will send the pending reward to the msg.sender` based on the mapping and decrement it.
• The change inside onBid function may look like this:

// send collateral to borrower
        if (collateralToBorrower != 0) {
-             IERC20(params.collateralToken).safeTransfer(
-               loans[loanId].borrower,
-               collateralToBorrower
-            );
+              collateralToBeRepayed(loans[loanId].borrower) += collateralToBorrower;
        }





## Assessed type

Token-Transfer

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #691

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1245

[c4-judge]

Trumpero changed the severity to QA (Quality Assurance)

[c4-judge]

Trumpero marked the issue as grade-b

[NicolaMirchev]

Hey, @Trumpero

About block stuffing:

• I think this issue is valid, because it combines other issue (blockstuffing), which is marked as valid (Auction manipulation by block stuffing and reverting on ERC-777 hooks #685 (comment)).

About Blacklisted ERC20s

• In current issue we describe that a super successful lending term, which has a lot of participants, who want to pay the debt of the malicious borrow to save their stakes, cannot be liquidated in the first phase, because of the blacklisted borrower transfer.
  Sponsor has written that it is safe, because stakers would exit when they see that bidding doesn't work as expected, but this is not enough to consider it safe, because: duration of auction is limited to 30 minutes, which means that all participants which are not active every 20 minutes will be slashed before they even notice the malicious activity

[Trumpero]

@NicolaMirchev Agree that this issue should be a dup of #685, since it also mentions the block stuffing attack in phase 2 of auction.

[c4-judge]

Trumpero removed the grade

[thebrittfactor]

For transparency, the judge requested duplicate labels, severity and grade to be updated.

[NicolaMirchev]

Hey, @Trumpero
What about the second point of my argument? Imagine being a whale with millions staked in a stable lending term. You go to sleep, or just no in front of the protocol for only 30 mins. malicious borrower cannot be liquidated, because it is always failing. When it could finally happen, term is slashed...