-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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.
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
Superuser: Upgradeable security for Smart Contracts #50
Comments
👍 nice idea. Maybe the example is not the best, true, but I see the value |
@federicobond would you like to make a PR to explore this idea? |
I am not sure if the use case is general enough to warrant its inclusion in Zeppelin, but here are my updated thoughts on it: A Superuser contract would inherit from Ownable. The superuser variable in this contract points to an address whose key should be kept in a cold wallet and never used for any other purpose. If there is a security breach and someone gets access to the owner key, this can be used as a last resort to regain control of the contract, transferring the ownership back to the original owner.
This could be useful if owner key must be used on a regular basis for administrative purposes. |
This could/should be implemented with RBAC.sol now that it's available :) |
@federicobond still sounds relevant to me. Having two owners instead of one, good deal. |
Hey, If we implement the Superuser contract with RBAC, instead of having a single superuser we would have an unlimited number of them, by creating the superuser role. Do you think this is okay in a practical scenario? Besides that, we should make sure that superusers can only be set once (either in the constructor or with a setSuperusers function). Once they are set, the owner shouldn't be able to modify them as the owner's account could get compromised in the future. If we only want a single superuser, then maybe a contract inheriting just from Ownable should be enough, following @federicobond's idea. Let me know your thoughts and I can work in a PR. (You should know I am doing my first steps in OZ and Blokchain in general :) ) Thanks! |
You can have the only function allowed to change superusers be contract Superuser is Ownable, RBAC {
ROLE_SUPERUSER = "superuser";
modifier onlySuperuser() {
checkRole(msg.sender, ROLE_SUPERUSER);
_;
}
constructor () {
addRole(msg.sender, ROLE_SUPERUSER);
}
function transferSuperuser(address _newSuperuser) onlySuperuser() {
removeRole(msg.sender, ROLE_SUPERUSER);
addRole(_newSuperuser, ROLE_SUPERUSER);
}
} that's pseudo code, but it seems to be the gist of it. This is pretty much how I implemented |
Implementing single-address roles in rbac is relatively straightforward. the only difference between RBAC with "only one person can have this role" and something like |
…when they are not superusers OpenZeppelin#50
* Migrate to SLF4J * Remove IDE warnings
Scenario: you create a small crowdsale for your widget and it becomes really popular very quickly. Now there is a million dollar price tag on the poorly secured private key you used to deploy your contract and you start sleeping badly at night. Nobody worth their salt would secure a million dollars in a hot wallet.
Luckily, you remember that your contract inherits from
Superuser
, so next morning you generate a key in cold storage and take note of its address. You then use your owner account toconfigureSuperuser(address)
and your sleep improves again because any sensitive method is using therequireSuperuser
modifier. This modifier ensures thatmsg.sender
is the superuser or (in case no superuser is configured, the owner). BOOM! Upgradeable security and peace of mind.I will have a working implementation later today. Feel free to leave any feedback.
UPDATE: As was pointed out in the Slack channel, this is not a good example, as you can just transfer the ownership of the contract. A better example is when you want to keep a hot key for performing managerial actions but have a cold storage key as superuser in case the hot key is compromised.
The text was updated successfully, but these errors were encountered: