From b4a47f190410c5db7fbef5673e6ea2d2caa65827 Mon Sep 17 00:00:00 2001 From: bitbeckers Date: Wed, 29 May 2024 11:56:51 +0200 Subject: [PATCH] fix(check): update interface and move signature check --- .../src/marketplace/ExecutionManager.sol | 1 - .../src/marketplace/LooksRareProtocol.sol | 4 +- contracts/src/marketplace/TransferManager.sol | 19 +++++-- .../src/marketplace/TransferSelectorNFT.sol | 3 -- .../StrategyHypercertCollectionOffer.sol | 8 +-- .../StrategyHypercertDutchAuction.sol | 6 +-- .../StrategyHypercertFractionOffer.sol | 18 +------ .../marketplace/helpers/OrderValidatorV2A.sol | 1 - .../marketplace/helpers/ProtocolHelpers.sol | 3 -- .../interfaces/IHypercert1155Token.sol | 12 +++++ .../libraries/LowLevelHypercertCaller.sol | 4 +- .../foundry/marketplace/TransferManager.t.sol | 53 +++++++++++++++++++ 12 files changed, 92 insertions(+), 40 deletions(-) create mode 100644 contracts/src/marketplace/interfaces/IHypercert1155Token.sol diff --git a/contracts/src/marketplace/ExecutionManager.sol b/contracts/src/marketplace/ExecutionManager.sol index ab421a71..c8ccf3f8 100644 --- a/contracts/src/marketplace/ExecutionManager.sol +++ b/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"; diff --git a/contracts/src/marketplace/LooksRareProtocol.sol b/contracts/src/marketplace/LooksRareProtocol.sol index 1d6cce63..95adc6ba 100644 --- a/contracts/src/marketplace/LooksRareProtocol.sol +++ b/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; diff --git a/contracts/src/marketplace/TransferManager.sol b/contracts/src/marketplace/TransferManager.sol index 7afa9bae..254897c1 100644 --- a/contracts/src/marketplace/TransferManager.sol +++ b/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); } } diff --git a/contracts/src/marketplace/TransferSelectorNFT.sol b/contracts/src/marketplace/TransferSelectorNFT.sol index 004120e7..2af130ac 100644 --- a/contracts/src/marketplace/TransferSelectorNFT.sol +++ b/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. diff --git a/contracts/src/marketplace/executionStrategies/StrategyHypercertCollectionOffer.sol b/contracts/src/marketplace/executionStrategies/StrategyHypercertCollectionOffer.sol index a6ef859b..0cff9e8c 100644 --- a/contracts/src/marketplace/executionStrategies/StrategyHypercertCollectionOffer.sol +++ b/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(); } diff --git a/contracts/src/marketplace/executionStrategies/StrategyHypercertDutchAuction.sol b/contracts/src/marketplace/executionStrategies/StrategyHypercertDutchAuction.sol index 861c1d75..47e1be8a 100644 --- a/contracts/src/marketplace/executionStrategies/StrategyHypercertDutchAuction.sol +++ b/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 { diff --git a/contracts/src/marketplace/executionStrategies/StrategyHypercertFractionOffer.sol b/contracts/src/marketplace/executionStrategies/StrategyHypercertFractionOffer.sol index 7bf6a24b..e3c7b221 100644 --- a/contracts/src/marketplace/executionStrategies/StrategyHypercertFractionOffer.sol +++ b/contracts/src/marketplace/executionStrategies/StrategyHypercertFractionOffer.sol @@ -23,13 +23,7 @@ import { // Base strategy contracts import {BaseStrategy, IStrategy} from "./BaseStrategy.sol"; -interface IHypercert1155Token { - 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); -} +import {IHypercert1155Token} from "../interfaces/IHypercert1155Token.sol"; /** * @title StrategyHypercertFractionOffer @@ -182,16 +176,6 @@ contract StrategyHypercertFractionOffer is BaseStrategy { revert AmountInvalid(); } - //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(); - } - //units, pricePerUnit, proof[] (uint256 unitAmount, uint256 pricePerUnit, bytes32[] memory proof) = abi.decode(takerBid.additionalParameters, (uint256, uint256, bytes32[])); diff --git a/contracts/src/marketplace/helpers/OrderValidatorV2A.sol b/contracts/src/marketplace/helpers/OrderValidatorV2A.sol index 12ff6367..216aabc1 100644 --- a/contracts/src/marketplace/helpers/OrderValidatorV2A.sol +++ b/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"; diff --git a/contracts/src/marketplace/helpers/ProtocolHelpers.sol b/contracts/src/marketplace/helpers/ProtocolHelpers.sol index 6b599d89..99c37c0d 100644 --- a/contracts/src/marketplace/helpers/ProtocolHelpers.sol +++ b/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. diff --git a/contracts/src/marketplace/interfaces/IHypercert1155Token.sol b/contracts/src/marketplace/interfaces/IHypercert1155Token.sol new file mode 100644 index 00000000..4f381a78 --- /dev/null +++ b/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); +} diff --git a/contracts/src/marketplace/libraries/LowLevelHypercertCaller.sol b/contracts/src/marketplace/libraries/LowLevelHypercertCaller.sol index 38165f8e..76df4e47 100644 --- a/contracts/src/marketplace/libraries/LowLevelHypercertCaller.sol +++ b/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(); diff --git a/contracts/test/foundry/marketplace/TransferManager.t.sol b/contracts/test/foundry/marketplace/TransferManager.t.sol index 7760a511..1358f55d 100644 --- a/contracts/test/foundry/marketplace/TransferManager.t.sol +++ b/contracts/test/foundry/marketplace/TransferManager.t.sol @@ -6,6 +6,7 @@ import {IOwnableTwoSteps} from "@looksrare/contracts-libs/contracts/interfaces/I // Libraries import {OrderStructs} from "@hypercerts/marketplace/libraries/OrderStructs.sol"; +import {OrderInvalid} from "@hypercerts/marketplace/errors/SharedErrors.sol"; // Core contracts import {LooksRareProtocol} from "@hypercerts/marketplace/LooksRareProtocol.sol"; @@ -670,6 +671,58 @@ contract TransferManagerTest is ITransferManager, TestHelpers, TestParameters { transferManager.removeOperator(address(0)); } + function testCannotExecuteHypercertSplitWhenSignerNotApprovedOrOwner() public { + _allowOperator(_transferrer); + _grantApprovals(_sender); + + address anon = address(666); + _grantApprovals(anon); + + uint256 itemId = fractionId1Hypercert; + + vm.prank(_sender); + mockHypercertMinter.mintClaim(_sender, units1Hypercert, "https://example.com/1", FROM_CREATOR_ONLY); + + uint256[] memory itemIds = new uint256[](1); + itemIds[0] = itemId; + uint256[] memory amounts = new uint256[](1); + amounts[0] = units1Hypercert - 3000; + + vm.prank(_transferrer); + vm.expectRevert(OrderInvalid.selector); + transferManager.splitItemsHypercert(address(mockHypercertMinter), anon, _recipient, itemIds, amounts); + + assertEq(mockHypercertMinter.unitsOf(_sender, itemId), units1Hypercert); + assertEq(mockHypercertMinter.unitsOf(_recipient, itemId + 1), 0); + } + + function testCanExecuteHypercertSplitWhenSignerApprovedOrOwner() public { + _allowOperator(_transferrer); + _grantApprovals(_sender); + + address anon = address(666); + _grantApprovals(anon); + + vm.prank(_sender); + mockHypercertMinter.setApprovalForAll(anon, true); + + uint256 itemId = fractionId1Hypercert; + + vm.prank(_sender); + mockHypercertMinter.mintClaim(_sender, units1Hypercert, "https://example.com/1", FROM_CREATOR_ONLY); + + uint256[] memory itemIds = new uint256[](1); + itemIds[0] = itemId; + uint256[] memory amounts = new uint256[](1); + amounts[0] = units1Hypercert - 3000; + + vm.prank(_transferrer); + transferManager.splitItemsHypercert(address(mockHypercertMinter), anon, _recipient, itemIds, amounts); + + assertEq(mockHypercertMinter.unitsOf(_sender, itemId), 3000); + assertEq(mockHypercertMinter.unitsOf(_recipient, itemId + 1), units1Hypercert - 3000); + } + function _generateValidBatchTransferItems() private returns (BatchTransferItem[] memory items) { items = new ITransferManager.BatchTransferItem[](3);