Merge pull request #172 from orcaprotocol/willkim/qsp-updates

Willkim/qsp updates

contracts/ControllerRegistry.sol

@@ -1,6 +1,7 @@
 pragma solidity 0.8.7;
 
 import "openzeppelin-contracts/access/Ownable.sol";
+import "openzeppelin-contracts/utils/Address.sol";
 import "./interfaces/IControllerRegistry.sol";
 
 contract ControllerRegistry is IControllerRegistry, Ownable {
@@ -9,11 +10,24 @@ contract ControllerRegistry is IControllerRegistry, Ownable {
     event ControllerRegister(address newController);
     event ControllerRemove(address newController);
 
+    /**
+     * @param _controller The address to check if registered as a controller
+     * @return Boolean representing if the address is a registered as a controller
+     */
+    function isRegistered(address _controller)
+        public
+        view
+        override
+        returns (bool)
+    {
+        return controllerRegistry[_controller];
+    }
+
     /**
      * @param _controller The address to register as a controller
      */
     function registerController(address _controller) external onlyOwner {
-        require(_controller != address(0), "Invalid address");
+        require(Address.isContract(_controller), "controller was not contract");
         emit ControllerRegister(_controller);
         controllerRegistry[_controller] = true;
     }
@@ -22,21 +36,8 @@ contract ControllerRegistry is IControllerRegistry, Ownable {
      * @param _controller The address to remove as a controller
      */
     function removeController(address _controller) external onlyOwner {
-        require(_controller != address(0), "Invalid address");
+        require(isRegistered(_controller), "not registered controller");
         emit ControllerRemove(_controller);
         controllerRegistry[_controller] = false;
     }
-
-    /**
-     * @param _controller The address to check if registered as a controller
-     * @return Boolean representing if the address is a registered as a controller
-     */
-    function isRegistered(address _controller)
-        external
-        view
-        override
-        returns (bool)
-    {
-        return controllerRegistry[_controller];
-    }
 }

contracts/ControllerV1.sol

@@ -264,7 +264,7 @@ contract ControllerV1 is
 
     /**
      * @param _podId The id number of the pod
-     * @param _isTransferLocked The address of the new pod admin
+     * @param _isTransferLocked - bool to set to
      */
     function setPodTransferLock(uint256 _podId, bool _isTransferLocked)
         external
@@ -273,16 +273,10 @@ contract ControllerV1 is
         address admin = podAdmin[_podId];
         address safe = podIdToSafe[_podId];
 
-        // if no pod admin it can only be set by safe
-        if (admin == address(0)) {
-            require(msg.sender == safe, "Only safe can set transfer lock");
-        } else {
-            // if admin then it can be set by admin or safe
-            require(
-                msg.sender == admin || msg.sender == safe,
-                "Only admin or safe can set transfer lock"
-            );
-        }
+        require(
+            msg.sender == admin || msg.sender == safe,
+            "Only admin or safe can set transfer lock"
+        );
 
         // set podid to transfer lock bool
         isTransferLocked[_podId] = _isTransferLocked;
@@ -424,7 +418,7 @@ contract ControllerV1 is
         address[] memory members = this.getSafeMembers(safe);
         memberToken.burnSingleBatch(members, podId);
 
-        isTransferLocked[podId] == false;
+        isTransferLocked[podId] = false;
 
         emit DeregisterPod(podId);
     }
@@ -439,7 +433,7 @@ contract ControllerV1 is
             msg.sender == safe || msg.sender == podAdmin[_podId],
             "not authorized"
         );
-        memberToken.mintSingleBatch(_mintMembers, _podId, bytes(" "));
+        memberToken.mintSingleBatch(_mintMembers, _podId, bytes(""));
         memberToken.burnSingleBatch(_burnMembers, _podId);
     }
 

contracts/InviteToken.sol

@@ -13,11 +13,11 @@ contract InviteToken is ERC20, AccessControl {
         _setupRole(BURNER_ROLE, msg.sender);
     }
 
