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

[Slashing] Slash locked gold #2317

Merged
merged 19 commits into from
Jan 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
79 changes: 78 additions & 1 deletion packages/protocol/contracts/governance/LockedGold.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
pragma solidity ^0.5.3;

import "openzeppelin-solidity/contracts/math/Math.sol";
import "openzeppelin-solidity/contracts/utils/ReentrancyGuard.sol";
import "openzeppelin-solidity/contracts/math/SafeMath.sol";
import "openzeppelin-solidity/contracts/ownership/Ownable.sol";
Expand Down Expand Up @@ -31,13 +32,28 @@ contract LockedGold is ILockedGold, ReentrancyGuard, Initializable, UsingRegistr
}

mapping(address => Balances) private balances;
mapping(address => bool) public isSlasher;

modifier onlySlasher() {
require(isSlasher[msg.sender], "Caller must be registered slasher");
_;
}

uint256 public totalNonvoting;
uint256 public unlockingPeriod;

event UnlockingPeriodSet(uint256 period);
event GoldLocked(address indexed account, uint256 value);
event GoldUnlocked(address indexed account, uint256 value, uint256 available);
event GoldWithdrawn(address indexed account, uint256 value);
event SlasherWhitelistAdded(address indexed slasher);
event SlasherWhitelistRemoved(address indexed slasher);
event AccountSlashed(
address indexed slashed,
uint256 penalty,
address indexed reporter,
uint256 reward
);

function initialize(address registryAddress, uint256 _unlockingPeriod) external initializer {
_transferOwnership(msg.sender);
Expand Down Expand Up @@ -238,6 +254,43 @@ contract LockedGold is ILockedGold, ReentrancyGuard, Initializable, UsingRegistr
list.length = lastIndex;
}

/**
* @notice Adds `slasher` to whitelist of approved slashing addresses.
* @param slasher Address to whitelist.
*/
function addSlasher(address slasher) external onlyOwner {
lucasege marked this conversation as resolved.
Show resolved Hide resolved
require(slasher != address(0) && !isSlasher[slasher], "Invalid address to `addSlasher`.");
isSlasher[slasher] = true;
emit SlasherWhitelistAdded(slasher);
}

/**
* @notice Removes `slasher` from whitelist of approved slashing addresses.
* @param slasher Address to remove from whitelist.
*/
function removeSlasher(address slasher) external onlyOwner {
require(isSlasher[slasher], "Address must be valid slasher.");
isSlasher[slasher] = false;
emit SlasherWhitelistRemoved(slasher);
}

