MohammedRizwan - Single-step process for critical ownership transfer can be dangerous

[sherlock-admin]

MohammedRizwan

medium

Single-step process for critical ownership transfer can be dangerous

Summary

Single-step process for critical ownership transfer can be dangerous

Vulnerability Detail

Impact

The EmergencyOracle.sol and Perpetual.sol contract are one of the most important contract in the project. The ownership of this contract is transferred to new owner address by constructor in both the smart contracts. This critical address transfer in one-step ownership transfer which is very risky as it is irrecoverable from any mistakes.

For ownership transfer, it uses openzeppelinOwnable.sol, transferOwnership( ) function as shown below,

File: contracts/access/Ownable.sol

69    function transferOwnership(address newOwner) public virtual onlyOwner {
71        require(newOwner != address(0), "Ownable: new owner is the zero address");
72        _transferOwnership(newOwner);
73    }

Link to code

As seen above the function set the newOnwer in single step without having approval from newOwner address. If an incorrect address, e.g. for which the private key is not known, is used accidentally then it prevents the use of all the onlyOwner( ) functions forever, which includes the changing/setting of various critical functions in both smart contracts as below,

1)EmergencyOracle.turnOnOracle() function, 2)EmergencyOracle.sol.turnOffOracle() function, 3)EmergencyOracle.setMarkPrice function, 4)EmergencyOracle.setFeeTo( ) function.

5)Perpetual.updateFundingRate() function,

This use of incorrect address may not even be immediately apparent given that these functions are probably not used immediately. When noticed, due to a failing onlyOwner( ) function call, it will force the redeployment of the EmergencyOracle.sol and Perpetual.sol contracts and require appropriate changes and notifications for switching from the old to new address. This will diminish trust in markets and can incur a significant reputational damage.

Code Snippet

In EmergencyOracle.sol,

File: contracts/adaptor/emergencyOracle.sol

26    constructor(address _owner, string memory _description) Ownable() {
27        transferOwnership(_owner);
28        description = _description;
29    }

Link to code

In Perpetual.sol,

File: contracts/impl/Perpetual.sol

50    constructor(address _owner) Ownable() {
51       transferOwnership(_owner);
52    }

Link to code

See similar High Risk severity finding from Trail-of-Bits Audit of Hermez: Link to reference

See similar Medium Risk severity finding from Trail-of-Bits Audit of Uniswap V3: Link to reference

Tool used

Manual Review

Recommendation

1)Use Openzeppelin Ownable2Step.sol for transferOwnership. Ownable2Step is safer than Ownable for smart contracts because the owner cannot accidentally transfer smart contract ownership to a non-existent address. Rather than directly transferring to the new owner, the transfer only completes when the new owner accepts ownership.

Here are the docs and the code reference links, Ownable2Step docs Ownable2Step code