aycozynfada - Protocol might be inoperable because _extractModule checks for address zero instead of setModule

[sherlock-admin2]

aycozynfada

high

Protocol might be inoperable because _extractModule checks for address zero instead of setModule

Summary

_executeModule function first of all calls the _extractModule to extract the Module address, the _extractModule function then checks for address zero before returning the module address and reverts if the module address is a zero address. This check however might be to late as it leads to a failure in the functionality of the protocol and makes the smart contract inoperable.

Vulnerability Detail.

Although sherlock states that zero address issues are not under scope, this I suppose can cause restrictions to user transactions because the _extractModule function contains address zero check instead of the _setModule function. With the way the functions are set, a zero address can be hard coded to a module, which will there after lead to issues when a user attempts a legitimate transaction. To have failed user transactions due to zero address, the check should have been implemented in the _setModule function

Impact

protocol become inoperable as expected returns are not processed

Code Snippet

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/usdo/modules/ModuleManager.sol#L38-L74

_executeModule function first of all calls the _extractModule to extract the Module address

    function _executeModule(uint8 _module, bytes memory _data, bool _forwardRevert)
        internal
        returns (bytes memory returnData)//execute module should return both success and return data to usdoReceiver
        //as _executeModule might sometimes return garbarge incorrect data.
    {
        bool success = true;
        address module = _extractModule(_module);

        (success, returnData) = module.delegatecall(_data);
        if (!success && !_forwardRevert) {// doesn't forward revert msg since !sucess is false
            revert(RevertMsgDecoder._getRevertMsg(returnData));
        }

the extract module however checks for zero address before returning the module

  function _extractModule(uint8 _module) internal view returns (address) {
        address module = _moduleAddresses[_module];
        if (module == address(0)) revert ModuleManager__ModuleNotAuthorized();

        return module;
    }

this check however should have been incorporated primarily in the _setModule function to avoid  a zero address been set right from the on set
```solidity
 function _setModule(uint8 _module, address _moduleAddress) internal {
    _moduleAddresses[_module] = _moduleAddress; //doesn;t check if module is address zero before setting
    //instead it checks whether required module from _extractmodule is address zero at which point it would have been too late
    //and there fore leads to a failed transaction
}

## Tool used

Manual Review, vs code

## Recommendation
put zero address check in the _setModule function instead of the _extractModule function

[cryptotechmaker]

Invalid; I don't understand the issue. Modules are not represented by address(0); Also the extractModule purposely checks if the module is address(0) and in this case reverts.

[nevillehuang]

Invalid, modules are contracts that can never be address zero, so this is basically invalid based on the following sherlock rule

3. Zero address checks: Check to make sure input values are not zero addresses