axieinfinity/ronin-dpos-contracts

The collections of smart contracts that power the Ronin Delegated Proof of Stake (DPoS) network.

Ronin DPoS Contracts

The collections of smart contracts that power the Ronin Delegated Proof of Stake (DPoS) network.

Development

Requirement

• Node@>=14 + Solc@^0.8.0

Compile & test

• Add Github NPM token to install packages, and then replace {YOUR_TOKEN} in .npmrc file by any arbitrary Github token with read:packages permission.

  Note: To create a new token, please refer to this article. The token must have read:packages permission.

  cp .npmrc.example .npmrc && vim .npmrc

• Install packages

  $ yarn --frozen-lockfile

• Install foundry libs

  $ git submodule add -b release-v0 https://github.com/PaulRBerg/prb-test lib/prb-test
  $ git submodule add -b release-v4 https://github.com/PaulRBerg/prb-math lib/prb-math
  

• Compile contracts

  $ yarn compile

• Run test

  $ yarn test

• Extract storage layout

  $ yarn plugin:storage-layout [--destination <output-path>] [--override <true|false>]

  • <output-path> (optional): The path to store generated storage layout file. If not provided, the default path is layout/storage.txt.
  • --override (optional): Indicates whether to override the destination file at <output-path> if it already exists. By default, it is set to false.

Target chain to deploy

This repo contains source code of contracts that will be either deployed on the mainchains, or on Ronin chain.

• On mainchains:
  • Governance contract: MainchainGovernanceAdmin
  • Bridge contract: MainchainGatewayV3
  • Trusted orgs contract: RoninTrustedOrganization
• On Ronin chain:
  • Governance contract: RoninGovernanceAdmin
  • Bridge operation: RoninGatewayV3
  • Trusted orgs contract: RoninTrustedOrganization
  • DPoS contracts

Upgradeability & Governance mechanism

Except for the governance contracts and vault forwarder contracts, all other contracts are deployed following the proxy pattern for upgradeability. The TransparentUpgradeableProxyV2, an extended version of OpenZeppelin's, is used for deploying the proxies.

To comply with the governance process, in which requires all modifications to a contract must be approved by a set of governors, the admin role of all proxies must be granted for the governance contract address.

Deployment steps

• Init the environment variables

  $ cp .env.example .env && vim .env

• Update the contract configuration in config.ts file

• Deploy the contracts

  $ yarn hardhat deploy --network <local|ronin-devnet|ronin-mainnet|ronin-testnet>

Documentation

See docs/README.md for the documentation of the contracts.

See docs/HACK.md for the structure of the repo.

For the contract interaction flow, please refer to DPoS Contract: Interaction Flow.

For the whitepaper, please refer to Ronin Whitepaper.

ronin-dpos-contracts's Issues

Event issues

    Comments for interfaces and documents

Originally posted by @nxqbao in #22 (review)

Missing event indexed for importance params

Comment issues

    I think we need a more detailed explanation for these three functions, in order to differentiate the usage of these three.

Originally posted by @nxqbao in #22 (comment)

ronin-dpos-contracts/contracts/interfaces/IStaking.sol

Lines 108 to 118 in 2ef51c7

	/**
	* @dev Deprecates the pool.
	*
	* Requirements:
	* - The method caller is validator contract.
	*
	* Emits the event `PoolsDeprecated` and `Unstaked` events.
	* Emits the event `StakedAmountDeprecated` if the contract cannot transfer RON back to the pool admin.
	*
	*/
	function deprecatePools(address[] calldata _pools) external;

Delegator still able to delegate to not-kicked-yet under balance validator

When a validator is slashed and gets deducted a staked amount to make his balance is under the minimum required, and he will be kicked in the next period, we expect no delegators can top-up to this validator.

The current implementation allows both delegators and validators can top-up, and the following side-effects may get broken.

Review variable casting

ronin-dpos-contracts/contracts/ronin/validator/RoninValidatorSet.sol

Line 215 in 2ef51c7

IStaking(_stakingContract).sinkPendingReward(_validatorAddr);

ronin-dpos-contracts/contracts/ronin/validator/RoninValidatorSet.sol

Line 222 in 2ef51c7

IStaking(_stakingContract).deductStakedAmount(_validatorAddr, _slashAmount);

   Redundant castings, and local variables

Originally posted by @nxqbao in #22 (comment)

[Maintenance] The validator candidates may schedule too far in the future

ronin-dpos-contracts/contracts/ronin/Maintenance.sol

Line 74 in 2ef51c7

require(block.number + minOffset <= _startedAtBlock, "Maintenance: invalid offset size");

We don't have any restrictions to prevent the validator candidates from scheduling too far in the future.

