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.

Sign up for GitHub

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

RFC 4: Secure upgrades for contracts operating staked balances #446

Merged
merged 3 commits into from
May 22, 2019
Merged

RFC 4: Secure upgrades for contracts operating staked balances #446

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
63f693c
Add initial version of securing contract upgrades RFC doc
ngrinkevich Dec 7, 2018
e27b702
Add more info on upgradeability risks and minor re-wording
ngrinkevich Dec 13, 2018
6dd64c2
Update RFC status to surpeseded
ngrinkevich May 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions docs/rfc/rfc-4-secure-contract-upgrades.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
:toc: macro

= RFC 4: Secure upgrades for contracts operating staked balances

.SUPERSEDED
****
IMPORTANT: This RFC has been superseded by an alternative described
in RFCs 9 and 11. It was never implemented as described.
****

:icons: font
:numbered:
toc::[]

== Background

Following best practices for ERC-20 Tokens, we have non-upgradable and
non-ownable Token and Token staking contracts, which means there is no
backdoor to modify the contract storage or change the implementation.
Amongst other things, these contracts only allow token transfers to be
executed by the token holders. Since token staking functionality might
need upgrading, it was decided to simply issue and deploy a new contract
and advise stakers to migrate their balances to it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With extended unstaking time, this may prove inconvenient in use; however, it's the most secure method and the staking contract can be completely upgrade-agnostic.

There's a risk of a "stakepocalypse" if a new staking contract is released and everybody rushes to unstake so they can migrate immediately.

However this means the work requested during the unstaking time gets distributed over those who stay in the old contract, making it disproportionately profitable to stay and unstake+restake later when the demand moves to the new contract.

Thus the stakepocalypse won't really happen, and the stakes will probably be migrated across over 2 * unstaking_time time, by:

  1. unstaking a fraction of one's tokens, keeping the rest on the old contract
  2. after the unstaked tokens are freed, staking them on the new contract
  3. unstaking the rest from the old contract
  4. staking the rest on the new contract

If the demand for work on the old contract within the time [T .. (T + unstaking_time)] is A and demand on the new contract from [(T + unstaking_time) .. (T + 2*unstaking_time)] is B = pA, and the fraction of stakers who unstake is u, the revenue increase one gains from not unstaking immediately is A / (1 - u) and the opportunity cost is B / u. This means that it's profitable to wait on unstaking if:

A / (1 - u) > B / u
(A * u) / ((1 - u) * u) > (B * (1 - u)) / (1 - u) * u)
A * u > B - B * u
(A + B) * u > B
u > B / (A + B)

It's probably easiest to just do 50/50; if everybody does it the income distribution doesn't change (correctly anticipating increased demand can improve on this unless everybody does it), and it can be done by always splitting one's tokens across two stakers even if no specific provisions are made for partial unstaking.


A different upgrade approach taken from Open Zeppelin was implemented for
the rest of the contracts; this RFC states the pitfalls of the approach and
proposes alternative way similar to what we already have for Token staking
contracts.


=== Current Functionality

Current upgrading functionality influenced by Open Zeppelin libraries makes
upgradable contract address and storage persistent and the logic can be
upgraded by the contract owner. They do so by updating the implementation
address in the persistent proxy contract. The method is also known as
"Eternal Storage". This was considered a good approach for all the
forthcoming Keep contracts including Group selection and Random beacon.
A concern has been raised during implementation of stake slashing
functionality where an upgradable contract such as Group Contract has to
be authorized to modify stake balances. Since the address and storage are
persistent in case of a compromised implementation all staked balances are
at risk. Imagine a deploy key that is used to update the implementation is
stolen, the hacker has full access to the contract storage immediately by
updating contract with his implementation. Besides if the contract address
was authorized to move stakers balances those will be lost as well.


=== Goal

Minimize the risk of lost/stolen staked tokens in the case of a hacked or
bad implementation upgrade of the contracts that require full access to
modify balance of a staker.

=== Implementation

Each new contract that does "slashing/reward" changes on staked token
balances must be a non-upgradable and non-ownable one to minimize attack
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really means any contract that affects staked token balances, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what's not clear here is that even though the contract is non-upgradable, it can be using the eternal storage pattern with an associated proxy, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really means any contract that affects staked token balances, right?

correct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what's not clear here is that even though the contract is non-upgradable, it can be using the eternal storage pattern with an associated proxy, right?

no, "eternal storage pattern" thats what we're trying to avoid now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible to make a layercake of:

  • "back-end": the operator-facing work contracts, upgraded atomically and securely
  • "front-end": an user-facing interface contract with its own, possibly more liberal upgrade process, which offloads work to the work contracts and does migrations across them

The front-end can safely implement eternal storage as long as the work contracts don't grant it dangerous privileges.

For example, in the random beacon the front-end contract could implement the beacon itself and its pricing/request process; the back-ends would handle signing the input they receive from the front-end, and creating new groups when the front-end asks them to.

When a request comes in:

  1. the frontend randomly selects which backend contract serves it, based on e.g. the number of groups each backend has active
  2. the frontend sends the request to the selected backend, along with payment
  3. the backend selects which of its groups will serve the request
  4. the selected group produces the signature
  5. the backend rewards (or punishes) signers and sends the signature to the frontend
  6. the frontend processes the signature and responds to the requester; the frontend might also request the creation of a new signing group in the most recent backend contract (which may differ from the contract that produced the entry), and send the DKG payment to it

This way the interface presented to the customers can be unchanging while stakers' funds remain perfectly secure against unauthorized upgrades; in the worst-case scenario the frontend breaks, making the system unable to do useful work but not causing stakes to be stolen or lost. The exact API separation needs to be architected individually for each usecase, however.

surface. Functionality upgrades are only possible by deploying a new
implementation as a new contract. The address of this contract must be
re-authorized by a staker. Stakers do so by calling the `authorize(address)`
method on a staking contract. A client should only authorize official
contract addresses from the metacontract list maintained by Keep
organization along with the confirmation by two or more Keybase-proven
channels.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can optionally be enforced in the staking contract, if Keep Org maintains a contract that tracks authorized versions.


The contracts with `authorize()` method should also include a "panic button"
- a method that cancels all authorizations in case of emergency. This
should be restricted to be called by a Keep organization account used to
deploy this contract.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can make the panic button act on a specific work contract, effectively freezing its functionality but protecting all (remaining) stakes. This makes it significantly more useful as the panic button doesn't have to stop absolutely everything, but only the unsafe contracts.

Making panic buttons work against staking contracts, however, is rather difficult and probably not worth the effort.



=== Limitations

Not particularly a limitation but a move from being able to do a seamless
and instant upgrade to a drawn-out one where stakers have to reauthorize
a newly deployed version.

=== Proof of Concept

The basis for the concept can be seen in the current Staking contracts
and StakingProxy.sol


[bibliography]
== Related Links

- Discussion on Flowdock:
https://www.flowdock.com/app/cardforcoin/tech/threads/6_Abd7qxhJrSNhQSrDxwgyvL0Pd