From d65ed26622ea3c4524833ffe86c88aa10c33823b Mon Sep 17 00:00:00 2001 From: Ayush Tiwari Date: Mon, 2 Oct 2023 15:18:58 +0530 Subject: [PATCH 1/8] feat: added access control to transfer manager --- .../contracts/exchange/Exchange.sol | 15 +----------- .../transfer-manager/TransferManager.sol | 23 +++++++++++++++++-- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/packages/marketplace/contracts/exchange/Exchange.sol b/packages/marketplace/contracts/exchange/Exchange.sol index ddc5f7cc0b..411565a4b3 100644 --- a/packages/marketplace/contracts/exchange/Exchange.sol +++ b/packages/marketplace/contracts/exchange/Exchange.sol @@ -3,9 +3,7 @@ pragma solidity 0.8.21; import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; -import {AccessControlUpgradeable} from "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; import {ContextUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/ContextUpgradeable.sol"; -import {ERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol"; import {ERC2771HandlerUpgradeable} from "@sandbox-smart-contracts/dependency-metatx/contracts/ERC2771HandlerUpgradeable.sol"; import {IOrderValidator} from "../interfaces/IOrderValidator.sol"; import {IAssetMatcher} from "../interfaces/IAssetMatcher.sol"; @@ -17,7 +15,7 @@ import {ExchangeCore} from "./ExchangeCore.sol"; /// @notice Used to exchange assets, that is, tokens. /// @dev Main functions are in ExchangeCore /// @dev TransferManager is used to execute token transfers -contract Exchange is Initializable, AccessControlUpgradeable, ExchangeCore, TransferManager, ERC2771HandlerUpgradeable { +contract Exchange is Initializable, ExchangeCore, TransferManager, ERC2771HandlerUpgradeable { /// @notice role erc1776 trusted meta transaction contracts (Sand for example). /// @return hash for ERC1776_OPERATOR_ROLE bytes32 public constant ERC1776_OPERATOR_ROLE = keccak256("ERC1776_OPERATOR_ROLE"); @@ -53,7 +51,6 @@ contract Exchange is Initializable, AccessControlUpgradeable, ExchangeCore, Tran IAssetMatcher newAssetMatcher ) external initializer { __ERC2771Handler_init(newTrustedForwarder); - __AccessControl_init(); __TransferManager_init_unchained( newProtocolFeePrimary, newProtocolFeeSecondary, @@ -128,16 +125,6 @@ contract Exchange is Initializable, AccessControlUpgradeable, ExchangeCore, Tran _setDefaultFeeReceiver(newDefaultFeeReceiver); } - /** - * @dev See {IERC165-supportsInterface}. - */ - function supportsInterface( - bytes4 interfaceId - ) public view virtual override(ERC165Upgradeable, AccessControlUpgradeable) returns (bool) { - return - ERC165Upgradeable.supportsInterface(interfaceId) || AccessControlUpgradeable.supportsInterface(interfaceId); - } - function _msgSender() internal view diff --git a/packages/marketplace/contracts/transfer-manager/TransferManager.sol b/packages/marketplace/contracts/transfer-manager/TransferManager.sol index b1770906fe..e1e2a6bd29 100644 --- a/packages/marketplace/contracts/transfer-manager/TransferManager.sol +++ b/packages/marketplace/contracts/transfer-manager/TransferManager.sol @@ -3,6 +3,7 @@ pragma solidity 0.8.21; import {ERC165Upgradeable, IERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol"; +import {AccessControlUpgradeable} from "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; import {IRoyaltiesProvider} from "../interfaces/IRoyaltiesProvider.sol"; import {BpLibrary} from "../lib-bp/BpLibrary.sol"; import {IRoyaltyUGC} from "./interfaces/IRoyaltyUGC.sol"; @@ -14,11 +15,15 @@ import {LibPart} from "../lib-part/LibPart.sol"; /// @notice responsible for transferring all Assets /// @dev this manager supports different types of fees /// @dev also it supports different beneficiaries -abstract contract TransferManager is ERC165Upgradeable, ITransferManager { +abstract contract TransferManager is ERC165Upgradeable, AccessControlUpgradeable, ITransferManager { using BpLibrary for uint; bytes4 internal constant INTERFACE_ID_IROYALTYUGC = 0xa30b4db9; + /// @notice role to identify the sandbox accounts + /// @return hash for TSB_WALLET + bytes32 public constant TSB_WALLET = keccak256("TSB_WALLET"); + /// @notice fee for primary sales /// @return uint256 of primary sale fee uint256 public protocolFeePrimary; @@ -61,6 +66,7 @@ abstract contract TransferManager is ERC165Upgradeable, ITransferManager { IRoyaltiesProvider newRoyaltiesProvider ) internal onlyInitializing { __ERC165_init(); + __AccessControl_init(); _setProtocolFee(newProtocolFeePrimary, newProtocolFeeSecondary); _setRoyaltiesRegistry(newRoyaltiesProvider); _setDefaultFeeReceiver(newDefaultFeeReceiver); @@ -120,8 +126,11 @@ abstract contract TransferManager is ERC165Upgradeable, ITransferManager { /// @param paymentSide DealSide of the fee-side order /// @param nftSide DealSide of the nft-side order function doTransfersWithFees(LibDeal.DealSide memory paymentSide, LibDeal.DealSide memory nftSide) internal { - uint256 rest = paymentSide.asset.value; + if (hasRole(TSB_WALLET, paymentSide.from)) { + return; + } + uint256 rest = paymentSide.asset.value; rest = transferRoyalties( paymentSide.asset.assetType, nftSide.asset.assetType, @@ -308,5 +317,15 @@ abstract contract TransferManager is ERC165Upgradeable, ITransferManager { } } + /** + * @dev See {IERC165-supportsInterface}. + */ + function supportsInterface( + bytes4 interfaceId + ) public view virtual override(ERC165Upgradeable, AccessControlUpgradeable) returns (bool) { + return + ERC165Upgradeable.supportsInterface(interfaceId) || AccessControlUpgradeable.supportsInterface(interfaceId); + } + uint256[46] private __gap; } From 3e5d692d66929b2f5986359edc71df7ac1856e64 Mon Sep 17 00:00:00 2001 From: Ayush Tiwari Date: Mon, 2 Oct 2023 18:04:14 +0530 Subject: [PATCH 2/8] test: match order for tsb seller --- .../transfer-manager/TransferManager.sol | 76 ++++++------ .../test/exchange/Exchange.test.ts | 114 +++++++++++++++++- 2 files changed, 149 insertions(+), 41 deletions(-) diff --git a/packages/marketplace/contracts/transfer-manager/TransferManager.sol b/packages/marketplace/contracts/transfer-manager/TransferManager.sol index e1e2a6bd29..78628198d1 100644 --- a/packages/marketplace/contracts/transfer-manager/TransferManager.sol +++ b/packages/marketplace/contracts/transfer-manager/TransferManager.sol @@ -67,6 +67,7 @@ abstract contract TransferManager is ERC165Upgradeable, AccessControlUpgradeable ) internal onlyInitializing { __ERC165_init(); __AccessControl_init(); + _grantRole(DEFAULT_ADMIN_ROLE, _msgSender()); _setProtocolFee(newProtocolFeePrimary, newProtocolFeeSecondary); _setRoyaltiesRegistry(newRoyaltiesProvider); _setDefaultFeeReceiver(newDefaultFeeReceiver); @@ -126,49 +127,48 @@ abstract contract TransferManager is ERC165Upgradeable, AccessControlUpgradeable /// @param paymentSide DealSide of the fee-side order /// @param nftSide DealSide of the nft-side order function doTransfersWithFees(LibDeal.DealSide memory paymentSide, LibDeal.DealSide memory nftSide) internal { - if (hasRole(TSB_WALLET, paymentSide.from)) { - return; - } - uint256 rest = paymentSide.asset.value; - rest = transferRoyalties( - paymentSide.asset.assetType, - nftSide.asset.assetType, - nftSide.payouts, - rest, - paymentSide.asset.value, - paymentSide.from - ); - - LibPart.Part[] memory origin = new LibPart.Part[](1); - origin[0].account = payable(defaultFeeReceiver); - - bool primaryMarket = false; - // check if primary or secondary market - if ( - nftSide.asset.assetType.assetClass == LibAsset.AssetClassType.ERC1155_ASSET_CLASS || - nftSide.asset.assetType.assetClass == LibAsset.AssetClassType.ERC721_ASSET_CLASS - ) { - (address token, uint256 tokenId) = abi.decode(nftSide.asset.assetType.data, (address, uint)); - try IERC165Upgradeable(token).supportsInterface(INTERFACE_ID_IROYALTYUGC) returns (bool result) { - if (result) { - address creator = IRoyaltyUGC(token).getCreatorAddress(tokenId); - if (nftSide.from == creator) { - primaryMarket = true; + if(!hasRole(TSB_WALLET, paymentSide.from)) { + rest = transferRoyalties( + paymentSide.asset.assetType, + nftSide.asset.assetType, + nftSide.payouts, + rest, + paymentSide.asset.value, + paymentSide.from + ); + + LibPart.Part[] memory origin = new LibPart.Part[](1); + origin[0].account = payable(defaultFeeReceiver); + + bool primaryMarket = false; + + // check if primary or secondary market + if ( + nftSide.asset.assetType.assetClass == LibAsset.ERC1155_ASSET_CLASS || + nftSide.asset.assetType.assetClass == LibAsset.ERC721_ASSET_CLASS + ) { + (address token, uint256 tokenId) = abi.decode(nftSide.asset.assetType.data, (address, uint)); + try IERC165Upgradeable(token).supportsInterface(INTERFACE_ID_IROYALTYUGC) returns (bool result) { + if (result) { + address creator = IRoyaltyUGC(token).getCreatorAddress(tokenId); + if (nftSide.from == creator) { + primaryMarket = true; + } } - } - // solhint-disable-next-line no-empty-blocks - } catch {} - } + // solhint-disable-next-line no-empty-blocks + } catch {} + } - if (primaryMarket) { - origin[0].value = uint96(protocolFeePrimary); - } else { - origin[0].value = uint96(protocolFeeSecondary); - } + if (primaryMarket) { + origin[0].value = uint96(protocolFeePrimary); + } else { + origin[0].value = uint96(protocolFeeSecondary); + } - (rest, ) = transferFees(paymentSide.asset.assetType, rest, paymentSide.asset.value, origin, paymentSide.from); + (rest, ) = transferFees(paymentSide.asset.assetType, rest, paymentSide.asset.value, origin, paymentSide.from); + } transferPayouts(paymentSide.asset.assetType, rest, paymentSide.from, nftSide.payouts); } diff --git a/packages/marketplace/test/exchange/Exchange.test.ts b/packages/marketplace/test/exchange/Exchange.test.ts index 1faedd4839..641b5a89e6 100644 --- a/packages/marketplace/test/exchange/Exchange.test.ts +++ b/packages/marketplace/test/exchange/Exchange.test.ts @@ -606,7 +606,7 @@ describe('Exchange.sol', function () { ); expect(await ERC721Contract.ownerOf(1)).to.be.equal(taker.address); - // check primary market protocol fee + // check protocol fee expect( await ERC20Contract.balanceOf(defaultFeeReceiver.address) ).to.be.equal(2500000000); // 250 * 10000000000 / 10000 = 250000000 @@ -718,7 +718,7 @@ describe('Exchange.sol', function () { ); expect(await ERC721Contract.ownerOf(1)).to.be.equal(taker.address); - // check primary market protocol fee + // check protocol fee expect( await ERC20Contract.balanceOf(defaultFeeReceiver.address) ).to.be.equal(2500000000); // 250 * 10000000000 / 10000 = 250000000 @@ -825,7 +825,7 @@ describe('Exchange.sol', function () { ); expect(await ERC721WithRoyaltyV2981.ownerOf(1)).to.be.equal(taker.address); - // check primary market protocol fee + // check protocol fee expect( await ERC20Contract.balanceOf(defaultFeeReceiver.address) ).to.be.equal(2500000000); // 250 * 10000000000 / 10000 = 250000000 @@ -840,6 +840,114 @@ describe('Exchange.sol', function () { ); }); + it('should execute a complete match order without fee and royalties for TSB seller', async function () { + const { + ExchangeContractAsUser, + ExchangeContractAsDeployer, + OrderValidatorAsDeployer, + ERC20Contract, + ERC721WithRoyaltyV2981, + defaultFeeReceiver, + deployer, + user1: maker, + user2: taker, + } = await loadFixture(deployFixtures); + + await ERC721WithRoyaltyV2981.mint(maker.address, 1, [ + await FeeRecipientsData(maker.address, 10000), + ]); + + await ERC721WithRoyaltyV2981.connect(maker).approve( + await ExchangeContractAsUser.getAddress(), + 1 + ); + + // set up receiver + await ERC721WithRoyaltyV2981.setRoyaltiesReceiver(1, deployer.address); + + // grant TSB Wallet role to seller + const TSB_WALLET = + '0x1c3ffa8a78d1cdbeb9812a2ae7c540d20292531d0254f9ac1fa85e0ac44b9ad0'; // keccak256("TSB_WALLET") + await ExchangeContractAsDeployer.connect(deployer).grantRole( + TSB_WALLET, + taker.address + ); + + await ERC20Contract.mint(taker.address, 100000000000); + await ERC20Contract.connect(taker).approve( + await ExchangeContractAsUser.getAddress(), + 100000000000 + ); + + expect(await ERC721WithRoyaltyV2981.ownerOf(1)).to.be.equal(maker.address); + expect(await ERC20Contract.balanceOf(taker.address)).to.be.equal( + 100000000000 + ); + const makerAsset = await AssetERC721(ERC721WithRoyaltyV2981, 1); + const takerAsset = await AssetERC20(ERC20Contract, 100000000000); + const leftOrder = await OrderDefault( + maker, + makerAsset, + ZeroAddress, + takerAsset, + 1, + 0, + 0 + ); + const rightOrder = await OrderDefault( + taker, + takerAsset, + ZeroAddress, + makerAsset, + 1, + 0, + 0 + ); + const makerSig = await signOrder( + leftOrder, + maker, + OrderValidatorAsDeployer + ); + const takerSig = await signOrder( + rightOrder, + taker, + OrderValidatorAsDeployer + ); + + expect(await ExchangeContractAsUser.fills(hashKey(leftOrder))).to.be.equal( + 0 + ); + expect(await ExchangeContractAsUser.fills(hashKey(rightOrder))).to.be.equal( + 0 + ); + + await ExchangeContractAsUser.matchOrders( + leftOrder, + makerSig, + rightOrder, + takerSig + ); + expect(await ExchangeContractAsUser.fills(hashKey(leftOrder))).to.be.equal( + 100000000000 + ); + expect(await ExchangeContractAsUser.fills(hashKey(rightOrder))).to.be.equal( + 1 + ); + expect(await ERC721WithRoyaltyV2981.ownerOf(1)).to.be.equal(taker.address); + + // no protocol fee paid + expect( + await ERC20Contract.balanceOf(defaultFeeReceiver.address) + ).to.be.equal(0); + + // no royalties paid + expect(await ERC20Contract.balanceOf(deployer.address)).to.be.equal(0); + + expect(await ERC20Contract.balanceOf(maker.address)).to.be.equal( + 100000000000 + ); + }); + it('should execute a complete match order between ERC721 and ERC20 tokens', async function () { const { ExchangeContractAsUser, From fcfbf5b495e7ca63d669c7ff1807f7024d0b616f Mon Sep 17 00:00:00 2001 From: Ayush Tiwari Date: Mon, 2 Oct 2023 18:06:05 +0530 Subject: [PATCH 3/8] refactor: fix lint --- .../contracts/transfer-manager/TransferManager.sol | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/marketplace/contracts/transfer-manager/TransferManager.sol b/packages/marketplace/contracts/transfer-manager/TransferManager.sol index 78628198d1..222a0af4af 100644 --- a/packages/marketplace/contracts/transfer-manager/TransferManager.sol +++ b/packages/marketplace/contracts/transfer-manager/TransferManager.sol @@ -129,7 +129,7 @@ abstract contract TransferManager is ERC165Upgradeable, AccessControlUpgradeable function doTransfersWithFees(LibDeal.DealSide memory paymentSide, LibDeal.DealSide memory nftSide) internal { uint256 rest = paymentSide.asset.value; - if(!hasRole(TSB_WALLET, paymentSide.from)) { + if (!hasRole(TSB_WALLET, paymentSide.from)) { rest = transferRoyalties( paymentSide.asset.assetType, nftSide.asset.assetType, @@ -167,7 +167,13 @@ abstract contract TransferManager is ERC165Upgradeable, AccessControlUpgradeable origin[0].value = uint96(protocolFeeSecondary); } - (rest, ) = transferFees(paymentSide.asset.assetType, rest, paymentSide.asset.value, origin, paymentSide.from); + (rest, ) = transferFees( + paymentSide.asset.assetType, + rest, + paymentSide.asset.value, + origin, + paymentSide.from + ); } transferPayouts(paymentSide.asset.assetType, rest, paymentSide.from, nftSide.payouts); From 66498c1d25ba217cc0e612b48881e6ba0f426cf6 Mon Sep 17 00:00:00 2001 From: Ayush Tiwari Date: Mon, 2 Oct 2023 18:38:19 +0530 Subject: [PATCH 4/8] fix: rebase conflicts --- .../transfer-manager/TransferManager.sol | 4 +-- .../test/exchange/Exchange.test.ts | 30 ++++++++++--------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/packages/marketplace/contracts/transfer-manager/TransferManager.sol b/packages/marketplace/contracts/transfer-manager/TransferManager.sol index 222a0af4af..e0312ef35e 100644 --- a/packages/marketplace/contracts/transfer-manager/TransferManager.sol +++ b/packages/marketplace/contracts/transfer-manager/TransferManager.sol @@ -146,8 +146,8 @@ abstract contract TransferManager is ERC165Upgradeable, AccessControlUpgradeable // check if primary or secondary market if ( - nftSide.asset.assetType.assetClass == LibAsset.ERC1155_ASSET_CLASS || - nftSide.asset.assetType.assetClass == LibAsset.ERC721_ASSET_CLASS + nftSide.asset.assetType.assetClass == LibAsset.AssetClassType.ERC1155_ASSET_CLASS || + nftSide.asset.assetType.assetClass == LibAsset.AssetClassType.ERC721_ASSET_CLASS ) { (address token, uint256 tokenId) = abi.decode(nftSide.asset.assetType.data, (address, uint)); try IERC165Upgradeable(token).supportsInterface(INTERFACE_ID_IROYALTYUGC) returns (bool result) { diff --git a/packages/marketplace/test/exchange/Exchange.test.ts b/packages/marketplace/test/exchange/Exchange.test.ts index 641b5a89e6..1f7e7b923c 100644 --- a/packages/marketplace/test/exchange/Exchange.test.ts +++ b/packages/marketplace/test/exchange/Exchange.test.ts @@ -885,7 +885,7 @@ describe('Exchange.sol', function () { ); const makerAsset = await AssetERC721(ERC721WithRoyaltyV2981, 1); const takerAsset = await AssetERC20(ERC20Contract, 100000000000); - const leftOrder = await OrderDefault( + const orderLeft = await OrderDefault( maker, makerAsset, ZeroAddress, @@ -894,7 +894,7 @@ describe('Exchange.sol', function () { 0, 0 ); - const rightOrder = await OrderDefault( + const orderRight = await OrderDefault( taker, takerAsset, ZeroAddress, @@ -904,33 +904,35 @@ describe('Exchange.sol', function () { 0 ); const makerSig = await signOrder( - leftOrder, + orderLeft, maker, OrderValidatorAsDeployer ); const takerSig = await signOrder( - rightOrder, + orderRight, taker, OrderValidatorAsDeployer ); - expect(await ExchangeContractAsUser.fills(hashKey(leftOrder))).to.be.equal( + expect(await ExchangeContractAsUser.fills(hashKey(orderLeft))).to.be.equal( 0 ); - expect(await ExchangeContractAsUser.fills(hashKey(rightOrder))).to.be.equal( + expect(await ExchangeContractAsUser.fills(hashKey(orderRight))).to.be.equal( 0 ); - await ExchangeContractAsUser.matchOrders( - leftOrder, - makerSig, - rightOrder, - takerSig - ); - expect(await ExchangeContractAsUser.fills(hashKey(leftOrder))).to.be.equal( + await ExchangeContractAsUser.matchOrders([ + { + orderLeft, + signatureLeft: makerSig, + orderRight, + signatureRight: takerSig, + }, + ]); + expect(await ExchangeContractAsUser.fills(hashKey(orderLeft))).to.be.equal( 100000000000 ); - expect(await ExchangeContractAsUser.fills(hashKey(rightOrder))).to.be.equal( + expect(await ExchangeContractAsUser.fills(hashKey(orderRight))).to.be.equal( 1 ); expect(await ERC721WithRoyaltyV2981.ownerOf(1)).to.be.equal(taker.address); From e9d46e3e9b57dac0c2b31a643226f2eef02ca8bb Mon Sep 17 00:00:00 2001 From: Ayush Tiwari Date: Tue, 3 Oct 2023 13:53:05 +0530 Subject: [PATCH 5/8] feat: added _skipFees function --- .../contracts/exchange/Exchange.sol | 30 ++++++++++++++++++- .../contracts/mocks/SimpleTest.sol | 2 ++ .../transfer-manager/TransferManager.sol | 21 ++----------- .../test/exchange/Exchange.test.ts | 11 +++---- 4 files changed, 40 insertions(+), 24 deletions(-) diff --git a/packages/marketplace/contracts/exchange/Exchange.sol b/packages/marketplace/contracts/exchange/Exchange.sol index 411565a4b3..ce9a63598c 100644 --- a/packages/marketplace/contracts/exchange/Exchange.sol +++ b/packages/marketplace/contracts/exchange/Exchange.sol @@ -2,6 +2,8 @@ pragma solidity 0.8.21; +import {ERC165Upgradeable, IERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol"; +import {AccessControlUpgradeable} from "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; import {ContextUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/ContextUpgradeable.sol"; import {ERC2771HandlerUpgradeable} from "@sandbox-smart-contracts/dependency-metatx/contracts/ERC2771HandlerUpgradeable.sol"; @@ -15,7 +17,14 @@ import {ExchangeCore} from "./ExchangeCore.sol"; /// @notice Used to exchange assets, that is, tokens. /// @dev Main functions are in ExchangeCore /// @dev TransferManager is used to execute token transfers -contract Exchange is Initializable, ExchangeCore, TransferManager, ERC2771HandlerUpgradeable { +contract Exchange is + Initializable, + ERC165Upgradeable, + AccessControlUpgradeable, + ExchangeCore, + TransferManager, + ERC2771HandlerUpgradeable +{ /// @notice role erc1776 trusted meta transaction contracts (Sand for example). /// @return hash for ERC1776_OPERATOR_ROLE bytes32 public constant ERC1776_OPERATOR_ROLE = keccak256("ERC1776_OPERATOR_ROLE"); @@ -24,6 +33,10 @@ contract Exchange is Initializable, ExchangeCore, TransferManager, ERC2771Handle /// @return hash for EXCHANGE_ADMIN_ROLE bytes32 public constant EXCHANGE_ADMIN_ROLE = keccak256("EXCHANGE_ADMIN_ROLE"); + /// @notice role to identify the sandbox accounts + /// @return hash for SKIP_FEES_ROLE + bytes32 public constant SKIP_FEES_ROLE = keccak256("SKIP_FEES_ROLE"); + /// @dev this protects the implementation contract from being initialized. /// @custom:oz-upgrades-unsafe-allow constructor constructor() { @@ -58,6 +71,7 @@ contract Exchange is Initializable, ExchangeCore, TransferManager, ERC2771Handle newRoyaltiesProvider ); __ExchangeCoreInitialize(orderValidatorAddress, newAssetMatcher); + __AccessControl_init(); _grantRole(DEFAULT_ADMIN_ROLE, admin); } @@ -125,6 +139,10 @@ contract Exchange is Initializable, ExchangeCore, TransferManager, ERC2771Handle _setDefaultFeeReceiver(newDefaultFeeReceiver); } + function _skipFees(address from) internal view override returns (bool) { + return !hasRole(SKIP_FEES_ROLE, from); + } + function _msgSender() internal view @@ -138,4 +156,14 @@ contract Exchange is Initializable, ExchangeCore, TransferManager, ERC2771Handle function _msgData() internal view override(ContextUpgradeable, ERC2771HandlerUpgradeable) returns (bytes calldata) { return ERC2771HandlerUpgradeable._msgData(); } + + /** + * @dev See {IERC165-supportsInterface}. + */ + function supportsInterface( + bytes4 interfaceId + ) public view virtual override(ERC165Upgradeable, AccessControlUpgradeable) returns (bool) { + return + ERC165Upgradeable.supportsInterface(interfaceId) || AccessControlUpgradeable.supportsInterface(interfaceId); + } } diff --git a/packages/marketplace/contracts/mocks/SimpleTest.sol b/packages/marketplace/contracts/mocks/SimpleTest.sol index bf58cfada1..7a51dbf6a1 100644 --- a/packages/marketplace/contracts/mocks/SimpleTest.sol +++ b/packages/marketplace/contracts/mocks/SimpleTest.sol @@ -10,4 +10,6 @@ contract SimpleTest is TransferManager, TransferExecutor { function getRoyaltiesByAssetTest(LibAsset.AssetType memory matchNft) external returns (LibPart.Part[] memory) { return getRoyaltiesByAssetType(matchNft); } + + function _skipFees(address from) internal virtual override returns (bool) {} } diff --git a/packages/marketplace/contracts/transfer-manager/TransferManager.sol b/packages/marketplace/contracts/transfer-manager/TransferManager.sol index e0312ef35e..ba2b19707b 100644 --- a/packages/marketplace/contracts/transfer-manager/TransferManager.sol +++ b/packages/marketplace/contracts/transfer-manager/TransferManager.sol @@ -3,7 +3,6 @@ pragma solidity 0.8.21; import {ERC165Upgradeable, IERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol"; -import {AccessControlUpgradeable} from "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; import {IRoyaltiesProvider} from "../interfaces/IRoyaltiesProvider.sol"; import {BpLibrary} from "../lib-bp/BpLibrary.sol"; import {IRoyaltyUGC} from "./interfaces/IRoyaltyUGC.sol"; @@ -15,15 +14,11 @@ import {LibPart} from "../lib-part/LibPart.sol"; /// @notice responsible for transferring all Assets /// @dev this manager supports different types of fees /// @dev also it supports different beneficiaries -abstract contract TransferManager is ERC165Upgradeable, AccessControlUpgradeable, ITransferManager { +abstract contract TransferManager is ERC165Upgradeable, ITransferManager { using BpLibrary for uint; bytes4 internal constant INTERFACE_ID_IROYALTYUGC = 0xa30b4db9; - /// @notice role to identify the sandbox accounts - /// @return hash for TSB_WALLET - bytes32 public constant TSB_WALLET = keccak256("TSB_WALLET"); - /// @notice fee for primary sales /// @return uint256 of primary sale fee uint256 public protocolFeePrimary; @@ -66,8 +61,6 @@ abstract contract TransferManager is ERC165Upgradeable, AccessControlUpgradeable IRoyaltiesProvider newRoyaltiesProvider ) internal onlyInitializing { __ERC165_init(); - __AccessControl_init(); - _grantRole(DEFAULT_ADMIN_ROLE, _msgSender()); _setProtocolFee(newProtocolFeePrimary, newProtocolFeeSecondary); _setRoyaltiesRegistry(newRoyaltiesProvider); _setDefaultFeeReceiver(newDefaultFeeReceiver); @@ -129,7 +122,7 @@ abstract contract TransferManager is ERC165Upgradeable, AccessControlUpgradeable function doTransfersWithFees(LibDeal.DealSide memory paymentSide, LibDeal.DealSide memory nftSide) internal { uint256 rest = paymentSide.asset.value; - if (!hasRole(TSB_WALLET, paymentSide.from)) { + if (_skipFees(paymentSide.from)) { rest = transferRoyalties( paymentSide.asset.assetType, nftSide.asset.assetType, @@ -323,15 +316,7 @@ abstract contract TransferManager is ERC165Upgradeable, AccessControlUpgradeable } } - /** - * @dev See {IERC165-supportsInterface}. - */ - function supportsInterface( - bytes4 interfaceId - ) public view virtual override(ERC165Upgradeable, AccessControlUpgradeable) returns (bool) { - return - ERC165Upgradeable.supportsInterface(interfaceId) || AccessControlUpgradeable.supportsInterface(interfaceId); - } + function _skipFees(address from) internal virtual returns (bool); uint256[46] private __gap; } diff --git a/packages/marketplace/test/exchange/Exchange.test.ts b/packages/marketplace/test/exchange/Exchange.test.ts index 1f7e7b923c..58776bb4d6 100644 --- a/packages/marketplace/test/exchange/Exchange.test.ts +++ b/packages/marketplace/test/exchange/Exchange.test.ts @@ -849,6 +849,7 @@ describe('Exchange.sol', function () { ERC721WithRoyaltyV2981, defaultFeeReceiver, deployer, + admin, user1: maker, user2: taker, } = await loadFixture(deployFixtures); @@ -865,11 +866,11 @@ describe('Exchange.sol', function () { // set up receiver await ERC721WithRoyaltyV2981.setRoyaltiesReceiver(1, deployer.address); - // grant TSB Wallet role to seller - const TSB_WALLET = - '0x1c3ffa8a78d1cdbeb9812a2ae7c540d20292531d0254f9ac1fa85e0ac44b9ad0'; // keccak256("TSB_WALLET") - await ExchangeContractAsDeployer.connect(deployer).grantRole( - TSB_WALLET, + // grant Skip Fees role to seller + const SKIP_FEES_ROLE = + '0x9c14d84aa0a4264a5c33560cb08e43e3ed227d0565fa3d19078d0f01304516eb'; // keccak256("SKIP_FEES_ROLE") + await ExchangeContractAsDeployer.connect(admin).grantRole( + SKIP_FEES_ROLE, taker.address ); From fe9d1be4f6f6f7cc28bb8ab6cfc56bd4b189d012 Mon Sep 17 00:00:00 2001 From: Ayush Tiwari Date: Tue, 3 Oct 2023 16:26:16 +0530 Subject: [PATCH 6/8] fix: function ordering --- .../contracts/exchange/Exchange.sol | 41 +++++++------------ .../contracts/mocks/SimpleTest.sol | 2 +- .../transfer-manager/TransferManager.sol | 4 +- .../test/exchange/Exchange.test.ts | 10 ++--- 4 files changed, 23 insertions(+), 34 deletions(-) diff --git a/packages/marketplace/contracts/exchange/Exchange.sol b/packages/marketplace/contracts/exchange/Exchange.sol index ce9a63598c..ae2df77fa7 100644 --- a/packages/marketplace/contracts/exchange/Exchange.sol +++ b/packages/marketplace/contracts/exchange/Exchange.sol @@ -2,10 +2,10 @@ pragma solidity 0.8.21; -import {ERC165Upgradeable, IERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol"; import {AccessControlUpgradeable} from "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; import {ContextUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/ContextUpgradeable.sol"; +import {ERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol"; import {ERC2771HandlerUpgradeable} from "@sandbox-smart-contracts/dependency-metatx/contracts/ERC2771HandlerUpgradeable.sol"; import {IOrderValidator} from "../interfaces/IOrderValidator.sol"; import {IAssetMatcher} from "../interfaces/IAssetMatcher.sol"; @@ -17,14 +17,7 @@ import {ExchangeCore} from "./ExchangeCore.sol"; /// @notice Used to exchange assets, that is, tokens. /// @dev Main functions are in ExchangeCore /// @dev TransferManager is used to execute token transfers -contract Exchange is - Initializable, - ERC165Upgradeable, - AccessControlUpgradeable, - ExchangeCore, - TransferManager, - ERC2771HandlerUpgradeable -{ +contract Exchange is Initializable, AccessControlUpgradeable, ExchangeCore, TransferManager, ERC2771HandlerUpgradeable { /// @notice role erc1776 trusted meta transaction contracts (Sand for example). /// @return hash for ERC1776_OPERATOR_ROLE bytes32 public constant ERC1776_OPERATOR_ROLE = keccak256("ERC1776_OPERATOR_ROLE"); @@ -33,10 +26,6 @@ contract Exchange is /// @return hash for EXCHANGE_ADMIN_ROLE bytes32 public constant EXCHANGE_ADMIN_ROLE = keccak256("EXCHANGE_ADMIN_ROLE"); - /// @notice role to identify the sandbox accounts - /// @return hash for SKIP_FEES_ROLE - bytes32 public constant SKIP_FEES_ROLE = keccak256("SKIP_FEES_ROLE"); - /// @dev this protects the implementation contract from being initialized. /// @custom:oz-upgrades-unsafe-allow constructor constructor() { @@ -64,6 +53,7 @@ contract Exchange is IAssetMatcher newAssetMatcher ) external initializer { __ERC2771Handler_init(newTrustedForwarder); + __AccessControl_init(); __TransferManager_init_unchained( newProtocolFeePrimary, newProtocolFeeSecondary, @@ -71,7 +61,6 @@ contract Exchange is newRoyaltiesProvider ); __ExchangeCoreInitialize(orderValidatorAddress, newAssetMatcher); - __AccessControl_init(); _grantRole(DEFAULT_ADMIN_ROLE, admin); } @@ -139,8 +128,18 @@ contract Exchange is _setDefaultFeeReceiver(newDefaultFeeReceiver); } - function _skipFees(address from) internal view override returns (bool) { - return !hasRole(SKIP_FEES_ROLE, from); + /** + * @dev See {IERC165-supportsInterface}. + */ + function supportsInterface( + bytes4 interfaceId + ) public view virtual override(ERC165Upgradeable, AccessControlUpgradeable) returns (bool) { + return + ERC165Upgradeable.supportsInterface(interfaceId) || AccessControlUpgradeable.supportsInterface(interfaceId); + } + + function _applyFees(address from) internal view override returns (bool) { + return !hasRole(EXCHANGE_ADMIN_ROLE, from); } function _msgSender() @@ -156,14 +155,4 @@ contract Exchange is function _msgData() internal view override(ContextUpgradeable, ERC2771HandlerUpgradeable) returns (bytes calldata) { return ERC2771HandlerUpgradeable._msgData(); } - - /** - * @dev See {IERC165-supportsInterface}. - */ - function supportsInterface( - bytes4 interfaceId - ) public view virtual override(ERC165Upgradeable, AccessControlUpgradeable) returns (bool) { - return - ERC165Upgradeable.supportsInterface(interfaceId) || AccessControlUpgradeable.supportsInterface(interfaceId); - } } diff --git a/packages/marketplace/contracts/mocks/SimpleTest.sol b/packages/marketplace/contracts/mocks/SimpleTest.sol index 7a51dbf6a1..bcce885051 100644 --- a/packages/marketplace/contracts/mocks/SimpleTest.sol +++ b/packages/marketplace/contracts/mocks/SimpleTest.sol @@ -11,5 +11,5 @@ contract SimpleTest is TransferManager, TransferExecutor { return getRoyaltiesByAssetType(matchNft); } - function _skipFees(address from) internal virtual override returns (bool) {} + function _applyFees(address from) internal virtual override returns (bool) {} } diff --git a/packages/marketplace/contracts/transfer-manager/TransferManager.sol b/packages/marketplace/contracts/transfer-manager/TransferManager.sol index ba2b19707b..7beeb26800 100644 --- a/packages/marketplace/contracts/transfer-manager/TransferManager.sol +++ b/packages/marketplace/contracts/transfer-manager/TransferManager.sol @@ -122,7 +122,7 @@ abstract contract TransferManager is ERC165Upgradeable, ITransferManager { function doTransfersWithFees(LibDeal.DealSide memory paymentSide, LibDeal.DealSide memory nftSide) internal { uint256 rest = paymentSide.asset.value; - if (_skipFees(paymentSide.from)) { + if (_applyFees(paymentSide.from)) { rest = transferRoyalties( paymentSide.asset.assetType, nftSide.asset.assetType, @@ -316,7 +316,7 @@ abstract contract TransferManager is ERC165Upgradeable, ITransferManager { } } - function _skipFees(address from) internal virtual returns (bool); + function _applyFees(address from) internal virtual returns (bool); uint256[46] private __gap; } diff --git a/packages/marketplace/test/exchange/Exchange.test.ts b/packages/marketplace/test/exchange/Exchange.test.ts index 58776bb4d6..da9a6f5d13 100644 --- a/packages/marketplace/test/exchange/Exchange.test.ts +++ b/packages/marketplace/test/exchange/Exchange.test.ts @@ -840,7 +840,7 @@ describe('Exchange.sol', function () { ); }); - it('should execute a complete match order without fee and royalties for TSB seller', async function () { + it('should execute a complete match order without fee and royalties for privileged seller', async function () { const { ExchangeContractAsUser, ExchangeContractAsDeployer, @@ -866,11 +866,11 @@ describe('Exchange.sol', function () { // set up receiver await ERC721WithRoyaltyV2981.setRoyaltiesReceiver(1, deployer.address); - // grant Skip Fees role to seller - const SKIP_FEES_ROLE = - '0x9c14d84aa0a4264a5c33560cb08e43e3ed227d0565fa3d19078d0f01304516eb'; // keccak256("SKIP_FEES_ROLE") + // grant exchange admin role to seller + const EXCHANGE_ADMIN_ROLE = + '0x541943c4a49765b7940b4b1392c4b1f8ede6efd4e23572572987ae02e569a786'; // keccak256("EXCHANGE_ADMIN_ROLE") await ExchangeContractAsDeployer.connect(admin).grantRole( - SKIP_FEES_ROLE, + EXCHANGE_ADMIN_ROLE, taker.address ); From 510792663e69762e9fbd050b1cc68d423a1e3dd9 Mon Sep 17 00:00:00 2001 From: Ayush Tiwari Date: Tue, 3 Oct 2023 16:30:17 +0530 Subject: [PATCH 7/8] fix: import ordering --- packages/marketplace/contracts/exchange/Exchange.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/marketplace/contracts/exchange/Exchange.sol b/packages/marketplace/contracts/exchange/Exchange.sol index ae2df77fa7..c873aa33c6 100644 --- a/packages/marketplace/contracts/exchange/Exchange.sol +++ b/packages/marketplace/contracts/exchange/Exchange.sol @@ -2,8 +2,8 @@ pragma solidity 0.8.21; -import {AccessControlUpgradeable} from "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; +import {AccessControlUpgradeable} from "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; import {ContextUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/ContextUpgradeable.sol"; import {ERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol"; import {ERC2771HandlerUpgradeable} from "@sandbox-smart-contracts/dependency-metatx/contracts/ERC2771HandlerUpgradeable.sol"; From fc7041a3d043d3fc37d5faebb3f41027b7311add Mon Sep 17 00:00:00 2001 From: Ayush Tiwari Date: Tue, 3 Oct 2023 16:51:15 +0530 Subject: [PATCH 8/8] fix: lint --- packages/marketplace/contracts/mocks/SimpleTest.sol | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/marketplace/contracts/mocks/SimpleTest.sol b/packages/marketplace/contracts/mocks/SimpleTest.sol index bcce885051..309de908c5 100644 --- a/packages/marketplace/contracts/mocks/SimpleTest.sol +++ b/packages/marketplace/contracts/mocks/SimpleTest.sol @@ -11,5 +11,8 @@ contract SimpleTest is TransferManager, TransferExecutor { return getRoyaltiesByAssetType(matchNft); } - function _applyFees(address from) internal virtual override returns (bool) {} + /// @dev returning false for mock contract + function _applyFees(address /* from */) internal pure override returns (bool) { + return false; + } }