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

A malicious borrower is able to brick the Auction when the collateralToken is ERC777 #185

Closed
c4-bot-1 opened this issue Dec 19, 2023 · 10 comments
Labels
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 duplicate-184 edited-by-warden sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-bot-1
Copy link
Contributor

c4-bot-1 commented Dec 19, 2023

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/AuctionHouse.sol#L180-L186
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L805-L808

Vulnerability details

Impact

When the collateralToken is ERC777, then a malicious borrower is able to brick the Auction and prevent the bidder from claiming/withdrawing the auctioned collateral.
Due to that, the bidder can't bid/claim/withdraw the collateral.

Proof of Concept

When bid is called, a portion of collateral will sent-back to the borrower:

LendingTerm(_lendingTerm).onBid(
    loanId,
    msg.sender,
    auctions[loanId].collateralAmount - collateralReceived, // @audit collateralToBorrower will be sent-back to borrower
    collateralReceived, // collateralToBidder
    creditAsked // creditFromBidder
);

Then it triggers onBid which sends-back that portion of collateral to the borrower:

if (collateralToBorrower != 0) {
     IERC20(params.collateralToken).safeTransfer(
          loans[loanId].borrower,
          collateralToBorrower
     );
}

safeTransfer will trigger the transfer function of collateralToken, if collateralToken is ERC777 then it triggers the tokensReceived function of borrower if the borrower is a contract.
So a malicious borrower-contract can simply write a revert() on its contract (in tokensReceived) which always reverts the transaction.
Now there is no way for bidder to bid and claim the collateral, because the transaction will be reverted by borrower-contract.

Note: There is not mentioned in README and Docs that the ERC777's are not supported for collateralToken and the sponsor confirmed it.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider correct handling of ERC777's collateral tokens.

Assessed type

DoS

@c4-bot-1 c4-bot-1 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Dec 19, 2023
c4-bot-8 added a commit that referenced this issue Dec 19, 2023
@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Jan 3, 2024
@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as sufficient quality report

@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as duplicate of #685

@c4-judge
Copy link
Contributor

Trumpero changed the severity to QA (Quality Assurance)

@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 grade-b and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Jan 27, 2024
@c4-judge
Copy link
Contributor

Trumpero marked the issue as grade-b

@c4-judge c4-judge reopened this Jan 31, 2024
@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed 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 labels Jan 31, 2024
@c4-judge
Copy link
Contributor

This previously downgraded issue has been upgraded by Trumpero

@c4-judge
Copy link
Contributor

Trumpero marked the issue as not a duplicate

@c4-judge
Copy link
Contributor

Trumpero marked the issue as duplicate of #184

@c4-judge
Copy link
Contributor

Trumpero marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Jan 31, 2024
@thebrittfactor
Copy link
Contributor

For transparency, the judge has requested for the grade-b label to be removed.

@c4-judge c4-judge removed the satisfactory satisfies C4 submission criteria; eligible for awards label Feb 3, 2024
@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Feb 3, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Feb 3, 2024

Trumpero marked the issue as unsatisfactory:
Out of scope

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 duplicate-184 edited-by-warden sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

5 participants