NFTXZap.sol

The NFTXZap allows users to buy, sell and redeem NFTs with ETH in a single transaction by combining a swap against the collection pool with the relevant Locker or Listings call.

The redeemFloorWithETH function and both of the sell paths enforce their slippage parameters correctly. The issue below is specific to buyNFTWithETH.

[MED] buyNFTWithETH price protection has no effect

The buyNFTWithETH function documents that _maxETHSpent will protect the buyer from a listing price being raised between their simulation and execution. Neither of the two protections in place actually achieve this.

The full msg.value is swapped into collection tokens as an exact input, and the full amount received is then passed to Listings.fillListings as the maxSpend. This means that the ceiling is always everything that the buyer has:

totalNFTs = _fillListings(_collection, _tokenIdsOut, tokensReceived);

The ETH spent is then calculated from the ETH that is left over. As the swap was an exact input for the full msg.value, there is nothing left over and ethSpent_ will always equal msg.value. The check is only comparing two values that were provided by the caller:

ethSpent_ = msg.value - _unspentInput(preQuote, preNative);

if (ethSpent_ > _maxETHSpent) revert ExcessiveInput(ethSpent_, _maxETHSpent);

If the listing costs more than the buyer expected, then this is only reflected in a smaller refund of collection tokens, which neither check looks at.

A buyer will always need to send more ETH than the exact price, as the swap is made against a moving pool price. This margin can be taken from them by front-running their transaction:

1. The lister calls modifyListings to raise their floorMultiple; or
2. Any third party calls relist against the token, including against floor items, and sets their own floorMultiple

As the fill then lands in the same block, the tax that was paid for the new listing is refunded in full, so the only cost is gas. In our tests a buyer that added a 3% buffer lost 0.04 of a collection token, and a buyer that sent 10 ETH for a listing priced at 1.5 tokens lost 8.30 tokens. The loss is only bounded by the maxFloorMultiple of the collection.

This requires the transaction to be ordered ahead of the buyer, so it is most relevant on Ethereum mainnet and less so on chains with a single sequencer. The same overpayment will also happen, without any malicious intent, if a lister happens to raise their price between the buyer's quote and execution.

Listings.fillListings already has the protection that is needed. When called directly with an accurate maxSpend, the same front-run reverts with FillCostExceedsMaxSpend. We would recommend adding a _maxTokensSpent parameter to buyNFTWithETH and passing this through:

totalNFTs = _fillListings(_collection, _tokenIdsOut, _maxTokensSpent);

The _maxETHSpent parameter should then either be removed, or the swap changed to an exact output for the collection tokens required so that any unspent ETH is refunded and ethSpent_ reflects the true cost. The code documentation in both NFTXZap.sol and INFTXZap.sol should be updated to reflect the changes.

[INFO] Tests do not cover a change in listing price

The only test for _maxETHSpent passes a value of 1 alongside a msg.value of 10 ETH, which will only revert because ethSpent_ always equals msg.value. We would recommend adding a test that raises the listing price with modifyListings between the quote and the fill.