code423n4 commented 1 year ago

Lines of code

https://github.com/ERC725Alliance/ERC725/blob/7171a0e25e83cfe4c4dec6262bb62b4422c0478f/implementations/contracts/custom/OwnableUnset.sol#L46

Vulnerability details

Impact

The OwnableUnset contract's current implementation does not enforce a two-step ownership transfer process, which is typically used to enhance the security of the contract. The two-step process usually consists of 1) the current owner proposing a new owner and 2) the new owner accepting the ownership. This process reduces the chances of transferring the ownership to a wrong or malicious address due to an error or attack.

In the OwnableUnset contract, the transferOwnership(address newOwner) function immediately transfers the ownership to the newOwner as long as the caller of the function is the current owner and the newOwner is not the zero address. This one-step process could pose a security risk.

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

Suggested Fix: To incorporate a two-step process, the contract can introduce an additional state variable _proposedOwner, which can be set in transferOwnership(address newOwner). The _proposedOwner can then claim the ownership using a separate function acceptOwnership(). Here's an example of how it could be implemented:

address private _proposedOwner;

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

function acceptOwnership() public virtual {
    require(msg.sender == _proposedOwner, "Ownable: caller is not the proposed owner");
    _setOwner(_proposedOwner);
    _proposedOwner = address(0);
}

In this proposed change, the current owner "proposes" a new owner by calling transferOwnership(). The proposed new owner then "accepts" the ownership by calling acceptOwnership(). This helps to reduce the risk of accidental or erroneous ownership transfers.

Severity: Medium

This issue may lead to potential ownership hijack if the contract owner accidentally inputs an incorrect or malicious address while calling the transferOwnership() function. The severity can be classified as medium as it could lead to unauthorized control over the contract, but it can only be exploited due to the owner's error, not by external attackers.