Unspent WETH is not considered in `buyNftFromMarket()`

[code423n4]

Lines of code

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

Vulnerability details

Unspent WETH is not considered in buyNftFromMarket()

Impact

In the buyNftFromMarket() function, the borrower buys an NFT in order to repay and close their loan. The purchase is executed in the internal function named _execBuyNftFromMarket().

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

395:     function _execBuyNftFromMarket(
396:         address collection,
397:         uint256 tokenId,
398:         uint256 amount,
399:         uint256 useToken,
400:         address marketplace,
401:         bytes calldata tradeData
402:     ) internal {
403:         if (!registeredMarketplaces[marketplace]) {
404:             revert Errors.UnregisteredMarketplace();
405:         }
406: 
407:         uint256 balanceBefore = address(this).balance;
408: 
409:         // execute raw order on registered marketplace
410:         bool success;
411:         if (useToken == 0) {
412:             // use ETH
413:             // solhint-disable-next-line avoid-low-level-calls
414:             (success, ) = marketplace.call{value: amount}(tradeData);
415:         } else if (useToken == 1) {
416:             // use WETH
417:             weth.deposit{value: amount}();
418:             weth.approve(marketplace, amount);
419:             // solhint-disable-next-line avoid-low-level-calls
420:             (success, ) = marketplace.call(tradeData);
421:         }
422: 
423:         if (!success) {
424:             revert Errors.MartketplaceFailedToTrade();
425:         }
426: 
427:         // verify that the declared NFT is acquired and the balance decrease is correct
428:         if (IERC721(collection).ownerOf(tokenId) != address(this) || balanceBefore - address(this).balance != amount) {
429:             revert Errors.InvalidNFTBuy();
430:         }
431:     }

As we can see in the previous snippet of code, there are mainly two paths: use ETH or use WETH, which mainly depends on the specifics of the selected marketplace.

The ETH path is straightforward, the specified amount is sent as callvalue to the marketplace. If the difference doesn't match after the execution, because some ETH was refunded or something went wrong, it would cause a revert (line 428).

However, the WETH side is a bit different. Line 417 wraps the specified amount as WETH which is then approved to the marketplace. Now, if the marketplace doesn't consume the full amount, then any unused amount will still be present in the contract as WETH, causing the check in line 428 to succeed. In fact, if the intention is to use WETH (useToken == 1), the check in line 428 will always succeed as the amount parameter is transferred to the WETH contract causing balanceBefore - address(this).balance == amount to always be true.

Proof of concept

Let's say Bob wants to purchase an NFT to close his loan and submits an amount of 5 ether. The selected marketplace uses WETH so 5 ether are wrapped as WETH. The NFT is listed as 4.5 ETH and has an associated fee of 10%. The marketplace then pulls 4.95 WETH from the exchange contract. This leaves 0.05 WETH in the contract that is deducted from Bob's payback, while the check succeeds as the difference in ETH is still 5 ether.

Recommendation

The implementation can unwrap any available WETH after the execution of the marketplace operation so that it can be considered in the calculation present in the amount validation.

  function _execBuyNftFromMarket(
      address collection,
      uint256 tokenId,
      uint256 amount,
      uint256 useToken,
      address marketplace,
      bytes calldata tradeData
  ) internal {
      if (!registeredMarketplaces[marketplace]) {
          revert Errors.UnregisteredMarketplace();
      }

      uint256 balanceBefore = address(this).balance;

      // execute raw order on registered marketplace
      bool success;
      if (useToken == 0) {
          // use ETH
          // solhint-disable-next-line avoid-low-level-calls
          (success, ) = marketplace.call{value: amount}(tradeData);
      } else if (useToken == 1) {
          // use WETH
          weth.deposit{value: amount}();
          weth.approve(marketplace, amount);
          // solhint-disable-next-line avoid-low-level-calls
          (success, ) = marketplace.call(tradeData);
      }

      if (!success) {
          revert Errors.MartketplaceFailedToTrade();
      }

+     uint256 wethBalance = weth.balanceOf(address(this));
+     if (wethBalance > 0) {
+         weth.withdraw(wethBalance);
+     }

      // verify that the declared NFT is acquired and the balance decrease is correct
      if (IERC721(collection).ownerOf(tokenId) != address(this) || balanceBefore - address(this).balance != amount) {
          revert Errors.InvalidNFTBuy();
      }
  }

Assessed type

ERC20

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as duplicate of #7

[wukong-particle]

Agreed with the judge, indeed duplication.

[c4-sponsor]

wukong-particle marked the issue as sponsor confirmed

[wukong-particle]

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