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

Owner has a rugpull function #73

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

Owner has a rugpull function #73

code423n4 opened this issue Aug 26, 2021 · 3 comments
Labels
3 (High Risk) bug Something isn't working duplicate This issue or pull request already exists sponsor disputed

Comments

@code423n4
Copy link
Contributor

Handle

tensors

Vulnerability details

Impact

The owner of the contract has a rugpull function. This can be unsafe if the private key for the owner account falls into the wrong hands, allowing instant withdrawal of all the funds. In general, having a single point of failure like this is not recommended best practice and doesn't create decentralized trust-less contracts.

Proof of Concept

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

Recommended Mitigation Steps

Create a function that lets users withdraw their own funds instead.

@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

Unfortunately it is not as simple as 'Create a function that lets users withdraw their own funds' because much of a user's funds will not just be sitting in their deposit balance but be locked in a market, i.e. in the form of rent paid. There is also a third state that a user's fund can be in, which is that it has been taken from the user's deposit but not yet collected by the market, i.e. it is in 'limbo'.

There is an enormous amount of logic behind calculating when a user's funds are part of their deposit balance, part of a market balance, and 'in limbo'. Errors in this logic can plausibly cause these balances to be incorrect, in which case a 'let a user withdraw their own funds' function will result in either too little, or even worse, too much, funds being withdrawn. The worst case is that a black hat could exploit an error in the logic and trick the contract into thinking 'their funds' are equal to the entire balance of the Treasury contract.

Therefore a, as you call it, 'rug pull' function, which simply transfers the entire token balance to the uberOwner is the only reliable way of getting funds out of the contract, in the case of an error in the logic elsewhere in the contract, and in addition minimises the risk of a black hat stealing user funds.

This function is not a 'single point of failure', the uberOwner role was created specifically to be managed by a Gnosis Safe multisig, with all the key holders known individuals in the space, the majority of whom will be outside of the Reality Cards team. This has not been implemented yet but will be prior to our launch. It is thus not possible for the private key to 'fall into the wrong hands'.

@Splidge
Copy link
Collaborator

Splidge commented Aug 26, 2021

I think this is a Duplicate of #74 "UberOwner has too much power"

@Splidge Splidge added the duplicate This issue or pull request already exists label Aug 26, 2021
@0xean
Copy link
Collaborator

0xean commented Sep 2, 2021

Commenting in #74 which is a duplicate.

@0xean 0xean closed this as completed Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) bug Something isn't working duplicate This issue or pull request already exists sponsor disputed
Projects
None yet
Development

No branches or pull requests

4 participants