Marketplace v3 audit macro fixes (#524)

* re-write marketplacev3 in new dynamic pattern

* update tests: direct listings

* update tests: english auctions

* update tests: offers

* update function for access control

* update storage position acc erc7201

* Use latest dynamic contracts

* Update dynamic-contracts deps

* POC test for M-1

* Fix [M-1] Native tokens can get locked when bidding or buying from the marketplace

* Fix [L-1] Everyone can send native tokens directly to the marketplace

* Fix [L-3] Cannot remove upgradability without revoking all default admins

* Fix [Q-8] Incorrect VERSION used

* Fix [Q-10] Use ERC721Holder and ERC1155Holder

* Fix [Q-11] Avoid code duplication by using _msgSender and _msgData

* Fix (code comments) [Q-12] Comply with ERC7201 standard

* Fix [G-1] Reorder struct members to save storage slots

* Fix [L-2] Missing sanity checks for PrimarySale and PlatformFee recipients

* Fix [Q-7] Adhere to naming convention for internal function

* Fix comment

* Addendum [L-1]: test that only weth can send native token to marketplace

---------

Co-authored-by: Yash <kumaryashcse@gmail.com>

Author: nkrishang

contracts/prebuilts/marketplace/IMarketplace.sol

@@ -59,21 +59,22 @@ interface IDirectListings {
      *  @param startTimestamp The UNIX timestamp at and after which NFTs can be bought from the listing.
      *  @param endTimestamp The UNIX timestamp at and after which NFTs cannot be bought from the listing.
      *  @param reserved Whether the listing is reserved to be bought from a specific set of buyers.
+     *  @param status The status of the listing (created, completed, or cancelled).
      *  @param tokenType The type of token listed (ERC-721 or ERC-1155)
      */
     struct Listing {
         uint256 listingId;
-        address listingCreator;
-        address assetContract;
         uint256 tokenId;
         uint256 quantity;
-        address currency;
         uint256 pricePerToken;
         uint128 startTimestamp;
         uint128 endTimestamp;
-        bool reserved;
+        address listingCreator;
+        address assetContract;
+        address currency;
         TokenType tokenType;
         Status status;
+        bool reserved;
     }
 
     /// @notice Emitted when a new listing is created.
@@ -268,21 +269,22 @@ interface IEnglishAuctions {
      *                      the current winning bid.
      *  @param startTimestamp The timestamp at and after which bids can be made to the auction
      *  @param endTimestamp The timestamp at and after which bids cannot be made to the auction.
+     *  @param status The status of the auction (created, completed, or cancelled).
      *  @param tokenType The type of NFTs auctioned (ERC-721 or ERC-1155)
      */
     struct Auction {
         uint256 auctionId;
-        address auctionCreator;
-        address assetContract;
         uint256 tokenId;
         uint256 quantity;
-        address currency;
         uint256 minimumBidAmount;
         uint256 buyoutBidAmount;
         uint64 timeBufferInSeconds;
         uint64 bidBufferBps;
         uint64 startTimestamp;
         uint64 endTimestamp;
+        address auctionCreator;
+        address assetContract;
+        address currency;
         TokenType tokenType;
         Status status;
     }
@@ -452,17 +454,18 @@ interface IOffers {
      *  @param currency The currency offered for the NFTs.
      *  @param totalPrice The total offer amount for the NFTs.
      *  @param expirationTimestamp The timestamp at and after which the offer cannot be accepted.
+     *  @param status The status of the offer (created, completed, or cancelled).
      *  @param tokenType The type of token (ERC-721 or ERC-1155) the offer is made for.
      */
     struct Offer {
         uint256 offerId;
-        address offeror;
-        address assetContract;
         uint256 tokenId;
         uint256 quantity;
-        address currency;
         uint256 totalPrice;
         uint256 expirationTimestamp;
+        address offeror;
+        address assetContract;
+        address currency;
         TokenType tokenType;
         Status status;
     }

contracts/prebuilts/marketplace/direct-listings/DirectListingsLogic.sol

@@ -285,6 +285,7 @@ contract DirectListingsLogic is IDirectListings, ReentrancyGuard, ERC2771Context
         if (_currency == CurrencyTransferLib.NATIVE_TOKEN) {
             require(msg.value == targetTotalPrice, "Marketplace: msg.value must exactly be the total price.");
         } else {
+            require(msg.value == 0, "Marketplace: invalid native tokens sent.");
             _validateERC20BalAndAllowance(buyer, _currency, targetTotalPrice);
         }
 

contracts/prebuilts/marketplace/english-auctions/EnglishAuctionsLogic.sol

@@ -135,6 +135,10 @@ contract EnglishAuctionsLogic is IEnglishAuctions, ReentrancyGuard, ERC2771Conte
             "Marketplace: inactive auction."
         );
         require(_bidAmount != 0, "Marketplace: Bidding with zero amount.");
+        require(
+            _targetAuction.currency == CurrencyTransferLib.NATIVE_TOKEN || msg.value == 0,
+            "Marketplace: invalid native tokens sent."
+        );
         require(
             _bidAmount <= _targetAuction.buyoutBidAmount || _targetAuction.buyoutBidAmount == 0,
             "Marketplace: Bidding above buyout price."

contracts/prebuilts/marketplace/entrypoint/MarketplaceV3.sol

@@ -16,6 +16,9 @@ pragma solidity ^0.8.0;
 import "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol";
 import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
 
+import { ERC721Holder } from "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";
+import { ERC1155Holder, ERC1155Receiver } from "@openzeppelin/contracts/token/ERC1155/utils/ERC1155Holder.sol";
+
 //  ==========  Internal imports    ==========
 import { BaseRouter, IRouter, IRouterState } from "@thirdweb-dev/dynamic-contracts/src/presets/BaseRouter.sol";
 import { ERC165 } from "../../../eip/ERC165.sol";
@@ -42,28 +45,41 @@ contract MarketplaceV3 is
     ReentrancyGuardInit,
     ERC2771ContextUpgradeable,
     RoyaltyPaymentsLogic,
-    IERC721Receiver,
-    IERC1155Receiver,
+    ERC721Holder,
+    ERC1155Holder,
     ERC165
 {
-    /// @dev Only MINTER_ROLE holders can sign off on `MintRequest`s.
+    /// @dev Only EXTENSION_ROLE holders can perform upgrades.
     bytes32 private constant EXTENSION_ROLE = keccak256("EXTENSION_ROLE");
 
     bytes32 private constant MODULE_TYPE = bytes32("MarketplaceV3");
-    uint256 private constant VERSION = 1;
+    uint256 private constant VERSION = 3;
+
+    /// @dev The address of the native token wrapper contract.
+    address private immutable nativeTokenWrapper;
 
     /*///////////////////////////////////////////////////////////////
                     Constructor + initializer logic
     //////////////////////////////////////////////////////////////*/
 
-    constructor(Extension[] memory _extensions, address _royaltyEngineAddress)
-        BaseRouter(_extensions)
-        RoyaltyPaymentsLogic(_royaltyEngineAddress)
+    /// @dev We accept constructor params as a struct to avoid `Stack too deep` errors.
+    struct MarketplaceConstructorParams {
+        Extension[] extensions;
+        address royaltyEngineAddress;
+        address nativeTokenWrapper;
+    }
+
+    constructor(MarketplaceConstructorParams memory _params)
+        BaseRouter(_params.extensions)
+        RoyaltyPaymentsLogic(_params.royaltyEngineAddress)
     {
+        nativeTokenWrapper = _params.nativeTokenWrapper;
         _disableInitializers();
     }
 
-    receive() external payable {}
+    receive() external payable {
+        assert(msg.sender == nativeTokenWrapper); // only accept ETH via fallback from the native token wrapper contract
+    }
 
     /// @dev Initializes the contract, like a constructor.
     function initialize(
@@ -88,6 +104,9 @@ contract MarketplaceV3 is
         _setupRole(EXTENSION_ROLE, _defaultAdmin);
         _setupRole(keccak256("LISTER_ROLE"), address(0));
         _setupRole(keccak256("ASSET_ROLE"), address(0));
+
+        _setupRole(EXTENSION_ROLE, _defaultAdmin);
+        _setRoleAdmin(EXTENSION_ROLE, EXTENSION_ROLE);
     }
 
     /*///////////////////////////////////////////////////////////////
@@ -108,36 +127,13 @@ contract MarketplaceV3 is
                         ERC 165 / 721 / 1155 logic
     //////////////////////////////////////////////////////////////*/
 
-    function onERC1155Received(
-        address,
-        address,
-        uint256,
-        uint256,
-        bytes memory
-    ) public virtual override returns (bytes4) {
-        return this.onERC1155Received.selector;
-    }
-
-    function onERC1155BatchReceived(
-        address,
-        address,
-        uint256[] memory,
-        uint256[] memory,
-        bytes memory
-    ) public virtual override returns (bytes4) {
-        return this.onERC1155BatchReceived.selector;
-    }
-
-    function onERC721Received(
-        address,
-        address,
-        uint256,
-        bytes calldata
-    ) external pure override returns (bytes4) {
-        return this.onERC721Received.selector;
-    }
-
-    function supportsInterface(bytes4 interfaceId) public view virtual override(ERC165, IERC165) returns (bool) {
+    function supportsInterface(bytes4 interfaceId)
+        public
+        view
+        virtual
+        override(ERC165, IERC165, ERC1155Receiver)
+        returns (bool)
+    {
         return
             interfaceId == type(IERC1155Receiver).interfaceId ||
             interfaceId == type(IERC721Receiver).interfaceId ||
@@ -177,21 +173,10 @@ contract MarketplaceV3 is
     }
 
     function _msgSender() internal view override(ERC2771ContextUpgradeable, Permissions) returns (address sender) {
-        if (isTrustedForwarder(msg.sender)) {
-            // The assembly code is more direct than the Solidity version using `abi.decode`.
-            assembly {
-                sender := shr(96, calldataload(sub(calldatasize(), 20)))
-            }
-        } else {
-            return msg.sender;
-        }
+        return ERC2771ContextUpgradeable._msgSender();
     }
 
     function _msgData() internal view override(ERC2771ContextUpgradeable, Permissions) returns (bytes calldata) {
-        if (isTrustedForwarder(msg.sender)) {
-            return msg.data[:msg.data.length - 20];
-        } else {
-            return msg.data;
-        }
+        return ERC2771ContextUpgradeable._msgData();
     }
 }

src/test/marketplace/DirectListings.t.sol

@@ -34,7 +34,9 @@ contract MarketplaceDirectListingsTest is BaseTest, IExtension {
 
         // Deploy implementation.
         Extension[] memory extensions = _setupExtensions();
-        address impl = address(new MarketplaceV3(extensions, address(0)));
+        address impl = address(
+            new MarketplaceV3(MarketplaceV3.MarketplaceConstructorParams(extensions, address(0), address(weth)))
+        );
 
         vm.prank(marketplaceDeployer);
         marketplace = address(
@@ -1865,6 +1867,38 @@ contract MarketplaceDirectListingsTest is BaseTest, IExtension {
         vm.prank(_seller);
         listingId = DirectListingsLogic(marketplace).createListing(listingParams);
     }
+
+    function test_audit_native_tokens_locked() public {
+        (uint256 listingId, IDirectListings.Listing memory existingListing) = _setup_buyFromListing();
+
+        uint256[] memory tokenIds = new uint256[](1);
+        tokenIds[0] = existingListing.tokenId;
+
+        // Verify existing auction at `auctionId`
+        assertEq(existingListing.assetContract, address(erc721));
+
+        vm.warp(existingListing.startTimestamp);
+
+        // No ether is locked in contract
+        assertEq(marketplace.balance, 0);
+
+        // buy from listing
+        erc20.mint(buyer, 10 ether);
+        vm.deal(buyer, 1 ether);
+
+        vm.prank(seller);
+        DirectListingsLogic(marketplace).approveBuyerForListing(listingId, buyer, true);
+
+        vm.startPrank(buyer);
+        erc20.approve(marketplace, 10 ether);
+
+        vm.expectRevert("Marketplace: invalid native tokens sent.");
+        DirectListingsLogic(marketplace).buyFromListing{ value: 1 ether }(listingId, buyer, 1, address(erc20), 1 ether);
+        vm.stopPrank();
+
+        // 1 ether is temporary locked in contract
+        assertEq(marketplace.balance, 0 ether);
+    }
 }
 
 contract IssueC2_MarketplaceDirectListingsTest is BaseTest, IExtension {
@@ -1885,7 +1919,9 @@ contract IssueC2_MarketplaceDirectListingsTest is BaseTest, IExtension {
 
         // Deploy implementation.
         Extension[] memory extensions = _setupExtensions();
-        address impl = address(new MarketplaceV3(extensions, address(0)));
+        address impl = address(
+            new MarketplaceV3(MarketplaceV3.MarketplaceConstructorParams(extensions, address(0), address(weth)))
+        );
 
         vm.prank(marketplaceDeployer);
         marketplace = address(