Lender can prevent borrower from returning NFT

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-particle/blob/1caf678bc20c24c96fc8f6b0046383ff0e9d2a6f/contracts/protocol/ParticleExchange.sol#L470

Vulnerability details

Impact

The lender can prevent the borrower of his NFT from returning it, forcing him to pay interest for longer.

Proof of Concept

The borrower returns the NFT owed by calling repayWithNft(lien, lienId, tokenId). This will then validateLien(lien, lienId) which checks that the lien is hashed to the same digest as is stored at liens[lienId]. The issue is that the lender can frontrun the repayWithNft() with a call to startLoanAuction() and then call stopLoanAuction(). startLoanAuction() updates the lien with a new auctionStartTime. This changes the liens[lineId] which causes the validateLien() in the call to repayWithNft() to revert. The lender then stops the auction (and there's no rush really since no one is expected to sell an NFT to the contract for the very cheap early auction price) to return to business as usual. liens[lineId] is now also back to normal and the process would have to repeat.

The same procedure can be done with addCredit(), which can be called by anyone, instead of startLoanAuction(). Only 1 wei needs to be spent. In this case liens[lineId] remains changed.

By persistently denying the borrower he will be forced to keep paying interest until his credit is consumed.

Assessed type

DoS

[c4-judge]

hansfriese marked the issue as duplicate of #16

[hansfriese]

Mitigation steps missing. Thus not chosen as a primary.

[c4-sponsor]

wukong-particle marked the issue as sponsor confirmed

[wukong-particle]

Judge is correct, indeed duplication.

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese changed the severity to 2 (Med Risk)

[d3e4]

I don't think this is a duplicate of #16.

16 is about DoS-ing lien validation by changing its hash by calling addCredit(). The mitigations would be to restrict access to the borrower or increase the minimum amount of credit one can add. My #41 is already duped to that issue, which I can agree with. What I said here about addCredit() was just a bonus and not the main issue.

The issue here in #40 is that lien validation can be broken by starting an auction. This is an entirely different lien validation DoS and cannot be mitigated by the mitigation of #16. The fact that the mitigations must be entirely different I think is evidence enough that they are separate issues. This is also why I didn't add a suggested mitigation here, because I couldn't think of one, even though I clearly had one for #16 in my #41.

To justify the High rating, note that the exploit here is to prevent the borrower from repaying his loan. This directly forces him to pay more interest. It is especially exploitable when the borrower is close to becoming insolvent. If the borrower is prevented from repaying so that he becomes insolvent, the lender can profitably liquidate him (the borrower can be DoS'd from adding credit to remain solvent separately if needed).

[wukong-particle]

I see, I think the flow is logically correct. I misread this issue with the judge earlier. But this doesn't look like falling outside the designed behavior. Note that lender can still execute buy during auction.

About mitigation, we can potentially force an opened auction to remain open for, say, 1 hour. Then lender can simply execute again under attack (borrower attack at most once every hour). Does this mitigation look reasonable to you @d3e4

---

The 1 hour restriction is added with https://github.com/Particle-Platforms/particle-exchange-protocol/pull/28

[d3e4]

I see, I think the flow is logically correct. I misread this issue with the judge earlier. But this doesn't look like falling outside the designed behaviour. Note that lender can still execute buy during auction.

The point of the issue is to cause the borrower's lien validation to fail by changing the lien data just in front of his call. If the borrower wants to try again by executing a buy during the auction, now with the new lien with an auctionStartTime, the lender can again DoS him by stopping the auction, if he hadn't already. The lender doesn't care about the auction running or not, only about changing the lien data.

About mitigation, we can potentially force an opened auction to remain open for, say, 1 hour. Then lender can simply execute again under attack (borrower attack at most once every hour). Does this mitigation look reasonable to you @d3e4

"Then lender can simply execute again under attack (borrower attack at most once every hour)." should swap "lender" and "borrower". But yes, this mitigation would prevent the lender from repeating the DoS on the borrower if he tries again within an hour. This makes the attack unviable and provides a guaranteed backup attempt for the borrower against this DoS. It also seems to make sense that all legitimate auctions would run for at least some time. A potential concern might be that a lender who starts an auction by mistake cannot immediately stop it, but since the price starts from zero there is plenty of margin already taken into account, so the first hour of the auction is effectively just a prelude during which no one is going to take the offer. The lender could in principle have set a lien price so high that the auction becomes interesting even within the first hour, but in this case he would probably not have found a borrower in the first place.

[hansfriese]

Please note that there were many DoS-related reports and I just grouped them into two.

1. High likelihood and no cost at all for the attacker. (#31)
2. Need frontrunning and changing lien data in any way to cause validateLien to fail

As a side note, I would like to mention that this report lack Recommended Mitigation steps that can lead to invalidation.

Per C4 documentation:

The validity of an audit report submission is not based on whether it is ‘true’ or not. A report may contain a finding which is factually 'true' (the most literal interpretation of 'valid'), but if it does not add value or if it is not presented in such a way that adds value to a sponsor, it may be deemed invalid by a judge. This may seem harsh and exclusive, but it is essential to consider that Code4rena runs audit contests, not gotcha-hunts, and Code4rena offers guaranteed payout for valid submissions. This means that wardens are providing a service to sponsors and the product of those services should meet what judges feel is a minimum standard in order to be deemed of value.

[d3e4]

Please note that there were many DoS-related reports and I just grouped them into two.

I can see your reasoning for grouping them based on changing the lien data. But it is important to note that changing the lien data is not an issue in itself and therefore cannot be the basis for grouping issues. It would be like saying that having funds in the contract is an issue and therefore group all issues which steal funds. The fact that changing the lien data will DoS lien validation is not going to change; what needs to be done is to make sure that this cannot be exploited. It can be exploited in several distinct ways, just like funds might be stolen in many ways. #16 and #40 are similar only in their connection to lien validation; the exposed vulnerabilities are entirely different. Please note that #16 doesn't describe a High severity exploit, but just the mechanism. #8 and #41 do however describe how this can be fully exploited. Unless the lien storage implementation is completely overhauled, fixing {#16; #8, #41} cannot fix #40; they are different ways of exploiting how liens are stored.

Even if you consider {#16; #8, #41} and #40 to be duplicates it is still a fact that #40 and #41 together are much broader in terms of root issues described and demonstrated impact than #16.

As a side note, I would like to mention that this report lack Recommended Mitigation steps that can lead to invalidation.

Revealing an issue is surely more critical than coming up with a mitigation? Would you not agree that a lack of recommended mitigation is better than a faulty one? sockdrawer and Trust think so. But note in the discussion with the sponsor above that this report did indeed lead to a mitigation (different from #16).

[hansfriese]

I can see your reasoning for grouping them based on changing the lien data. But it is important to note that changing the lien data is not an issue in itself and therefore cannot be the basis for grouping issues. It would be like saying that having funds in the contract is an issue and therefore group all issues which steal funds.

It sounds good you see the reasoning for grouping these issues although it's a pity your response is not respectful with an inappropriate generalization.

The fact that changing the lien data will DoS lien validation is not going to change; what needs to be done is to make sure that this cannot be exploited. It can be exploited in several distinct ways, just like funds might be stolen in many ways. https://github.com/code-423n4/2023-05-particle-findings/issues/16 and https://github.com/code-423n4/2023-05-particle-findings/issues/40 are similar only in their connection to lien validation; the exposed vulnerabilities are entirely different. Please note that https://github.com/code-423n4/2023-05-particle-findings/issues/16 doesn't describe a High severity exploit, but just the mechanism. https://github.com/code-423n4/2023-05-particle-findings/issues/8 and https://github.com/code-423n4/2023-05-particle-findings/issues/41 do however describe how this can be fully exploited. Unless the lien storage implementation is completely overhauled, fixing {https://github.com/code-423n4/2023-05-particle-findings/issues/16; https://github.com/code-423n4/2023-05-particle-findings/issues/8, https://github.com/code-423n4/2023-05-particle-findings/issues/41} cannot fix https://github.com/code-423n4/2023-05-particle-findings/issues/40; they are different ways of exploiting how liens are stored.

Sorry, I can't find new information here.

Even if you consider {https://github.com/code-423n4/2023-05-particle-findings/issues/16; https://github.com/code-423n4/2023-05-particle-findings/issues/8, https://github.com/code-423n4/2023-05-particle-findings/issues/41} and https://github.com/code-423n4/2023-05-particle-findings/issues/40 to be duplicates it is still a fact that https://github.com/code-423n4/2023-05-particle-findings/issues/40 and https://github.com/code-423n4/2023-05-particle-findings/issues/41 together are much broader in terms of root issues described and demonstrated impact than https://github.com/code-423n4/2023-05-particle-findings/issues/16.

What's the point of your statement "grouping two issues is much broader"? Do you mean you want your two findings including all your comments to be judged as a single report?

Revealing an issue is surely more critical than coming up with a mitigation? Would you not agree that a lack of recommended mitigation is better than a faulty one? sockdrawer and Trust think so. But note in the discussion with the sponsor above that this report did indeed lead to a mitigation (different from https://github.com/code-423n4/2023-05-particle-findings/issues/16).

Being included in the mitigation does not justify your report should be awarded. And I would like to remind you I am the judge for this contest.

Please remember the guideline.

At the point that a judge provides a response, the conversation is over, unless you wish to provide additional facts which demonstrate inaccurate assumptions in the judge's response. Further pursuit of the conversation will result in having backstage access suspended.

[d3e4]

It sounds good you see the reasoning for grouping these issues although it's a pity your response is not respectful with an inappropriate generalization.

I apologise, I did not mean to be disrespectful. I just used this as an extreme example to illustrate my point, the point of contact being something which is an accepted part of the protocol.

What's the point of your statement "grouping two issues is much broader"? Do you mean you want your two findings including all your comments to be judged as a single report?

Not my comments. My two issues #40 and #41, if considered one report (since you consider them duplicates), includes more vulnerabilities, exploits and impact than #16 and #8. #16 and #8 are a subset of what is reported in #40 and #41. I argued that #40 and #41 should be considered two separate issues, but if you think they are too similar and should be awarded as one issue then that's a judgement I cannot dispute. But in that case #16 and #8 should be considered partials of the #40/#41 issue, since they are but subsets.

Please remember the guideline.

At the point that a judge provides a response, the conversation is over, unless you wish to provide additional facts which demonstrate inaccurate assumptions in the judge's response. Further pursuit of the conversation will result in having backstage access suspended.

This is indeed about inaccurate assumptions.