Ownable2StepUpgradeable incorrect (?) initialization method

Support Contracts
1

Ownable2StepUpgradeable is the new 2-step version of OwnableUpgradeable (which also serves as its parent-contract) allowing for safer ownership transfer.

For some reason, Ownable2StepUpgradeable's init method - __Ownable2Step_init - does not set the current owner to be msg.sender , as is the case with OwnableUpgradeable's init method: __Ownable_init.

Any idea why is this so? I am aware of the fact that I can still call OwnableUpgradeable's init to get this effect but I consider this to be suboptimal.

1 Like
2

See discussion in https://github.com/OpenZeppelin/openzeppelin-contracts/issues/4690

1 Like
3

Thanks Eric. It appears to be a v4.9.3 => v5.0.0 regression issue. Anyway for now I will just add the parent init to the calling code. If you pursue your thread with the O|penzeppelin team kindly insist on a fix. This is actually a solid improvement over the older Ownable set behavior and I hate to see it not used due to lacking such a simple fix.

1 Like
4

From this comment in that thread, we may need to keep this behavior and add a check in the Upgrades Plugins. So the correct approach is as you mentioned, to add the call to __Ownable_init.

1 Like
5

Yes, so for future readers of this thread let me sum the problem and the workaround:

CoreLinearVesting is an upgradeable contract that implements 2-step ownership so that on owner update the new owner must accept his new role for the change to take place.

Since Ownable2StepUpgradeable's init function is a no-op invoking it will not result in setting owne's value, rather, the init function of OwnableUpgradeable (Ownable2StepUpgradeable's base-contract) must be invoked to do just that:


// CoreLinearVesting is a contract implementing 2-step ownership

contract CoreLinearVesting is Ownable2StepUpgradeable {
     function initialize() {
              __Ownable2Step_init() // Bad: owner() not initialized
              __Ownable_init(msg.sender); // Good: owner() initialized to msg.sender
    }
}

I'm also adding some ownable Foundry unit-tests that you can basically copy and paste for your own project:


    function test_no1StepOwnershipChange() public {
        vesting = new CoreLinearVesting();
        _verifyOwner(vesting, address(0)); // owner not set at CTOR
        vesting.initialize();
        _verifyOwner(vesting, address(this)); // owner not set on initialize
        TwoStepOwner pendingOwner = new TwoStepOwner();
        vesting.transferOwnership(address(pendingOwner));
        _verifyOwner(vesting, address(this)); // no immediate ownership change
    }

    function test_2StepOwnershipChangeWorks() public {
        vesting = new CoreLinearVesting();
        _verifyOwner(vesting, address(0)); 
        vesting.initialize();
        _verifyOwner(vesting, address(this)); 
        TwoStepOwner pendingOwner = new TwoStepOwner();
        vesting.transferOwnership(address(pendingOwner));
        // verify 2-step works
        pendingOwner.approve(vesting);
        _verifyOwner(vesting, address(pendingOwner)); 
    }

    function test_verify2StepOwnershipChangeOnlyWorksForPendingOwner() public {
        vesting = new CoreLinearVesting();
        _verifyOwner(vesting, address(0)); 
        vesting.initialize();
        _verifyOwner(vesting, address(this)); 
        TwoStepOwner pendingOwner = new TwoStepOwner();
        
        vesting.transferOwnership(address(pendingOwner));
        TwoStepOwner someoneElse = new TwoStepOwner();

        // 2-step should fail for all but the pendingOwner
        vm.expectRevert(
               abi.encodeWithSelector(OwnableUnauthorizedAccount.selector, address(someoneElse)));
        someoneElse.approve(vesting);
        _verifyOwner(vesting, address(this)); 
    }

    function test_verify2StepOwnershipChangeOnlyWorksForTheLastPendingOwner() public {
        vesting = new CoreLinearVesting();
        vesting.initialize();
        TwoStepOwner pendingOwner_1 = new TwoStepOwner();
        vesting.transferOwnership(address(pendingOwner_1));
        TwoStepOwner pendingOwner_2 = new TwoStepOwner();
        vesting.transferOwnership(address(pendingOwner_2));
        TwoStepOwner pendingOwner_3 = new TwoStepOwner();
        vesting.transferOwnership(address(pendingOwner_3));
        
        // 2-step should fail for all but the *last* pendingOwner
        vm.expectRevert(abi.encodeWithSelector(
                   OwnableUnauthorizedAccount.selector, address(pendingOwner_1)));
        pendingOwner_1.approve(vesting);
        _verifyOwner(vesting, address(this)); 

        vm.expectRevert(abi.encodeWithSelector(
                 OwnableUnauthorizedAccount.selector, address(pendingOwner_2)));
        pendingOwner_2.approve(vesting);
        _verifyOwner(vesting, address(this)); 

        pendingOwner_3.approve(vesting);
        _verifyOwner(vesting, address(pendingOwner_3)); 
    }

    function _verifyOwner(CoreLinearVesting _vesting, address expectedOwner) private view {
        require(_vesting.owner() == expectedOwner, "bad owner");
    }


    contract TwoStepOwner {
        // a test-only contract that allows approval of a 2-step ownership change
        function approve(Ownable2StepUpgradeable requestingContract) public {
            requestingContract.acceptOwnership();
        }
    }
1 Like