Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

[WIP] First Implementation of Staking Contracts (ZEIP 31) #1910

Merged
merged 154 commits into from
Aug 21, 2019

Conversation

hysz
Copy link
Contributor

@hysz hysz commented Jun 29, 2019

Description

This PR includes the first implementation of ZEIP-31 - Stake-based Liquidity Incentives.

Functionality:

  1. Stake your ZRX tokens to unlock their utility within the 0x ecosystem.
    • Earn rebates on market making on the 0x protocol
    • Participate in governance over the 0x protocol
  2. Create staking pools to leverage the weight of other stakers.
    • Increase your market making rebates.
    • Increase your voting power.
  3. Delegate your Zrx to staking pools to
    • Earn a portion of the pool's market making rebates.
    • Support a pool's mission sharing your voting power.

Next Steps:

  1. Add more extensive testing (presently includes basic end-to-end testing)
  2. Optimize Staking contract Nth Root Algorithm / Cobb Douglas
  3. Implement rich revert reasons throughout the contracts
  4. Integrate with 0x Exchange V3 contracts
  5. Test Nets / Audits / Bounties

Notes:
We've iterated on the design many times over the past several months, and this is the first end-to-end implementation. The core protocol team will continue iterating on this, and any feedback/ideas/concerns are welcome.

Architecture

This system is composed of four deployed contracts:

  1. Staking Contract, which is an upgradeable/stateless contract that implements staking logic.
  2. Staking Contract Proxy, which stores staking state and delegates to the Staking Contract.
  3. Zrx Vault, which securely holds staked Zrx Tokens.
  4. Staking Pool Reward Vault, which securely holds rewards earned by staking pools.

These contracts connect to each other and the broader 0x ecosystem like this:

architecture_normal_conditions

Architecture (Kill Switch)

If a vulnerability is discovered in the staking contract, operations may be halted to conduct forensics:

  1. The 0x Exchange contract stops charging protocol fees
  2. The staking contract is set to read-only mode
  3. Vaults may be detached from the staking contract, depending on the vulnerability.

architecture_kill_switch

Architecture (Catastrophic Failures)

In this worst-case scenario, state has been irreperably corrupted and the staking contracts must be re-deployed. Users withdraw their funds from the vaults and re-stake under the new system, at will.

  1. Vaults enter "Catostrophic Failure Mode" allowing users to withdraw their ZRX and Rewards.
  2. A Balance Oracle is deployed for determining the Reward balance of each user. (*)

architecture_failure_mode

(*) A Balance Oracle is implemented retroactively, and depends on how state has been corrupted. For example, if state used to compute rewards is not corrupted, then it would be used by the oracle. Conversely, if this state is corrupted, we may need to reconstruct balances from previous state. (No balance oracle is required for ZRX.)

Contracts Directory Structure

The contracts can be found in contracts/src.

* Staking.sol        | This is a stateless contract that encapsulates all the staking logic.
* StakingProxy.sol   | This is a stateful contract that proxies into the Staking contract.
* fees/              | This contains mixins that implement the logic for 0x Protocol fees & rebates.
* immutable/         | This contains mixins that should not be changed.
* interfaces/        | This contains interfaces used throughout the entire staking system.
* libs/              | This contains libraries used by the staking contract; for example, math and signature validation.
* stake/             | This contains mixins that implement the core staking logic.
* staking_pools/     | This contains mixins that implement logic for creating and managing staking pools.
* sys/               | This contains mixins that implement low-level functionality, like scheduling.
* vaults/            | This contains the vaults (like the Zrx Token Vault).

Testing Architecture

These contracts use an actor/simulation pattern. A simulation runs with a specified set of actors, initial state and expected output. Actors have a specific role and validate each call they make to the staking system; for example, there is a Staking Actor who stakes/unstakes their Zrx tokens and validates balances/events. Similarly, there could exist an actor who tries to steal funds.

@coveralls
Copy link

coveralls commented Jun 29, 2019

Coverage Status

Coverage remained the same at 75.804% when pulling c0acc8d on feature/contracts-staking/first-implementation into 7407890 on 3.0.

Copy link
Member

@abandeali1 abandeali1 left a comment

Choose a reason for hiding this comment

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

I did a first pass on this, lots to discuss! I did not review any of the math and will need to give this a second pass for some of the more complex areas.

@@ -21,8 +21,7 @@ pragma solidity ^0.5.9;
import "@0x/contracts-utils/contracts/src/LibEIP712.sol";


contract LibEIP712CoordinatorDomain is
LibEIP712
contract LibEIP712CoordinatorDomain
Copy link
Member

Choose a reason for hiding this comment

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

The contract opening { should be on this line.


contract MixinOwnable is
IStakingEvents,
MixinDeploymentConstants,
Copy link
Member

Choose a reason for hiding this comment

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

Why do all of these contracts need to be inherited?

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 is to remove ambiguity in the contract linearization. Basically, the rule-of-thumb is to order dependencies from most base-like to most derived. However, this can break when there are multiple levels of inheritance. So, to avoid this problem altogether, the inheritance lists are generated in full. In this case - MixinStorage depends on MixinConstants which depends on MixinDeploymentConstants.

Example of general problem:

Ex:
contract A {}
contract B {}
contract C {}
contract D is A, B {}

Suppose now that there is a contract E that will inherit from B{} and D{}
The following follows "most base-like" to "most-derived":
contract E is B, D {}
However, it unfolds to: B{}, A{}, B{}, D{}, E{} - which is wrong because B{} appears both before and after A{}.
The correct contract inheritance for E{} should be:
contract E is A{}, B{}, D{}

So, even though contract E{} does not directly depend on A{} - it still must be included in the dependency list
Because of this, we include all descendants in the output.

payable
{
address _stakingContract = stakingContract;
if (_stakingContract == NIL_ADDRESS) {
Copy link
Member

Choose a reason for hiding this comment

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

We use NIL_ADDRESS sometimes and address(0) sometimes. Isn't address(0) cheaper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah I actually intended to replace all instances of address(0)with NIL_ADDRESS. I must've missed this one. I believe the cost is the same now that we're using the constant optimizer.

constructor(address _stakingContract)
public
{
owner = msg.sender;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be handled by the Ownable constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mm yeah this actually raises an interesting design decision that I'm curious to get your thoughts on: none of the mixins have constructors because they all share the proxy's state, which is not available during constructing of the staking contract. Their state is instead placed in immutable/MixinStorage.sol and inherited by both the proxy and the staking contract.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe an _init() pattern for mixins is cleaner here.

{
address _stakingContract = stakingContract;
if (_stakingContract == NIL_ADDRESS) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

We should probably revert when the stakingContract is detached, no? Otherwise this contract can just collect Ether without registeing it to a maker.

/// @param amount Amount in ETH to record.
function withdrawForMember(bytes32 poolId, uint256 amount)
external
onlyStakingContract
Copy link
Member

Choose a reason for hiding this comment

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

Missing a modifier here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(see note above)

{
// operator share must be a valid percentage
require(
poolOperatorShare <= 100,
Copy link
Member

Choose a reason for hiding this comment

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

I think this denominator is reused elsewhere and should probably be made a constant. Note that we can easily increase the precision here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: we discussed offline and have a task for this.

/// @param owner of Stake
/// @param poolId Unique Id of staking pool to delegate stake to.
/// @param amount of Stake to delegate.
function _delegateStake(
Copy link
Member

Choose a reason for hiding this comment

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

This delegation begins during the current epoch, right? If so, the stake needs to be somehow time-weighted to prevent users from delegating near the end of an epoch with reduced risk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: we discussed offline and have a task for this.

return (payoutInRealAsset, payoutInShadowAsset);
}

/// @dev Computes how much shadow asset to mint a member who wants to
Copy link
Member

Choose a reason for hiding this comment

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

Can we better define "shadow asset" here?

/// @return totalRewardsPaid Total rewards paid out across all active pools.
/// @return initialContractBalance Balance of this contract before paying rewards.
/// @return finalContractBalance Balance of this contract after paying rewards.
function _distributeFeesAmongMakerPools()
Copy link
Member

Choose a reason for hiding this comment

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

We should add a TODO that this function needs a way to be executed among multiple blocks (or reduced to ~O(1) complexity). This will be a hard requirement, but can be done in another PR.

Copy link
Contributor

@dorothy-zbornak dorothy-zbornak left a comment

Choose a reason for hiding this comment

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

Seems like you put a lot of thought into this! 👍
Got halfway through. Need to do another pass.

// solhint-disable no-empty-blocks
function ()
external
payable
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the fallback payable? Why even define a fallback? I get why a payable fallback is in the StakingProxy contract, but not here. Is there a need to ever fund either of these contracts via explicit fallback?

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 gets called one deposits into the staking proxy. It's more for future proofing, in case we want additional logic for handling deposits.

constructor(address _stakingContract)
public
{
owner = msg.sender;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe an _init() pattern for mixins is cleaner here.


/// @dev Detach the current staking contract.
/// Note that this is callable only by this contract's owner.
function detachStakingContract()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a scenario where we would just leave the Staking contract detached?
My understanding is that in failure mode, we would simply detach the StakingProxy from the Exchange.
During upgrades, wouldn't we want to immediately swap in a new version of the Staking contract, so why not just an atomic replaceStakingContract() function?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, should we do something like _stakingContract.init.delegatecall() when it's being attached? Could be a useful migration hook...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: we discussed offline and have a task for both atomic migrations and init.

MixinStakeBalances
{

/// @dev This mixin contains the logic for 0x protocol fees.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think this natspec actually belongs before the contract definition.

uint256 totalRewardsPaid,
uint256 initialContractBalance,
uint256 finalContractBalance) = _distributeFeesAmongMakerPools();
emit RewardsPaid(
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be more useful to emit something like this per active pool? Logs are cheap, finalization is infrequent, and we aren't anticipating a large number of active pools (right?). This makes me wonder if there should be dust thresholds for payouts as well...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: we discussed offline and have a task for this.

// step 3/3 send total payout to vault
require(
totalRewardsPaid <= initialContractBalance,
"MISCALCULATED_REWARDS"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a recoverable state?

Copy link
Contributor Author

@hysz hysz Aug 20, 2019

Choose a reason for hiding this comment

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

If this is triggered then we likely have a bug in the code and would have to redeploy the logic contract in order to finalize the epoch. It is possible that the state is not recoverable in which case we could either subsidize the fees from the current epoch or enter catastrophic failure mode.


uint256 constant internal TOKEN_MULTIPLIER = 1000000000000000000; // 10**18

bytes32 constant internal INITIAL_POOL_ID = 0x0000000000000000000000000000000100000000000000000000000000000000;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect this has something to do with ERC1155/NFT compat...


uint256 constant internal REWARD_PAYOUT_DELEGATED_STAKE_PERCENT_VALUE = 90;

uint256 constant internal CHAIN_ID = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Def needs to be set in a constructor. Maybe all the way down in StakingProxy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: we discussed offline and have a task for this.



// solhint-disable max-states-count
contract MixinStorage is
Copy link
Contributor

Choose a reason for hiding this comment

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

At first I was trying to come up with ways for this to be more natural (each mixin manages their own storage). It's possible, but storage locations would likely be unreliable (breaking upgrades) because we keep reordering things to get around C3 linearization. 😫

Copy link
Member

Choose a reason for hiding this comment

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

Can this contract just inherit Ownable? We probably also want to have tests that ensure storage slots don't get changes in future upgrades. Let's create a task for these 2 things.

/// Note that this is only callable by the staking contract, and when
/// not in catastrophic failure mode.
/// @param poolId Unique Id of pool.
function depositFor(bytes32 poolId)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about who actually calls this function. I can't find any instance of the staking contract calling it.

It seems we only do a recordDepositFor(), per pool, then bulk transfer to the vault via fallback during finalization.

deposit() also seems unused, as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: we discussed offline and have a task for this.

/// not in catastrophic failure mode.
/// @param poolId Unique Id of pool.
/// @param poolOperatorShare Percentage of rewards given to the pool operator.
function registerStakingPool(bytes32 poolId, uint8 poolOperatorShare)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like that we duplicate poolOperatorShare state in both this contract and the Staking contract. It creates a kind of coupling between the two contracts where this value must be in sync. It also makes it awkward if we decide to allow operators to change their share percentage or if we want to change how they're computed because this contract is not upgrade-able.

I think we should explore not storing the poolOperatorShare in this contract and instead have the deposit functions accept explicit values for the operatorAmount and membersAmount.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: we discussed offline and have a task for this.

@hysz hysz force-pushed the feature/contracts-staking/first-implementation branch 3 times, most recently from fad99eb to d800f14 Compare August 21, 2019 00:33
@hysz hysz force-pushed the feature/contracts-staking/first-implementation branch from 54563cf to 697e5df Compare August 21, 2019 20:05
Copy link
Contributor

@dorothy-zbornak dorothy-zbornak left a comment

Choose a reason for hiding this comment

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

👀 👍
I think we should change it back to "timelock" :trollface:

Copy link
Member

@abandeali1 abandeali1 left a comment

Choose a reason for hiding this comment

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

I still have a lot of comments, but I don't think there is anything blocking the initial implementation from being merged. Excited to dig into this more :)

/// @dev Calculated the EIP712 hash of the StakingPool approval mesasage using the domain separator of this contract.
/// @param approval StakingPool approval message containing the transaction hash, transaction signature, and expiration of the approval.
/// @return EIP712 hash of the StakingPool approval message with the domain separator of this contract.
function _hashStakingPoolApprovalMessage(
Copy link
Member

Choose a reason for hiding this comment

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

Can we create a task to remove this if we haven't already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 there is a task & it's part of this sprint. Will be addressed shortly after merge.



// solhint-disable max-states-count
contract MixinStorage is
Copy link
Member

Choose a reason for hiding this comment

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

Can this contract just inherit Ownable? We probably also want to have tests that ensure storage slots don't get changes in future upgrades. Let's create a task for these 2 things.

@hysz hysz force-pushed the feature/contracts-staking/first-implementation branch from 43faea5 to c0acc8d Compare August 21, 2019 23:30
@hysz hysz merged commit 249948e into 3.0 Aug 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants