Owner Change Implementation

[0d4rujd]

Finding discovered during Red4Sec security audit.

Description

As it has been detected in the code, currently the smart contract does not contain any functionality to change the owner of the contract. This functionality may be very useful in the future, so we highly recommend using an OpenZeppelin's library that allows you to change the owner.

This library is https://github.com/OpenZeppelin/openzeppelin-contracts-ethereum-package/blob/master/contracts/ownership/Ownable.sol and it would be enough if the contract CentralizedBlockRelay.sol:26-28 and WitnetRequestBoard.sol:129 inherited Ownable.sol contract from OpenZeppelin to be able to modify the owner.
 

Impact

We consider that including this feature would be a good option in some extreme cases such as;

• Transfer of the Token and its Infrastructure to third-parties.
• Cyberattacks.
• Human Failures.
• Exposure of the owner's private key.
• Internal attacks of disgruntled employees or anyone who at some point have had access to the private key.

By implementing this new feature, the team could face these possible situations in the future.  

Code Reference

https://github.com/witnet/witnet-ethereum-bridge/blob/cf4ffe9434be79ec29079a3fcaf885adfb213cb8/contracts/WitnetRequestsBoard.sol#L129

https://github.com/witnet/witnet-ethereum-block-relay/blob/a3bf55cc58dd8295d3ac4daed45de48a708823fc/contracts/CentralizedBlockRelay.sol#L27-L28

 

Mitigation

• Implement mechanisms for modifying the contract owner.
• Inherit contracts from OpenZeppelin since these have been previously audited and can facilitate this implementation.

[aesedepece]

I may be wrong on this, but to my best knowledge this feature is already enabled implicitly by its proxy.

Given that the proxy allows updating the address of the WRB "controller" as long as the conditions set by the current instance are met, my impression is that replacing the current instance for a different one that sets a new owner offers a somehow equivalent degree of functionality in terms of dealing with the extreme situations you explained.

Of course replacing the WRB could have a bigger cost impact (as a new instance needs to be deployed) and there could be long term consequences of a lengthier controller history. The consequences of replacing the WRB for a bridge node operator are not trivial either, but that's easy to if the bridges use the proxy to resolve the address of the latest controller.

So, taking all of that into account, and also considering that CentralizedBlockRelay.sol is a provisional contract that should only live on mainnet for as little time as possible and should be superseded by ABSBlockRelay.sol as soon as possible, is still your recommendation in force?

Thanks in advance for your feedback!