Zero address & `bytes32` zero value checks for additional safety

[hats-bug-reporter[bot]]

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

Description:

Description

Governance

It's considered best practice to implement zero address checks to prevent accidents. Contracts would have to be redeployed if one of the critical addresses remains zero upon initialization. There are also no checks whether the comittee is a zero address in setComittee().

User

Another problem could be that the user sends a sendRequest() or sendRequestNative() with bytes32(0) as a destReceiverAddress accidentally. These funds will be lost if there are no additional safeguards implemented in the relayer.

Most.sol - initialize()

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

Worth to also mention that __Ownable_init() has no zero address checks, however __Ownable_init_unchained() has.

Recommendation

Consider to implement zero address checks to prevent governance accidents. Consider to implement destination receiver zero value bytes32 checks to prevent users from losing funds mistakenly sent to a null destReceiverAddress.

[krzysztofziobro]

Valid submission - minor

[rodiontr]

Valid submission - minor

It’s more of a recommendation than a vulnerability - it says “it’s a best practice” so it’s your design choice not to add this check. Hence not a vulnerability

[0xfuje]

It’s more of a recommendation than a vulnerability - it says “it’s a best practice” so it’s your design choice not to add this check. Hence not a vulnerability

Based on AZERO contest guidelines by Hats Minor severities are: Attacks not mentioned in higher categories, but having a negative impact on user experience, or putting one of the contracts in an unexpected state.

Does leaving bytes32 zero value option to sendRequest() functions has a negative impact on user experience? Yes, as the user can lose funds.

Does allowing setComittee() and initialize() a zero address option put contracts in an unexpected state? Yes, as a zero address is not expected as a comittee member and will dilute the comittee size. The contract would be in an unexpected state if initialize() would be called with zero address for _wethAddress or owner and would have to be redeployed.

I know these are not big improvements, but that's why I think minor severity is appropriate here as the additional safety checks outweigh the very minimal gas cost + complexity. Of course I leave the final decision to the judge on the severity.

[0xfuje]

Fix for #10 reviewed, looks good!