Critical uberOwner address changes should be a two-step process

[code423n4]

Handle

0xRajeev

Vulnerability details

Impact

As specified, uberOwners of Factory, Orderbook and Treasury have the highest privileges in the system because they can upgrade contracts of market, Nfthub, order book, treasury, token and factory which form the critical components of the protocol.

The contracts allow for uberOwners to be changed to a different address from the contract owner/deployer using the changeUberOwner() function which is callable by the current uberOwner. While this function checks for zero-address, there is no validation of the new address being correct. If the current uberOwner incorrectly uses an invalid address for which they do not have the private key, then the system gets locked because the uberOwner cannot be corrected and none of the other functions that require uberOwner caller can be executed.

Impact: The current uberOwner uses a non-zero but incorrect address as the new uberOwner. This gets set and now the system is locked and none of the uberOwner-only callable functions are callable. This error cannot be fixed either and will require redeployment of contracts which will mean that all existing markets have to be terminated. The system will have to be shut and restarted completely from scratch which will take a reputation hit and have a serious technical and business impact.

Proof of Concept

https://github.com/code-423n4/2021-06-realitycards#mortar_board-governance-mortar_board

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCFactory.sol#L444-L449

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCOrderbook.sol#L117-L121

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCTreasury.sol#L264-L268

Tools Used

Manual Analysis

Recommended Mitigation Steps

Change the single-step change of uberOwner address to a two-step process where the current uberOwner first approves a new address as a pendingUberOwner. That pendingUberOwner has to then claim the ownership in a separate transaction which cannot be done if they do not have the correct private key. An incorrectly set pendingUberOwner can be reset by changing it again to the correct one who can then successfully claim it in the second step.

[Splidge]

Duplicate of #5

[dmvt]

There is a very low probability coupled with a very high impact, making this a Medium risk issue in my opinion.