SoundCreatorV1 ownership (#65)

* SoundCreatorV1 ownership * nonNullImpl * Adds test_implementationAddressOfZeroReverts
soundxyz · Aug 18, 2022 · bd54832 · bd54832
1 parent e59c7ca
commit bd54832
Show file tree

Hide file tree

Showing 4 changed files with 91 additions and 24 deletions.
diff --git a/.changeset/gorgeous-ties-fly.md b/.changeset/gorgeous-ties-fly.md
@@ -0,0 +1,6 @@
+---
+"sound-protocol": minor
+---
+
+-   Implements OZ Ownable on SoundCreatorV1
+-   Adds setEditionImplementation
diff --git a/.changeset/silly-turkeys-mate.md b/.changeset/silly-turkeys-mate.md
@@ -2,4 +2,4 @@
 "sound-protocol": minor
 ---
 
-Adds SoundCreated event to SoundCreator
+Adds SoundEditionCreated event to SoundCreator
diff --git a/contracts/SoundCreator/SoundCreatorV1.sol b/contracts/SoundCreator/SoundCreatorV1.sol
@@ -31,30 +31,22 @@ pragma solidity ^0.8.16;
 import "../SoundEdition/ISoundEditionV1.sol";
 import "chiru-labs/ERC721A-Upgradeable/ERC721AUpgradeable.sol";
 import "openzeppelin/proxy/Clones.sol";
+import "openzeppelin/access/Ownable.sol";
 
 /**
  * @title Sound Creator V1
  * @dev Factory for deploying Sound edition contracts.
  */
-contract SoundCreatorV1 {
-    /***********************************
-                EVENTS
-    ***********************************/
+contract SoundCreatorV1 is Ownable {
+    event SoundEditionCreated(address indexed soundEdition, address indexed creator);
+    event SoundEditionImplementationSet(address newImplementation);
 
-    event SoundCreated(address indexed soundEdition, address indexed creator);
+    error ImplementationAddressCantBeZero();
 
-    /***********************************
-                STORAGE
-    ***********************************/
+    address public soundEditionImplementation;
 
-    address public nftImplementation;
-
-    /***********************************
-              PUBLIC FUNCTIONS
-    ***********************************/
-
-    constructor(address _nftImplementation) {
-        nftImplementation = _nftImplementation;
+    constructor(address _soundEditionImplementation) implementationNotZero(_soundEditionImplementation) {
+        soundEditionImplementation = _soundEditionImplementation;
     }
 
     /**
@@ -71,7 +63,7 @@ contract SoundCreatorV1 {
         uint32 randomnessLockedTimestamp
     ) external returns (address soundEdition) {
         // Create Sound Edition proxy
-        soundEdition = Clones.clone(nftImplementation);
+        soundEdition = Clones.clone(soundEditionImplementation);
         // Initialize proxy
         ISoundEditionV1(soundEdition).initialize(
             msg.sender,
@@ -85,6 +77,26 @@ contract SoundCreatorV1 {
             randomnessLockedTimestamp
         );
 
-        emit SoundCreated(soundEdition, msg.sender);
+        emit SoundEditionCreated(soundEdition, msg.sender);
+    }
+
+    /**
+     * @dev Changes the SoundEdition implementation contract address.
+     */
+    function setEditionImplementation(address newImplementation)
+        external
+        onlyOwner
+        implementationNotZero(newImplementation)
+    {
+        soundEditionImplementation = newImplementation;
+
+        emit SoundEditionImplementationSet(soundEditionImplementation);
+    }
+
+    modifier implementationNotZero(address implementation) {
+        if (implementation == address(0)) {
+            revert ImplementationAddressCantBeZero();
+        }
+        _;
     }
 }
diff --git a/tests/SoundCreator.t.sol b/tests/SoundCreator.t.sol
@@ -6,29 +6,78 @@ import "../contracts/SoundEdition/SoundEditionV1.sol";
 import "../contracts/SoundCreator/SoundCreatorV1.sol";
 
 contract SoundCreatorTests is TestConfig {
-    event SoundCreated(address indexed soundEdition, address indexed creator);
+    event SoundEditionCreated(address indexed soundEdition, address indexed creator);
+    event SoundEditionImplementationSet(address newImplementation);
 
     // Tests that the factory deploys
     function test_deploysSoundCreator() public {
-        SoundEditionV1 soundEditionImplementation = new SoundEditionV1();
-        SoundCreatorV1 _soundCreator = new SoundCreatorV1(address(soundEditionImplementation));
+        SoundEditionV1 editionImplementation = new SoundEditionV1();
+        SoundCreatorV1 _soundCreator = new SoundCreatorV1(address(editionImplementation));
 
         assert(address(_soundCreator) != address(0));
 
-        assertEq(address(_soundCreator.nftImplementation()), address(soundEditionImplementation));
+        assertEq(address(_soundCreator.soundEditionImplementation()), address(editionImplementation));
     }
 
     // Tests that the factory creates a new sound NFT
     function test_createSound() public {
         // Can't test edition address is emitted from event unless we precalculate it,
         // but cloneDeterminstic would require a salt (==more gas & complexity)
         vm.expectEmit(false, true, false, false);
-        emit SoundCreated(address(0), address(this));
+        emit SoundEditionCreated(address(0), address(this));
 
         SoundEditionV1 soundEdition = createGenericEdition();
 
         assert(address(soundEdition) != address(0));
         assertEq(soundEdition.name(), SONG_NAME);
         assertEq(soundEdition.symbol(), SONG_SYMBOL);
     }
+
+    function test_ownership(address attacker) public {
+        vm.assume(attacker != address(this));
+
+        SoundEditionV1 soundEdition = createGenericEdition();
+        SoundCreatorV1 soundCreator = new SoundCreatorV1(address(soundEdition));
+
+        assertEq(address(soundCreator.owner()), address(this));
+
+        vm.expectRevert("Ownable: caller is not the owner");
+        vm.prank(attacker);
+        soundCreator.transferOwnership(attacker);
+    }
+
+    function test_ownerCanSetNewImplementation(address newImplementation) public {
+        vm.assume(newImplementation != address(0));
+
+        SoundEditionV1 soundEdition = createGenericEdition();
+        SoundCreatorV1 soundCreator = new SoundCreatorV1(address(soundEdition));
+
+        vm.expectEmit(false, false, false, true);
+        emit SoundEditionImplementationSet(newImplementation);
+
+        soundCreator.setEditionImplementation(newImplementation);
+        assertEq(address(soundCreator.soundEditionImplementation()), newImplementation);
+    }
+
+    function test_attackerCantSetNewImplementation(address attacker) public {
+        vm.assume(attacker != address(this));
+
+        SoundEditionV1 soundEdition = createGenericEdition();
+        SoundCreatorV1 soundCreator = new SoundCreatorV1(address(soundEdition));
+
+        vm.expectRevert("Ownable: caller is not the owner");
+        vm.prank(attacker);
+        soundCreator.setEditionImplementation(address(0));
+    }
+
+    function test_implementationAddressOfZeroReverts() public {
+        vm.expectRevert(SoundCreatorV1.ImplementationAddressCantBeZero.selector);
+        new SoundCreatorV1(address(0));
+
+        SoundEditionV1 soundEdition = createGenericEdition();
+        SoundCreatorV1 soundCreator = new SoundCreatorV1(address(soundEdition));
+
+        vm.expectRevert(SoundCreatorV1.ImplementationAddressCantBeZero.selector);
+        soundCreator.setEditionImplementation(address(0));
+    }
 }