For example, we only allow 5 maintenance at a time, and these 5 slots of maintenance are filled up to far in the future. A validator suddenly crashes, and needs emergency go into maintenance, but he cannot since there is no slot of maintenance available.

We need to set a furthest time restriction that allows a validator to set a schedule of maintenance.

Related to the post by @nxqbao in #22 (comment)
Related to the post by @nxqbao in #22 (comment)

The candidates can share the same bridge operator and other data

    We should not allow validators to share the same admin, treasury, or bridge operator

Originally posted by @ducthotran2010 in #22 (comment)

ronin-dpos-contracts/contracts/ronin/validator/CandidateManager.sol

Lines 51 to 67 in 603d7f6

	uint256 _length = _candidates.length;
	require(_length < maxValidatorCandidate(), "CandidateManager: exceeds maximum number of candidates");
	require(!isValidatorCandidate(_consensusAddr), "CandidateManager: query for already existent candidate");
	require(_commissionRate <= _MAX_PERCENTAGE, "CandidateManager: invalid comission rate");
	_candidateIndex[_consensusAddr] = ~_length;
	_candidates.push(_consensusAddr);
	_candidateInfo[_consensusAddr] = ValidatorCandidate(
	_admin,
	_consensusAddr,
	_treasuryAddr,
	_bridgeOperatorAddr,
	_commissionRate,
	type(uint256).max,
	new bytes(0)
	);
	emit CandidateGranted(_consensusAddr, _treasuryAddr, _admin, _bridgeOperatorAddr);

Duplicated functions to get credit configuration

We already have the function to get the credit score configuration here:

ronin-dpos-contracts/contracts/ronin/slash-indicator/CreditScore.sol

Lines 128 to 146 in 00fa60e

	/**
	* @inheritdoc ICreditScore
	*/
	function getCreditScoreConfigs()
	external
	view
	override
	returns (
	uint256 _gainCreditScore,
	uint256 _maxCreditScore,
	uint256 _bailOutCostMultiplier,
	uint256 _cutOffPercentageAfterBailout
	)
	{
	_gainCreditScore = gainCreditScore;
	_maxCreditScore = maxCreditScore;
	_bailOutCostMultiplier = bailOutCostMultiplier;
	_cutOffPercentageAfterBailout = cutOffPercentageAfterBailout;
	}

Because these variables are public, it is also exposed in the ABI:

ronin-dpos-contracts/contracts/ronin/slash-indicator/CreditScore.sol

Lines 17 to 24 in 00fa60e

	/// @dev The max gained number of credit score per period.
	uint256 public gainCreditScore;
	/// @dev The max number of credit score that a validator can hold.
	uint256 public maxCreditScore;
	/// @dev The number that will be multiplied with the remaining jailed time to get the cost of bailing out.
	uint256 public bailOutCostMultiplier;
	/// @dev The percentage of reward to be cut off from the validator in the rest of the period after bailed out.
	uint256 public cutOffPercentageAfterBailout;

The ABI consumers may be confused

Potential bugs in `isValidator` callers

Potential problems in calling isValidator.

contracts/ronin/Maintenance.sol:

• Does we allow a candidate to schedule maintenance? Because an arbitrary candidate may not know if they will be picked to become a validator in the future. In the current implementation, we don't have any time restriction on the _startedAtBlock.

contracts/ronin/SlashIndicator.sol:

• Same for the Maintenance contract. In case a misbehaved ex-validator becomes a candidate, we cannot slash him since he is not a validator anymore.

Originally posted by @nxqbao in #32 (comment)

Confusing variable name in Staking contract

Multiple meanings of staking amount

• This staking amount implies the delegating amount:
  

  ronin-dpos-contracts/contracts/interfaces/staking/IRewardPool.sol

  Lines 45 to 56 in 22060de

  	/**
  	* @dev Returns the staking amount of an user.
  	*/
  	function stakingAmountOf(address _poolAddr, address _user) external view returns (uint256);
  	/**
  	* @dev Returns the staking amounts of the users.
  	*/
  	function bulkStakingAmountOf(address[] calldata _poolAddrs, address[] calldata _userList)
  	external
  	view
  	returns (uint256[] memory);

• This staking amount implies the self-staking amount of a validator/pool:
  

  ronin-dpos-contracts/contracts/interfaces/staking/IBaseStaking.sol

  Lines 16 to 17 in 22060de

  	// Self-staking amount
  	uint256 stakingAmount;

Recommend

Delegate -> Stake; Stake -> Self-Staking

• stake/unstake -> selfStake/selfUnstake
• This variable can be renamed to selfStakingAmount:
  

  ronin-dpos-contracts/contracts/interfaces/staking/IBaseStaking.sol

  Lines 16 to 17 in 22060de

  	// Self-staking amount
  	uint256 stakingAmount;