-    function decimals() public view override returns (uint8) {
+    function decimals() public pure override returns (uint8) {
         return 0;
     }
 
-    function batchMint(address[] calldata accounts, uint256 amount) public {
+    function batchMint(address[] calldata accounts, uint256 amount) external {
         for (uint256 i = 0; i < accounts.length; i++) {
             mint(accounts[i], amount);
         }
@@ -28,7 +28,7 @@ contract InviteToken is ERC20, AccessControl {
         _mint(account, amount);
     }
 
-    function burn(address account, uint256 amount) public {
+    function burn(address account, uint256 amount) external {
         require(hasRole(BURNER_ROLE, msg.sender), "Only burners can burn");
         _burn(account, amount);
     }

contracts/MemberTeller.sol

@@ -14,6 +14,7 @@ contract MemberTeller {
     uint8 internal constant SYNC_EVENT = 0x02;
 
     constructor(address _memberToken) {
+        require(_memberToken != address(0), "Invalid address");
         memberToken = IMemberToken(_memberToken);
     }
 
@@ -39,7 +40,7 @@ contract MemberTeller {
     ) internal {
         if (bytes4(data) == ENCODED_SIG_ADD_OWNER && safe == to) {
             // Ensure data is at minimum, the length required for the below logic.
-            require(data.length >= 24, "incorrect data length");
+            require(data.length == 68, "incorrect data length");
             address mintMember;
             assembly {
                 // shift 0x4 for the sig + 0x20 padding
@@ -49,7 +50,7 @@ contract MemberTeller {
         }
         if (bytes4(data) == ENCODED_SIG_REMOVE_OWNER && safe == to) {
             // Ensure data is at minimum, the length required for the below logic.
-            require(data.length >= 44, "incorrect data length");
+            require(data.length == 100, "incorrect data length");
             address burnMember;
             assembly {
                 // note: consecutive addresses are packed into a single memory slot
@@ -61,7 +62,7 @@ contract MemberTeller {
         }
         if (bytes4(data) == ENCODED_SIG_SWAP_OWNER && safe == to) {
             // Ensure data is at minimum, the length required for the below logic.
-            require(data.length >= 64, "incorrect data length");
+            require(data.length == 100, "incorrect data length");
             address burnMember;
             address mintMember;
             assembly {

contracts/SafeTeller.sol

@@ -17,8 +17,6 @@ contract SafeTeller is DelegateSetupHelper {
 
     string public constant FUNCTION_SIG_SETUP =
         "setup(address[],uint256,address,bytes,address,address,uint256,address)";
-    string public constant FUNCTION_SIG_EXEC =
-        "execTransaction(address,uint256,bytes,uint8,uint256,uint256,uint256,address,address,bytes)";
 
     string public constant FUNCTION_SIG_ENABLE = "delegateSetup(address)";
 

contracts/ens/PodEnsRegistrar.sol

@@ -31,9 +31,9 @@ contract PodEnsRegistrar is Ownable, IPodEnsRegistrar {
     ENS public override ens;
     Resolver public override resolver;
     ReverseRegistrar public override reverseRegistrar;
-    IControllerRegistry controllerRegistry;
-    bytes32 rootNode;
-    IInviteToken inviteToken;
+    IControllerRegistry public controllerRegistry;
+    bytes32 public rootNode;
+    IInviteToken public inviteToken;
     State public state = State.onlySafeWithShip;
 
     /**
@@ -72,20 +72,19 @@ contract PodEnsRegistrar is Ownable, IPodEnsRegistrar {
         address podSafe,
         address podCreator
     ) public override returns (address) {
+        require(label != bytes32(0), "label cannot be blank");
+
         if (state == State.closed) {
             revert("registrations are closed");
-        }
-
-        if (state == State.onlySafeWithShip) {
+        } else if (state == State.onlySafeWithShip) {
             // This implicitly prevents safes that were created in this transaction
             // from registering, as they cannot have a SHIP token balance.
             require(
                 inviteToken.balanceOf(podSafe) > 0,
                 "safe must have SHIP token"
             );
             inviteToken.burn(podSafe, 1);
-        }
-        if (state == State.onlyShip) {
+        } else if (state == State.onlyShip) {
             // Prefer the safe's token over the user's
             if (inviteToken.balanceOf(podSafe) > 0) {
                 inviteToken.burn(podSafe, 1);
@@ -96,7 +95,7 @@ contract PodEnsRegistrar is Ownable, IPodEnsRegistrar {
             }
         }
 
-        bytes32 node = keccak256(abi.encodePacked(rootNode, label));
+        bytes32 node = getEnsNode(label);
 
         require(
             controllerRegistry.isRegistered(msg.sender),
@@ -129,7 +128,12 @@ contract PodEnsRegistrar is Ownable, IPodEnsRegistrar {
      * e.g., the node of mypod.addr.reverse.
      * @param input - an ENS registered address
      */
-    function addressToNode(address input) public override returns (bytes32) {
+    function addressToNode(address input)
+        public
+        view
+        override
+        returns (bytes32)
+    {
         return reverseRegistrar.node(input);
     }
 

test/controller.js

@@ -330,7 +330,7 @@ describe("Controller safe integration test", () => {
     });
     it("should throw if user toggles transfer lock", async () => {
       await expect(controller.connect(bob).setPodTransferLock(POD_ID, true)).to.be.revertedWith(
-        "Only safe can set transfer lock",
+        "Only admin or safe can set transfer lock",
       );
     });
   });

test/foundry/Controller.t.sol

@@ -298,7 +298,7 @@ contract ControllerV1Test is Test {
 
     function test_cantSetTransferLockByNonSafe() public {
         uint256 podId = createPod(false);
-        vm.expectRevert("Only safe can set transfer lock");
+        vm.expectRevert("Only admin or safe can set transfer lock");
         controller.setPodTransferLock(podId, true);
     }
 

test/foundry/MemberToken.t.sol

@@ -248,17 +248,18 @@ contract MemberTokenTest is Test {
     // CONTROLLER MIGRATE TESTS
     // should migrate to new controller version
     function test_MigrateMemberController() public {
-        address newController = address(0x1337);
-        controllerRegistry.registerController(newController);
+        MockController newController = new MockController();
+
+        controllerRegistry.registerController(address(newController));
 
         address[] memory addresses = new address[](1);
         addresses[0] = ALICE;
 
         vm.startPrank(address(mockController));
         uint256 podId = memberToken.createPod(addresses, "1337");
 
-        memberToken.migrateMemberController(podId, newController);
-        assertEq(memberToken.memberController(podId), newController);
+        memberToken.migrateMemberController(podId, address(newController));
+        assertEq(memberToken.memberController(podId), address(newController));
     }
 
     // should revert if called with zero address

test/foundry/Permissions.t.sol

@@ -24,9 +24,12 @@ contract PermissionsTest is Test {
 
         permissions.callAsOwner(
             address(registry),
-            abi.encodeWithSignature("registerController(address)", testAddress)
+            abi.encodeWithSignature(
+                "registerController(address)",
+                address(permissions)
+            )
         );
-        assertEq(registry.isRegistered(testAddress), true);
+        assertEq(registry.isRegistered(address(permissions)), true);
     }
 
     // Attempt to make a call when Permission is not owner of Registry should fail.
@@ -66,9 +69,13 @@ contract PermissionsTest is Test {
         vm.prank(address(0x1338));
         permissions.callAsOwner(
             address(registry),
-            abi.encodeWithSignature("registerController(address)", testAddress)
+            // Registering permissions contract because we need to register a contract
+            abi.encodeWithSignature(
+                "registerController(address)",
+                address(permissions)
+            )
         );
-        assertEq(registry.isRegistered(testAddress), true);
+        assertEq(registry.isRegistered(address(permissions)), true);
     }
 
     /**
@@ -89,7 +96,10 @@ contract PermissionsTest is Test {
 
         permissions.callAsOwner(
             address(registry),
-            abi.encodeWithSignature("registerController(address)", testAddress)
+            abi.encodeWithSignature(
+                "registerController(address)",
+                address(permissions)
+            )
         );
         permissions.grantRole(bytes32(0), testAddress);
         PermissionManager permissions2 = new PermissionManager();