3.6 Improper Fund Return in PopMarket::withdrawRejectBid()

• ID: PVE-006

• Severity: High

• Likelihood: High

• Impact: High

• Target: PopMarket

• Category: Business Logic []

• CWE subcategory: CWE-841 []

Description

In Pop, there is a built-in NFT market place PopMarket that allows to buy and sell ERC721 or ERC1155 NFTs with any ERC20 or native token. In the process of analyzing the bidding logic, we notice one key function does not properly refund back the intended recipient when a bid is rejected.

To elaborate, we show below the related withdrawRejectBid() routine. This routine can be invoked when the seller (or bidder) rejects (or withdraws) the bid on an order. If the bidder involves the deposit of certain ERC20 tokens, the deposited ERC20 token needs to properly returned back to the _bid.bidder, not the msg.sender – even they are identical in the user withdrawl case. However, when it is a bid-rejecting case, the caller may not be the same as the _bid.bidder.

function withdraw Reject Bid ( uint 256 order Id, uint 256 bid I d ,
bool is Reject
) external non Reentrant {
Orders to rage _order = order[ order Id ] ; Bid storage _bid = bids [ order Id ] [ bid Id ] ;
require ( _bid . status == Bid Status.Placed , " cant process " ) ;

if ( is Reject ) {
require ( _order . seller == msg . sender , " cant process " ) ;
}  else  {
require ( _bid . bidder == msg . sender , " cant process " ) ;
}

if ( is Reject ) {
bids [ order Id ] [ bid Id ] . status = Bid Status. Rejected ;
emit Bid Rejected ( order Id , bid Id ) ;
}  else  {
bids [ order Id ] [ bid Id ] . status = Bid Status. Withdraw ;
	emit Bid Withdraw ( order Id , bid Id ) ;
	}
	
	bool is Native = _order . paymentToken == address ( 0 ) ;
	
	uint 256 total Amount = _bid . price Per NFT ∗
	( _bid . copies == 0 ? 1 : _bid . copies ) ;
	
	if ( is Native ) {
	( bool success , ) = payable ( _bid . bidder ). call { value : total Amount }(
	""
	) ;
	require ( success, " Transfer Failed " ) ;
	}  else  {
	safeTransfer Amount ( _order . paymentToken, msg . sender, total Amount ) ;
	}
}	

Recommendation

Properly return funds back to the intended recipient. Note another routine acceptBid() may be similarly improved. Fortunately, in the acceptBid() case, the caller is always the same as _order.seller. The issue has been fixed by this commit: f04d50e.