From 43d1d0b2171b6cc76d0dc7e33177f0b6a3c263ad Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Mon, 16 Sep 2019 17:04:16 -0700 Subject: [PATCH] more explicit sanity checks for computing balance in interval (previously all failed with div-by-zero) typos --- .../src/libs/LibStakingRichErrors.sol | 29 +++++++++++ .../contracts/src/stake/MixinStake.sol | 2 +- .../staking_pools/MixinCumulativeRewards.sol | 52 ++++++++++++++++--- .../test/cumulative_reward_tracking_test.ts | 4 +- contracts/staking/test/rewards_test.ts | 5 +- packages/order-utils/CHANGELOG.json | 4 ++ .../order-utils/src/staking_revert_errors.ts | 22 ++++++++ packages/utils/src/safe_math_revert_errors.ts | 1 + 8 files changed, 106 insertions(+), 13 deletions(-) diff --git a/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol b/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol index 4e14b81f7b..3daf14c8cb 100644 --- a/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol +++ b/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol @@ -51,6 +51,12 @@ library LibStakingRichErrors { PoolIsFull } + enum CumulativeRewardIntervalErrorCode { + BeginEpochMustBeLessThanEndEpoch, + BeginEpochDoesNotHaveReward, + EndEpochDoesNotHaveReward + } + // bytes4(keccak256("MiscalculatedRewardsError(uint256,uint256)")) bytes4 internal constant MISCALCULATED_REWARDS_ERROR_SELECTOR = 0xf7806c4e; @@ -147,6 +153,10 @@ library LibStakingRichErrors { bytes internal constant INVALID_WETH_ASSET_DATA_ERROR = hex"24bf322c"; + // bytes4(keccak256("CumulativeRewardIntervalError(uint8,bytes32,uint256,uint256)")) + bytes4 internal constant CUMULATIVE_REWARD_INTERVAL_ERROR_SELECTOR = + 0x1f806d55; + // solhint-disable func-name-mixedcase function MiscalculatedRewardsError( uint256 totalRewardsPaid, @@ -455,4 +465,23 @@ library LibStakingRichErrors { { return INVALID_WETH_ASSET_DATA_ERROR; } + + function CumulativeRewardIntervalError( + CumulativeRewardIntervalErrorCode errorCode, + bytes32 poolId, + uint256 beginEpoch, + uint256 endEpoch + ) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + CUMULATIVE_REWARD_INTERVAL_ERROR_SELECTOR, + errorCode, + poolId, + beginEpoch, + endEpoch + ); + } } diff --git a/contracts/staking/contracts/src/stake/MixinStake.sol b/contracts/staking/contracts/src/stake/MixinStake.sol index ad32097915..0603d780a2 100644 --- a/contracts/staking/contracts/src/stake/MixinStake.sol +++ b/contracts/staking/contracts/src/stake/MixinStake.sol @@ -190,7 +190,7 @@ contract MixinStake is // increment how much stake has been delegated to pool _incrementNextBalance(delegatedStakeByPoolId[poolId], amount); - // synchronizes reward state in the pool that the staker is undelegating from + // synchronizes reward state in the pool that the staker is delegating to IStructs.StoredBalance memory finalDelegatedStakeToPoolByOwner = _loadAndSyncBalance(delegatedStakeToPoolByOwner[owner][poolId]); _syncRewardsForDelegator(poolId, owner, initDelegatedStakeToPoolByOwner, finalDelegatedStakeToPoolByOwner); } diff --git a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol index c5a8494f1c..8e19176efc 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinCumulativeRewards.sol @@ -263,7 +263,7 @@ contract MixinCumulativeRewards is /// @dev Computes a member's reward over a given epoch interval. /// @param poolId Uniqud Id of pool. - /// @param memberStakeOverInterval Stake delegated to pool by meber over the interval. + /// @param memberStakeOverInterval Stake delegated to pool by member over the interval. /// @param beginEpoch beginning of interval. /// @param endEpoch end of interval. /// @return rewards accumulated over interval [beginEpoch, endEpoch] @@ -277,19 +277,55 @@ contract MixinCumulativeRewards is view returns (uint256) { - // sanity check + // sanity check inputs if (memberStakeOverInterval == 0) { return 0; } + // sanity check interval + if (beginEpoch >= endEpoch) { + LibRichErrors.rrevert( + LibStakingRichErrors.CumulativeRewardIntervalError( + LibStakingRichErrors.CumulativeRewardIntervalErrorCode.BeginEpochMustBeLessThanEndEpoch, + poolId, + beginEpoch, + endEpoch + ) + ); + } + + // sanity check begin reward + IStructs.Fraction memory beginReward = cumulativeRewardsByPool[poolId][beginEpoch]; + if (!_isCumulativeRewardSet(beginReward)) { + LibRichErrors.rrevert( + LibStakingRichErrors.CumulativeRewardIntervalError( + LibStakingRichErrors.CumulativeRewardIntervalErrorCode.BeginEpochDoesNotHaveReward, + poolId, + beginEpoch, + endEpoch + ) + ); + } + + // sanity check end reward + IStructs.Fraction memory endReward = cumulativeRewardsByPool[poolId][endEpoch]; + if (!_isCumulativeRewardSet(endReward)) { + LibRichErrors.rrevert( + LibStakingRichErrors.CumulativeRewardIntervalError( + LibStakingRichErrors.CumulativeRewardIntervalErrorCode.EndEpochDoesNotHaveReward, + poolId, + beginEpoch, + endEpoch + ) + ); + } + // compute reward - IStructs.Fraction memory beginRatio = cumulativeRewardsByPool[poolId][beginEpoch]; - IStructs.Fraction memory endRatio = cumulativeRewardsByPool[poolId][endEpoch]; uint256 reward = LibFractions.scaleFractionalDifference( - endRatio.numerator, - endRatio.denominator, - beginRatio.numerator, - beginRatio.denominator, + endReward.numerator, + endReward.denominator, + beginReward.numerator, + beginReward.denominator, memberStakeOverInterval ); return reward; diff --git a/contracts/staking/test/cumulative_reward_tracking_test.ts b/contracts/staking/test/cumulative_reward_tracking_test.ts index 59bc6d6ed0..e7e2f6f6cc 100644 --- a/contracts/staking/test/cumulative_reward_tracking_test.ts +++ b/contracts/staking/test/cumulative_reward_tracking_test.ts @@ -34,7 +34,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => { }); describe('Tracking Cumulative Rewards (CR)', () => { - it('should set CR hen a pool is created is epoch 0', async () => { + it('should set CR when a pool is created at epoch 0', async () => { await simulation.runTestAsync([], [TestAction.CreatePool], [{ event: 'SetCumulativeReward', epoch: 0 }]); }); it('should set CR and Most Recent CR when a pool is created in epoch >0', async () => { @@ -166,7 +166,7 @@ blockchainTests.resets('Cumulative Reward Tracking', env => { ], ); }); - it('should set CR and update Most Recent CR when redelegating, plus remove the CR that is no longer depends on.', async () => { + it('should set CR and update Most Recent CR when redelegating, plus remove the CR that it no longer depends on.', async () => { await simulation.runTestAsync( [ TestAction.CreatePool, // creates CR in epoch 0 diff --git a/contracts/staking/test/rewards_test.ts b/contracts/staking/test/rewards_test.ts index 97e434706d..7822daac27 100644 --- a/contracts/staking/test/rewards_test.ts +++ b/contracts/staking/test/rewards_test.ts @@ -620,7 +620,9 @@ blockchainTests.resets('Testing Rewards', env => { membersRewardVaultBalance: rewardsForDelegator[1], }); }); - it('Should collect fees correctly when there is a payout for `currentEpochBalance` but not `nextEpochBalance`', async () => { + it('Should collect fees correctly when re-delegating after un-delegating', async () => { + // Note - there are two ranges over which payouts are computed (see _computeRewardBalanceOfDelegator). + // This triggers the first range (rewards for `delegatedStake.currentEpoch`), but not the second. // first staker delegates (epoch 0) const rewardForDelegator = toBaseUnitAmount(10); const stakeAmount = toBaseUnitAmount(4); @@ -640,7 +642,6 @@ blockchainTests.resets('Testing Rewards', env => { ); // this should go to the delegator await payProtocolFeeAndFinalize(rewardForDelegator); - // delegate stake ~ this will result in a payout where rewards are computed on // the balance's `currentEpochBalance` field but not the `nextEpochBalance` field. await stakers[0].moveStakeAsync( diff --git a/packages/order-utils/CHANGELOG.json b/packages/order-utils/CHANGELOG.json index 105be7fd17..acd5110299 100644 --- a/packages/order-utils/CHANGELOG.json +++ b/packages/order-utils/CHANGELOG.json @@ -93,6 +93,10 @@ { "note": "Add `InitializationError`, `InvalidParamValue` to `StakingRevertErrors`.", "pr": 2131 + }, + { + "note": "Add `CumulativeRewardIntervalError`.", + "pr": 2154 } ] }, diff --git a/packages/order-utils/src/staking_revert_errors.ts b/packages/order-utils/src/staking_revert_errors.ts index 7ac661781a..9955d5675e 100644 --- a/packages/order-utils/src/staking_revert_errors.ts +++ b/packages/order-utils/src/staking_revert_errors.ts @@ -30,6 +30,12 @@ export enum InitializationErrorCode { MixinParamsAlreadyInitialized, } +export enum CumulativeRewardIntervalErrorCode { + BeginEpochMustBeLessThanEndEpoch, + BeginEpochDoesNotHaveReward, + EndEpochDoesNotHaveReward, +} + export class MiscalculatedRewardsError extends RevertError { constructor(totalRewardsPaid?: BigNumber | number | string, initialContractBalance?: BigNumber | number | string) { super( @@ -225,6 +231,21 @@ export class ProxyDestinationCannotBeNilError extends RevertError { } } +export class CumulativeRewardIntervalError extends RevertError { + constructor( + errorCode?: CumulativeRewardIntervalErrorCode, + poolId?: string, + beginEpoch?: BigNumber | number | string, + endEpoch?: BigNumber | number | string, + ) { + super( + 'CumulativeRewardIntervalError', + 'CumulativeRewardIntervalError(uint8 errorCode, bytes32 poolId, uint256 beginEpoch, uint256 endEpoch)', + { errorCode, poolId, beginEpoch, endEpoch }, + ); + } +} + const types = [ AmountExceedsBalanceOfPoolError, BlockTimestampTooLowError, @@ -249,6 +270,7 @@ const types = [ RewardVaultNotSetError, WithdrawAmountExceedsMemberBalanceError, ProxyDestinationCannotBeNilError, + CumulativeRewardIntervalError, ]; // Register the types we've defined. diff --git a/packages/utils/src/safe_math_revert_errors.ts b/packages/utils/src/safe_math_revert_errors.ts index 874e9bdaf5..2d0bce62ea 100644 --- a/packages/utils/src/safe_math_revert_errors.ts +++ b/packages/utils/src/safe_math_revert_errors.ts @@ -11,6 +11,7 @@ export enum BinOpErrorCodes { } export enum DowncastErrorCodes { + ValueTooLargeToDowncastToUint32, ValueTooLargeToDowncastToUint64, ValueTooLargeToDowncastToUint96, }