addRewardController and setVaultDescription functions will not be callable because of the wrongly written `onlyRegistryOwner` modifier

[hats-bug-reporter[bot]]

Github username: @ololade97 Submission hash (on-chain): 0xdb761c8b6418415462587b12cb43b50f568a4bf127b8649979c8a0f28ab931ae Severity: high

Description: Description\ Here's the onlyRegistryOwner modifier:

modifier onlyRegistryOwner() { if (registry.owner() != _msgSender()) revert OnlyRegistryOwner(); _; }

It means if the registry.owner() is not the deployer address (_msgSender()), revert should occur.

The vulnerabililty is the use of _msgSender(). _msgSender auto set the deployer address as the sender.

Whereas, in the initialize function, ownership is transferred to another address.

registry.owner() shouldn't be checked against_msgSender().

Functions such as addRewardController, setVaultDescription will always revert and not accessible by anyone.

Attack Scenario\ addRewardController and setVaultDescription functions will not be callable because of the wrongly written onlyRegistryOwner modifier.

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

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

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

1. Proof of Concept (PoC) File

   Wrong use of _msgSender() below:

modifier onlyRegistryOwner() { if (registry.owner() != _msgSender()) revert OnlyRegistryOwner(); _; }

2. *Recommendation

The vulnerability is in the use of _msgSender() in the modifier. msg.sender should be used instead.

modifier onlyRegistryOwner() { if (registry.owner() != msg.sender) revert OnlyRegistryOwner(); _; }

[bahurum]

_msgSender() is the caller in the context of the current call. The modifier works correctly.

[ololade97]

How is the _msgSender() set?

[bahurum]

    function _msgSender() internal view virtual returns (address) {
        return msg.sender;
    }

See https://github.com/OpenZeppelin/openzeppelin-contracts/blob/94697be8a3f0dfcd95dfb13ffbd39b5973f5c65d/contracts/utils/Context.sol#L16-L19

[ololade97]

This is exactly what I'm talking about - function _msgSender() internal view virtual returns (address) { return msg.sender; }

What the above means is that the deployer of the contract auto becomes the sender.

However, in the initialize function ownership was transferred.

[bahurum]

The msg.sender of the call is not set at any time, and it is the EOA or contract that has called the external or public function. If Alice calls addRewardController() then msg.sender is Alice's address, if Bob calls addRewardController() then msg.sender is Bob's address

[ololade97]

@bahurum, I disagree with your explanation. Here's why:

Here's the onlyRegistryOwner() modifier:

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

In the modifier, registry.owner() returns Ownable.owner declared in HATVaultsRegistry.sol. Here's the code:

https://github.com/hats-finance/hats-contracts/blob/0d6ebbde912bc272d9b310140d434ee2aacd36d3/contracts/HATVaultsRegistry.sol#L404-L406

And looking up Ownable.owner function, it returns _owner. Here's the code:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4e419d407cb13c4ebd64d3f47faa78964cbbdd71/contracts/access/Ownable.sol#L21

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4e419d407cb13c4ebd64d3f47faa78964cbbdd71/contracts/access/Ownable.sol#L53-L59

And the _owner variable is reset in the _transferOwnership function. Here's the code:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4e419d407cb13c4ebd64d3f47faa78964cbbdd71/contracts/access/Ownable.sol#L95-L99

Note that by calling the _transferOwnership function, you're resetting from an oldOwner to a new owner.

The first owner in this regard is the deployer of the contract which _msgSender() represents. The new owner is set by calling the _transferOwnership in the initialize function. Here's the code:

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

Now back to the onlyRegistryOwner modifier, the modifier says if registry.owner() != _msgSender() - is not the caller of the functions (addRewardController and setVaultDescription), the funtion should revert.

The bug is, ownership has been reset and transferred to a new address in the Initialize function by calling _transferOwnership in the initialize function. This means that owner is no longer the _msgSender(). And registry.owner() can never be _msgSender() again.

Thus, addRewardController and setVaultDescription will forever revert.

As I said, the bug lies in the use of != _msgSender().

[ololade97]

@jellegerbrandy, could you let me know why this report is marked invalid?

[jellegerbrandy]

This issue is similar to #42 ; similar reasoning applies here.

[ololade97]

The test you referred to doesn't cover the vulnerability described in the report.