[entropy] audit: 3. two step transfer (#1178)

* two step owner transfer

* admin two step transfer

* remove redundant test

Author: Dev Kalra

target_chains/ethereum/contracts/contracts/entropy/EntropyGovernance.sol

@@ -9,31 +9,49 @@ import "./EntropyState.sol";
  * @dev `Governance` defines a means to enacting changes to the Entropy contract.
  */
 abstract contract EntropyGovernance is EntropyState {
-    event AdminSet(address oldAdmin, address newAdmin);
     event PythFeeSet(uint oldPythFee, uint newPythFee);
     event DefaultProviderSet(
         address oldDefaultProvider,
         address newDefaultProvider
     );
 
-    function getAdmin() external view returns (address) {
-        return _state.admin;
+    event NewAdminProposed(address oldAdmin, address newAdmin);
+    event NewAdminAccepted(address oldAdmin, address newAdmin);
+
+    /**
+     * @dev Returns the address of the proposed admin.
+     */
+    function proposedAdmin() public view virtual returns (address) {
+        return _state.proposedAdmin;
     }
 
     /**
-     * @dev Set the admin of the contract.
-     *
-     * Calls {_authoriseAdminAction}.
-     *
-     * Emits an {AdminSet} event.
+     * @dev Proposes a new admin of the contract. Replaces the proposed admin if there is one.
+     * Can only be called by either admin or owner.
      */
-    function setAdmin(address newAdmin) external {
+    function proposeAdmin(address newAdmin) public virtual {
         _authoriseAdminAction();
 
+        _state.proposedAdmin = newAdmin;
+        emit NewAdminProposed(_state.admin, newAdmin);
+    }
+
+    /**
+     * @dev The proposed admin accepts the admin transfer.
+     */
+    function acceptAdmin() external {
+        if (msg.sender != _state.proposedAdmin)
+            revert EntropyErrors.Unauthorized();
+
         address oldAdmin = _state.admin;
-        _state.admin = newAdmin;
+        _state.admin = msg.sender;
 
-        emit AdminSet(oldAdmin, newAdmin);
+        _state.proposedAdmin = address(0);
+        emit NewAdminAccepted(oldAdmin, msg.sender);
+    }
+
+    function getAdmin() external view returns (address) {
+        return _state.admin;
     }
 
     /**

target_chains/ethereum/contracts/contracts/entropy/EntropyState.sol

@@ -34,6 +34,9 @@ contract EntropyInternalStructs {
         mapping(bytes32 => EntropyStructs.Request) requestsOverflow;
         // Mapping from randomness providers to information about each them.
         mapping(address => EntropyStructs.ProviderInfo) providers;
+        // proposedAdmin is the new admin's account address proposed by either the owner or the current admin.
+        // If there is no pending transfer request, this value will hold `address(0)`.
+        address proposedAdmin;
     }
 }
 

target_chains/ethereum/contracts/contracts/entropy/EntropyUpgradable.sol

@@ -3,15 +3,15 @@ pragma solidity ^0.8.0;
 
 import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
-import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
+import "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol";
 import "@pythnetwork/entropy-sdk-solidity/EntropyErrors.sol";
 
 import "./EntropyGovernance.sol";
 import "./Entropy.sol";
 
 contract EntropyUpgradable is
     Initializable,
-    OwnableUpgradeable,
+    Ownable2StepUpgradeable,
     UUPSUpgradeable,
     Entropy,
     EntropyGovernance
@@ -42,7 +42,7 @@ contract EntropyUpgradable is
         );
 
         // We need to transfer the ownership from deployer to the new owner
-        transferOwnership(owner);
+        _transferOwnership(owner);
     }
 
     /// Ensures the contract cannot be uninitialized and taken over.

target_chains/ethereum/contracts/forge-test/EntropyAuthorized.t.sol

@@ -24,6 +24,8 @@ contract EntropyAuthorized is Test, EntropyTestUtils {
     address public provider1 = address(4);
     address public provider2 = address(5);
 
+    address public owner2 = address(6);
+
     uint128 pythFeeInWei = 7;
 
     function setUp() public {
@@ -39,24 +41,6 @@ contract EntropyAuthorized is Test, EntropyTestUtils {
         random.initialize(owner, admin, pythFeeInWei, provider1, false);
     }
 
-    function testSetAdminByAdmin() public {
-        vm.prank(admin);
-        random.setAdmin(admin2);
-        assertEq(random.getAdmin(), admin2);
-    }
-
-    function testSetAdminByOwner() public {
-        vm.prank(owner);
-        random.setAdmin(admin2);
-        assertEq(random.getAdmin(), admin2);
-    }
-
-    function testExpectRevertSetAdminByUnauthorized() public {
-        vm.expectRevert(EntropyErrors.Unauthorized.selector);
-        vm.prank(admin2);
-        random.setAdmin(admin);
-    }
-
     function testSetPythFeeByAdmin() public {
         vm.prank(admin);
         random.setPythFee(10);
@@ -120,4 +104,74 @@ contract EntropyAuthorized is Test, EntropyTestUtils {
         vm.prank(owner);
         random.upgradeTo(address(randomDifferentMagic));
     }
+
+    function testExpectRevertRequestOwnershipTransferByUnauthorized() public {
+        vm.expectRevert("Ownable: caller is not the owner");
+        vm.prank(provider1);
+        random.transferOwnership(owner2);
+    }
+
+    function testExpectRevertRequestOwnershipTransferByAdmin() public {
+        vm.expectRevert("Ownable: caller is not the owner");
+        vm.prank(admin);
+        random.transferOwnership(owner2);
+    }
+
+    function testRequestAndAcceptOwnershipTransfer() public {
+        vm.prank(owner);
+        random.transferOwnership(owner2);
+        assertEq(random.pendingOwner(), owner2);
+
+        vm.prank(owner2);
+        random.acceptOwnership();
+        assertEq(random.owner(), owner2);
+    }
+
+    function testRequestAndAcceptOwnershipTransferUnauthorizedAccept() public {
+        vm.prank(owner);
+        random.transferOwnership(owner2);
+        assertEq(random.pendingOwner(), owner2);
+
+        vm.prank(admin);
+        vm.expectRevert("Ownable2Step: caller is not the new owner");
+        random.acceptOwnership();
+    }
+
+    function testProposeAdminByOwner() public {
+        vm.prank(owner);
+        random.proposeAdmin(admin2);
+
+        assertEq(random.proposedAdmin(), admin2);
+    }
+
+    function testProposeAdminByAdmin() public {
+        vm.prank(admin);
+        random.proposeAdmin(admin2);
+
+        assertEq(random.proposedAdmin(), admin2);
+    }
+
+    function testProposeAdminByUnauthorized() public {
+        vm.expectRevert(EntropyErrors.Unauthorized.selector);
+        random.proposeAdmin(admin2);
+    }
+
+    function testAcceptAdminByPropsed() public {
+        vm.prank(owner);
+        random.proposeAdmin(admin2);
+
+        vm.prank(admin2);
+        random.acceptAdmin();
+
+        assertEq(random.getAdmin(), admin2);
+    }
+
+    function testAcceptAdminByUnauthorized() public {
+        vm.prank(owner);
+        random.proposeAdmin(admin2);
+
+        vm.prank(provider1);
+        vm.expectRevert(EntropyErrors.Unauthorized.selector);
+        random.acceptAdmin();
+    }
 }