hats-finance / HATs-Arbitration-Contracts-0x79a618f675857b45934ca1c413fd5f409cf89735

MIT License
0 stars 0 forks source link

Contracts will have no owner after deployment #63

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

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

Github username: -- Submission hash (on-chain): 0x4a32c63276ea369a4bb26db77f667339dc5cf9c5139a61f34ad1cd6454c7f691 Severity: high

Description: Description\ Some contracts like HATVault , HATClaimsManager and RewardController have initialize method, which replaces the constructor and need to be called manually after deployment but reverts due to non-initialisation of __Ownable_init.

Attack Scenario\ After the contract is deployed the initialize method will be called for setting important roles like transferring ownership and assigning claimsManager role that will not take place as the method revert as there will no owner of the contract.

This will make all the function controlled by onlyOwner function to not work and the contract has to be redeployed causing direct loss of funds.

Attachments

https://github.com/hats-finance/hats-contracts/blob/0d6ebbde912bc272d9b310140d434ee2aacd36d3/contracts/HATClaimsManager.sol#L122

https://github.com/hats-finance/hats-contracts/blob/0d6ebbde912bc272d9b310140d434ee2aacd36d3/contracts/HATVault.sol#L79

https://github.com/hats-finance/hats-contracts/blob/0d6ebbde912bc272d9b310140d434ee2aacd36d3/contracts/RewardController.sol#L47

  1. Proof of Concept (PoC) File

HATVault , HATClaimsManager and RewardController have initialize method but does not have __Ownable_init which makes these contracts without any owner and all administered role based functions will revert when called.

Even these contracts are designed to have minimal proxy and non-upgradeable but need to be initialized by depending on the call to initialize method.

  1. Revised Code File (Optional)

Initialize owner properly in the initialize function of all above mentioned contract as mentioned in OZ’s docs.

    __Ownable_init();
jellegerbrandy commented 8 months ago

Ownable_init() transfers ownership to the msg.sender. We transfer ownership to params.owner in l. 83:

_transferOwnership(_params.owner);