Sparkn

CodeFox Inc.
DeFiFoundryProxy
15,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

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

bauchibred

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
/**
* @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
/**
* @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);
}

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.