058.md

Huge Tiger Pike

Medium

Can't change owner of multiple contracts

Summary

The Shadowing Effect explained. The storage owner value is never changed inside the function. This renders the changeOwner() function, utilized across several contracts, entirely unusable.

Furthermore, if a governance system is introduced later, transferring ownership to a DAO or governance contract would be impossible.

Root Cause

In AuctionFactory.changeOwner(),buyOrderFactory.changeOwner(),DebitaV3Aggregator.changeOwner() we encounter The Shadowing Effect, preventing us from updating the owner of these contracts

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

Core contract behaviour is broken

PoC

In test/fork/BuyOrders/BuyOrder.t.sol

    function testTryToChangeOwner() public {
        address newOwner = 0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045;
        address currentOwner = factory.owner(); 
        vm.startPrank(currentOwner);
        // even currentOwner is unable to change owner
        vm.expectRevert("Only owner");
        factory.changeOwner(newOwner);
        vm.stopPrank();

        // we can also call the method as a random user
        vm.startPrank(newOwner);
        factory.changeOwner(newOwner);
        // ownership of the contract has not changed
        assertEq(currentOwner, factory.owner());
   }

The same could be done for the other two contracts.

Mitigation

In all three contracts:

-   function changeOwner(address owner) public {
+   function changeOwner(address _owner) public {
        require(msg.sender == owner, "Only owner");
        require(deployedTime + 6 hours > block.timestamp, "6 hours passed");
-      owner = owner;
+      owner = _owner;
    }

It is also advised to make ownership transfer a two-step process such as by using the Ownable2Step contract by Openzeppelin.