diff --git a/contracts/ControllerRegistry.sol b/contracts/ControllerRegistry.sol index 2cc94cf..0c47e64 100644 --- a/contracts/ControllerRegistry.sol +++ b/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]; - } } diff --git a/contracts/ControllerV1.sol b/contracts/ControllerV1.sol index 9afe69f..fc3f374 100644 --- a/contracts/ControllerV1.sol +++ b/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); } diff --git a/contracts/InviteToken.sol b/contracts/InviteToken.sol index 3e965e2..a1bb2d8 100644 --- a/contracts/InviteToken.sol +++ b/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); } diff --git a/contracts/MemberTeller.sol b/contracts/MemberTeller.sol index 1aa9bc6..9236f76 100644 --- a/contracts/MemberTeller.sol +++ b/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 { diff --git a/contracts/SafeTeller.sol b/contracts/SafeTeller.sol index 8c3dda8..23d20b3 100644 --- a/contracts/SafeTeller.sol +++ b/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)"; diff --git a/contracts/ens/PodEnsRegistrar.sol b/contracts/ens/PodEnsRegistrar.sol index 31dd841..407f60f 100644 --- a/contracts/ens/PodEnsRegistrar.sol +++ b/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,11 +72,11 @@ 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( @@ -84,8 +84,7 @@ contract PodEnsRegistrar is Ownable, IPodEnsRegistrar { "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); } diff --git a/test/controller.js b/test/controller.js index 076071c..165aa2d 100644 --- a/test/controller.js +++ b/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", ); }); }); diff --git a/test/foundry/Controller.t.sol b/test/foundry/Controller.t.sol index 1604cbd..e008aac 100644 --- a/test/foundry/Controller.t.sol +++ b/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); } diff --git a/test/foundry/MemberToken.t.sol b/test/foundry/MemberToken.t.sol index 2f316b7..0dd5bee 100644 --- a/test/foundry/MemberToken.t.sol +++ b/test/foundry/MemberToken.t.sol @@ -248,8 +248,9 @@ 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; @@ -257,8 +258,8 @@ contract MemberTokenTest is Test { 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 diff --git a/test/foundry/Permissions.t.sol b/test/foundry/Permissions.t.sol index 5012a80..6cbc921 100644 --- a/test/foundry/Permissions.t.sol +++ b/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();