Borrowers can still close loan normally while being defaulted

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L338 https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L466 https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L544

Vulnerability details

Borrowers can still close loan normally while being defaulted

A borrower can repay a loan normally while having outstanding debt and close it causing losses to the lender.

Impact

Loans in the Particle protocol are subject to an interest rate defined by the lender. A borrower deposits a certain margin while taking a loan which is stored in the credit field of the lien. Over time interests are accrued to the lender which are then paid from the borrower's credit.

If a loan gets defaulted, i.e. the credit is not enough to account for the payable interests, the lender has the ability to call the withdrawEthWithInterest() function in order to liquidate a borrower.

https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L192-L223

192:     function withdrawEthWithInterest(Lien calldata lien, uint256 lienId) external override validateLien(lien, lienId) {
193:         if (msg.sender != lien.lender) {
194:             revert Errors.Unauthorized();
195:         }
196: 
197:         if (lien.loanStartTime == 0) {
198:             revert Errors.InactiveLoan();
199:         }
200: 
201:         uint256 payableInterest = _calculateCurrentPayableInterest(lien);
202: 
203:         // verify that the liquidation condition has met (borrower insolvent or auction concluded)
204:         if (payableInterest < lien.credit && !_auctionConcluded(lien.auctionStartTime)) {
205:             revert Errors.LiquidationHasNotReached();
206:         }
207: 
208:         // delete lien (delete first to prevent reentrancy)
209:         delete liens[lienId];
210: 
211:         // transfer ETH with interest back to lender
212:         payable(lien.lender).transfer(lien.price + payableInterest);
213: 
214:         // transfer PnL to borrower
215:         if (lien.credit > payableInterest) {
216:             payable(lien.borrower).transfer(lien.credit - payableInterest);
217:         }
218: 
219:         emit WithdrawETH(lienId);
220: 
221:         // withdraw interest from this account too
222:         _withdrawAccountInterest(payable(msg.sender));
223:     }

Line 204 checks that payable interests are at least equal to the lien's credit. If conditions are met, then line 212 transfers the lender the lien's price along with the payable interests. Notice that the implementation of the _calculateCurrentPayableInterest function caps the payable interest to the credit fields of the lien. This is needed as the lender can't really receive more ETH than the available credit portion.

https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L754-L758

754:     function _calculateCurrentPayableInterest(Lien calldata lien) internal view returns (uint256) {
755:         uint256 payableInterest = MathUtils.calculateCurrentInterest(lien.price, lien.rate, lien.loanStartTime);
756:         payableInterest = payableInterest > lien.credit ? lien.credit : payableInterest;
757:         return payableInterest;
758:     }

However, it is important to note that a borrower can close a loan normally by calling buyNftFromMarket(), repayWithNft() or refinanceLoan() even if their loan is defaulted, there's nothing guarding against this possibility. At first this may seem desirable, the borrower is actually closing the loan and repaying the lender. But there is one particular detail that may allow the borrower to not repay their loan in full. All 3 cases are similar, but let's focus on the implementation of repayWithNft():

https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L466-L515

466:     function repayWithNft(
467:         Lien calldata lien,
468:         uint256 lienId,
469:         uint256 tokenId
470:     ) external override validateLien(lien, lienId) {
471:         if (msg.sender != lien.borrower) {
472:             revert Errors.Unauthorized();
473:         }
474: 
475:         if (lien.loanStartTime == 0) {
476:             revert Errors.InactiveLoan();
477:         }
478: 
479:         // update lien (by default, the lien is open to accept new loan)
480:         /// @dev update lien before paybacks to prevent reentrancy
481:         liens[lienId] = keccak256(
482:             abi.encode(
483:                 Lien({
484:                     lender: lien.lender,
485:                     borrower: address(0),
486:                     collection: lien.collection,
487:                     tokenId: tokenId,
488:                     price: lien.price,
489:                     rate: lien.rate,
490:                     loanStartTime: 0,
491:                     credit: 0,
492:                     auctionStartTime: 0
493:                 })
494:             )
495:         );
496: 
497:         // transfer NFT to the contract
498:         /// @dev collection.setApprovalForAll should have been called by this point
499:         IERC721(lien.collection).safeTransferFrom(msg.sender, address(this), tokenId);
500: 
501:         // accure interest to lender
502:         uint256 payableInterest = _calculateCurrentPayableInterest(lien);
503:         interestAccrued[lien.lender] += payableInterest;
504: 
505:         // pay PnL to borrower
506:         // since: credit = previous sold amount + position margin - lien.price
507:         // and:   payback = previous sold amount + position margin - interest
508:         // hence: payback = credit + lien.price - interest
509:         uint256 payback = lien.credit + lien.price - payableInterest;
510:         if (payback > 0) {
511:             payable(lien.borrower).transfer(payback);
512:         }
513: 
514:         emit RepayWithNFT(lienId, tokenId);
515:     }

