code423n4 commented 1 year ago

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L9

Vulnerability details

Severity

Likelihood: Low, because it requires an error on the admin side

Impact: High, because important protocol functionality will be bricked

Impact

Single-step ownership transfer means that if a wrong address was passed when transferring ownership or admin rights it can mean that role is lost forever. The ownership pattern implementation for the protocol is in OwnableUpgradeable.sol where a single-step transfer is implemented. This can be a problem for all methods marked in onlyOwner throughout the protocol, some of which are core protocol functionality.

Tools Used

Manual audit

Recommended Mitigation Steps

It is a best practice to use a two-step ownership transfer pattern, meaning ownership transfer gets to a "pending" state and the new owner should claim his new rights, otherwise, the old owner still has control of the contract. Consider using OpenZeppelin's Ownable2Step contract

c4-judge commented 1 year ago

kirk-baird changed the severity to QA (Quality Assurance)

c4-sponsor commented 1 year ago

waynehoover marked the issue as sponsor acknowledged