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.

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

UberOwner has too much power #74

Open
code423n4 opened this issue Aug 26, 2021 · 3 comments
Open

UberOwner has too much power #74

code423n4 opened this issue Aug 26, 2021 · 3 comments
Labels

Comments

@code423n4
Copy link
Contributor

Handle

tensors

Vulnerability details

Impact

The Uber Owner has too much power within the system. This makes the protocol closer to a centralized prediction market whose rules are determined by the Uber Owner.

Proof of Concept

https://github.com/code-423n4/2021-08-realitycards/blob/39d711fdd762c32378abf50dc56ec51a21592917/contracts/RCTreasury.sol#L293
https://github.com/code-423n4/2021-08-realitycards/blob/39d711fdd762c32378abf50dc56ec51a21592917/contracts/RCTreasury.sol#L321
https://github.com/code-423n4/2021-08-realitycards/blob/39d711fdd762c32378abf50dc56ec51a21592917/contracts/RCTreasury.sol#L331

The above functions can be used by the Uber Owner to completely change the functionality of the system.
This goes well beyond simple setting new constants and fees, the Uber Owner can basically reprogram how the entire protocol works. Not to mention if the address falls into the wrong hands.

Recommended Mitigation Steps

Limit the permission of the Uber Owner to something more manageable and trustable. If upgrades to underlying contracts are required they can be done through a proxy instead, in the standard way.

@code423n4 code423n4 added 3 (High Risk) bug Something isn't working labels Aug 26, 2021
code423n4 added a commit that referenced this issue Aug 26, 2021
@mcplums
Copy link
Collaborator

mcplums commented Aug 26, 2021

This is a subjective opinion- there is always going to be a compromise between decentralisation and the ability to respond to potential problems. The latter is especially important with a protocol that is so new.

There is no correct answer here, but the current abilities of uberOwner were decided after a lot of thought and are in line with other DeFi protocols.

@Splidge
Copy link
Collaborator

Splidge commented Aug 26, 2021

I'd just like to add that we did recognize the power of the UberOwner which is why it is separated from the Owner specifically so that we can add additional security to it (in the form of a multisig) and so that we can relinquish this control at the appropriate time.
This was covered in the readme.
And also commented in the code.

@0xean
Copy link
Collaborator

0xean commented Sep 2, 2021

I think the warden(s) have a valid point here. This is an incredible amount of power for a single address to yield over the protocol, even if backed by a multi-sig.

Is it no an option to 1) pause all activity, and unlock all funds allowing users to withdraw their own funds or 2) pause all activity besides withdraws and implement a time delay between that and the "rug pull" function being called.

The readme also states

Alternatively we may wish for this to be a multisig but the normal owner to not be, for convenience.

Without a multisig, I believe this absolutely qualifies as a high severity issue as a compromise of a single end user address compromises the entire system, with a multisig it potentially lowers the severity down to a medium, but its still a risk that is worth highlighting in the system and for the sponsor to scrutinize if there are indeed other mitigation paths that could be taken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants