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

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

Attacker can initialize `Most.sol` to set critical parameters #4

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

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

Github username: @0xfuje Twitter username: 0xfuje Submission hash (on-chain): 0x0cc5d64fcb1309e8ea2a654336ebcb870e364cc55ea00ec7eed9ab765aec8c5b Severity: low

Description:

Description

Most.sol implements openzeppelin's upgradeable model. The problem is that it's unprotected from an attacker initializing the contract. The uninitialized contract can be taken over by the attacker for example by front-running the original deployer's intialize() call. This applies to both the proxy and its implementation contract.

See OpenZeppelin's comment in their documentation:

Do not leave an implementation contract uninitialized. An uninitialized implementation contract can be taken over by an attacker, which may impact the proxy. To prevent the implementation contract from being used, you should invoke the _disableInitializers function in the constructor to automatically lock it when it is deployed:

    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();
    }

Recommendation

Consider to add _disableInitializers() function and the constructor to Most.sol:

constructor() {
    _disableInitializers();
}
krzysztofziobro commented 6 months ago

As there is no clear vulnerability here, but one of the contracts can indeed be put in an unexpected state:

Valid submission - minor

0xfuje commented 6 months ago

Fix for #4 reviewed, looks good!