code423n4 commented 1 year ago

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

Particle Exchange can be used to swap NFTs within the collection for free

Impact

In Particle, a borrower can take an NFT as a loan and close it using another NTF within the same collection. This is present in the different functions that can be used to repay a loan, buyNftFromMarket(), repayWithNft() or refinanceLoan(), that an arbitrary tokenId to fulfill the loan. The logic is similar in all cases, but let's use repayWithNft() as the example:

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:     }

Here we can see the loan being repaid by using any token from the collection. Line 487 updates the lien with the new tokenId while the NFT is transferred to the contract in line 499.

While the intention is to let the borrower close the short by using any NFT from the collection, this can simply be used by the borrower to execute a swap for free. The borrower is allowed to take a particular NFT and return any other NFT from the same collection. Being non-fungible assets, it is expected that these have different prices according to rarity or certain particularities that make them have different valuations. This can even be done in a "flash swap" fashion, i.e. without paying any interest and without any liquidation risk if executed from a contract that would ensure atomicity of the different calls.

The corollary of the issue is that a lender will most likely always end up owning the cheapest NFT within the collection. Arbitrage and an efficient market will ensure this happens. From the point of view of the protocol this isn't too bad, a borrower can still short an NFT of a certain collection expecting price will fall (usually market movements affect the entire collection). But this will have a really high impact on lenders, as they will most surely lose value in their assets when the intention is to offer low risk yields (quoting the documentation, "NFT suppliers can earn customizable, low-risk yields").

Proof of concept

Alice creates a lien by supplying an NFT.
Bob takes loan and withdraws NFT.
Bob sells NFT.
Bob purchases cheapest NFT from the same collection.
Bob repays loan by supplying cheap NFT.

Recommendation

The quick solution would be to just make the borrower repay the loan using the same NFT, but this goes against a design decision and will most likely cripple the functionality of the exchange. If a borrower is trying to short the NFT and sells it, it would be unfeasible to think of making the borrower repurchase the same NFT to close the loan.

Another alternative would be to impose a fixed fee while taking the loans, or a minimum stay period, to ensure at least some earnings for the lender.

A third alternative, and the one I would recommend, is to allow the lender to specify a list of token ids that could be used to repay the loan. This would safely cover the lender knowing he can only receive one the approved NFTs. So if a lender decides to create a lien with an NFT of some category, they can build a list with all the other tokens from the same collection that have the same category (i.e. similar value or price). This can be implemented efficiently by using a Merkle tree, the lien can have the Merkle root and the functions can receive the path in the tree to prove the given token id belongs to the allowed list.

Assessed type

Other

hansfriese commented 1 year ago

I believe this is a design choice. Will leave it open for the sponsor's review for now but likely to invalidate.

c4-judge commented 1 year ago

hansfriese marked the issue as unsatisfactory: Insufficient quality

romeroadrian commented 1 year ago

I believe this is a design choice. Will leave it open for the sponsor's review for now but likely to invalidate.

hey hans looks like the issue is closed, could it be re-opened so it can be reviewed by the sponsor?

wukong-particle commented 1 year ago

Judge is right, fungibility of floor NFTs is a basic assumption. This swap is by design. Knowing this, rational lender should only bring floor asset (e.g., the NFT they get from collection bid).

However, we do appreciate the suggestion. We are considering a minimum fee in the interest to pay to the lender in order to prevent not-so-meaningful flash swap.

The third suggestion about Merkle tree and specified list is really smart! We will put this implementation in our roadmap as it supports trait level trading (restrict the fungibility to trait level, across trait still non-fungible).

c4-sponsor commented 1 year ago

wukong-particle marked the issue as disagree with severity

wukong-particle commented 1 year ago

We acknowledge that this is a great suggestion (especially the third point), but it shouldn't be a high risk issue because the phenomenon does not deviate from the design.