Function `buyNftFromMarket()` should not be payable

[code423n4]

Lines of code

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

Vulnerability details

Function buyNftFromMarket() should not be payable

The buyNftFromMarket() function is marked as payable but fails to consider callvalue.

Impact

The buyNftFromMarket() function present in the ParticleExchange contract implements the flow in which the borrower buys an NFT in the marketplace in order to repay and close the loan.

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

338:     function buyNftFromMarket(
339:         Lien calldata lien,
340:         uint256 lienId,
341:         uint256 tokenId,
342:         uint256 amount,
343:         uint256 useToken,
344:         address marketplace,
345:         bytes calldata tradeData
346:     ) external payable override validateLien(lien, lienId) {
347:         if (msg.sender != lien.borrower) {
348:             revert Errors.Unauthorized();
349:         }
350: 
351:         if (lien.loanStartTime == 0) {
352:             revert Errors.InactiveLoan();
353:         }
354: 
355:         uint256 payableInterest = _calculateCurrentPayableInterest(lien);
356: 
357:         /// @dev cannot overspend (will revert if payback to borrower < 0)
358:         // since: credit = sold amount + position margin - lien.price
359:         // and:   payback = sold amount + position margin - bought amount - interest
360:         // hence: payback = credit + lien.price - bought amount - interest
361:         uint256 payback = lien.credit + lien.price - amount - payableInterest;
362: 
363:         // update lien (by default, the lien is open to accept new loan)
364:         /// @dev update lien before paybacks to prevent reentrancy
365:         liens[lienId] = keccak256(
366:             abi.encode(
367:                 Lien({
368:                     lender: lien.lender,
369:                     borrower: address(0),
370:                     collection: lien.collection,
371:                     tokenId: tokenId,
372:                     price: lien.price,
373:                     rate: lien.rate,
374:                     loanStartTime: 0,
375:                     credit: 0,
376:                     auctionStartTime: 0
377:                 })
378:             )
379:         );
380: 
381:         // route trade execution to marketplace
382:         _execBuyNftFromMarket(lien.collection, tokenId, amount, useToken, marketplace, tradeData);
383: 
384:         // accure interest to lender
385:         interestAccrued[lien.lender] += payableInterest;
386: 
387:         // payback PnL to borrower
388:         if (payback > 0) {
389:             payable(lien.borrower).transfer(payback);
390:         }
391: 
392:         emit BuyMarketNFT(lienId, tokenId, amount);
393:     }

The required funds to purchase the NFT are used from the contract. As we can see in line 361, the amount value (which is the purchase price) is subtracted from the borrower's quota (credit and lien price) along with the due interests (payableInterest). If the amount weren't enough this calculation would overflow.

The particular issue here is that the function is marked as payable and could potentially receive ETH, but the function doesn't consider any attached value during its implementation.

This might be caused by an initial version of the function that could receive ETH and was later iterated and changed. If the borrower needs to increase their margin they could call the addCredit() function.

We can double check this by noting that msg.value isn't taken into account in the implementation of buyNftFromMarket() or the internal function _execBuyNftFromMarket(). This means that any ETH sent to this function will be effectively lost in the contract.

Recommendation

Remove the payable modifier from the buyNftFromMarket() function.

Assessed type

Payable

[hansfriese]

More like a suggestion for improvement. Likely to downgrade to LOW. Leave open for sponsor's review for now.

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-sponsor]

wukong-particle marked the issue as sponsor confirmed

[c4-sponsor]

wukong-particle marked the issue as disagree with severity

[wukong-particle]

Agree with Judge, severity can be low.

[hansfriese]

Downgrading to LOW with the sponsor's comment and precedents in mind.

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-b

[wukong-particle]

Fixed with https://github.com/Particle-Platforms/particle-exchange-protocol/pull/7