Failed transfers in `LendingTerm.onBid()` will lead to protocol loss

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/LendingTerm.sol#L804-L817

Vulnerability details

Impact

The LendingTerm.onBid() function requires successful transfers to both the borrower and the bidder. This mandatory requirement can lead to a loss in the protocol under several circumstances.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/LendingTerm.sol#L804-L817

Proof of Concept

Two scenarios can lead to this issue, not considering ERC-777 callbacks:

1. Borrower Blacklisted: If a borrower gets themselves blacklisted by the collateral token, they will not be able to receive tokens. In this scenario, only the edge case where elapseTime == _startTime + midPoint will succeed without a loss to the protocol.

2. Transfers Paused on Collateral Token: If a loan is called when transfers are paused on the collateral token (e.g., USDC), any bid will fail. Neither of the two transactions will succeed and the loan will have to be forgiven, leading to a loss in the protocol.

Tools Used

Manual review

Recommended Mitigation Steps

To mitigate this issue, a try-catch mechanism can be implemented around the transfer calls in the onBid function. If a transfer fails, the function should catch the error and increase the allowance accordingly so the recipient can later pull the tokens themselves. This would prevent the protocol from incurring a loss due to failed transfers.

Assessed type

Token-Transfer

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[0xSorryNotSorry]

Combining two primary issues under this one as the submission covers both;

1. Borrower is blacklisted
2. Collateral is paused

[eswak]

The forgive() path at the end of the auction is supposed to handle this situation

[c4-sponsor]

eswak (sponsor) disputed

[Trumpero]

In this case, bidding will revert in phase 1 of the auction, but it can still succeed in phase 2 to liquidate that loan. I believe this is the correct behavior to deprecate the lending term in cases of token pausing or blacklisted borrowers, which are very rare occurrences. Guild holders should avoid slashing by withdrawing gauges in phase 1 of the auction when they notice bidding is broken.

[eswak]

In this case, bidding will revert in phase 1 of the auction, but it can still succeed in phase 2 to liquidate that loan.

correct, if it's just the borrower that is blacklisted, bid can happen in phase 2. if all transfers revert the end of the auction has to be waited & then forgive() has to be used

I believe this is the correct behavior to deprecate the lending term in cases of token pausing or blacklisted borrowers, which are very rare occurrences.

either guild token holders would observe this and offboard the term, or the term can stay active if the situation is expected to resolve in the future... I don't think there is a good answer that can be enforced by code here, it would have to be a social agreement between guild holders, coordinated around a vote

Guild holders should avoid slashing by withdrawing gauges in phase 1 of the auction when they notice bidding is broken.

some will be able to decrementWeight but not too many, as otherwise the debt ceiling would decrease below issuance and the gauge weight cannot decrease anymore

[c4-sponsor]

eswak (sponsor) confirmed

[c4-sponsor]

eswak marked the issue as disagree with severity

[eswak]

The mitigation proposed is interesting and could reduce losses of bids in the 2nd phase, so on 2nd thought I think we can confirm this one & mark as QA

[c4-judge]

Trumpero changed the severity to QA (Quality Assurance)

[c4-judge]

Trumpero marked the issue as grade-a

[c4-judge]

Trumpero marked the issue as grade-c