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

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

Merged
merged 3 commits into from
May 22, 2019

Conversation

ngrinkevich
Copy link
Contributor

Proposal to use alternative upgrading approach that is safe for critically important operations dealing with staked balances.

@ngrinkevich ngrinkevich requested a review from a team December 6, 2018 20:51
Proposal to use alternative upgrading approach that is safe for critically
important operations dealing with staked balances.
Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

Left several suggestions + some thoughts. This is a good overview, but I think we need a touch more detail in the RFC.

docs/rfc/rfc-4-secure-contract-upgrades.adoc Outdated Show resolved Hide resolved
docs/rfc/rfc-4-secure-contract-upgrades.adoc Outdated Show resolved Hide resolved
docs/rfc/rfc-4-secure-contract-upgrades.adoc Outdated Show resolved Hide resolved
docs/rfc/rfc-4-secure-contract-upgrades.adoc Outdated Show resolved Hide resolved
docs/rfc/rfc-4-secure-contract-upgrades.adoc Outdated Show resolved Hide resolved
docs/rfc/rfc-4-secure-contract-upgrades.adoc Outdated Show resolved Hide resolved
docs/rfc/rfc-4-secure-contract-upgrades.adoc Outdated Show resolved Hide resolved
docs/rfc/rfc-4-secure-contract-upgrades.adoc Outdated Show resolved Hide resolved
implementation as a new contract. Address of this contract must be
re-authorized by a staker. Stakers do so by calling authorize(address)
method on a staking contract. To further secure the network only official
Keep org contracts addresses can be authorized. The metacontract list of
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 this should be “a client should only authorize official contract addresses from the Keep organization”. Otherwise someone who would have access to upgrade Keep contracts presumably also has access to update a global contract-based list of authorizable contracts.

I'm thinking the contract list update should be more of a multifactor thing… e.g. it's listed in the metacontract and you've seen it confirmed two or more Keybase-proven channels or something like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Depends on whether we want client-side or on-chain verification. Per the flowdock discussion, if we have separate upgrade keys for each service and its list of authorizable contract addresses, and master keys for rekeying the upgrade keys subject to staker approval, it should be reasonably secure:

the metacontract has upgrade(contract, address) and panic_button(contract) which check that the signature matches the upgrade keys for that contract, and rekey(contract, pubkey) which then waits for a supermajority of stakers to approve the new upgrade keys

If the client authorises a non-Keep org address, it would present a risk of unexpected behaviour even if the only functionality exposed to the authorised contract is slash(staker, penalty) and slash_and_transfer(staker, penalty, tattletale, reward), reward < penalty. With client-side multi factor validation, this can't be enforced on-chain.

docs/rfc/rfc-4-secure-contract-upgrades.adoc Outdated Show resolved Hide resolved
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.

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.

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.

=== 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.

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.

@mhluongo
Copy link
Member

Is this superseded by RFC 9 / #726? If so, can we link back to the discussion from the new RFC and close?

@Shadowfiend
Copy link
Contributor

can we link back to the discussion

If we do this we should probably also renumber RFC 9 to RFC 4 as there will be no actual RFC 4.

@mhluongo
Copy link
Member

I suppose the alternative is merge with a deprecation notice and a back-link? We've been referring to RFC 9 as RFC 9 so changing the numbering now wouldn't be awesome

@Shadowfiend
Copy link
Contributor

Could also just be a not-accepted status via an admonition up top.

@ngrinkevich ngrinkevich requested a review from Shadowfiend May 22, 2019 19:10
@mhluongo mhluongo dismissed Shadowfiend’s stale review May 22, 2019 20:14

From 6 months ago :)

Copy link
Member

@mhluongo mhluongo left a comment

Choose a reason for hiding this comment

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

Considering the the RFC is superseded, this LGTM

@mhluongo mhluongo merged commit 991c7ad into master May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants