Skip to content

[C02] Rewards pay out incorrectly when the service provider has a pending “decrease stake” request#562

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.

Sign up for GitHub

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

Merged
hareeshnagaraj merged 5 commits into from
Jun 25, 2020
Merged

[C02] Rewards pay out incorrectly when the service provider has a pending “decrease stake” request #562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
54ad03f
Fix rewards distribution
hareeshnagaraj Jun 23, 2020
d5b5400
Updated test
hareeshnagaraj Jun 23, 2020
22a48b5
Evaluate active stake on rewards
hareeshnagaraj Jun 24, 2020
34c6b22
Removing unused field
hareeshnagaraj Jun 24, 2020
896cf99
Remove dupe op
hareeshnagaraj Jun 25, 2020
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
6 changes: 4 additions & 2 deletions eth-contracts/contracts/ClaimsManager.sol
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -227,7 +227,7 @@ contract ClaimsManager is InitializableV2 {
function processClaim(
address _claimer,
uint _totalLockedForSP
) external
) external returns (uint mintedRewards)
{
_requireIsInitialized();
require(
Expand DownExpand Up@@ -262,7 +262,7 @@ contract ClaimsManager is InitializableV2 {
// Total rewards can be zero if all stake is currently locked up
if (!withinBounds || rewardsForClaimer == 0) {
stakingContract.updateClaimHistory(0, _claimer);
return;
return 0;
}

// ERC20Mintable always returns true
Expand All@@ -286,6 +286,8 @@ contract ClaimsManager is InitializableV2 {
totalStakedAtFundBlockForClaimer,
newTotal
);

return rewardsForClaimer;
}

/**
Expand Down
67 changes: 40 additions & 27 deletions eth-contracts/contracts/DelegateManager.sol
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -338,18 +338,19 @@ contract DelegateManager is InitializableV2 {
(
uint totalBalanceInStaking,
uint totalBalanceInSPFactory,
uint totalBalanceOutsideStaking
uint totalActiveFunds,
uint spLockedStake,
uint totalRewards
) = _validateClaimRewards(spFactory);

// No-op if balance is already equivalent
// This case can occur if no rewards due to bound violation or all stake is locked
if (totalBalanceInStaking == totalBalanceOutsideStaking) {
if (totalRewards == 0) {
return;
}

// Total rewards
// Equal to (balance in staking) - ((balance in sp factory) + (balance in delegate manager))
uint totalRewards = totalBalanceInStaking.sub(totalBalanceOutsideStaking);

// Emit claim event
emit Claim(msg.sender, totalRewards, totalBalanceInStaking);
Expand All@@ -359,11 +360,6 @@ contract DelegateManager is InitializableV2 {
uint spDeployerCutRewards = 0;
uint totalDelegatedStakeIncrease = 0;

// Total valid funds used to calculate rewards distribution
uint totalActiveFunds = (
totalBalanceOutsideStaking.sub(spDelegateInfo[msg.sender].totalLockedUpStake)
);

// Traverse all delegates and calculate their rewards
// As each delegate reward is calculated, increment SP cut reward accordingly
for (uint i = 0; i < spDelegateInfo[msg.sender].delegators.length; i++) {
Expand DownExpand Up@@ -412,8 +408,10 @@ contract DelegateManager is InitializableV2 {
);

// Rewards directly allocated to service provider for their stake
// Total active funds for direct deployer reward share
/// totalActiveDeployerFunds = totalBalanceInSPFactory.sub(spLockedStake);
uint spRewardShare = (
totalBalanceInSPFactory.mul(totalRewards)
(totalBalanceInSPFactory.sub(spLockedStake)).mul(totalRewards)
).div(totalActiveFunds);

spFactory.updateServiceProviderStake(
Expand DownExpand Up@@ -700,41 +698,56 @@ contract DelegateManager is InitializableV2 {
* @notice Helper function for claimRewards to get balances from Staking contract
and do validation
* @param spFactory - reference to ServiceProviderFactory contract
* @return (totalBalanceInStaking, totalBalanceInSPFactory, totalBalanceOutsideStaking)
* @return (totalBalanceInStaking, totalBalanceInSPFactory, totalActiveFunds, spLockedStake, totalRewards)
*/
function _validateClaimRewards(ServiceProviderFactory spFactory)
internal returns (uint totalBalanceInStaking, uint totalBalanceInSPFactory, uint totalBalanceOutsideStaking)
{

internal returns (
uint totalBalanceInStaking,
uint totalBalanceInSPFactory,
uint totalActiveFunds,
uint spLockedStake,
uint totalRewards
)
{
// Account for any pending locked up stake for the service provider
(uint spLockedStake,) = spFactory.getPendingDecreaseStakeRequest(msg.sender);
(spLockedStake,) = spFactory.getPendingDecreaseStakeRequest(msg.sender);
uint totalLockedUpStake = spDelegateInfo[msg.sender].totalLockedUpStake.add(spLockedStake);

// Process claim for msg.sender
// Total locked parameter is equal to delegate locked up stake + service provider locked up stake
ClaimsManager(claimsManagerAddress).processClaim(
uint mintedRewards = ClaimsManager(claimsManagerAddress).processClaim(
msg.sender,
(spDelegateInfo[msg.sender].totalLockedUpStake.add(spLockedStake))
totalLockedUpStake
);

// Amount stored in staking contract for owner
uint _totalBalanceInStaking = Staking(stakingAddress).totalStakedFor(msg.sender);
require(_totalBalanceInStaking > 0, "Stake required for claim");
totalBalanceInStaking = Staking(stakingAddress).totalStakedFor(msg.sender);
require(totalBalanceInStaking > 0, "Stake required for claim");

// Amount in sp factory for claimer
(uint _totalBalanceInSPFactory,,,,,) = spFactory.getServiceProviderDetails(msg.sender);

// Decrease total balance by any locked up stake
_totalBalanceInSPFactory = _totalBalanceInSPFactory.sub(spLockedStake);

(totalBalanceInSPFactory,,,,,) = spFactory.getServiceProviderDetails(msg.sender);
// Require active stake to claim any rewards
require(_totalBalanceInSPFactory > 0, "Service Provider stake required");
require(totalBalanceInSPFactory.sub(spLockedStake) > 0, "Service Provider stake required");

// Amount in delegate manager staked to service provider
uint _totalBalanceOutsideStaking = (
_totalBalanceInSPFactory.add(spDelegateInfo[msg.sender].totalDelegatedStake)
uint totalBalanceOutsideStaking = (
totalBalanceInSPFactory.add(spDelegateInfo[msg.sender].totalDelegatedStake)
);

return (_totalBalanceInStaking, _totalBalanceInSPFactory, _totalBalanceOutsideStaking);
totalActiveFunds = totalBalanceOutsideStaking.sub(totalLockedUpStake);

require(
mintedRewards == totalBalanceInStaking.sub(totalBalanceOutsideStaking),
"Reward amount mismatch"
);

return (
totalBalanceInStaking,
totalBalanceInSPFactory,
totalActiveFunds,
spLockedStake,
mintedRewards
);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion eth-contracts/contracts/test/MockDelegateManager.sol
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -21,7 +21,7 @@ contract MockDelegateManager is InitializableV2 {
function testProcessClaim(
address _claimer,
uint _totalLockedForSP
) external {
) external returns (uint) {
ClaimsManager claimsManager = ClaimsManager(
claimsManagerAddress
);
Expand Down
25 changes: 14 additions & 11 deletions eth-contracts/test/delegateManager.test.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -270,9 +270,10 @@ contract('DelegateManager', async (accounts) => {
let spDetails = await serviceProviderFactory.getServiceProviderDetails(account)
spFactoryStake = spDetails.deployerStake
totalInStakingContract = await staking.totalStakedFor(account)
let spDecreaseRequest = await serviceProviderFactory.getPendingDecreaseStakeRequest(account)

let delegatedStake = await delegateManager.getTotalDelegatedToServiceProvider(account)
let lockedUpStake = await delegateManager.getTotalLockedDelegationForServiceProvider(account)
let lockedUpDelegatorStake = await delegateManager.getTotalLockedDelegationForServiceProvider(account)
let delegatorInfo = {}
let delegators = await delegateManager.getDelegatorsList(account)
for (var i = 0; i < delegators.length; i++) {
Expand All@@ -286,16 +287,17 @@ contract('DelegateManager', async (accounts) => {
}
}
let outsideStake = spFactoryStake.add(delegatedStake)
let totalActiveStake = outsideStake.sub(lockedUpStake)
let totalActiveStake = outsideStake.sub(lockedUpDelegatorStake)
let stakeDiscrepancy = totalInStakingContract.sub(outsideStake)
let accountSummary = {
totalInStakingContract,
delegatedStake,
spFactoryStake,
delegatorInfo,
outsideStake,
lockedUpStake,
totalActiveStake
lockedUpDelegatorStake,
totalActiveStake,
spDecreaseRequest
}

if (print) {
Expand DownExpand Up@@ -914,7 +916,7 @@ contract('DelegateManager', async (accounts) => {
)

let preSlashInfo = await getAccountStakeInfo(stakerAccount, false)
let preSlashLockupStake = preSlashInfo.lockedUpStake
let preSlashLockupStake = preSlashInfo.lockedUpDelegatorStake
assert.isTrue(
preSlashLockupStake.eq(initialDelegateAmount),
'Initial delegate amount not found')
Expand All@@ -924,7 +926,7 @@ contract('DelegateManager', async (accounts) => {

let postRewardInfo = await getAccountStakeInfo(stakerAccount, false)

let postSlashLockupStake = postRewardInfo.lockedUpStake
let postSlashLockupStake = postRewardInfo.lockedUpDelegatorStake
assert.equal(
postSlashLockupStake,
0,
Expand DownExpand Up@@ -1483,11 +1485,12 @@ contract('DelegateManager', async (accounts) => {
let fundsPerRound = await claimsManager.getFundsPerRound()
let expectedIncrease = fundsPerRound.div(_lib.toBN(4))
// Request decrease in stake corresponding to 1/2 of DEFAULT_AMOUNT
await serviceProviderFactory.requestDecreaseStake(DEFAULT_AMOUNT.div(_lib.toBN(2)), { from: stakerAccount })
let info = await getAccountStakeInfo(stakerAccount)
let decreaseStakeAmount = DEFAULT_AMOUNT.div(_lib.toBN(2))
await serviceProviderFactory.requestDecreaseStake(decreaseStakeAmount, { from: stakerAccount })
let info = await getAccountStakeInfo(stakerAccount, true)
await claimsManager.initiateRound({ from: stakerAccount })
await delegateManager.claimRewards({ from: stakerAccount })
let info2 = await getAccountStakeInfo(stakerAccount)
let info2 = await getAccountStakeInfo(stakerAccount, true)
let stakingDiff = (info2.totalInStakingContract).sub(info.totalInStakingContract)
let spFactoryDiff = (info2.spFactoryStake).sub(info.spFactoryStake)
assert.isTrue(stakingDiff.eq(expectedIncrease), 'Expected increase not found in Staking.sol')
Expand All@@ -1499,7 +1502,7 @@ contract('DelegateManager', async (accounts) => {
let requestInfo = await serviceProviderFactory.getPendingDecreaseStakeRequest(stakerAccount)
assert.isTrue((requestInfo.lockupExpiryBlock).gt(_lib.toBN(0)), 'Expected lockup expiry block to be set')
assert.isTrue((requestInfo.amount).gt(_lib.toBN(0)), 'Expected amount to be set')

// Slash
await _lib.slash(_lib.audToWei(5), slasherAccount, governance, delegateManagerKey, guardianAddress)

Expand All@@ -1512,7 +1515,7 @@ contract('DelegateManager', async (accounts) => {
let duration = await serviceProviderFactory.getDecreaseStakeLockupDuration()
// Double decrease stake duration
let newDuration = duration.add(duration)

await _lib.assertRevert(
serviceProviderFactory.updateDecreaseStakeLockupDuration(newDuration),
"Only callable by Governance contract"
Expand Down