Skip to content

Latest commit

 

History

History
97 lines (62 loc) · 2.96 KB

054.md

File metadata and controls

97 lines (62 loc) · 2.96 KB

Abundant Alabaster Toad

High

Shadow Variable in changeOwner() will prevent owner update new ownership

Summary

Shadowing variable owner in AuctionFactory.sol, and DebitaV3Aggregator.sol cause changeOwner() function to use local variable owner instead of storage variable owner.

This result in no new owner being updated to storage. Also, anyone can bypass changeOwner() function owner permission check. Because owner is not changed so it is harmless for now.

Root Cause

Here is shadowing variable code part https://github.com/sherlock-audit/2024-11-debita-finance-v3/blob/main/Debita-V3-Contracts/contracts/auctions/AuctionFactory.sol#L218-L222 https://github.com/sherlock-audit/2024-11-debita-finance-v3/blob/main/Debita-V3-Contracts/contracts/DebitaV3Aggregator.sol#L682-L686

contract auctionFactoryDebita {
  ...
    address owner; // owner of the contract

    function changeOwner(address owner) public {
        require(msg.sender == owner, "Only owner");//@audit H anyone can change Factory owner due to shadow variable.
        require(deployedTime + 6 hours > block.timestamp, "6 hours passed");
        owner = owner;//@owner never changed. this just updated local variable not storage variable
    }
  ...
}

owner variable above always point to local function variable. So storage variable address owner never used.

Internal pre-conditions

  • Admin create AuctionFactory.sol contract. Owner address is Admin address.
  • Owner variable storage return Admin address

External pre-conditions

  • Any random user can call changeOwner() and bypass permission check
  • Current Admin want to call changeOwner() to update new owner address

Attack Path

  • Admin after deployed try to change owner to DAO address will fail.
  • Any user can call changeOwner(address) with their own address as input and call will always success.

Impact

Not possible to change new admin after deployed. Resulting in redeployment.

Because anyone can bypass admin permission check, I consider this issue High despite any attempt to change admin always failed.

PoC

Tested with Auction.t.sol

    function testDebugChangeOwner() public {        
        address fakeOwner = address(0x1011);        
        vm.startPrank(fakeOwner);

        vm.expectRevert("Only the owner");// @not the owner
        factory.setFeeAddress(address(0));
        
        //try call change owner success
        factory.changeOwner(fakeOwner);
        // owner never change

        //fake owner try to call with admin still fail
        vm.expectRevert("Only the owner");// @not the owner
        factory.setFeeAddress(address(0));
        
        vm.stopPrank();

        // previous owner still work
        factory.setFeeAddress(address(0x0));
    }

Mitigation

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