Skip to content

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

Closed
hareeshnagaraj wants to merge 5 commits into
mainnetfrom
hn_rewards_fix
Closed

[C02] Rewards pay out incorrectly when the service provider has a pending “decrease stake” request#548
hareeshnagaraj wants to merge 5 commits into
mainnetfrom
hn_rewards_fix

Conversation

@hareeshnagaraj

Copy link
Copy Markdown
Contributor
  • Directly return rewards after mint operation occurs in ClaimsManager, eliminate potential for miscalculated totalRewards when decrease is pending

@hareeshnagarajhareeshnagaraj added the audit-feedback All changes stemming from audit feedback label Jun 23, 2020
@hareeshnagaraj

hareeshnagaraj commented Jun 23, 2020

Copy link
Copy Markdown
ContributorAuthor

Interestingly enough, we already have a test case to cover reward distribution when a decrease stake request is pending. However this was passing due to what seems like a coincidence.

Example: (assume no delegators)

sp1Stake - 100, sp1LockedUp - 50
sp2Stake - 100
Total Stake = 200

Rewards - 1,000

Inside validateClaimRewards:

totalLockedUp - 50
mintedRewards - 250, ((100 - 50) / 200) * 1,000 = 250
totalBalanceInStaking - 350
totalBalanceInSPFac = 100
totalBalanceInSPFac -= 50 = 50 <---- This is the incorrect operation
returns (totalBalanceInStaking=350, totalBalanceInSPFac=50, totalBalanceOutsideStaking=50)

In claimReward (after validateClaimRewards)
(totalBalanceInStaking=350, totalBalanceInSPFac=50, totalBalanceOutsideStaking=50)
totalRewards = 300 <---- This value is SKEWED, it should be 250.

So how did our final balance still come out correctly?

totalActiveFunds = 50
spRewardShare = (spFactoryBalance * totalRewards)/ totalActive --> (50 * 300) / 50 = 300
newSpFactoryBalance = 50 + 300 = 350 <--- This is the right final value even though our internal reward calculation was incorrect, because our input spFactoryBalance is already offset by the increase in totalRewards

@hareeshnagarajhareeshnagaraj changed the title Rewards pay out incorrectly when the service provider has a pending “decrease stake” requestRewards pay out incorrectly when the service provider has a pending “decrease stake” request (C02)Jun 23, 2020
@hareeshnagarajhareeshnagaraj changed the title Rewards pay out incorrectly when the service provider has a pending “decrease stake” request (C02)[C02] Rewards pay out incorrectly when the service provider has a pending “decrease stake” requestJun 23, 2020
Comment threadeth-contracts/contracts/DelegateManager.sol Outdated
Comment threadeth-contracts/contracts/DelegateManager.sol
Comment threadeth-contracts/contracts/DelegateManager.sol
Comment threadeth-contracts/contracts/DelegateManager.sol Outdated
@hareeshnagaraj

Copy link
Copy Markdown
ContributorAuthor

follow-up - is this really a critical vulnerability if insolvency is not possible per the above example? because the final calculation is based on a skewed base value for spFactory balance (decreased by the same amount of increase in totalRewards), final state in spFactory is exactly right.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

audit-feedbackAll changes stemming from audit feedbacketh-contracts

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@hareeshnagaraj@SidSethi