Remove redundant access control checks can save gas

[code423n4]

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-10-slingshot/blob/9c0432cca2e43731d5a0ae9c151dacf7835b8719/contracts/ModuleRegistry.sol#L35-L64

/// @param _moduleAddress Address of the module to register
function registerSwapModule(address _moduleAddress) public onlyAdmin {
    require(!modulesIndex[_moduleAddress], "oops module already exists");
    require(ISlingshotModule(_moduleAddress).slingshotPing(), "not a module");

    modulesIndex[_moduleAddress] = true;
    emit ModuleRegistered(_moduleAddress);
}

/// @param _moduleAddresses Addresses of modules to register
function registerSwapModuleBatch(address[] memory _moduleAddresses) external onlyAdmin {
    for (uint256 i = 0; i < _moduleAddresses.length; i++) {
        registerSwapModule(_moduleAddresses[i]);
    }
}

/// @param _moduleAddress Address of the module to unregister
function unregisterSwapModule(address _moduleAddress) public onlyAdmin {
    require(modulesIndex[_moduleAddress], "module does not exist");

    delete modulesIndex[_moduleAddress];
    emit ModuleUnregistered(_moduleAddress);
}

/// @param _moduleAddresses Addresses of modules to unregister
function unregisterSwapModuleBatch(address[] memory _moduleAddresses) external onlyAdmin {
    for (uint256 i = 0; i < _moduleAddresses.length; i++) {
        unregisterSwapModule(_moduleAddresses[i]);
    }
}

registerSwapModuleBatch() and unregisterSwapModuleBatch() already got onlyAdmin check, and registerSwapModule() will check it again multiple times.

Consider creating two private functions without access control and call them inside the public functions.

Recommendation

Change to:

function _registerSwapModule(address _moduleAddress) private {
    require(!modulesIndex[_moduleAddress], "oops module already exists");
    require(ISlingshotModule(_moduleAddress).slingshotPing(), "not a module");

    modulesIndex[_moduleAddress] = true;
    emit ModuleRegistered(_moduleAddress);
}

/// @param _moduleAddress Address of the module to register
function registerSwapModule(address _moduleAddress) public onlyAdmin {
    _registerSwapModule(_moduleAddress);
}

/// @param _moduleAddresses Addresses of modules to register
function registerSwapModuleBatch(address[] memory _moduleAddresses) external onlyAdmin {
    for (uint256 i = 0; i < _moduleAddresses.length; i++) {
        _registerSwapModule(_moduleAddresses[i]);
    }
}

function _unregisterSwapModule(address _moduleAddress) private {
    require(modulesIndex[_moduleAddress], "module does not exist");

    delete modulesIndex[_moduleAddress];
    emit ModuleUnregistered(_moduleAddress);
}

/// @param _moduleAddress Address of the module to unregister
function unregisterSwapModule(address _moduleAddress) public onlyAdmin {
    _unregisterSwapModule(_moduleAddress);
}

/// @param _moduleAddresses Addresses of modules to unregister
function unregisterSwapModuleBatch(address[] memory _moduleAddresses) external onlyAdmin {
    for (uint256 i = 0; i < _moduleAddresses.length; i++) {
        _unregisterSwapModule(_moduleAddresses[i]);
    }
}

[tommyz7]

Duplicate of #98