As we can see in the previous snippet of code, the borrower transfers the NFT back in line 499 and interests are accrued to the lender in line 502. However, we need to recall that the calculation of _calculateCurrentPayableInterest() is capped to the available credit. This means that payableInterest will be at most lien.credit and the calculation in line 509 won't overflow. The function will succeed even if the borrower hasn't repaid their debt in full.

This means that a borrower can effectively close a loan without repaying the debt, causing losses to the lender.

Note also that a borrower may intentionally take advantage of this situation and eventually front-run the lender when they try to call withdrawEthWithInterest() to liquidate the borrower.

Proof of concept

1. Alice creates a lien with a price of 5 ETH and a rate of 100%.
2. Bob takes the loan and supplies 5 ETH to cover the lien price and 0.1 ETH to cover for potential interests.
3. 10 days later, the payable interest is ~0.137 ETH.
4. Bob can repay the loan normally by supplying the NFT back. Alice will only get 0.1 ETH as the interest of the loan.

Recommendation

The borrower should not be allowed to close the loan while having outstanding debt, i.e. if the payable interests are greater than the current credit. In this situation, the borrower should first add margin by calling addCredit() to cover for the missing debt and then close the loan.

Assessed type

Other

[hansfriese]

Design choice. The loss described in the report is NOT an actual loss for the lender because the lender received the desired amount after all.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid

[romeroadrian]

Design choice. The loss described in the report is NOT an actual loss for the lender because the lender received the desired amount after all.

I have further comments here, will respectfully wait until judgement is done. But could it be re-opened so it can be reviewed by the sponsor?

[wukong-particle]

Judge is correct. This is a design choice that we made so that the players are responsible to take final actions (e.g., withdrawETH, repayWithNFT) at each major events (e.g., position closing, liquidation).

At auction conclusion time, the lender is responsible for withdrawing ETH explicitly. Otherwise, trader is allowed to exit the position at some lateral time by e.g., repaying an NFT from the market. In this exchange, no party is technically at any unexpected loss (for lender, she is either getting an NFT from the same collection back, or getting the desired ETH back).

This is similar to the L1 issue in https://github.com/code-423n4/2023-05-particle-findings/issues/11 This shouldn't be a high risk issue because the phenomenon does not deviate from the design.

But looking forward to your further comments @romeroadrian thanks!

[c4-sponsor]

wukong-particle marked the issue as disagree with severity

[c4-sponsor]

wukong-particle marked the issue as sponsor acknowledged

[romeroadrian]

Design choice. The loss described in the report is NOT an actual loss for the lender because the lender received the desired amount after all.

@hansfriese I'm not sure I understand this comment. The lender is expecting interests to be paid in full for all the time that the loan has been active, anything less should be consider a loss for the lender. Let me try to explain this better since I also believe that not only the lender can be underpaid, but also be anticipated in their right to seize the collateral.

• Let's assume that the lender has an NFT that has a market floor price of 4 ETH. The lender will obviously list this NFT in Particle for a higher price than the floor price (else he would just sell it at market rates), let's say he lists it for 5 ETH.
• Borrower takes the loan, deposits 5 ETH plus 0.1 ETH as margin.
• At some point the interests for the loan start to exceed the 0.1 margin.
• Now two things can happen here that could affect the lender:
  • As described in the issue the borrower can close the loan normally. He would return the NFT and get back this 5 ETH. The lender gets his NFT back but he will only get 0.1 ETH for the interests. Any unpaid period after the borrower has ran out of margin will be a loss for the lender.
  • This also means that the borrower can anticipate their own liquidation while being underwater. As soon as the margin isn't enough to cover the interests, the lender is able to liquidate the borrower. However, the borrower can simply anticipate the liquidation and close the loan normally, or in an extreme scenario also try to front-run the liquidation transaction (lender's call to withdrawEthWithInterest()) . This should also be considered a loss for the lender, as the lender is financially interested in seizing the collateral as it should represent more value. Following the example, if the borrower front-runs the liquidation the lender will have the NFT (4 ETH) + 0.1 ETH in interest while in the case of a liquidation he could have the 5.1 ETH.

