Merge pull request #1320 from hypercerts-org/fix/marketplace_signatur…

…e_checks

Bugfix in signature validations marketplace

contracts/audits/marketplace/2024_05_29_signature_bug.md

@@ -0,0 +1,52 @@
+## Issue
+
+When a user has set the approvals on the marketplace, an attacker could submit a transaction signed by the attacker for
+the (partial) sale of a hypercert fraction. The attacker would receive the funds, the order taker would receive the
+fraction and the fraction owner would lose -a part of- the fraction.
+
+## Fix
+
+To mitigate this, we added a check on the split method. Specifically, a check on ownership of or approval on the
+fraction by the signer of the message by calling the HyperMinter contract at time of order execution.
+
+```solidity
+if (
+    from != IHypercert1155Token(collection).ownerOf(itemIds[0])
+        && !IHypercert1155Token(collection).isApprovedForAll(
+            IHypercert1155Token(collection).ownerOf(itemIds[0]), from)
+    ) {
+        revert OrderInvalid();
+    }
+```
+
+The vulnerability is specific to the split function call because there is no check on the `operator` or `msg.sender`
+relations/approval as common in the `transferFrom` methods. This is an artifact from changing the original design where
+only the owner or somebody allowed by the owner would be able to split. The marketplace widens the attack vector because
+anybody can operate the marketplace, compared to a trusted operator you specifically set the approval for.
+
+To validate the changes, tests have been added to the TransferManager and one on the protocol level for 721 as a sanity
+check.
+
+## Additional information
+
+### `libraries/IHypercert1155Token.sol`
+
+- Because the check required calls to the HyperMinter not specified in the protocol interface, since the
+  `approvalForAll` call is part of the 1155 spec, we've added an interface file to the marketplace contract directory.
+- As we have the custom inferface file in the marketplace, imports from the `procotol` directory have been replaced with
+  the custom interface file.
+
+### Remove unused imports
+
+- Removed unused imports from the `IHypercertToken.sol` file
+
+### Replaced `_assertMerkleTreeAssumptions`
+
+- Unrelated tests on `BatchMakerOrders` failed with the following error:
+  `[FAIL. Reason: The `vm.assume` cheatcode rejected too many inputs (65536 allowed)]`
+- Replace the assumption with bounded variables:
+
+```solidity
+    numberOrders = bound(numberOrders, 1, 2 ** MAX_CALLDATA_PROOF_LENGTH);
+    orderIndex = bound(orderIndex, 0, numberOrders - 1);
+```

contracts/src/marketplace/ExecutionManager.sol

@@ -7,7 +7,6 @@ import {OrderStructs} from "./libraries/OrderStructs.sol";
 // Interfaces
 import {IExecutionManager} from "./interfaces/IExecutionManager.sol";
 import {ICreatorFeeManager} from "./interfaces/ICreatorFeeManager.sol";
-import {IHypercertToken} from "../protocol/interfaces/IHypercertToken.sol";
 
 // Direct dependencies
 import {InheritedStrategy} from "./InheritedStrategy.sol";

contracts/src/marketplace/LooksRareProtocol.sol

@@ -16,7 +16,7 @@ import {OrderStructs} from "./libraries/OrderStructs.sol";
 
 // Interfaces
 import {ILooksRareProtocol} from "./interfaces/ILooksRareProtocol.sol";
