Merge pull request #1117 from thesandboxgame/catalyst-audit/L-11-the-…

…catalyst-contract-allows-the-burning-and-transfer-of-non-existent-tokens

Catalyst audit: L-11 The Catalyst Contract Allows the Burning and Transfer of Non-Existent Tokens

packages/asset/contracts/Asset.sol

@@ -336,7 +336,7 @@ contract Asset is
         return TokenIdUtils.isBridged(tokenId);
     }
 
-    /// @notice This function is used to register Asset contract on the Operator Filterer Registry of Opensea.can only be called by admin.
+    /// @notice This function is used to register Asset contract on the Operator Filterer Registry of OpenSea.can only be called by admin.
     /// @dev used to register contract and subscribe to the subscriptionOrRegistrantToCopy's black list.
     /// @param subscriptionOrRegistrantToCopy registration address of the list to subscribe.
     /// @param subscribe bool to signify subscription "true"" or to copy the list "false".

packages/asset/contracts/Catalyst.sol

@@ -30,7 +30,7 @@ import {ICatalyst} from "./interfaces/ICatalyst.sol";
 
 /// @title Catalyst
 /// @author The Sandbox
-/// @notice THis contract manages catalysts which are used to mint new assets.
+/// @notice This contract manages catalysts which are used to mint new assets.
 /// @dev An ERC1155 contract that manages catalysts, extends multiple OpenZeppelin contracts to
 /// provide a variety of features including, AccessControl, URIStorage, Burnable and more.
 /// The contract includes support for meta transactions.
@@ -77,7 +77,7 @@ contract Catalyst is
         address _defaultMinter,
         string[] memory _catalystIpfsCID,
         address _royaltyManager
