hats-finance / Most--Aleph-Zero-Bridge-0xab7c1d45ae21e7133574746b2985c58e0ae2e61d

Aleph Zero bridge to Ethereum
Apache License 2.0
0 stars 1 forks source link

`requestNonce` must be initialize in `initialize()` function #38

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

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

Github username: -- Twitter username: -- Submission hash (on-chain): 0xd5e77af60ffeb6596482e2e6d64f0fe9ebffe4b8c82d2ed83a94d01bbabf9d91 Severity: low

Description: Description\

In Most.sol, initialize() function is used to intitialize the Most contract which is an upgradeable contract with input arguments to set the state variables.


    function initialize(
        address[] calldata _committee,
        uint256 _signatureThreshold,
        address owner,
        address payable _wethAddress
    ) public initializer {
        committeeId = 0;
        wethAddress = _wethAddress;

        _setCommittee(_committee, _signatureThreshold);

        __Ownable_init(owner);
        __Pausable_init();
        _pause();
    }

State variables like, committeeId and wethAddress is initialized with their values, However, requestNonce is missed in initialize() function.

    uint256 public requestNonce;     @audit // missed initialization
    uint256 public committeeId;
    address payable public wethAddress;

Solidity allows defining initial values for fields when declaring them in a contract.In current implementation being an upgradeable contract, this is equivalent to setting these values in the constructor, and as such, will not work for upgradeable contracts.

It must be ensure that all initial values are set in an initializer function as shown recommendation otherwise, any upgradeable instances will not have this fields set.

For more details, Below link can be referred- https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#avoid-initial-values-in-field-declarations

POC\

1) When the owne plans to upgrade the Most contract, there would be an issue with requestNonce will always be defaulted to 0 in upgradeable contract instance. This is due to the fact that openzeppelin specifically mentions It must be ensure that all initial values are set in an initializer function, in Most current implementation, requestNonce is missed to initialize. 2) Therefore, when the upgradeable instance would be created, requestNonce will not work for upgradeable contracts. 3) This is due to its value missed to initialize as done similar to committeeId and wethAddress.

This issue is made low severity as it would break the design in upgradeable most contract instance. If the sponsor finds it more severe then i believe severity can be upgraded to Medium.

Recommendation to fix\ Initialize requestNonce in initialize().


    function initialize(
        address[] calldata _committee,
        uint256 _signatureThreshold,
        address owner,
        address payable _wethAddress
    ) public initializer {
+      requestNonce = 0;
        committeeId = 0;
        wethAddress = _wethAddress;

        _setCommittee(_committee, _signatureThreshold);

        __Ownable_init(owner);
        __Pausable_init();
        _pause();
    }
krzysztofziobro commented 4 months ago

requestNonce is initialized to 0 by default, so this is just about clarity, right?

0xRizwan commented 4 months ago

requestNonce is initialized to 0 by default, so this is just about clarity, right?

requestNonce is one of the important variable in contract so when the contract will be upgraded, requestNonce should be the part of upgradeable instance therefore, initializing requestNonce in initialize() function is important as you did for other variables like committeeId and wethAddress.

krzysztofziobro commented 4 months ago

Valid submission - minor