-import {IHypercertToken} from "../protocol/interfaces/IHypercertToken.sol";
+import {IHypercert1155Token} from "./interfaces/IHypercert1155Token.sol";
 
 // Shared errors
 import {
@@ -709,7 +709,7 @@ contract LooksRareProtocol is
         returns (uint256 unitsHeldByItems)
     {
         for (uint256 i; i < itemIds.length;) {
-            unitsHeldByItems += IHypercertToken(collection).unitsOf(itemIds[i]);
+            unitsHeldByItems += IHypercert1155Token(collection).unitsOf(itemIds[i]);
 
             unchecked {
                 ++i;

contracts/src/marketplace/TransferManager.sol

@@ -11,9 +11,9 @@ import {LowLevelHypercertCaller} from "./libraries/LowLevelHypercertCaller.sol";
 
 // Interfaces and errors
 import {ITransferManager} from "./interfaces/ITransferManager.sol";
-import {AmountInvalid, LengthsInvalid} from "./errors/SharedErrors.sol";
+import {IHypercert1155Token} from "./interfaces/IHypercert1155Token.sol";
+import {AmountInvalid, LengthsInvalid, OrderInvalid} from "./errors/SharedErrors.sol";
 import {UnitAmountInvalid} from "./errors/HypercertErrors.sol";
-import {IHypercertToken} from "../protocol/interfaces/IHypercertToken.sol";
 
 // Libraries
 import {OrderStructs} from "./libraries/OrderStructs.sol";
@@ -205,13 +205,24 @@ contract TransferManager is
 
         //The new amount is the difference between the total amount and the amount being split.
         //This will underflow if the amount being split is greater than the total amount.
-        newAmounts[0] = IHypercertToken(collection).unitsOf(itemIds[0]) - amounts[0];
+        newAmounts[0] = IHypercert1155Token(collection).unitsOf(itemIds[0]) - amounts[0];
         newAmounts[1] = amounts[0];
 
-        // If the new amount is 0, then the split is will revert but the whole fraction can be transferred.
+        // If the new amount is 0, the split is will revert but the whole fraction can be transferred.
         if (newAmounts[0] == 0) {
             _executeERC1155SafeTransferFrom(collection, from, to, itemIds[0], 1);
         } else {
+            // Before splitting, the user must be the owner or approved for all.
+            // This needs to be checked because the marketplace doesn't require signer validation as other token
+            // standards handle this.
+            if (
+                from != IHypercert1155Token(collection).ownerOf(itemIds[0])
+                    && !IHypercert1155Token(collection).isApprovedForAll(
+                        IHypercert1155Token(collection).ownerOf(itemIds[0]), from
+                    )
+            ) {
+                revert OrderInvalid();
+            }
             _executeHypercertSplitFraction(collection, to, itemIds[0], newAmounts);
         }
     }

contracts/src/marketplace/TransferSelectorNFT.sol

@@ -14,9 +14,6 @@ import {CollectionTypeInvalid} from "./errors/SharedErrors.sol";
 // Enums
 import {CollectionType} from "./enums/CollectionType.sol";
 
-// Interfaces
-import {IHypercertToken} from "../protocol/interfaces/IHypercertToken.sol";
-
 /**
  * @title TransferSelectorNFT
  * @notice This contract handles the logic for transferring non-fungible items.

contracts/src/marketplace/executionStrategies/StrategyHypercertCollectionOffer.sol

@@ -2,7 +2,7 @@
 pragma solidity 0.8.17;
 
 // Interface
-import {IHypercertToken} from "@hypercerts/protocol/interfaces/IHypercertToken.sol";
+import {IHypercert1155Token} from "../interfaces/IHypercert1155Token.sol";
 
 // Libraries
 import {OrderStructs} from "../libraries/OrderStructs.sol";
@@ -74,7 +74,7 @@ contract StrategyHypercertCollectionOffer is BaseStrategy {
         // A collection order can only be executable for 1 fraction
         if (
             amounts.length != 1 || amounts[0] != 1 || itemUnitsTaker != itemUnitsMaker
-                || IHypercertToken(makerBid.collection).unitsOf(offeredItemId) != itemUnitsMaker
+                || IHypercert1155Token(makerBid.collection).unitsOf(offeredItemId) != itemUnitsMaker
         ) {
             revert OrderInvalid();
         }
@@ -117,7 +117,7 @@ contract StrategyHypercertCollectionOffer is BaseStrategy {
         // A collection order can only be executable for 1 fraction
         if (
             amounts.length != 1 || amounts[0] != 1 || itemUnitsTaker != itemUnitsMaker
-                || IHypercertToken(makerBid.collection).unitsOf(offeredItemId) != itemUnitsMaker
+                || IHypercert1155Token(makerBid.collection).unitsOf(offeredItemId) != itemUnitsMaker
         ) {
             revert OrderInvalid();
         }
@@ -193,7 +193,7 @@ contract StrategyHypercertCollectionOffer is BaseStrategy {
         // A collection order can only be executable for 1 fraction
         if (
             amounts.length != 1 || amounts[0] != 1 || itemUnitsTaker != itemUnitsMaker
-                || IHypercertToken(collection).unitsOf(offeredItemId) != itemUnitsMaker
+                || IHypercert1155Token(collection).unitsOf(offeredItemId) != itemUnitsMaker
         ) {
             revert OrderInvalid();
         }

contracts/src/marketplace/executionStrategies/StrategyHypercertDutchAuction.sol

@@ -2,7 +2,7 @@
 pragma solidity 0.8.17;
 
 // Interface
-import {IHypercertToken} from "@hypercerts/protocol/interfaces/IHypercertToken.sol";
+import {IHypercert1155Token} from "../interfaces/IHypercert1155Token.sol";
 
 // Libraries
 import {OrderStructs} from "../libraries/OrderStructs.sol";
@@ -64,7 +64,7 @@ contract StrategyHypercertDutchAuction is BaseStrategy {
 
         uint256 unitsPerItemLength = unitsPerItem.length;
         for (uint256 i; i < unitsPerItemLength;) {
-            if (IHypercertToken(makerAsk.collection).unitsOf(makerAsk.itemIds[i]) != unitsPerItem[i]) {
+            if (IHypercert1155Token(makerAsk.collection).unitsOf(makerAsk.itemIds[i]) != unitsPerItem[i]) {
                 revert OrderInvalid();
             }
             unchecked {
@@ -124,7 +124,7 @@ contract StrategyHypercertDutchAuction is BaseStrategy {
 
         for (uint256 i; i < itemIdsLength;) {
             _validateAmountNoRevert(makerAsk.amounts[i], makerAsk.collectionType);
-            if ((IHypercertToken(makerAsk.collection).unitsOf(makerAsk.itemIds[i]) != unitsPerItem[i])) {
+            if ((IHypercert1155Token(makerAsk.collection).unitsOf(makerAsk.itemIds[i]) != unitsPerItem[i])) {
                 return (isValid, OrderInvalid.selector);
             }
             unchecked {

contracts/src/marketplace/executionStrategies/StrategyHypercertFractionOffer.sol

@@ -1,9 +1,6 @@
 // SPDX-License-Identifier: MIT
 pragma solidity 0.8.17;
 
-// Interface
-import {IHypercertToken} from "@hypercerts/protocol/interfaces/IHypercertToken.sol";
-
 // Libraries
 import {OrderStructs} from "../libraries/OrderStructs.sol";
 
@@ -26,6 +23,8 @@ import {
 // Base strategy contracts
 import {BaseStrategy, IStrategy} from "./BaseStrategy.sol";
 
+import {IHypercert1155Token} from "../interfaces/IHypercert1155Token.sol";
+
 /**
  * @title StrategyHypercertFractionOffer
  * @notice This contract offers a single execution strategy for users to bid on
@@ -69,11 +68,22 @@ contract StrategyHypercertFractionOffer is BaseStrategy {
         returns (uint256 price, uint256[] memory itemIds, uint256[] memory amounts, bool isNonceInvalidated)
     {
         itemIds = makerAsk.itemIds;
+
         // A collection order can only be executable for 1 itemId but the actual quantity to fill can vary
         if (makerAsk.amounts.length != 1 || itemIds.length != 1) {
             revert LengthsInvalid();
         }
 
+        //TODO Apply to other HC strats
+        if (
+            makerAsk.signer != IHypercert1155Token(makerAsk.collection).ownerOf(itemIds[0])
+                && !IHypercert1155Token(makerAsk.collection).isApprovedForAll(
+                    IHypercert1155Token(makerAsk.collection).ownerOf(itemIds[0]), makerAsk.signer
+                )
+        ) {
+            revert OrderInvalid();
+        }
+
         // The amount to fill must be 1 because fractions are NFTs
         if (makerAsk.amounts[0] != 1) {
             revert AmountInvalid();
@@ -101,12 +111,12 @@ contract StrategyHypercertFractionOffer is BaseStrategy {
             // If the unitAmount is lower than the specified minUnitAmount to sell
             if (unitAmount < minUnitAmount) {
                 // We expect to sale to be executed only if the units held are equal to the minUnitsToKeep
-                if (IHypercertToken(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount != minUnitsToKeep) {
+                if (IHypercert1155Token(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount != minUnitsToKeep) {
                     revert OrderInvalid();
                 }
             } else {
                 // Don't allow the sale to let the units held get below the minUnitsToKeep
-                if ((IHypercertToken(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) < minUnitsToKeep) {
+                if ((IHypercert1155Token(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) < minUnitsToKeep) {
                     revert OrderInvalid();
                 }
             }
@@ -117,7 +127,7 @@ contract StrategyHypercertFractionOffer is BaseStrategy {
             }
 
             // Don't allow the sale to let the units held get below the minUnitsToKeep
-            if ((IHypercertToken(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) < minUnitsToKeep) {
+            if ((IHypercert1155Token(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) < minUnitsToKeep) {
                 revert OrderInvalid();
             }
         }
@@ -130,7 +140,8 @@ contract StrategyHypercertFractionOffer is BaseStrategy {
         price = unitAmount * pricePerUnit;
         // If the amount to fill is equal to the amount of units in the hypercert, we transfer the fraction.
         // Otherwise, we do not invalidate the nonce because it is a partial fill.
-        isNonceInvalidated = (IHypercertToken(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) == minUnitsToKeep;
+        isNonceInvalidated =
+            (IHypercert1155Token(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) == minUnitsToKeep;
     }
 
     /**
@@ -179,15 +190,15 @@ contract StrategyHypercertFractionOffer is BaseStrategy {
             if (
                 minUnitAmount > maxUnitAmount || unitAmount == 0 || unitAmount > maxUnitAmount
                     || pricePerUnit < makerAsk.price || makerAsk.price == 0
-                    || (IHypercertToken(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) < minUnitsToKeep
+                    || (IHypercert1155Token(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) < minUnitsToKeep
             ) {
                 revert OrderInvalid();
             }
         } else {
             if (
                 minUnitAmount > maxUnitAmount || unitAmount == 0 || unitAmount < minUnitAmount
                     || unitAmount > maxUnitAmount || pricePerUnit < makerAsk.price || makerAsk.price == 0
-                    || (IHypercertToken(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) < minUnitsToKeep
+                    || (IHypercert1155Token(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) < minUnitsToKeep
             ) {
                 revert OrderInvalid();
             }
@@ -201,7 +212,8 @@ contract StrategyHypercertFractionOffer is BaseStrategy {
 
         // If the amount to fill is equal to the amount of units in the hypercert, we transfer the fraction.
         // Otherwise, we do not invalidate the nonce because it is a partial fill.
-        isNonceInvalidated = (IHypercertToken(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) == minUnitsToKeep;
+        isNonceInvalidated =
+            (IHypercert1155Token(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) == minUnitsToKeep;
 
         bytes32 node = keccak256(abi.encodePacked(takerBid.recipient));
 
@@ -238,7 +250,19 @@ contract StrategyHypercertFractionOffer is BaseStrategy {
         if (
             makerAsk.amounts.length != 1 || makerAsk.itemIds.length != 1 || minUnitAmount > maxUnitAmount
                 || makerAsk.price == 0 || maxUnitAmount == 0
-                || IHypercertToken(makerAsk.collection).unitsOf(makerAsk.itemIds[0]) <= minUnitsToKeep
+                || IHypercert1155Token(makerAsk.collection).unitsOf(makerAsk.itemIds[0]) <= minUnitsToKeep
+        ) {
+            return (isValid, OrderInvalid.selector);
+        }
+
+        // Because the split call is made by the marketplace which has the makers approval, we need to check whether the
+        // signer is the owner of the hypercert
+        // This is a side-effect of the procotol design as theres no `operator` declared in the split function
+        if (
+            makerAsk.signer != IHypercert1155Token(makerAsk.collection).ownerOf(makerAsk.itemIds[0])
+                && !IHypercert1155Token(makerAsk.collection).isApprovedForAll(
+                    IHypercert1155Token(makerAsk.collection).ownerOf(makerAsk.itemIds[0]), makerAsk.signer
+                )
         ) {
             return (isValid, OrderInvalid.selector);
         }

contracts/src/marketplace/helpers/OrderValidatorV2A.sol

@@ -16,7 +16,6 @@ import {MerkleProofCalldataWithNodes} from "../libraries/OpenZeppelin/MerkleProo
 import {ICreatorFeeManager} from "../interfaces/ICreatorFeeManager.sol";
 import {IStrategy} from "../interfaces/IStrategy.sol";
 import {IRoyaltyFeeRegistry} from "../interfaces/IRoyaltyFeeRegistry.sol";
-import {IHypercertToken} from "@hypercerts/protocol/interfaces/IHypercertToken.sol";
 
 // Shared errors
 import {OrderInvalid, CollectionTypeInvalid} from "../errors/SharedErrors.sol";

contracts/src/marketplace/helpers/ProtocolHelpers.sol

@@ -10,9 +10,6 @@ import {OrderStructs} from "../libraries/OrderStructs.sol";
 // Other dependencies
 import {LooksRareProtocol} from "../LooksRareProtocol.sol";
 
-// Interfaces
-import {IHypercertToken} from "@hypercerts/protocol/interfaces/IHypercertToken.sol";
-
 /**
  * @title ProtocolHelpers
  * @notice This contract contains helper view functions for order creation.

contracts/src/marketplace/interfaces/IHypercert1155Token.sol

@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.17;
+
+interface IHypercert1155Token {
+    function splitFraction(address to, uint256 tokenID, uint256[] memory _values) external;
+
+    function unitsOf(uint256 tokenId) external view returns (uint256);
+
+    function ownerOf(uint256 tokenId) external view returns (address);
+
+    function isApprovedForAll(address account, address operator) external view returns (bool);
+}

contracts/src/marketplace/libraries/LowLevelHypercertCaller.sol

@@ -2,7 +2,7 @@
 pragma solidity 0.8.17;
 
 // Interfaces
-import {IHypercertToken} from "../../protocol/interfaces/IHypercertToken.sol";
+import {IHypercert1155Token} from "../interfaces/IHypercert1155Token.sol";
 
 /**
  * @title LowLevelHypercertCaller
@@ -27,7 +27,7 @@ contract LowLevelHypercertCaller {
             revert NotAContract();
         }
 
-        (bool status,) = collection.call(abi.encodeCall(IHypercertToken.splitFraction, (to, tokenId, amounts)));
+        (bool status,) = collection.call(abi.encodeCall(IHypercert1155Token.splitFraction, (to, tokenId, amounts)));
 
         if (!status) {
             revert HypercertSplitFractionError();