sherlock-audit / 2023-01-optimism-judging

25 stars 10 forks source link

unforgiven - [Medium] SystemDictator transfer ownership in one step and without checking for 0x0 address, any mistake can cause loss of admin access to protocol contracts #181

Closed github-actions[bot] closed 1 year ago

github-actions[bot] commented 1 year ago

unforgiven

medium

[Medium] SystemDictator transfer ownership in one step and without checking for 0x0 address, any mistake can cause loss of admin access to protocol contracts

Summary

Contract The SystemDictator is responsible for coordinating the deployment of a full Bedrock system. there are some ownership transfers in the functions of the contract and there is no check for 0x0 and also the ownership transferring is done in one step. any mistake can cause total loss of admin access to protocol contracts and funds would be lost.

Vulnerability Detail

Contract SystemDictator transfers ownership of some contract during the deployment process. Function initialize() transfer ownership of the SystemDictator to config.globalConfig.controller but there is no check that this new address is not 0x0 and also ownership transfer is done in one step.

    function initialize(DeployConfig memory _config) public initializer {
        config = _config;
        currentStep = 1;
        __Ownable_init();
        _transferOwnership(config.globalConfig.controller);
    }

In Function finalize() code transfer ownership of the contracts l1CrossDomainMessenger, proxyAdmin, addressManager and l1StandardBridge and l1ERC721Bridge to config.globalConfig.finalOwner without checking the value and without performing the action in two step.

if there were any mistake during the deployment and migration and ownership of the contracts transferred to 0x0 address then the admin access to protocol contract would be lost and may cause fund loss too.

Impact

any mistake can cause total admin access loss and probable funds losses.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/deployment/SystemDictator.sol#L145-L153

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/deployment/SystemDictator.sol#L249-L267

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/deployment/SystemDictator.sol#L375-L403

Tool used

Manual Review

Recommendation

when the impact of the mistake is too high then implement two step ownership transfer and also check for the 0x0 address.

rcstanciu commented 1 year ago

Comment from Optimism


Description: Dicator might transfer ownership of system to 0 address

Reason: Cannot prevent all classes of user error, including by ourselves.