Merge pull request #1105 from thesandboxgame/catalyst-audit/L-01-abst…

…ract-contracts-allow-direct-modification-of-state-variables

Catalyst audit: L-01 Abstract contracts allow direct modification of state variables.

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".
@@ -352,6 +352,6 @@ contract Asset is
     /// @param registry the address of the registry
     function setOperatorRegistry(address registry) external onlyRole(DEFAULT_ADMIN_ROLE) {
         require(registry != address(0), "Asset: registry can't be zero address");
-        operatorFilterRegistry = IOperatorFilterRegistry(registry);
+        OperatorFiltererUpgradeable._setOperatorFilterRegistry(registry);
     }
 }

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");
-        operatorFilterRegistry = IOperatorFilterRegistry(registry);
+        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/contracts/mock/MockAsset.sol

@@ -13,6 +13,8 @@ contract MockAsset is Asset {
     /// @param registry address of registry
     /// @param subscription address to subscribe
     function setRegistryAndSubscribe(address registry, address subscription) external {
+        _setOperatorFilterRegistry(registry);
+        IOperatorFilterRegistry operatorFilterRegistry = _getOperatorFilterRegistry();
         operatorFilterRegistry = IOperatorFilterRegistry(registry);
         operatorFilterRegistry.registerAndSubscribe(address(this), subscription);
     }

packages/asset/contracts/mock/MockCatalyst.sol

@@ -9,6 +9,8 @@ contract MockCatalyst is Catalyst {
     /// @param registry address of registry
     /// @param subscription address to subscribe
     function setRegistryAndSubscribe(address registry, address subscription) external {
+        _setOperatorFilterRegistry(registry);
+        IOperatorFilterRegistry operatorFilterRegistry = _getOperatorFilterRegistry();
         operatorFilterRegistry = IOperatorFilterRegistry(registry);
         operatorFilterRegistry.registerAndSubscribe(address(this), subscription);
     }

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/Asset.test.ts

@@ -1025,7 +1025,7 @@ describe('Base Asset Contract (/packages/asset/contracts/Asset.sol)', function (
         await Asset.connect(assetAdmin).setOperatorRegistry(
           operatorFilterRegistry.address
         );
-        expect(await Asset.operatorFilterRegistry()).to.be.equals(
+        expect(await Asset.getOperatorFilterRegistry()).to.be.equals(
           operatorFilterRegistry.address
         );
 
@@ -1616,6 +1616,7 @@ describe('Base Asset Contract (/packages/asset/contracts/Asset.sol)', function (
 
       it('it should not setApprovalForAll blacklisted market places', async function () {
         const {mockMarketPlace1, users} = await setupOperatorFilter();
+
         await expect(
           users[0].Asset.setApprovalForAll(mockMarketPlace1.address, true)
         ).to.be.reverted;

packages/asset/test/AssetReveal.test.ts

@@ -122,6 +122,7 @@ describe('AssetReveal (/packages/asset/contracts/AssetReveal.sol)', function ()
         sameMetadataHash,
         [revealHashA]
       );
+
       expect(result.events[3].event).to.equal('AssetRevealMint');
       const newTokenId = result.events[3].args.newTokenIds[0];
       const revealNonce = await TokenIdUtilsContract.getRevealNonce(newTokenId);
@@ -142,8 +143,8 @@ describe('AssetReveal (/packages/asset/contracts/AssetReveal.sol)', function ()
         [revealHashB]
       );
 
-      expect(result2.events[2].event).to.equal('AssetRevealMint');
-      const newTokenId2 = result2.events[2].args.newTokenIds[0];
+      expect(result2.events[1].event).to.equal('AssetRevealMint');
+      const newTokenId2 = result2.events[1].args.newTokenIds[0];
       const revealNonce2 = await TokenIdUtilsContract.getRevealNonce(
         newTokenId2
       );
@@ -648,6 +649,7 @@ describe('AssetReveal (/packages/asset/contracts/AssetReveal.sol)', function ()
             newMetadataHashes,
             [revealHashA]
           );
+
           expect(result.events[3].event).to.equal('AssetRevealMint');
           const newTokenId = result.events[3].args.newTokenIds[0];
           const balance = await AssetContract.balanceOf(
@@ -717,6 +719,7 @@ describe('AssetReveal (/packages/asset/contracts/AssetReveal.sol)', function ()
             newMetadataHashes,
             [revealHashA]
           );
+
           const newTokenId = result.events[3].args.newTokenIds[0];
           const balance = await AssetContract.balanceOf(
             user.address,
@@ -985,6 +988,7 @@ describe('AssetReveal (/packages/asset/contracts/AssetReveal.sol)', function ()
           );
 
           expect(result.events[5].event).to.equal('AssetRevealMint');
+
           const args = result.events[5].args;
           const {
             recipient,