catalyst-audit: L-04 ERC20 transfer return value lacks validation

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");
@@ -154,7 +154,7 @@ contract Catalyst is
     }
 
     /// @notice Add a new catalyst type, limited to DEFAULT_ADMIN_ROLE only
-    /// @param ipfsCID The royalty bps for the catalyst
+    /// @param ipfsCID The IPFS content identifiers for the catalyst
     function addNewCatalystType(string memory ipfsCID) external onlyRole(DEFAULT_ADMIN_ROLE) {
         require(bytes(ipfsCID).length != 0, "Catalyst: CID can't be empty");
         uint256 newCatId = ++highestTierIndex;
@@ -187,6 +187,7 @@ contract Catalyst is
     function setBaseURI(string memory baseURI) external onlyRole(DEFAULT_ADMIN_ROLE) {
         require(bytes(baseURI).length != 0, "Catalyst: base uri can't be empty");
         _setBaseURI(baseURI);
+        emit BaseURISet(baseURI);
     }
 
     /// @notice returns full token URI, including baseURI and token metadata URI
@@ -223,12 +224,18 @@ contract Catalyst is
         return ERC2771HandlerUpgradeable._msgData();
     }
 
+    /// @dev Sets `baseURI` as the `_baseURI` for all tokens
+    function _setBaseURI(string memory baseURI) internal virtual override {
+        super._setBaseURI(baseURI);
+        emit BaseURISet(baseURI);
+    }
+
     /// @notice Transfers `value` tokens of type `id` from  `from` to `to`  (with safety call).
     /// @param from address from which tokens are transfered.
     /// @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,
@@ -245,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,
@@ -286,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".
@@ -298,10 +305,13 @@ contract Catalyst is
         _registerAndSubscribe(subscriptionOrRegistrantToCopy, subscribe);
     }
 
-    /// @notice sets filter registry address deployed in test
+    /// @notice sets filter registry address
     /// @param registry the address of the registry
     function setOperatorRegistry(address registry) external onlyRole(DEFAULT_ADMIN_ROLE) {
         require(registry != address(0), "Catalyst: registry can't be zero address");
         OperatorFiltererUpgradeable._setOperatorFilterRegistry(registry);
+        emit OperatorRegistrySet(registry);
     }
+
+    uint256[49] private __gap;
 }

packages/asset/contracts/interfaces/ICatalyst.sol

@@ -7,6 +7,8 @@ interface ICatalyst {
     event TrustedForwarderChanged(address indexed newTrustedForwarderAddress);
     event NewCatalystTypeAdded(uint256 catalystId);
     event DefaultRoyaltyChanged(address indexed newDefaultRoyaltyRecipient, uint256 newDefaultRoyaltyAmount);
+    event BaseURISet(string baseURI);
+    event OperatorRegistrySet(address indexed registry);
 
     /// @notice Mints a new token, limited to MINTER_ROLE only
     /// @param to The address that will own the minted token

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
       );
 
@@ -828,58 +828,27 @@ describe('Asset Royalties', function () {
     });
   });
 
-  it('Can view all the royalty recipient of each asset', async function () {
-    const {
-      Asset,
-      seller,
-      commonRoyaltyReceiver,
-      creator,
-      deployer,
-      assetAsMinter,
-    } = await assetRoyaltyDistribution();
-
-    const id = generateAssetId(creator.address, 1);
-    await assetAsMinter.mint(seller.address, id, 1, '0x');
-    const id2 = generateAssetId(deployer.address, 1);
-    await assetAsMinter.mint(seller.address, id2, 1, '0x01');
-    const tokenRoyalties = await Asset.getTokenRoyalties();
-    expect(tokenRoyalties[0].tokenId).to.be.equal(id);
-    expect(tokenRoyalties[0].recipients[0].recipient).to.be.equal(
-      creator.address
-    );
-    expect(tokenRoyalties[0].recipients[1].recipient).to.be.equal(
-      commonRoyaltyReceiver.address
-    );
-    expect(tokenRoyalties[1].tokenId).to.be.equal(id2);
-    expect(tokenRoyalties[1].recipients[0].recipient).to.be.equal(
-      deployer.address
-    );
-    expect(tokenRoyalties[1].recipients[1].recipient).to.be.equal(
-      commonRoyaltyReceiver.address
-    );
-  });
-
   describe('Roles on Asset and Manager contract', function () {
     it('creator could change the recipient for his splitter', async function () {
       const {seller, RoyaltyManagerContract, creator, assetAsMinter} =
         await assetRoyaltyDistribution();
 
       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

@@ -384,6 +384,14 @@ describe('Catalyst (/packages/asset/contracts/Catalyst.sol)', function () {
         `AccessControl: account ${user1.address.toLocaleLowerCase()} is missing role ${catalystAdminRole}`
       );
     });
+    it('emits BaseURISet event on setting base uri', async function () {
+      const {catalystAsAdmin, catalyst} = await runCatalystSetup();
+
+      const setBaseURITx = await catalystAsAdmin.setBaseURI('ipfs////');
+      await expect(setBaseURITx)
+        .to.emit(catalyst, 'BaseURISet')
+        .withArgs(`ipfs////`);
+    });
     it('cant add invalid token uri', async function () {
       const {catalystAsAdmin} = await runCatalystSetup();
       await expect(catalystAsAdmin.addNewCatalystType('')).to.be.revertedWith(
@@ -605,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 () {
@@ -670,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 () {