040.md

Modern Hazel Buffalo

High

AuctionFactory's changeOwner is completely broken

Summary

Ever since the owner is set during the AuctionFactory's contract construction, it can never be changed due to not using a locally-scoped _owner variable as an argument in the changeOwner function.

Root Cause

• https://github.com/sherlock-audit/2024-11-debita-finance-v3/blob/main/Debita-V3-Contracts/contracts/auctions/AuctionFactory.sol#L218-L221

// file: AuctionFactory.sol

    // ...

    uint public publicAuctionFee = 50; // fee for public auctions 0.5%
    uint deployedTime;
    address owner; // owner of the contract

    // ...

    constructor() {
        owner = msg.sender;
        feeAddress = msg.sender;
        deployedTime = block.timestamp;
    }

    modifier onlyOwner() {
        require(msg.sender == owner, "Only the owner");
        _;
    }

// file: AuctionFactory.sol

    // ...

    function setFeeAddress(address _feeAddress) public onlyOwner {
        feeAddress = _feeAddress;
    }

    function changeOwner(address owner) public {
        require(msg.sender == owner, "Only owner");
        require(deployedTime + 6 hours > block.timestamp, "6 hours passed");
     @@ owner = owner; // @ <=== here is the problem
    }

    function emitAuctionDeleted(
        address _auctionAddress,
        address creator
    ) public onlyAuctions {
        emit auctionEnded(_auctionAddress, creator);
    }

As you can see here, the owner variable in the changeOwner function's agruments "shadows" the global owner variable in the storage.

Hence:

1. The comparison owner == msg.sender is absolutely wrong;
2. The global owner function can never be updated because the owner = owner assignment just updates the local calldata owner variable's value, and never touches the owner that was declated in the AuctionFactory's real storage.

Internal pre-conditions

None.

External pre-conditions

The current owner address of the AuctionFactory contract intends to update the owner, setting it to another address, via calling the changeOwner function.

Attack Path

Whenever changeOwner is called, it will likely just revert as the owner passed in the arguments will barely ever be the msg.sender (otherwise there'd be no sense in calling changeOwner).

In any case, changeOwner will either revert on the check (in 99,99% of the cases), or as long as owner (which is supposed to be the "newOwner" or _owner in this context to locally scope it) just update the locally-scoped owner variable (i.e. itself!).

Impact

There's no way to update the current AuctionFactory's owner: the only way is through via changeOwner, which is completely broken due to referring to a locally-scoped owner variable (its own argument!), and shadowing the globally-scoped owner due to the same naming of the variable.

In other words, changeOwner is essentially a view-only pure function due to that aforementioned mistake.

PoC

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

Mitigation

-   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;
    }