hats-finance / Metrom-0xfdfc6d4ac5807d7460da20a3a1c0c84ef2b9c5a2

Smart contracts for the Metrom project.
GNU General Public License v3.0
0 stars 0 forks source link

Missing access control on initialize() function #32

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

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

Github username: -- Twitter username: MrPotatoMagic Submission hash (on-chain): 0xa8b676c16f64bdab432bd794f1018073da4fcfde6307a27b6b4ffc2ad7489bc0 Severity: medium

Description: Description\ The initialize() function is used to initialize the configuration/state variables of the system.

Attack Scenario\ Since the function is missing access control, anyone can frontrun the team to initialize the contract with malicious/unintended values. This DOSes the team from initializing the contract correctly. If gone unnoticed by the team, it could cause problems since the owner, updater, fees and campaign durations could be changed anytime.

Attachments See here - https://github.com/hats-finance/Metrom-0xfdfc6d4ac5807d7460da20a3a1c0c84ef2b9c5a2/blob/e9d6b1e594d5bb3694bfe68f73399156ebb5d3a4/src/Metrom.sol#L70

  1. Proof of Concept (PoC) File

As we can see below, the function is missing access control, thus allowing anyone to call it.

    function initialize(
        address _owner,
        address _updater,
        uint32 _globalFee,
        uint32 _minimumCampaignDuration,
        uint32 _maximumCampaignDuration
    ) external override initializer {

Mitigation: Instead of setting up the owner variable in the initialize() function, consider setting it up in the constructor and then using that value to apply an access control modifier on the initialize() function.

luzzif commented 4 months ago

The initialize function is there because the contract is intended to be used with a ERC1967Proxy. Extra care was taken to make sure nobody could tamper or initialize the Metrom smart contract "directly" through the _disableInitializer() call in the constructor. That only makes it possible for the contract to be used when proxied, and obviously it will be of utmost importance to call the initialize function as soon as the proxy is deployed. You can see that happens in the Deploy script. As such, I don't think this issue is valid, but I'd be happy to see a PoC.