091.md

Atomic Butter Bison

High

[H-5] DebitaV3Aggregator::changeOwner functionality is broken

Summary

The DebitaV3Aggregator::changeOwner function is meant to allow the current owner of the protocol to transfer the ownership to a new address up to 6 hours after deployment. The issue is that the function's input parameter is called owner and it shadows the existing state variable owner. Because of this, within the function's scope, all references to owner will not point to the state variable owner, they will point to the input parameter owner.

//@audit input param `owner` shadows state variable `owner`
    function changeOwner(address owner) public {
        //@audit this check will revert if the current `owner` attempts to pass in an `owner` input param different
        //than his own address
        require(msg.sender == owner, "Only owner");
        require(deployedTime + 6 hours > block.timestamp, "6 hours passed");
        //@audit this line has no effect. It will not produce any state changes
        owner = owner;
    }

Root Cause

Function changeOwner input parameter owner shadows the existing state variable owner.

Internal pre-conditions

N/A

External pre-conditions

N/A

Attack Path

N/A

Impact

There are three major issues with this function:

1. If the current legitimate owner attempts to transfer ownership to a new address and calls this function with let's say Alice's address as input param, the call will revert because of the check require(msg.sender == owner, "Only owner");. Within this function's scope, msg.sender (which is the legitimate owner) has to be equal to the owner input param, which is Alice's address. Since they are different, the call will fail.

2. As it stands, any user can call this function with their own address as input parameter, and they can bypass the require(msg.sender == owner, "Only owner"); check. If Alice calls this function with her own address as input parameter, she will bypass this check.

3. The last line of code in the function does nothing. It will assign the value of the input parameter owner back to itself, which means that NO state changes occur. Going back to point nr. 2, if Alice tries to set herself as the owner, even if she bypasses the check, no state changes occur. The owner state variable will remain set to whoever deployed this contract.

This means that this function is useless. If the current owner tries to transfer the ownership of the contract to a new address within the first 6 hours, this WON'T happen, because NO state changes occur. After 6 hours pass, this function will always revert because of the second check.

PoC

Create a new Test file and put it into the test folder.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "@contracts/DebitaV3Aggregator.sol";
import {Test, console} from "forge-std/Test.sol";

contract Tests is Test {
    DebitaV3Aggregator public debitaV3Aggregator;

    function setUp() public {
        debitaV3Aggregator = new DebitaV3Aggregator(
            address(this),
            address(this),
            address(this),
            address(this),
            address(this),
            address(this)
        );
    }

    function testDebitaV3AggregatorChangeOwnerFails() public {
        address currentOwner = debitaV3Aggregator.owner();
        console.log("Current owner is: ", currentOwner);

        //make a new user
        address alice = makeAddr("alice");

        //try to transfer ownership as legit owner and fail
        vm.prank(currentOwner);
        vm.expectRevert();
        debitaV3Aggregator.changeOwner(alice);
        //the above call fails because the function compares msg.sender with the input parameter owner instead of the state variable owner
        //this causes the function call to revert, because msg.sender needs to be == owner input param

        //assert that no state changes occur
        assertNotEq(address(alice), debitaV3Aggregator.owner());
        assertEq(currentOwner, debitaV3Aggregator.owner());
        console.log(
            "Owner after failed attempt is still: ",
            debitaV3Aggregator.owner()
        );

        //prove that alice can bypass the check `require(msg.sender == owner, "Only owner");` because the contract compares
        //the input param `owner` with msg.sender instead of the actual state variable
        vm.prank(address(alice));
        debitaV3Aggregator.changeOwner(alice);
        //this call passed

        //prove that even though the call succeeded, no state changes occured
        assertNotEq(address(alice), debitaV3Aggregator.owner());
        assertEq(currentOwner, debitaV3Aggregator.owner());
        console.log(
            "Owner after successful attempt is still: ",
            debitaV3Aggregator.owner()
        );
    }
}

Test output

Ran 1 test for test/Tests.sol:Tests
[PASS] testDebitaV3AggregatorChangeOwnerFails() (gas: 31162)
Logs:
  Current owner is:  0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496
  Owner after failed attempt is still:  0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496
  Owner after successful attempt is still:  0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 13.88ms (4.62ms CPU time)

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

Mitigation

Rename the input parameter owner in order to avoid shadowing.

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