ProxyFactory imports OpenZeppelin's Ownable.sol which lacks a 2-step ownership transfer

[codehawks-bot]

ProxyFactory imports OpenZeppelin's Ownable.sol which lacks a 2-step ownership transfer

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L26

Summary

The owner is essentially the backbone for SparkN, i.e everything directly/indirectly falls back to be their responsibility from deployment down to cases where if an organizer does not distribute the rewards after the expiration time it's the owner's job to do this, but current implementation puts the whole protocol at a risk when owners are going to be changed.

Vulnerability Detail

First do note that the ProxyFactory.sol contract inherits OpenZeppelin's Ownable.sol

Click to see code reference

```solidity /** * @dev Transfers ownership of the contract to a new account (`newOwner`). * Can only be called by the current owner. */ function transferOwnership(address newOwner) public virtual onlyOwner { if (newOwner == address(0)) { revert OwnableInvalidOwner(address(0)); } _transferOwnership(newOwner); } /** * @dev Transfers ownership of the contract to a new account (`newOwner`). * Internal function without access restriction. */ function _transferOwnership(address newOwner) internal virtual { address oldOwner = _owner; _owner = newOwner; emit OwnershipTransferred(oldOwner, newOwner); } ```

As summarized in Summary, it's key to note that the onlyOwner is an important modifier which enables certain functions of the contracts (including the distributeByOwner() in the case where organizer doesn't distribute it before the expiration) to be only executed by the owner.

The above means that we all would agree that the owner of the contract is too important and needs to be handled with optimum care, especially when trying to change the ownership, an extra caution should be applied to the process, cause as at present implementation the wau the ownership is handled the entire protocol would be broken if anything goes wrong since all onlyOwner modifier functions will not be usable there after.

One step ownership change could lead to irreversibly setting a wrong and address but 2 step ownership change would allow the new owner to confirm their address to effect the change. This could lead to rendering onlyOwner functions inoperable.

Impact

Impact is High cause asides the complete of halt of protocol when it comes to deployment of new contests, when this happens for any contest an organizer does not distribute the rewards then the funds for winners are completely stuck in the contract

Tool used

Manual Audit

Recommendation

Consider importing and inheriting the OpenZeppelin's Ownable2Step.sol contract instead, since it implements two- step transfers for the owner address

Click to see code reference

```solidity /** * @dev Starts the ownership transfer of the contract to a new account. Replaces the pending transfer if there is one. * Can only be called by the current owner. */ function transferOwnership(address newOwner) public virtual override onlyOwner { _pendingOwner = newOwner; emit OwnershipTransferStarted(owner(), newOwner); } /** * @dev Transfers ownership of the contract to a new account (`newOwner`) and deletes any pending owner. * Internal function without access restriction. */ function _transferOwnership(address newOwner) internal virtual override { delete _pendingOwner; super._transferOwnership(newOwner); } /** * @dev The new owner accepts the ownership transfer. */ function acceptOwnership() public virtual { address sender = _msgSender(); require(pendingOwner() == sender, "Ownable2Step: caller is not the new owner"); _transferOwnership(sender); } ```

[serial-coder]

Escalate

This should be a MEDIUM issue (if we're using the same judging criteria as the Beedle contest).

For more details, please look at my issue #900.

[0xRizwan]

Escalate

This is an invalid issue.

First of all there is NO transfer of ownership is happening in contract.

ProxyFactory has inherited from openzeppelin ownable.sol

contract ProxyFactory is Ownable, EIP712 {

This can be further seen in constructor where it uses ownable()

    constructor(address[] memory _whitelistedTokens) EIP712("ProxyFactory", "1") Ownable() {

Please be noted, Ownable(() will by default transfer the ownership of contract to the contract depoyer address. This can be checked here

abstract contract Ownable is Context {

 ....

    /**
     * @dev Initializes the contract setting the deployer as the initial owner.
     */
    constructor() {
        _transferOwnership(_msgSender());
    }

The issue would have been valid in case there is direct transfer of ownership via. some function, but this is not applicable here.

[serial-coder]

First of all there is NO transfer of ownership is happening in contract.

I argue the above statement.

The owner can execute the transferOwnership().

[PatrickAlphaC]

Yeah... This is bad input by trusted user -> which isn't valid or low at best. I think we need to move away from this as a low and should just be a default info/qa.