092.md

Modern Hazel Buffalo

High

DebitaV3Aggregator's changeOwner function 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/DebitaV3Aggregator#L682-L685

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

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 DebitaV3Aggregator's real storage.

Internal pre-conditions

None.

External pre-conditions

The current owner address of the DebitaV3Aggregator 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 DebitaV3Aggregator'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;
    }