0x00000002 / staking-app

MIT License
0 stars 0 forks source link

Single owner has too much power #14

Open ryu9827 opened 6 years ago

ryu9827 commented 6 years ago

Single owner has far too much power, add new owner and remove other owners. This is not good practice and carries significant risk for the app. Suggest to add multiple signature for improving security.

ryu9827 commented 6 years ago

Because security of the contract is our most concerned. As each single owner has too much equal authority (like add owner, set operator and set minter, then fully control these two contract), we suggest adding more security logic, like sigature mechanism, to make sure those key functions are confirmed by multiple owners instead of single owner.

0x00000002 commented 6 years ago

I have a lot of questions in this regards:

  1. Whats the problem to have several owners? Why you think it's an issue?
  2. Who are the owners? Why do you think they are malicious actors? Could it be only one owner, but the multisig wallet?
  3. Yes, owner can do all of these things. Why it's an issue?
  4. What's the problem? Do you think he shouldn't? Do you think that anyone should have such opportunity? Could you please explain what that function does? I thought that is an FEE generator, do you disagree?

And more questions: Did you think about what the _wallet variable means in the stake.sol constructor? Could it be related somehow to some multisig wallet? Yes, we don't know, but it could be. Probably we should ask the question to the LEVERJ.IO development team?

On more thing...

Bruce, I think it is better to create many issues, not just one like "There are many problems in the code, including, a).. b)... c)... ".

It is a good practice to keep each one specific about only one problem.

Then we can report, discuss, fix, check, confirm and / or close each issue separately.

Thanks.

ryu9827 commented 6 years ago

Thanks for providing the advise. The reason I packed these questions into one issue is just they are all about system administration authority.

  1. Based on the logic, each of these owners has same level authority to add new owner and remove other owners. If one malicious owner been added accidentally, he can remove all other owners and set the operator and the minter, then fully control all these contracts.
  2. We don't know who are the owners. But we cannot just assume they are all good person. I am just assuming the worst situation and suggest our client get prepared for that.
  3. Just to remind the client that each owner has high level authority.
  4. Just to remind the client that operator has high control to the contract. These 5 questions are not separated. They need to be read and understood as a pack. Thank you for the suggestion again.
Janther commented 6 years ago

@ryu9827 @tikonoff I agree with this issue. I never feel too comfortable with superadmin's scenarios (even in my own computer). But it's not a critical issue according to the scale that we use. I would categorize this as Major

0x00000002 commented 6 years ago

Sure, we struggle with the priorities and decided to discuss it later with Matthew. Btw, please have a look at the new report on Slack.

0x00000002 commented 6 years ago

https://blockchainlabsnz.slack.com/files/U8QRHV86B/F9BRM4ADU/leverj_staking_audit.docx