/**
* @notice Slashes `account` by reducing its nonvoting locked gold by `penalty`.
* If there is not enough nonvoting locked gold to slash, calls into
* `Election.slashVotes` to slash the remaining gold. If `account` does not have
* `penalty` worth of locked gold, slashes `account`'s total locked gold.
* Also sends `reward` gold to the reporter, and penalty-reward to the Community Fund.
* @param account Address of account being slashed.
* @param penalty Amount to slash account.
* @param reporter Address of account reporting the slasher.
* @param reward Reward to give reporter.
* @param lessers The groups receiving fewer votes than i'th group, or 0 if the i'th group has
* the fewest votes of any validator group.
* @param greaters The groups receiving more votes than the i'th group, or 0 if the i'th group
* has the most votes of any validator group.
* @param indices The indices of the i'th group in `account`'s voting list.
* @dev Fails if `reward` is greater than `account`'s total locked gold.
*/
function slash(
Copy link
Contributor

Choose a reason for hiding this comment

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

Emit an event

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I emit one event AccountSlashed(account, penalty, reporter, reward) or two events to split account/penalty and reporter/reward. Or is the reporter/reward not as important

Copy link
Contributor

Choose a reason for hiding this comment

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

one event is ok imo

address account,
uint256 penalty,
Expand All @@ -246,5 +299,29 @@ contract LockedGold is ILockedGold, ReentrancyGuard, Initializable, UsingRegistr
address[] calldata lessers,
address[] calldata greaters,
uint256[] calldata indices
) external {}
) external onlySlasher {
lucasege marked this conversation as resolved.
Show resolved Hide resolved
uint256 maxSlash = Math.min(penalty, getAccountTotalLockedGold(account));
require(maxSlash >= reward, "reward cannot exceed penalty.");
// Local scoping is required to avoid Solc "stack too deep" error from too many locals.
{
uint256 nonvotingBalance = balances[account].nonvoting;
uint256 difference = 0;
// If not enough nonvoting, revoke the difference
if (nonvotingBalance < maxSlash) {
difference = maxSlash.sub(nonvotingBalance);
require(
Copy link
Contributor

Choose a reason for hiding this comment

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

What should we do if there's not enough to slash anymore?

Copy link
Contributor Author

@lucasege lucasege Dec 27, 2019

Choose a reason for hiding this comment

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

I believe we would want to just slash all of the account's gold, I'm assuming this is handled on the caller side (Slasher contract) by passing in for penalty max(normalSlashPenalty, totalGoldOwnedByAccount) where normalSlashPenalty is the default penalty and totalGoldOwnedByAccount is the total balance + voting balance for the account.

Copy link
Contributor

@jfoutts-celo jfoutts-celo Dec 30, 2019

Choose a reason for hiding this comment

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

Now that the other PRs are in, we can update e.g. https://github.com/celo-org/celo-monorepo/blob/master/packages/protocol/contracts/governance/SlasherUtil.sol#L81 to do the max(normalSlashPenalty, totalGoldOwnedByAccount).

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it's easier to handle in LockedGold, because the needed balances are stored there.

Copy link
Contributor Author

@lucasege lucasege Jan 1, 2020

Choose a reason for hiding this comment

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

Based on this I am changing LockedGold.slash to only slash min(totalLockedGold, penalty). A problematic implication is that if totalLockedGold is somehow less than the reward passed in to slash, the call will revert. However, without doing a bunch of wasteful calculations (adjusting reward based on its original proportion or something similar) slash doesn't have the right info to change reward properly. Just something to keep in mind

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 is OK for now, but i would follow the discussion on this.

My concern is that Given the min() i will be considered "slashed" and thus can't be slashed again for the same thing, but i wouldn't be really paying the penalty that i'm due... i wonder if i can use that to shield me from penalties somehow.

But to analyze this we need to see the bigger picture...

getElection().forceDecrementVotes(account, difference, lessers, greaters, indices) ==
difference,
"Cannot revoke enough voting gold."
);
}
// forceDecrementVotes does not increment nonvoting account balance, so we can't double count
_decrementNonvotingAccountBalance(account, maxSlash.sub(difference));
_incrementNonvotingAccountBalance(reporter, reward);
}
address communityFund = registry.getAddressForOrDie(GOVERNANCE_REGISTRY_ID);
address payable communityFundPayable = address(uint160(communityFund));
communityFundPayable.transfer(maxSlash.sub(reward));
emit AccountSlashed(account, maxSlash, reporter, reward);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,11 @@ interface IElection {
function markGroupIneligible(address) external;
function markGroupEligible(address, address, address) external;
function electValidatorSigners() external view returns (address[] memory);
function forceDecrementVotes(
address,
uint256,
address[] calldata,
address[] calldata,
uint256[] calldata
) external returns (uint256);
}
11 changes: 11 additions & 0 deletions packages/protocol/contracts/governance/test/MockElection.sol
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,15 @@ contract MockElection is IElection {
function electValidatorSigners() external view returns (address[] memory) {
return electedValidators;
}

function forceDecrementVotes(
address,
uint256 value,
address[] calldata,
address[] calldata,
uint256[] calldata
) external returns (uint256) {
this.setActiveVotes(this.getActiveVotes() - value);
return value;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import "../interfaces/IGovernance.sol";
contract MockGovernance is IGovernance {
mapping(address => bool) public isVoting;

function() external payable {} // solhint-disable no-empty-blocks

function setVoting(address voter) external {
isVoting[voter] = true;
}
Expand Down
16 changes: 13 additions & 3 deletions packages/protocol/migrations/19_governance_slasher.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import { CeloContractName } from '@celo/protocol/lib/registry-utils'
import { deploymentForCoreContract } from '@celo/protocol/lib/web3-utils'
import {
deploymentForCoreContract,
getDeployedProxiedContract,
} from '@celo/protocol/lib/web3-utils'
import { config } from '@celo/protocol/migrationsConfig'
import { GovernanceSlasherInstance } from 'types'
import { GovernanceSlasherInstance, LockedGoldInstance } from 'types'

const initializeArgs = async (_: string): Promise<any[]> => {
return [config.registry.predeployedProxyAddress]
Expand All @@ -11,5 +14,12 @@ module.exports = deploymentForCoreContract<GovernanceSlasherInstance>(
web3,
artifacts,
CeloContractName.GovernanceSlasher,
initializeArgs
initializeArgs,
async (slasher: GovernanceSlasherInstance) => {
const lockedGold: LockedGoldInstance = await getDeployedProxiedContract<LockedGoldInstance>(
'LockedGold',
artifacts
)
await lockedGold.addSlasher(slasher.address)
}
)
Loading