Revamps `deprecated` mappings

https://skymavis.atlassian.net/browse/PSC-131?atlOrigin=eyJpIjoiY2FiYjYxMzM3NGJlNDRhM2FjZjk0ZmUzYmU4NDY2OGYiLCJwIjoiaiJ9
I guess we can clear these mappings since the reward amount is stored in decimal number:

ronin-dpos-contracts/contracts/ronin/validator/storage-fragments/JailingStorage.sol

Lines 9 to 10 in 11e5947

	/// @dev Mapping from consensus address => period number => block producer has no pending reward
	mapping(address => mapping(uint256 => bool)) internal _miningRewardDeprecatedAtPeriod;

ronin-dpos-contracts/contracts/ronin/validator/storage-fragments/JailingStorage.sol

Lines 13 to 14 in 11e5947

	/// @dev Mapping from consensus address => period number => block operator has no pending reward
	mapping(address => mapping(uint256 => bool)) internal _bridgeRewardDeprecatedAtPeriod;

Waiting time of renouncing can be cheated

A validator who requests to renounce has to wait at least 7 days before the request takes effect.

He can skip the waiting time by unstaking his self-staking amount to the required minimum balance, and intentionally getting slashed. Once slashed, and becoming under-balance, he will be kicked in the next period, which means the waiting time for renouncing is less than 1 day.

We should kick the validator after 7 days when he becomes under-balance.

Set of bridge operator mismatched

The check of if (isBlockProducer(_validators[_i])) should be if (isValidator(_validators[_i])), since a validator can hold only one of the two roles at a time. Current check requires an operator to be a block producer, which might yield incorrect results.

ronin-dpos-contracts/contracts/ronin/validator/storage-fragments/ValidatorInfoStorage.sol

Lines 85 to 97 in e97d653

	function getBridgeOperators() public view override returns (address[] memory _result) {
	_result = new address[](validatorCount);
	uint256 _count = 0;
	for (uint _i = 0; _i < _result.length; _i++) {
	if (isBlockProducer(_validators[_i])) {
	_result[_count++] = _bridgeOperatorOf(_validators[_i]);
	}
	}
	assembly {
	mstore(_result, _count)
	}
	}

Support Ronin voting without signatures

AFAIK the primary purpose of requiring the governor's signatures is to relay on mainchains. Therefore, the contract on mainchain MainchainGovernanceAdmin can verify whether the relayed proposal is accurate, based on the submitted signatures.

The signatures are required while casting votes:

ronin-dpos-contracts/contracts/ronin/RoninGovernanceAdmin.sol

Lines 105 to 129 in 11e5947

	/**
	* @dev See `GovernanceProposal-_proposeProposalStructAndCastVotes`.
	*
	* Requirements:
	* - The method caller is governor.
	*
	*/
	function proposeProposalStructAndCastVotes(
	Proposal.ProposalDetail calldata _proposal,
	Ballot.VoteType[] calldata _supports,
	Signature[] calldata _signatures
	) external onlyGovernor {
	_proposeProposalStructAndCastVotes(_proposal, _supports, _signatures, DOMAIN_SEPARATOR, msg.sender);
	}
	/**
	* @dev See `GovernanceProposal-_castProposalBySignatures`.
	*/
	function castProposalBySignatures(
	Proposal.ProposalDetail calldata _proposal,
	Ballot.VoteType[] calldata _supports,
	Signature[] calldata _signatures
	) external {
	_castProposalBySignatures(_proposal, _supports, _signatures, DOMAIN_SEPARATOR);
	}

After that it's relayed on the mainchain network:

ronin-dpos-contracts/contracts/mainchain/MainchainGovernanceAdmin.sol

Lines 41 to 54 in 11e5947

	/**
	* @dev See `GovernanceRelay-_relayProposal`.
	*
	* Requirements:
	* - The method caller is relayer.
	*
	*/
	function relayProposal(
	Proposal.ProposalDetail calldata _proposal,
	Ballot.VoteType[] calldata _supports,
	Signature[] calldata _signatures
	) external onlyRole(RELAYER_ROLE) {
	_relayProposal(_proposal, _supports, _signatures, DOMAIN_SEPARATOR, msg.sender);
	}

But the per-chain proposal on the Ronin network is not being relayed anywhere and I guess we can support voting on the Ronin network (chainId = 2020 | 2021 | 2022) without requiring any signatures.

Review naming

ronin-dpos-contracts/contracts/ronin/Maintenance.sol

Line 142 in 2ef51c7

