Merge pull request #10 from Unstoppable-DeFi/fix-review-46

fix issue #46

contracts/Vault.vy

@@ -109,6 +109,9 @@ positions: public(HashMap[uint256, Position])
 total_shares: public(uint256)
 amount_claimable_per_share: public(uint256)
 
+owner: public(address)
+suggested_owner: public(address)
+
 is_operator: public(HashMap[address, bool])
 is_depositor: public(HashMap[address, bool])
 
@@ -174,6 +177,14 @@ event NewMigrationAdminSuggested:
     new_admin: indexed(address)
     suggested_by: indexed(address)
 
+event OwnerTransferred:
+    new_owner: indexed(address)
+    promoted_by: indexed(address)
+
+event NewOwnerSuggested:
+    new_owner: indexed(address)
+    suggested_by: indexed(address)
+
 event MigrationActivated:
     migrator_address: address
     active_at: uint256
@@ -188,6 +199,7 @@ def __init__(
     assert _nft_address != empty(address), "invalid nft address"
     NFT = _nft_address
 
+    self.owner = msg.sender
     self.is_operator[msg.sender] = True
     self.fund_receiver = msg.sender
 
@@ -586,13 +598,43 @@ def accept_migration_admin():
     log MigrationAdminTransferred(self.migration_admin, prev_admin)
 
 
+@external
+def suggest_owner(_new_owner: address):
+    """
+    @notice
+        Step 1 of the 2 step process to transfer ownership.
+        Current owner suggests a new owner.
+        Requires the new owner to accept ownership in step 2.
+    @param _new_admin
+        The address of the new owner.
+    """
+    assert msg.sender == self.owner, "unauthorized"
+    assert _new_owner != empty(address), "cannot set owner to zero address"
+    self.suggested_owner = _new_owner
+    log NewOwnerSuggested(_new_owner, msg.sender)
+
+
+@external
+def accept_owner():
+    """
+    @notice
+        Step 2 of the 2 step process to transfer ownership.
+        The suggested owner accepts the transfer and becomes the
+        new owner.
+    """
+    assert msg.sender == self.suggested_owner, "unauthorized"
+    prev_owner: address = self.owner
+    self.owner = self.suggested_owner
+    log OwnerTransferred(self.owner, prev_owner)
+
+
 @external
 def add_operator(_new_operator: address):
     """
     @notice
         Add a new address to the priviledged operators.
     """
-    assert self.is_operator[msg.sender], "unauthorized"
+    assert msg.sender == self.owner, "unauthorized"
     assert self.is_operator[_new_operator] == False, "already operator"
 
     self.is_operator[_new_operator] = True
@@ -606,7 +648,7 @@ def remove_operator(_to_remove: address):
     @notice
         Remove an existing operator from the priviledged addresses.
     """
-    assert self.is_operator[msg.sender], "unauthorized"
+    assert msg.sender == self.owner, "unauthorized"
     assert self.is_operator[_to_remove], "not an operator"
 
     self.is_operator[_to_remove] = False

tests/test_Vault_auth.py

@@ -1,24 +1,33 @@
 import boa
+import pytest
 
-
-def test_operator_can_add_operator(vault, owner, alice):
-    assert vault.is_operator(owner)
-    assert vault.is_operator(alice) == False
+def test_owner_can_add_operator(vault, owner, alice):
+    assert vault.owner() == owner
 
     vault.add_operator(alice)
 
     assert vault.is_operator(alice) == True
 
 
-def test_operator_can_remove_operator(vault, owner, alice):
-    assert vault.is_operator(owner)
+def test_owner_can_remove_operator(vault, owner, alice):
+    assert vault.owner() == owner
     vault.add_operator(alice)
     assert vault.is_operator(alice)
 
     vault.remove_operator(alice)
 
     assert vault.is_operator(alice) == False
 
+def test_operator_cannot_remove_operator(vault, owner, alice):
+    vault.add_operator(alice)
+    assert vault.is_operator(alice)
+
+    with boa.env.prank(alice):
+        with boa.reverts("unauthorized"):
+            vault.remove_operator(alice)
+
+    assert vault.is_operator(alice)
+
 
 def test_non_operator_cannot_add_operator(vault, alice):
     assert vault.is_operator(alice) == False
@@ -44,7 +53,7 @@ def test_NON_owner_cannot_suggest_new_owner(vault, owner, alice):
             vault.suggest_migration_admin(alice)
 
 
-def test_suggested_owner_can_accept_and_become_new_owner(vault, owner, alice):
+def test_suggested_migration_admin_can_accept_and_become_new_migration_admin(vault, owner, alice):
     assert vault.migration_admin() == owner
     vault.suggest_migration_admin(alice)
 
@@ -57,11 +66,50 @@ def test_suggested_owner_can_accept_and_become_new_owner(vault, owner, alice):
     assert vault.migration_admin() == alice
 
 
-def test_NON_suggested_owner_cannot_call_accept_ownership(vault, alice, bob):
+def test_NON_suggested_admin_cannot_call_accept_ownership(vault, alice, bob):
     vault.suggest_migration_admin(alice)
 
     assert vault.suggested_migration_admin() != bob
 
     with boa.env.prank(bob):
         with boa.reverts("unauthorized"):
             vault.accept_migration_admin()
+
+
+def test_owner_can_suggest_new_owner(vault, owner, alice):
+    assert vault.owner() == owner
+
+    vault.suggest_owner(alice)
+    assert vault.suggested_owner() == alice
+
+
+def test_NON_owner_cannot_suggest_new_owner(vault, owner, alice):
+    assert vault.owner() == owner
+    assert vault.owner() != alice
+
+    with boa.env.prank(alice):
+        with boa.reverts("unauthorized"):
+            vault.suggest_owner(alice)
+
+
+def test_suggested_owner_can_accept_and_become_new_owner(vault, owner, alice):
+    assert vault.owner() == owner
+    vault.suggest_owner(alice)
+
+    assert vault.owner() != alice
+    assert vault.suggested_owner() == alice
+
+    with boa.env.prank(alice):
+        vault.accept_owner()
+
+    assert vault.owner() == alice
+
+
+def test_NON_suggested_owner_cannot_call_accept_ownership(vault, alice, bob):
+    vault.suggest_owner(alice)
+
+    assert vault.suggested_owner() != bob
+
+    with boa.env.prank(bob):
+        with boa.reverts("unauthorized"):
+            vault.accept_owner()

tests/test_Vault_setup.py

@@ -16,6 +16,10 @@ def test_deployment_sets_nft_address(vault, nft):
     assert vault.NFT() == nft.address
 
 
+def test_deployment_sets_owner(vault, owner):
+    assert vault.owner() == owner
+
+
 def test_operator_can_set_alchemist(vault, owner):
     assert vault.is_operator(owner)
     assert vault.alchemist() != VALID_ADDRESS