[hansfriese]

@romeroadrian I do not agree with:

The lender is expecting interests to be paid in full for all the time that the loan has been active, anything less should be consider a loss for the lender

As long as the borrower does not add credit, there is no way for the lender to force the borrower to pay the full interest. So this is not considered a loss for the lender. The lender acknowledges this risk and set the desired price. So as long as the lender gets the desired ETH, it's not a loss for him.

Regarding your example:

As described in the issue the borrower can close the loan normally. He would return the NFT and get back this 5 ETH. The lender gets his NFT back but he will only get 0.1 ETH for the interests. Any unpaid period after the borrower has ran out of margin will be a loss for the lender. This also means that the borrower can anticipate their own liquidation while being underwater. As soon as the margin isn't enough to cover the interests, the lender is able to liquidate the borrower. However, the borrower can simply anticipate the liquidation and close the loan normally, or in an extreme scenario also try to front-run the liquidation transaction (lender's call to withdrawEthWithInterest()) . This should also be considered a loss for the lender, as the lender is financially interested in seizing the collateral as it should represent more value. Following the example, if the borrower front-runs the liquidation the lender will have the NFT (4 ETH) + 0.1 ETH in interest while in the case of a liquidation he could have the 5.1 ETH.

I believe we can assume the lender will try the liquidation as soon as the default happens. This is kind of a lender's responsibility as an actor in the protocol as the sponsor commented.

• If a liquidation succeeds, no loss for him obviously as you would agree.
• If a borrower manages to front-run that and repay, it's not a loss for the lender again because the lender received the expected interest (0.1ETH is the expected interest at that time point) while he gets his NFT back.

[romeroadrian]

@hansfriese I understand your point, let me just iterate the discussion one last time.

As long as the borrower does not add credit, there is no way for the lender to force the borrower to pay the full interest. So this is not considered a loss for the lender.

No, you are right, but the protocol could prevent the borrower from closing a loan while there are unpaid interests.

The lender acknowledges this risk and set the desired price. So as long as the lender gets the desired ETH, it's not a loss for him.

The lender just creates the lien with a rate, and has the expectation that he will paid for the time his NFT is loaned. There's no agreement between the two parties on how much credit should the borrower commit at the moment the lien is taken. I believe it's the way around, the borrower should understand the risks and the protocol should default on the lender side. I don't see how the lender would get "the desired ETH" given the borrower can simply close a loan with positive debt.

[hansfriese]

@romeroadrian

No, you are right, but the protocol could prevent the borrower from closing a loan while there are unpaid interests.

Because it's not considered a loss, this is informational.

The lender just creates the lien with a rate, and has the expectation that he will paid for the time his NFT is loaned. There's no agreement between the two parties on how much credit should the borrower commit at the moment the lien is taken. I believe it's the way around, the borrower should understand the risks and the protocol should default on the lender side. I don't see how the lender would get "the desired ETH" given the borrower can simply close a loan with positive debt.

Your statement is wrong. I recommend you check the documentation and the code again.

struct Lien {
    address lender; // NFT supplier address
    address borrower; // NFT trade executor address
    address collection; // NFT collection address
    uint256 tokenId; // NFT ID
    uint256 price; // NFT supplier's demanded sold price <- Desired ETH
    uint256 rate; // APY in bips, _BASIS_POINTS defined in MathUtils.sol
    uint256 loanStartTime; // loan start block.timestamp
    uint256 credit; // actual price + position margin - demanded price
    uint256 auctionStartTime; // auction start block.timestamp
}

Please keep in mind that the post-judging QA period was ended and I might not answer further questions.

[romeroadrian]

That's fine, you have the final call here. Thanks for taking the time to answer.