From 63f693c7233feb371daf948365fbad51c237475d Mon Sep 17 00:00:00 2001 From: Nik G Date: Fri, 7 Dec 2018 17:46:06 -0500 Subject: [PATCH 1/3] Add initial version of securing contract upgrades RFC doc Proposal to use alternative upgrading approach that is safe for critically important operations dealing with staked balances. --- docs/rfc/rfc-4-secure-contract-upgrades.adoc | 81 ++++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 docs/rfc/rfc-4-secure-contract-upgrades.adoc diff --git a/docs/rfc/rfc-4-secure-contract-upgrades.adoc b/docs/rfc/rfc-4-secure-contract-upgrades.adoc new file mode 100644 index 0000000000..da3e1ad3ea --- /dev/null +++ b/docs/rfc/rfc-4-secure-contract-upgrades.adoc @@ -0,0 +1,81 @@ +:toc: macro + += RFC 4: Secure upgrades for contracts operating staked balances + +: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 and +it is left that token transfers can only be executed by the token holders. +As a workaround for Token staking functionality upgrades it was decided to +simply issue and deploy a new contract and advise stakers to migrate their +balances to it. + +A different upgrade approach taken from Open Zeppelin was considered for the +rest of the contracts and 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. 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. + + +== Proposal + +Use alternative upgrading approach that is safe for critically important +operations dealing with staked balances. + +=== 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 +surface. Functionality upgrades are only possible by deploying new +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 +official contracts is maintained by Keep org. + +The contracts with authorize() method should also include a "panic button" +- a method that cancels all authorizations in case of emergency and this +should be restricted to be called by a Keep org account used to deploy +this contract. + + +=== 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 From e27b7028c12f3faede0cf392c7c7d606492409f6 Mon Sep 17 00:00:00 2001 From: Nik G Date: Thu, 13 Dec 2018 09:20:22 -0500 Subject: [PATCH 2/3] Add more info on upgradeability risks and minor re-wording --- docs/rfc/rfc-4-secure-contract-upgrades.adoc | 64 ++++++++++---------- 1 file changed, 33 insertions(+), 31 deletions(-) diff --git a/docs/rfc/rfc-4-secure-contract-upgrades.adoc b/docs/rfc/rfc-4-secure-contract-upgrades.adoc index da3e1ad3ea..edc73ef07f 100644 --- a/docs/rfc/rfc-4-secure-contract-upgrades.adoc +++ b/docs/rfc/rfc-4-secure-contract-upgrades.adoc @@ -9,15 +9,15 @@ 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 and -it is left that token transfers can only be executed by the token holders. -As a workaround for Token staking functionality upgrades it was decided to -simply issue and deploy a new contract and advise stakers to migrate their -balances to it. - -A different upgrade approach taken from Open Zeppelin was considered for the -rest of the contracts and this RFC states the pitfalls of the approach 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. + +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. @@ -26,19 +26,20 @@ contracts. 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. 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. +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. -== Proposal - -Use alternative upgrading approach that is safe for critically important -operations dealing with staked balances. - === Goal Minimize the risk of lost/stolen staked tokens in the case of a hacked or @@ -49,17 +50,18 @@ modify balance of a staker. Each new contract that does "slashing/reward" changes on staked token balances must be a non-upgradable and non-ownable one to minimize attack -surface. Functionality upgrades are only possible by deploying new -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 -official contracts is maintained by Keep org. - -The contracts with authorize() method should also include a "panic button" -- a method that cancels all authorizations in case of emergency and this -should be restricted to be called by a Keep org account used to deploy -this contract. +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. + +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. === Limitations From 6dd64c22c959a0e4cfc004dfb385ed692346016e Mon Sep 17 00:00:00 2001 From: Nik G Date: Wed, 22 May 2019 12:47:43 -0500 Subject: [PATCH 3/3] Update RFC status to surpeseded --- docs/rfc/rfc-4-secure-contract-upgrades.adoc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/rfc/rfc-4-secure-contract-upgrades.adoc b/docs/rfc/rfc-4-secure-contract-upgrades.adoc index edc73ef07f..b3a078eada 100644 --- a/docs/rfc/rfc-4-secure-contract-upgrades.adoc +++ b/docs/rfc/rfc-4-secure-contract-upgrades.adoc @@ -2,6 +2,12 @@ = 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::[]