030.md

Furry Cloud Cod

Medium

The auctionFactoryDebita::changeOwner fails to change owner as it should

Impact

Summary

The auctionFactoryDebita::changeOwner is designed to change the owner of the auctionFactoryDebita contract, passing owner privileges from the old owner to the new owner. However, this function will not be able to change the owner of the contract due to conflicting variable naming convention.

Vulnerability Details

The vulnerability of this function lies in the fact that the name of the input parameter for the auctionFactoryDebita::changeOwner function conflicts with the storage variable owner in the sense that the two variables are not differentiated. Here is the link to the function in question https://github.com/sherlock-audit/2024-11-debita-finance-v3/blob/main/Debita-V3-Contracts/contracts/auctions/auctionFactoryDebita.sol#L218-L222 and also shown in the code snippet below

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

Impact

As a result of this conflicting variable naming convention, the solidity compiler fumbles in when to user owner as the parameter input of the auctionFactoryDebita::changeOwner function and when to use owner as state varable in the auctionFactoryDebita contract. In particular, if the auctionFactoryDebita::changeOwner function is called by the auctionFactoryDebita::owner, the call reverts due to the first require statement. This is because msg.sender is compared against owner, the parameter input instead of auctionFactoryDebita::owner. On the other hand, if owner as in the input parameter calls the auctionFactoryDebita::changeOwner function, the function executes but owner is not changed. Hence, the owner cannot be changed, breaking the protocol's functionality.

Proof of Concept

Note that auctionFactoryDebita::owner is an internal variable and the auctionFactoryDebita contract has no getter function for auctionFactoryDebita::owner. In order for us to view auctionFactoryDebita::owner, we create a similar contract that inherits the auctionFactoryDebita contract and in addition, we construct a getter function for the auctionFactoryDebita::owner variable.

1. Create another contract FactoryOwnerViewer inside Auction.t.sol. This contract should inherit auctionFactoryDebita contract, and have a getter function for auctionFactoryDebita::owner in addition.
2. Prank the owner of FactoryOwnerViewer to call the FactoryOwnerViewer::changeOwner function which reverts with Only owner message.
3. Prank the address we wish to set as the new owner to call the FactoryOwnerViewer::changeOwner function. This executes successfully but using the getter function shows that the FactoryOwnerViewer::owner has not changed.

PoC

Place the following code into `Auction.t.sol`.

contract FactoryOwnerViewer is auctionFactoryDebita {
    

    function getFactoryOwner() public view returns(address) {
        return owner;
    }

}

contract FactoryOwnerViewerTest is Test {
    FactoryOwnerViewer factoryViewer;

    function setUp() external {
        factoryViewer = new FactoryOwnerViewer();

    }

    function test_SpomariaPoC_AuctionFactoryCantChangeOwner() public {

        // address factoryOwner = factory.owner();
        address factoryOwner = factoryViewer.getFactoryOwner();

        address _newFactoryOwner = makeAddr("new_owner");

        vm.startPrank(factoryOwner);
        vm.expectRevert("Only owner");
        factoryViewer.changeOwner(_newFactoryOwner);
        vm.stopPrank();

        vm.startPrank(_newFactoryOwner);
        factoryViewer.changeOwner(_newFactoryOwner);
        vm.stopPrank();

        // assert that owner was not changed
        assertEq(factoryViewer.getFactoryOwner(), factoryOwner);
    }
}

Now run forge test --match-test test_SpomariaPoC_AuctionFactoryCantChangeOwner -vvvv

Output:

    ├─ [0] VM::startPrank(FactoryOwnerViewerTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
    │   └─ ← [Return] 
    ├─ [0] VM::expectRevert(Only owner)
    │   └─ ← [Return] 
    ├─ [681] FactoryOwnerViewer::changeOwner(new_owner: [0x8138d5842F59D3ce76a371b64D60b577155EF7E4])
    │   └─ ← [Revert] revert: Only owner
    ├─ [0] VM::stopPrank()
    │   └─ ← [Return] 
    ├─ [0] VM::startPrank(new_owner: [0x8138d5842F59D3ce76a371b64D60b577155EF7E4])
    │   └─ ← [Return] 
    ├─ [2748] FactoryOwnerViewer::changeOwner(new_owner: [0x8138d5842F59D3ce76a371b64D60b577155EF7E4])
    │   └─ ← [Return] 
    ├─ [0] VM::stopPrank()
    │   └─ ← [Return] 
    ├─ [782] FactoryOwnerViewer::getFactoryOwner() [staticcall]
    │   └─ ← [Return] FactoryOwnerViewerTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]
    ├─ [0] VM::assertEq(FactoryOwnerViewerTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], FactoryOwnerViewerTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]) [staticcall]
    │   └─ ← [Return] 
    └─ ← [Return] 

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.23ms (339.67µs CPU time)

Ran 1 test suite in 17.21ms (1.23ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review and Foundry

Recommended Mitigation Steps

Consider changing the name of the input parameter of the auctionFactoryDebita::changeOwner function in such a way that it does not conflict with any state variables. For instance, we could use address _owner instead of address owner as shown below:

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