function maintaining(address _consensusAddr, uint256 _block) public view returns (bool) {

    Note: same for `bulkMaintaining`

1. Naming: confusing names

2. As stated in the comment of scheduled, the checking must be done in range of blocks, not just by a timestamp indicated by a single block.

Originally posted by @nxqbao in #22 (comment)

[Maintenance] Confusing name `_minMaintenanceBlockPeriod`

The name _minMaintenanceBlockPeriod and _maxMaintenanceBlockPeriod seem confusing since they conflict with period in the ValidatorSet contract. These two values are counted in the number of blocks, but the name can confuse them to be counted in the number of periods.

They could be changed to
minMaintenanceBlockPeriod -> minMaintenanceDurationInBlock
maxMaintenanceBlockPeriod -> maxMaintenanceDurationInBlock

ronin-dpos-contracts/contracts/ronin/Maintenance.sol

Lines 17 to 20 in 2b98be9

	/// @dev The min block period to maintenance
	uint256 public minMaintenanceBlockPeriod;
	/// @dev The max block period to maintenance
	uint256 public maxMaintenanceBlockPeriod;

[ValidatorSet] Refactor period length of `1 days` to a constant

    I think making `1 days` a constant looks better

Originally posted by @minh-bq in #22 (comment)

Missing public function for setters

    These 2 functions have no corresponding public functions

Originally posted by @minh-bq in #22 (comment)

All the `Has...Contract.sol` missed the constraint `The new address is a contract.`

    All the `Has...Contract.sol` missed the constraint `The new address is a contract.` - I guess

Originally posted by @ducthotran2010 in #22 (comment)

[StakingVesting] Too small value for `blockProducerBonusPerBlock` and `bridgeOperatorBonusPerBlock`

Currently, in many test scripts, the blockProducerBonusPerBlock and bridgeOperatorBonusPerBlock variables of the StakingVesting contract are both set to 1. These values are then multiplied by the percentile of the commission rate, which causes incorrect results in these tests.

Should update the value of both to at least 100 and fix the corresponding test cases.

[ValidatorSet] Wrong event name

See belows:

ronin-dpos-contracts/contracts/interfaces/IRoninValidatorSet.sol

Lines 32 to 33 in 603d7f6

	/// @dev Emitted when the reward of the block producer is deprecated.
	event BlockRewardRewardDeprecated(address indexed coinbaseAddr, uint256 rewardAmount);

ronin-dpos-contracts/contracts/interfaces/IRoninValidatorSet.sol

Line 81 in 603d7f6

* Emits the event `MiningRewardDeprecated` if the coinbase is slashed or no longer be a block producer.

Optimize calculation for bridge reward

    I think this will be better, the rounding error is smaller too

_bridgeOperatingReward[_validator] = _totalBridgeReward / totalBridgeOperators();

Same with the below formula

Originally posted by @minh-bq in #22 (comment)

[ValidatorSet] Support withdrawing jailed reward

    Should this contract support withdraw function to withdraw jailed reward?

Originally posted by @minh-bq in #22 (comment)

Function issues: naming, exposing and params

ronin-dpos-contracts/contracts/ronin/validator/RoninValidatorSet.sol

Line 821 in 2ef51c7

function _setPrioritizedValidatorNumber(uint256 _number) internal {

    I can't see public corresponding function of this.

Originally posted by @minh-bq in #22 (comment)

[ValidatorSet] Filter unstatisfied candidates by self-staking amount, not by total staking amount

See the comment in the following codes.

ronin-dpos-contracts/contracts/ronin/validator/CandidateManager.sol

Lines 134 to 136 in 603d7f6

	// NOTE: we should filter the ones who does not keep the minium candidate staking amount here
	// IE: _staking.bulkSelftStakingAmount(_candidates);
	_stakingTotals = _staking.bulkStakingTotal(_candidates);

Missing names for returned params

Should provide names for these outputs. Although the names are provided in the interface, the ABI in the deployment file are generated by the implementation file. Missing names cause frustrated for DApps usage.

ronin-dpos-contracts/contracts/ronin/slash-indicator/CreditScore.sol

Lines 112 to 123 in 1d3f5e3

	* @inheritdoc ICreditScore
	*/
	function getCreditScoreConfigs()
	external
	view
	override
	returns (
	uint256,
	uint256,
	uint256,
	uint256
	)

Invalid function `epochOf()`

    This looks incorrect but has no impacts, so block 0 -> epoch 0, block 1,2,3,... -> epoch 1?

Originally posted by @minh-bq in #22 (comment)

ronin-dpos-contracts/contracts/ronin/validator/RoninValidatorSet.sol

Lines 338 to 340 in 2ef51c7

	function epochOf(uint256 _block) public view virtual override returns (uint256) {
	return _block == 0 ? 0 : _block / _numberOfBlocksInEpoch + 1;
	}