Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Missing Transfer Ownership Pattern #62

Open
code423n4 opened this issue Sep 20, 2021 · 2 comments
Open

Missing Transfer Ownership Pattern #62

code423n4 opened this issue Sep 20, 2021 · 2 comments
Labels
1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Warden finding disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

Handle

leastwood

Vulnerability details

Impact

Factory.sol inherits Openzeppelin's Ownable contract which is used to restrict certain functions to a specific owner account. By default, the owner account is set to the deployer of Factory.sol. However, the owner can transfer ownership of this contract by calling transferOwnership() with the newOwner as input. If the current owner of the contract mistakenly transfers ownership to an account that is not actively controlled, the ownership of Factory.sol will be lost.

Proof of Concept

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider implementing a proper transfer ownership pattern whereby the current owner nominates a new owner. This new owner then has to claim ownership for the change to be applied. This can be done by overriding the current Ownable implementation of transferOwnership() and adding a claimOwnership() function alongside it.
@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Warden finding labels Sep 20, 2021
code423n4 added a commit that referenced this issue Sep 20, 2021
@code423n4
leastwood issue #62
210bd31
@frank-beard frank-beard added sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) labels Oct 19, 2021
@frank-beard
Copy link
Collaborator

not an exploit but a good point to bring up. it is assumed the owner is trusted.

@GalloDaSballo
Copy link
Collaborator

Similarly to #185 I agree with the finding but will downgrade to Low

@GalloDaSballo GalloDaSballo added 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Dec 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Warden finding disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@GalloDaSballo @code423n4 @frank-beard