-    ) public initializer {
+    ) external initializer {
         require(bytes(_baseUri).length != 0, "Catalyst: base uri can't be empty");
         require(_trustedForwarder != address(0), "Catalyst: trusted forwarder can't be zero");
         require(_subscription != address(0), "Catalyst: subscription can't be zero");
@@ -235,7 +235,7 @@ contract Catalyst is
     /// @param to address to which the token will be transfered.
     /// @param id the token type transfered.
     /// @param value amount of token transfered.
-    /// @param data aditional data accompanying the transfer.
+    /// @param data additional data accompanying the transfer.
     function safeTransferFrom(
         address from,
         address to,
@@ -252,7 +252,7 @@ contract Catalyst is
     /// @param to address to which the token will be transfered.
     /// @param ids ids of each token type transfered.
     /// @param values amount of each token type transfered.
-    /// @param data aditional data accompanying the transfer.
+    /// @param data additional data accompanying the transfer.
     function safeBatchTransferFrom(
         address from,
         address to,
@@ -293,7 +293,7 @@ contract Catalyst is
         return super.supportsInterface(interfaceId);
     }
 
-    /// @notice This function is used to register Catalyst contract on the Operator Filterer Registry of Opensea.can only be called by admin.
+    /// @notice This function is used to register Catalyst contract on the Operator Filterer Registry of OpenSea. Can only be called by admin.
     /// @dev used to register contract and subscribe to the subscriptionOrRegistrantToCopy's black list.
     /// @param subscriptionOrRegistrantToCopy registration address of the list to subscribe.
     /// @param subscribe bool to signify subscription "true"" or to copy the list "false".

packages/asset/docs/Asset.md

@@ -176,14 +176,15 @@ Sets a new trusted forwarder for meta-transactions.
 Parameters:
 
 - `trustedForwarder` - The new trusted forwarder.
+
 ### setTokenRoyalties
 
 ```solidity
 function setTokenRoyalties(
         uint256 tokenId,
         address payable recipient,
         address creator
-) external override onlyRole(DEFAULT_ADMIN_ROLE) 
+) external override onlyRole(DEFAULT_ADMIN_ROLE)
 ```
 
 Sets token royalty i.e. the creator splitter address as EIP2981 royalty recipient. deploys a splitter if there is none deployed for a creator. Only admin can call it.
@@ -254,7 +255,6 @@ Parameters:
 
 - `tokenId` - the id of the token.
 
-
 ### isBridged
 
 ```solidity
@@ -275,7 +275,7 @@ function registerAndSubscribe(address subscriptionOrRegistrantToCopy, bool subsc
     onlyRole(DEFAULT_ADMIN_ROLE)
 ```
 
-Used to register and subscribe on the operator filter registry of Opensea
+Used to register and subscribe on the operator filter registry of OpenSea
 
 Parameters:
 
@@ -286,7 +286,7 @@ Parameters:
 
 ```solidity
 function setOperatorRegistry(address registry) external onlyRole(DEFAULT_ADMIN_ROLE)
-```fu
+```
 
 Used to the address of the operator filter registry
 

packages/asset/test/AssetRoyalty.test.ts

@@ -39,7 +39,7 @@ describe('Asset Royalties', function () {
         seller.address,
         true
       );
-      const splitter = await RoyaltyManagerContract._creatorRoyaltiesSplitter(
+      const splitter = await RoyaltyManagerContract.creatorRoyaltiesSplitter(
         creator.address
       );
 
@@ -103,7 +103,7 @@ describe('Asset Royalties', function () {
         seller.address,
         true
       );
-      const splitter = await RoyaltyManagerContract._creatorRoyaltiesSplitter(
+      const splitter = await RoyaltyManagerContract.creatorRoyaltiesSplitter(
         creator.address
       );
 
@@ -170,7 +170,7 @@ describe('Asset Royalties', function () {
         seller.address,
         true
       );
-      const splitter = await RoyaltyManagerContract._creatorRoyaltiesSplitter(
+      const splitter = await RoyaltyManagerContract.creatorRoyaltiesSplitter(
         creator.address
       );
 
@@ -416,7 +416,7 @@ describe('Asset Royalties', function () {
       const id = generateAssetId(creator.address, 1);
       await assetAsMinter.mint(seller.address, id, 1, '0x');
 
-      const splitter = await RoyaltyManagerContract._creatorRoyaltiesSplitter(
+      const splitter = await RoyaltyManagerContract.creatorRoyaltiesSplitter(
         creator.address
       );
 
@@ -425,15 +425,15 @@ describe('Asset Royalties', function () {
         splitter
       );
 
-      expect(await splitterContract._recipient()).to.be.equal(creator.address);
+      expect(await splitterContract.recipient()).to.be.equal(creator.address);
 
       const tnx = await RoyaltyManagerContract.connect(
         await ethers.getSigner(creator.address)
       ).setRoyaltyRecipient(royaltyReceiver.address);
 
       await tnx.wait();
 
-      expect(await splitterContract._recipient()).to.be.equal(
+      expect(await splitterContract.recipient()).to.be.equal(
         royaltyReceiver.address
       );
 
@@ -666,22 +666,22 @@ describe('Asset Royalties', function () {
 
       const id = generateAssetId(creator.address, 1);
       await assetAsMinter.mint(seller.address, id, 1, '0x');
-      const splitter = await RoyaltyManagerContract._creatorRoyaltiesSplitter(
+      const splitter = await RoyaltyManagerContract.creatorRoyaltiesSplitter(
         creator.address
       );
       const splitterContract = await ethers.getContractAt(
         splitterAbi,
         splitter
       );
 
-      expect(await splitterContract._recipient()).to.be.equal(creator.address);
+      expect(await splitterContract.recipient()).to.be.equal(creator.address);
       const tnx = await RoyaltyManagerContract.connect(
         await ethers.getSigner(creator.address)
       ).setRoyaltyRecipient(royaltyReceiver.address);
 
       await tnx.wait();
 
-      expect(await splitterContract._recipient()).to.be.equal(
+      expect(await splitterContract.recipient()).to.be.equal(
         royaltyReceiver.address
       );
 
@@ -835,20 +835,20 @@ describe('Asset Royalties', function () {
 
       const id = generateAssetId(creator.address, 1);
       await assetAsMinter.mint(seller.address, id, 1, '0x');
-      const splitter = await RoyaltyManagerContract._creatorRoyaltiesSplitter(
+      const splitter = await RoyaltyManagerContract.creatorRoyaltiesSplitter(
         creator.address
       );
       const splitterContract = await ethers.getContractAt(
         splitterAbi,
         splitter
       );
 
-      expect(await splitterContract._recipient()).to.be.equal(creator.address);
+      expect(await splitterContract.recipient()).to.be.equal(creator.address);
       const tnx = await RoyaltyManagerContract.connect(
         await ethers.getSigner(creator.address)
       ).setRoyaltyRecipient(seller.address);
       await tnx.wait();
-      expect(await splitterContract._recipient()).to.be.equal(seller.address);
+      expect(await splitterContract.recipient()).to.be.equal(seller.address);
     });
 
     it('only creator could change the recipient for his splitter', async function () {

packages/asset/test/Catalyst.test.ts

@@ -613,6 +613,24 @@ describe('Catalyst (/packages/asset/contracts/Catalyst.sol)', function () {
       expect(await catalyst.balanceOf(user1.address, 1)).to.be.equal(3);
       expect(await catalyst.balanceOf(user1.address, 2)).to.be.equal(4);
     });
+    it('should fail on burning non existing token', async function () {
+      const {catalystAsBurner, user1} = await runCatalystSetup();
+      await expect(
+        catalystAsBurner.burnFrom(user1.address, 1, 1)
+      ).to.be.revertedWith('ERC1155: burn amount exceeds totalSupply');
+    });
+    it('should fail on batch burning non existing tokens', async function () {
+      const {catalystAsBurner, user1} = await runCatalystSetup();
+      const catalystId = [1, 2];
+      const catalystAmount = [2, 2];
+      await expect(
+        catalystAsBurner.burnBatchFrom(
+          user1.address,
+          catalystId,
+          catalystAmount
+        )
+      ).to.be.revertedWith('ERC1155: burn amount exceeds totalSupply');
+    });
   });
   describe('Metadata', function () {
     it("user can view token's metadata", async function () {
@@ -678,6 +696,31 @@ describe('Catalyst (/packages/asset/contracts/Catalyst.sol)', function () {
       expect(await catalyst.balanceOf(user2.address, 1)).to.be.equal(10);
       expect(await catalyst.balanceOf(user2.address, 2)).to.be.equal(10);
     });
+    it('should fail on transfering non existing token', async function () {
+      const {catalyst, user1, user2} = await runCatalystSetup();
+
+      await expect(
+        catalyst
+          .connect(user1)
+          .safeTransferFrom(user1.address, user2.address, 1, 1, '0x')
+      ).to.be.revertedWith('ERC1155: insufficient balance for transfer');
+    });
+    it('should fail on batch transfering non existing tokens', async function () {
+      const {catalyst, user1, user2} = await runCatalystSetup();
+      const catalystId = [1, 2];
+      const catalystAmount = [2, 2];
+      await expect(
+        catalyst
+          .connect(user1)
+          .safeBatchTransferFrom(
+            user1.address,
+            user2.address,
+            catalystId,
+            catalystAmount,
+            '0x'
+          )
+      ).to.be.revertedWith('ERC1155: insufficient balance for transfer');
+    });
   });
   describe('OperatorFilterer', function () {
     describe('common subscription setup', function () {

packages/asset/test/Splitter.abi.ts

@@ -88,32 +88,6 @@ export const splitterAbi = [
     name: 'RecipientSet',
     type: 'event',
   },
-  {
-    inputs: [],
-    name: '_recipient',
-    outputs: [
-      {
-        internalType: 'address payable',
-        name: '',
-        type: 'address',
-      },
-    ],
-    stateMutability: 'view',
-    type: 'function',
-  },
-  {
-    inputs: [],
-    name: '_royaltyManager',
-    outputs: [
-      {
-        internalType: 'contract IRoyaltyManager',
-        name: '',
-        type: 'address',
-      },
-    ],
-    stateMutability: 'view',
-    type: 'function',
-  },
   {
     inputs: [],
     name: 'getRecipients',
@@ -143,12 +117,12 @@ export const splitterAbi = [
     inputs: [
       {
         internalType: 'address payable',
-        name: 'recipient',
+        name: '_recipient',
         type: 'address',
       },
       {
         internalType: 'address',
-        name: 'royaltyManager',
+        name: '_royaltyManager',
         type: 'address',
       },
     ],
@@ -189,13 +163,39 @@ export const splitterAbi = [
     stateMutability: 'view',
     type: 'function',
   },
+  {
+    inputs: [],
+    name: 'recipient',
+    outputs: [
+      {
+        internalType: 'address payable',
+        name: '',
+        type: 'address',
+      },
+    ],
+    stateMutability: 'view',
+    type: 'function',
+  },
   {
     inputs: [],
     name: 'renounceOwnership',
     outputs: [],
     stateMutability: 'nonpayable',
     type: 'function',
   },
+  {
+    inputs: [],
+    name: 'royaltyManager',
+    outputs: [
+      {
+        internalType: 'contract IRoyaltyManager',
+        name: '',
+        type: 'address',
+      },
+    ],
+    stateMutability: 'view',
+    type: 'function',
+  },
   {
     inputs: [
       {

packages/dependency-operator-filter/contracts/OperatorFilterRegistrant.md

@@ -1,27 +1,28 @@
-# OperatorFilterRegistry 
-Opensea in an attempt to regularize market places and creator earnings deployed a registry(https://github.com/ProjectOpenSea/operator-filter-registry) and asked NFT token contract to add filter logic(https://github.com/ProjectOpenSea/operator-filter-registry/blob/main/src/OperatorFilterer.sol). 
+# OperatorFilterRegistry
 
-These filter has two modifier 
-1st : onlyAllowedOperator 
+OpenSea in an attempt to regularize market places and creator earnings deployed a registry(https://github.com/ProjectOpenSea/operator-filter-registry) and asked NFT token contract to add filter logic(https://github.com/ProjectOpenSea/operator-filter-registry/blob/main/src/OperatorFilterer.sol).
+
+These filter has two modifier
+1st : onlyAllowedOperator
 2nd : onlyAllowedOperatorApproval
 
-These modifiers are added to to the transfer functions(onlyAllowedOperator) and approval function(onlyAllowedOperatorApproval) such that the when not an owner tried to transfer a Token(ex: Marketplace) or owner approves an operator(ex : Marketplace) they would be filtered on the OperatorFilterRegistry. 
+These modifiers are added to to the transfer functions(onlyAllowedOperator) and approval function(onlyAllowedOperatorApproval) such that the when not an owner tried to transfer a Token(ex: Marketplace) or owner approves an operator(ex : Marketplace) they would be filtered on the OperatorFilterRegistry.
 
 If the operator or the token transfer is not approved by the registry the transaction would be reverted.
 
-On OperatorFilterRegistry a contract owner or the contract can register and maintain there own blacklist or subscribe to other blacklists but that blacklist should contain the default marketplaces blacklisted by Opensea. 
+On OperatorFilterRegistry a contract owner or the contract can register and maintain there own blacklist or subscribe to other blacklists but that blacklist should contain the default marketplaces blacklisted by OpenSea.
 
-# OperatorFiltererRegistrant  
+# OperatorFiltererRegistrant
 
-The OperatorFiltererRegistrant contract is made to be registered on the OperatorFilterRegistry and copy the default blacklist of the openSea. This contract would then be subscribed by the contract such as AssetERC721 and AssetERC1155.
+The OperatorFiltererRegistrant contract is made to be registered on the OperatorFilterRegistry and copy the default blacklist of the OpenSea. This contract would then be subscribed by the contract such as AssetERC721 and AssetERC1155.
 
 The OperatorFiltererRegistrant would be the subscription for our token contracts on a layer(layer-1 : Ethereum , layer-2: Polygon), such that when a address is added or removed from OperatorFiltererRegistrant's blacklist it would be come in affect for each contact which subscribe to the OperatorFiltererRegistrant's blacklist.
 
 # Intended usage
 
 The OperatorFiltererRegistrant is so that sandbox will have a common blacklist that would be utilized by every Token contract on a layer. This would create a single list that would be subscribed by each contract to provide uniformity to which market places sandbox wants to blacklist. This would also provide a focal point to remove and add market places such that it would be applicable to every contract that subscribe to it.
 
-# Implementation 
+# Implementation
 
 We didn't use the npm package as its solidity pragma(solidity version) doesn't match the one we have for our Asset contracts and updating our solidity version for Assets would have been to time consuming.