hats-finance / Blast-Futures-Exchange-0x97895c329b950755566ddcdad3395caaea395074

0 stars 0 forks source link

In `PoolDeposit::contructor()`, allowing `owner` address to be set in the parameter `_owner` is not in congruence with the `onlyOwner()` modifier logic #63

Open hats-bug-reporter[bot] opened 7 months ago

hats-bug-reporter[bot] commented 7 months ago

Github username: @0xsnowbear Twitter username: 0xsnowbear Submission hash (on-chain): 0xfe1a6090a2575c1b69c934c53b34168aab32c5161987097fb2575d67c937680e Severity: low

Description: Description\ In the PoolDeposit::contructor(), the owner address is set in the parameter _owner. During deployment, it should be the address of the deployer of the contract in congruence with the onlyOwner() modifer logic.

However, there is a possibility that this may be overlooked during deployment and mistakenly be set into a wrong address.

Impact\ If that happens, the protocol team will be obliged to re-deploy the contract. The occurence is in the following contracts namely: Bfx::constructor(), BfxVault::constructor() and PoolDeposit::constructor().

Below is the snippet of the occurence in PoolDeposit::constructor().

    constructor(address _owner, address _rabbit, address _paymentToken) {
        owner = _owner;
        rabbit = _rabbit;
        paymentToken = IERC20(_paymentToken);
    }

    modifier onlyOwner() {
        require(msg.sender == owner, "ONLY_OWNER"); // @audit The `owner` is the msg.sender as stated here
        _;
    }

Mitigation\ To fix the issue, just set the owner as the msg.sender in the constructor (shown below) and do it to the rest of the contracts affected.

--  constructor(address _owner, address _rabbit, address _paymentToken) {
++  constructor(address _rabbit, address _paymentToken) {
--      owner = _owner;
++      owner = msg.sender;
        rabbit = _rabbit;
        paymentToken = IERC20(_paymentToken);
    }
alex-sumner commented 7 months ago

This is not a bug. Setting the owner of a contract during deployment is a common pattern and there is no need to force the owner to be the deployer. On the contrary, the owner will typically be a multisig whilst the deployer will not.