Missing zero address checks

[hats-bug-reporter[bot]]

Github username: @erictee2802 Twitter username: 0xEricTee Submission hash (on-chain): 0xf646b2d965867a74bd44065300b9ca07c9f31ecab9afe46b1aa70766252bf3b7 Severity: low

Description: Description\

constructors lacks zero address validation:

OnRecvIncentivizedMockEscrow.sol:

   constructor(address sendLostGasTo, address messagingProtocol) IMETimeoutExtension(sendLostGasTo) { 
        MESSAGING_PROTOCOL_CALLER = messagingProtocol;
        UNIQUE_SOURCE_IDENTIFIER = bytes32(uint256(111));  // Actual implementation should call to messagingProtocol
    }

Missing zero address check for MESSAGING_PROTOCOL_CALLER.

IncentivizedWormholeEscrow.sol:

  constructor(address sendLostGasTo, address wormhole_) IncentivizedMessageEscrow(sendLostGasTo) WormholeVerifier(wormhole_) {
        WORMHOLE = IWormhole(wormhole_);
    }

Missing zero address check for wormhole_. (Note: check for zero address for parameter sendLostGasTo in IncentivizedMessageEscrow.sol).

IncentivizedMessageEscrow.sol:

    constructor(address sendLostGasTo) {
        SEND_LOST_GAS_TO = sendLostGasTo;
    }

Missing zero address check for sendLostGasTo.

CatalystChainInterface.sol:

   constructor(address GARP_, address defaultOwner) {
        require(address(GARP_) != address(0));  // dev: GARP_ cannot be zero address
        GARP = IIncentivizedMessageEscrow(GARP_);
        _transferOwnership(defaultOwner);

        emit MaxUnderwriteDuration(INITIAL_MAX_UNDERWRITE_BLOCK_DURATION);
    }

Missing zero address check for defaultOwner.

CatalystFactory.sol:

 constructor(address defaultOwner) {
        _transferOwnership(defaultOwner);
        _governanceFeeDestination = defaultOwner;
    }

Missing zero address check for defaultOwner.

CatalystVaultCommon.sol:

    constructor(address factory_, address mathlib) ERC20("Catalyst Vault Template", "", DECIMALS) {
        FACTORY = factory_;
        MATHLIB = mathlib;

        // Disable the contract from being initialized. This ensures the factory is
        // used to deploy vaults.
        _disableInitializers();
    }

Missing zero address check for factory_ & mathlib.

Attack Scenario\

Since parameter of costructor are used to initialize state variable which are used in other function of the contract, error in these state variable can lead to redeployment of contract.

Attachments

NA

1. Proof of Concept (PoC) File

   NA

2. Revised Code File (Optional)

   It is recommended to add zero address validation.

[reednaa]

Please describe how thebehavior of the contracts would differ from the intended behavior, where common sense would not apply.

Many of the reports are also known. See audit by Veridise.

[0xEricTee]

@reednaa, From CatalystChainInterface.sol contract, we know that the code checks for require(address(GARP_) != address(0)); // dev: GARP_ cannot be zero address, meaning that protocol team are aware that some parameters need zero address checks, however they forget to check also that require(address(defaultOwner) != address(0));, if defaultOwner address ever got set to address(0), it is equivalent to renounceOwnership() function from Ownable.sol. As a result, all functions with onlyOwner modifier set cannot be called successfully forever. With that being said, I think zero address checks are necessary.

[reednaa]

The owner address is dangerous in all of the contracts. They can cause the protocol to behave in unintended ways or even drain the vaults.

Setting the owner to address(0) seems like a feature rather than a bug.

[0xEricTee]

@reednaa Yes I totally agree with your point, but the thing is it's in constructor, all the variables are still not yet initialized. For instance, if address(0) is directly passed in to the CatalystChainInterface.sol's constructor, all the setters such as CatalystChainInterface.sol::setMinGasFor, CatalystChainInterface.sol::setMaxUnderwritingDuration and CatalystChainInterface.sol::connectNewChain will never be able to call successfully. If the intention is to set owner's address to address(0), renounceOwnership() function should be used instead.

[reednaa]

setMaxUnderwritingDuration does not need to be called but you can make a case for the other 2. Let me discuss internally but I am leaning won't fix.