search

nickjm / cryptospinners-bounty

CryptoSpinners contracts for bounty program.
Other
1 stars 1 forks source link

Contract ownership can become unrecoverable #1

Closed BinaryQuasar closed 6 years ago

BinaryQuasar commented 6 years ago

Description

The contract inherits from Open Zeppelin's Ownable to manage ownership. This can cause the contract to become unrecoverable.

https://github.com/nickjm/cryptospinners-bounty/blob/015cbeb0088f9fb9deb2f7d4ad5d2a82e7988ef9/contracts/AccessControl.sol#L11

Severity

Note

Scenario

Contract ownership is accidentally transferred to an address for which the private key is unknown, or to a contract, e.g. due to a typo or accidentally copying the wrong address.

Impact

This makes all actions requiring owner access impossible.

Fix

Use Open Zeppelin's Claimable. This requires a 2-step transfer of the contract, where the new owner has to confirm the transfer by calling claimOwnership. This gives peace of mind.

nickjm commented 6 years ago

I'll first say that luckily for our users, the ownership of spinners, market functionality, and game would keep on running just fine in such a case. In the battling contract (not yet added to repo) anyone can call the functions to pay out winners and reset the battling season and leagues. Additionally, treasurer could still withdraw revenue.

With that out of the way, I agree with your recommendation. I will look at Claimable and likely push an update soon, assuming it doesn't make the contract too big or require any big changes.

nickjm commented 6 years ago

This is a part of the code now. +50 pts for Note