Lack of two step process change of critical address roles especially where lacking address(0) checks

Summary

Access controlled roles can be lost due to lack of 2 step process changes

Vulnerability Detail

configManager set as deployer in constructor is changed in a single step

Impact

This role is responsible for various whitelist functions and if lost e.g address(0) as additionally changing it does not also check if zero address or transferred to owner without keys or access or to wrong address by error can result in inability to change key admin functions or malicious changes like adding NFT Managers of choice etc

Code Snippet

  function updateConfigMaster(address _configMaster) external override onlyConfigMaster {
    emit ConfigMasterUpdated(configMaster, _configMaster);
    configMaster = _configMaster;
  }

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/main/ks-elastic-sc/contracts/Factory.sol#L114C1-L118C1

Tool used

Manual Review

Recommendation

It is recommended to make use of 2 step process to change critical admin or access controlled roles to avoid loss of these roles by ensuring role is proposed and then claimed first. Ideally can inherit and make use of OpenZeppelin Ownable2Step contracts

sherlock-audit / 2023-07-kyber-swap-judging

MatricksDeCoder - Lack of two step process change of critical address roles especially where lacking address(0) checks #79