Functions such as makePayout, setWithdrawPaused, startVault, destroyVault having the `onlyClaimsManager` modifier will always revert because of the wrongly written `onlyClaimsManager` modifier

[hats-bug-reporter[bot]]

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

Description: Description\ The HATVault.sol contract uses Openzeppelin's ContextUpgradeable.sol's _msgSender() through the OwnableUpgradeable contract the HATVault.sol imported.

The _msgSender() is automatically set to the address of the HATVault.sol's deployer address.

Now, the onlyClaimsManager modifier says if claimsManager is not _msgSender(), revert should occur:

if (claimsManager != _msgSender()) revert OnlyClaimsManager();

However, claimsManager variable was set to a new address in the initialize function. It means the claimsManager address is different from the deployer address (_msgSender()).

Where the problem lies is in the construction of the onlyClaimsManager modifier. That is, if claimsManager is not _msgSender(), revert.

When the claimsManager address is set in the initialize function, the claimsManager will forever not be the _msgSender().

As a result, functions such as makePayout, setWithdrawPaused, startVault, destroyVault having the onlyClaimsManager modifier will always revert.

Attack Scenario\ Functions such as makePayout, setWithdrawPaused, startVault, destroyVault having the onlyClaimsManager modifier will always revert because of the wrongly written onlyClaimsManager modifier.

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

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

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

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

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

1. Proof of Concept (PoC) File

   Here's a foundry test similar to the vulnerability described:

// SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.13;

contract Testing {

address claimManager; address owner;
uint256 number; error WrongClaimManager();

modifier onlyClaimanager() { if(claimManager != owner) revert WrongClaimManager(); _; }

function initialize(address _claimManager) public { claimManager = _claimManager; }

function addNumber(uint256 _number) public onlyClaimanager { number = _number; } }

// SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.13;

import {Test, console2} from "forge-std/Test.sol"; import {Testing} from "../src/Testing.sol";

contract TestingTest is Test { Testing public testing; address owner;

   function setUp() public {
    testing = new Testing();
    owner = msg.sender;
}

function testaddNumber() public {
    testing.initialize(address(2));
    vm.startPrank(owner);
    vm.expectRevert("WrongClaimanager");
    testing.addNumber(5);
    vm.stopPrank();
}

}

2. Recommendation

   I suggest _msgSender() shouldn't be used in the onlyClaimsManager modifier. Rather, the modifier should be written this way:

modifier onlyClaimsManager() { if (claimsManager != msg.sender) revert OnlyClaimsManager(); _; }

[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, claimsManager was set again in the initialize function.

[jellegerbrandy]

@ololade97 we have tests for the behavior of the contracts which would fail if what you said were true

[ololade97]

@jellegerbrandy I'm not privileged to see the test. Based on the contract, there is a difference between the _msgSender() and msg.sender.

Besides, no reason was given for invalidated the report.

[ololade97]

*invalidating

[ololade97]

I've seen a protocol test their code but the test coverage is limited.

[ololade97]

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

[jellegerbrandy]

The report is marked invalid because the behaviour that you describe (that "Functions such as makePayout, setWithdrawPaused, startVault, destroyVault having the onlyClaimsManager modifier will always revert ") does not actually occur.

The function startVault is called when HATSClaimsManager.committeeCheckin is called. There are various tests that are part of this repository and run for each commit in github actions that show the function behaves as expected

[ololade97]

@jellegerbrandy the tests you are referring to don't cover the vulnerabilityy described in this report.